offset_in_range() question

Message ID bafda425-c566-15d8-3fc7-89b8809c0b4c@suse.com
State New
Headers show
Series
  • offset_in_range() question
Related show

Commit Message

Jan Beulich July 13, 2020, 12:50 p.m.
H.J.,

the dependency on presence of an address size override, when the
function is also used on immediate operands, struck me as being
a possible source for problems. On 2009-09-15 you've made two
changes to the construct in question, but I wonder whether even
back then this code wasn't already dead. When, after quite a bit
of playing, I couldn't come up with any immediate that this
would go wrong on, I thought I'll give a try to removing that
code altogether. And indeed - no fallout. Looking more closely
then suggested that respective logic in optimize_imm() and
optimize_disp() are already arranging for values to never need
further massaging here.

Do you agree that the code could be removed (see patch below in
case of any uncertainty about what block of code I mean), or are
you aware of things that might break this way?

As an aside, I don't think the handling of out-of-range
immediates is quite correct, but I'll get to this in more detail
after thinking some more on possible solutions.

Jan

Comments

David Faust via Binutils July 13, 2020, 1:11 p.m. | #1
On Mon, Jul 13, 2020 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> H.J.,

>

> the dependency on presence of an address size override, when the

> function is also used on immediate operands, struck me as being

> a possible source for problems. On 2009-09-15 you've made two

> changes to the construct in question, but I wonder whether even

> back then this code wasn't already dead. When, after quite a bit

> of playing, I couldn't come up with any immediate that this

> would go wrong on, I thought I'll give a try to removing that

> code altogether. And indeed - no fallout. Looking more closely

> then suggested that respective logic in optimize_imm() and

> optimize_disp() are already arranging for values to never need

> further massaging here.

>

> Do you agree that the code could be removed (see patch below in

> case of any uncertainty about what block of code I mean), or are

> you aware of things that might break this way?

>

> As an aside, I don't think the handling of out-of-range

> immediates is quite correct, but I'll get to this in more detail

> after thinking some more on possible solutions.

>

> Jan

>

> --- a/gas/config/tc-i386.c

> +++ b/gas/config/tc-i386.c

> @@ -2539,14 +2539,6 @@ offset_in_range (offsetT val, int size)

>      default: abort ();

>      }

>

> -#ifdef BFD64

> -  /* If BFD64, sign extend val for 32bit address mode.  */

> -  if (flag_code != CODE_64BIT

> -      || i.prefix[ADDR_PREFIX])

> -    if ((val & ~(((addressT) 2 << 31) - 1)) == 0)

> -      val = (val ^ ((addressT) 1 << 31)) - ((addressT) 1 << 31);

> -#endif

> -


This code came from

commit 3e73aa7c956514ce5bd5fa6320fb239229ac8a7b
Author: Jan Hubicka <jh@suse.cz>
Date:   Wed Dec 20 13:24:13 2000 +0000

            * tc-i386.h (i386_target_format): Define even for ELFs.

My changes only enabled it when BFD64 is defined.  If this code
dead, please replace it with abort for now.

Thanks.

>    if ((val & ~mask) != 0 && (val & ~mask) != ~mask)

>      {

>        char buf1[40], buf2[40];




-- 
H.J.
Jan Beulich July 13, 2020, 2:43 p.m. | #2
On 13.07.2020 15:11, H.J. Lu wrote:
> On Mon, Jul 13, 2020 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> H.J.,

>>

>> the dependency on presence of an address size override, when the

>> function is also used on immediate operands, struck me as being

>> a possible source for problems. On 2009-09-15 you've made two

>> changes to the construct in question, but I wonder whether even

>> back then this code wasn't already dead. When, after quite a bit

>> of playing, I couldn't come up with any immediate that this

>> would go wrong on, I thought I'll give a try to removing that

>> code altogether. And indeed - no fallout. Looking more closely

>> then suggested that respective logic in optimize_imm() and

>> optimize_disp() are already arranging for values to never need

>> further massaging here.

>>

>> Do you agree that the code could be removed (see patch below in

>> case of any uncertainty about what block of code I mean), or are

>> you aware of things that might break this way?

>>

>> As an aside, I don't think the handling of out-of-range

>> immediates is quite correct, but I'll get to this in more detail

>> after thinking some more on possible solutions.

>>

>> Jan

>>

>> --- a/gas/config/tc-i386.c

>> +++ b/gas/config/tc-i386.c

>> @@ -2539,14 +2539,6 @@ offset_in_range (offsetT val, int size)

>>      default: abort ();

>>      }

>>

>> -#ifdef BFD64

>> -  /* If BFD64, sign extend val for 32bit address mode.  */

>> -  if (flag_code != CODE_64BIT

>> -      || i.prefix[ADDR_PREFIX])

>> -    if ((val & ~(((addressT) 2 << 31) - 1)) == 0)

>> -      val = (val ^ ((addressT) 1 << 31)) - ((addressT) 1 << 31);

>> -#endif

>> -

> 

> This code came from

> 

> commit 3e73aa7c956514ce5bd5fa6320fb239229ac8a7b

> Author: Jan Hubicka <jh@suse.cz>

> Date:   Wed Dec 20 13:24:13 2000 +0000

> 

>             * tc-i386.h (i386_target_format): Define even for ELFs.

> 

> My changes only enabled it when BFD64 is defined.  If this code

> dead, please replace it with abort for now.


I guess I don't understand: There's no condition to abort() on right
now. The code I'm proposing to delete simply does nothing useful. Or
do you mean to turn the assignment into an != to control when to
abort()?

Doing so would undermine the main purpose of deleting this code: Its
dependency on address prefix presence is what needs to go away.

Jan
David Faust via Binutils July 13, 2020, 3:11 p.m. | #3
On Mon, Jul 13, 2020 at 7:43 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 13.07.2020 15:11, H.J. Lu wrote:

> > On Mon, Jul 13, 2020 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> H.J.,

> >>

> >> the dependency on presence of an address size override, when the

> >> function is also used on immediate operands, struck me as being

> >> a possible source for problems. On 2009-09-15 you've made two

> >> changes to the construct in question, but I wonder whether even

> >> back then this code wasn't already dead. When, after quite a bit

> >> of playing, I couldn't come up with any immediate that this

> >> would go wrong on, I thought I'll give a try to removing that

> >> code altogether. And indeed - no fallout. Looking more closely

> >> then suggested that respective logic in optimize_imm() and

> >> optimize_disp() are already arranging for values to never need

> >> further massaging here.

> >>

> >> Do you agree that the code could be removed (see patch below in

> >> case of any uncertainty about what block of code I mean), or are

> >> you aware of things that might break this way?

> >>

> >> As an aside, I don't think the handling of out-of-range

> >> immediates is quite correct, but I'll get to this in more detail

> >> after thinking some more on possible solutions.

> >>

> >> Jan

> >>

> >> --- a/gas/config/tc-i386.c

> >> +++ b/gas/config/tc-i386.c

> >> @@ -2539,14 +2539,6 @@ offset_in_range (offsetT val, int size)

> >>      default: abort ();

> >>      }

> >>

> >> -#ifdef BFD64

> >> -  /* If BFD64, sign extend val for 32bit address mode.  */

> >> -  if (flag_code != CODE_64BIT

> >> -      || i.prefix[ADDR_PREFIX])

> >> -    if ((val & ~(((addressT) 2 << 31) - 1)) == 0)

> >> -      val = (val ^ ((addressT) 1 << 31)) - ((addressT) 1 << 31);

> >> -#endif

> >> -

> >

> > This code came from

> >

> > commit 3e73aa7c956514ce5bd5fa6320fb239229ac8a7b

> > Author: Jan Hubicka <jh@suse.cz>

> > Date:   Wed Dec 20 13:24:13 2000 +0000

> >

> >             * tc-i386.h (i386_target_format): Define even for ELFs.

> >

> > My changes only enabled it when BFD64 is defined.  If this code

> > dead, please replace it with abort for now.

>

> I guess I don't understand: There's no condition to abort() on right

> now. The code I'm proposing to delete simply does nothing useful. Or

> do you mean to turn the assignment into an != to control when to

> abort()?

>

> Doing so would undermine the main purpose of deleting this code: Its

> dependency on address prefix presence is what needs to go away.


I didn't add the code in question.  I only changed it to BFD64 only.
I don't know the history behind it.  If it is dead code, just change it
to

if (...)
  abort ();

We will keep it for a few months and then delete the whole thing.

-- 
H.J.
Jan Beulich July 13, 2020, 3:40 p.m. | #4
On 13.07.2020 17:11, H.J. Lu wrote:
> On Mon, Jul 13, 2020 at 7:43 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 13.07.2020 15:11, H.J. Lu wrote:

>>> On Mon, Jul 13, 2020 at 5:50 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>

>>>> H.J.,

>>>>

>>>> the dependency on presence of an address size override, when the

>>>> function is also used on immediate operands, struck me as being

>>>> a possible source for problems. On 2009-09-15 you've made two

>>>> changes to the construct in question, but I wonder whether even

>>>> back then this code wasn't already dead. When, after quite a bit

>>>> of playing, I couldn't come up with any immediate that this

>>>> would go wrong on, I thought I'll give a try to removing that

>>>> code altogether. And indeed - no fallout. Looking more closely

>>>> then suggested that respective logic in optimize_imm() and

>>>> optimize_disp() are already arranging for values to never need

>>>> further massaging here.

>>>>

>>>> Do you agree that the code could be removed (see patch below in

>>>> case of any uncertainty about what block of code I mean), or are

>>>> you aware of things that might break this way?

>>>>

>>>> As an aside, I don't think the handling of out-of-range

>>>> immediates is quite correct, but I'll get to this in more detail

>>>> after thinking some more on possible solutions.

>>>>

>>>> Jan

>>>>

>>>> --- a/gas/config/tc-i386.c

>>>> +++ b/gas/config/tc-i386.c

>>>> @@ -2539,14 +2539,6 @@ offset_in_range (offsetT val, int size)

>>>>      default: abort ();

>>>>      }

>>>>

>>>> -#ifdef BFD64

>>>> -  /* If BFD64, sign extend val for 32bit address mode.  */

>>>> -  if (flag_code != CODE_64BIT

>>>> -      || i.prefix[ADDR_PREFIX])

>>>> -    if ((val & ~(((addressT) 2 << 31) - 1)) == 0)

>>>> -      val = (val ^ ((addressT) 1 << 31)) - ((addressT) 1 << 31);

>>>> -#endif

>>>> -

>>>

>>> This code came from

>>>

>>> commit 3e73aa7c956514ce5bd5fa6320fb239229ac8a7b

>>> Author: Jan Hubicka <jh@suse.cz>

>>> Date:   Wed Dec 20 13:24:13 2000 +0000

>>>

>>>             * tc-i386.h (i386_target_format): Define even for ELFs.

>>>

>>> My changes only enabled it when BFD64 is defined.  If this code

>>> dead, please replace it with abort for now.

>>

>> I guess I don't understand: There's no condition to abort() on right

>> now. The code I'm proposing to delete simply does nothing useful. Or

>> do you mean to turn the assignment into an != to control when to

>> abort()?

>>

>> Doing so would undermine the main purpose of deleting this code: Its

>> dependency on address prefix presence is what needs to go away.

> 

> I didn't add the code in question.  I only changed it to BFD64 only.


You didn't add the sign extension, true, but the thing that caught my
eye here is the use of i.prefix[ADDR_PREFIX], which you added in
9de868bf63da. And that's what should go away one way or another.
Initially I thought the caller may need to pass in whether we're
processing a displacement (where the address override matters) vs an
immediate (where it doesn't matter), until I figured the code is not
doing anything useful at all.

> I don't know the history behind it.  If it is dead code, just change it

> to

> 

> if (...)

>   abort ();


Again - what's to go inside the if() should not have any undue use
of i.prefix[ADDR_PREFIX], so I'm afraid I still don't follow what
exactly you want me to put there.

Jan

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2539,14 +2539,6 @@  offset_in_range (offsetT val, int size)
     default: abort ();
     }
 
-#ifdef BFD64
-  /* If BFD64, sign extend val for 32bit address mode.  */
-  if (flag_code != CODE_64BIT
-      || i.prefix[ADDR_PREFIX])
-    if ((val & ~(((addressT) 2 << 31) - 1)) == 0)
-      val = (val ^ ((addressT) 1 << 31)) - ((addressT) 1 << 31);
-#endif
-
   if ((val & ~mask) != 0 && (val & ~mask) != ~mask)
     {
       char buf1[40], buf2[40];