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

[H.J. Lu]

On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 13.07.2020 19:23, H.J. Lu wrote:
> > 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.
>
> So that's exactly what I proposed, but with, I'm afraid, a misleading
> description: While the sign extension indeed is unnecessary for
> encoding (and for 32-bit addresses it really is wrong, as addresses
> get zero-extended), it is my understanding that the logic was thought
> to be necessary for the subsequent conditional around the warning to
> yield false. I'm meanwhile wondering whether something breaks this
> way with 16-bit addressing.
>
> Nevertheless, I've meanwhile thought of a (contrived) case that was
> broken with the code present:
>
>         addr32 mov $0x89abcdef, %rax
>
> would have got the immediate sign-extended from 32 to 64 bits.
>

I opened:

https://sourceware.org/bugzilla/show_bug.cgi?id=26237

There are multiple issues.

-- 
H.J.