[PATCH] x86: Remove 32-bit sign extension in offset_in_range

[H.J. Lu]

On Mon, Jul 13, 2020 at 8:40 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> 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.
>

I am checking in this.

-- 
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-x86-Remove-32-bit-sign-extension-in-offset_in_range.patch
Type: text/x-patch
Size: 1475 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20200713/2f2f7d56/attachment.bin>