offset_in_range() question

offset_in_range() question

[Jan Beulich]

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
-
   if ((val & ~mask) != 0 && (val & ~mask) != ~mask)
     {
       char buf1[40], buf2[40];

Re: offset_in_range() question

[H.J. Lu]

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.

Re: offset_in_range() question

[Jan Beulich]

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

Re: offset_in_range() question

[H.J. Lu]

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.

Re: offset_in_range() question

[Jan Beulich]

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] x86: Remove 32-bit sign extension in offset_in_range

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 3623 bytes --]

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.

[-- Attachment #2: 0001-x86-Remove-32-bit-sign-extension-in-offset_in_range.patch --]
[-- Type: text/x-patch, Size: 1475 bytes --]

From 0e11977474fd73f6ca7b8423c2941f02988b931c Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 13 Jul 2020 10:18:39 -0700
Subject: [PATCH] x86: Remove 32-bit sign extension in offset_in_range

When encoding a 32-bit offset, there is no need to sign-extend it to 64
bits since only the lower 32 bits are used.

	* config/tc-i386.c (offset_in_range): Remove 32-bit sign
	extension.
---
 gas/ChangeLog        | 4 ++++
 gas/config/tc-i386.c | 8 --------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/gas/ChangeLog b/gas/ChangeLog
index 01862e0875..c1f1a5403f 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,7 @@
+2020-07-13  H.J. Lu  <hongjiu.lu@intel.com>
+
+	* config/tc-i386.c (offset_in_range): Remove sign extension.
+
 2020-07-13  Nick Clifton  <nickc@redhat.com>
 
 	* po/fr.po: Updated French translation.
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 18f685c8b1..192c5e1ae3 100644
--- 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];
-- 
2.26.2

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

[Jan Beulich]

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.

Jan

Re: [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.

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

[Jan Beulich]

On 14.07.2020 14:43, H.J. Lu wrote:
> On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
>> 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.

Hmm, the former two lines there look correct to me, while the latter
two lines look to have been translated with a gas that didn't have
yesterday's change yet. IOW - I'm somewhat confused.

Jan

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

[H.J. Lu]

On Tue, Jul 14, 2020 at 5:51 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 14.07.2020 14:43, H.J. Lu wrote:
> > On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
> >> 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.
>
> Hmm, the former two lines there look correct to me, while the latter
> two lines look to have been translated with a gas that didn't have
> yesterday's change yet. IOW - I'm somewhat confused.

The bug is against binutils 2.35, not master.


-- 
H.J.

[PATCH] x86-64: Zero-extend lower 32 bits displacement to 64 bits

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]

On Tue, Jul 14, 2020 at 5:55 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jul 14, 2020 at 5:51 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 14.07.2020 14:43, H.J. Lu wrote:
> > > On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
> > >> 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.
> >
> > Hmm, the former two lines there look correct to me, while the latter
> > two lines look to have been translated with a gas that didn't have
> > yesterday's change yet. IOW - I'm somewhat confused.
>
> The bug is against binutils 2.35, not master.
>

Here is the patch for master branch.  I will backport it to 2.35 branch
together with

https://sourceware.org/pipermail/binutils/2020-July/112356.html


-- 
H.J.

[-- Attachment #2: 0001-x86-64-Zero-extend-lower-32-bits-displacement-to-64-.patch --]
[-- Type: text/x-patch, Size: 11011 bytes --]

From 7366f8eedf075444a06bd0ba6c69e0aaf8b1ad23 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 14 Jul 2020 09:35:24 -0700
Subject: [PATCH] x86-64: Zero-extend lower 32 bits displacement to 64 bits

Since the addr32 (0x67) prefix zero-extends the lower 32 bits address to
64 bits, change disassembler to zero-extend the lower 32 bits displacement
to 64 bits when there is no base nor index registers.

gas/

	PR gas/26237
	* testsuite/gas/i386/addr32.s: Add tests for 32-bit wrapped around
	address.
	* testsuite/gas/i386/x86-64-addr32.s: Likewise.
	* testsuite/gas/i386/addr32.d: Updated.
	* testsuite/gas/i386/x86-64-addr32-intel.d: Likewise.
	* testsuite/gas/i386/x86-64-addr32.d: Likewise.
	* testsuite/gas/i386/ilp32/x86-64-addr32-intel.d: Likewise.
	* testsuite/gas/i386/ilp32/x86-64-addr32.d: Likewise.

opcodes/

	PR gas/26237
	* i386-dis.c (OP_E_memory): Without base nor index registers,
	32-bit displacement to 64 bits.
---
 gas/testsuite/gas/i386/addr32.d               |  6 +++++
 gas/testsuite/gas/i386/addr32.s               |  6 +++++
 .../gas/i386/ilp32/x86-64-addr32-intel.d      | 26 +------------------
 gas/testsuite/gas/i386/ilp32/x86-64-addr32.d  | 26 +------------------
 gas/testsuite/gas/i386/x86-64-addr32-intel.d  | 10 +++++--
 gas/testsuite/gas/i386/x86-64-addr32.d        | 10 +++++--
 gas/testsuite/gas/i386/x86-64-addr32.s        |  6 +++++
 opcodes/i386-dis.c                            |  9 +++++--
 8 files changed, 43 insertions(+), 56 deletions(-)

diff --git a/gas/testsuite/gas/i386/addr32.d b/gas/testsuite/gas/i386/addr32.d
index 8553fc3e0c..a355c30f3f 100644
--- a/gas/testsuite/gas/i386/addr32.d
+++ b/gas/testsuite/gas/i386/addr32.d
@@ -13,4 +13,10 @@ Disassembly of section .text:
 [	 ]*19:[	 ]+67 a3 98 08 60 00[	 ]+addr32[	 ]+mov[ 	]+%ax,0x600898
 [	 ]*1f:[	 ]+67 66 a3 98 08 60 00[	 ]+addr32[	 ]+mov[ 	]+%eax,0x600898
 [	 ]*26:[	 ]+67 66 c7 04 24 01 00 00 00[	 ]+movl[	 ]+\$0x1,\(%esp\)
+[	 ]*2f:[	 ]+67 66 a1 ef cd ab 89[	 ]+addr32[	 ]+mov[ 	]+0x89abcdef,%eax
+[	 ]*36:[	 ]+67 66 8b 1d ef cd ab 89[	 ]+addr32[	 ]+mov[ 	]+0x89abcdef,%ebx
+[	 ]*3e:[	 ]+67 66 b8 ef cd ab 89[	 ]+addr32[	 ]+mov[ 	]+\$0x89abcdef,%eax
+[	 ]*45:[	 ]+67 66 bb ef cd ab 89[	 ]+addr32[	 ]+mov[ 	]+\$0x89abcdef,%ebx
+[	 ]*4c:[	 ]+67 66 a3 ef cd ab 89[	 ]+addr32[	 ]+mov[ 	]+%eax,0x89abcdef
+[	 ]*53:[	 ]+67 66 89 1d ef cd ab 89[	 ]+addr32[	 ]+mov[ 	]+%ebx,0x89abcdef
 #pass
diff --git a/gas/testsuite/gas/i386/addr32.s b/gas/testsuite/gas/i386/addr32.s
index b899ebd5a3..0f45560602 100644
--- a/gas/testsuite/gas/i386/addr32.s
+++ b/gas/testsuite/gas/i386/addr32.s
@@ -7,3 +7,9 @@
 	addr32 mov	%ax,0x600898
 	addr32 mov	%eax,0x600898
 	addr32 movl	$0x1,(%esp)
+	addr32 mov	0x89abcdef,%eax
+	addr32 mov	0x89abcdef,%ebx
+	addr32 mov	$0x89abcdef,%eax
+	addr32 mov	$0x89abcdef,%ebx
+	addr32 mov	%eax,0x89abcdef
+	addr32 mov	%ebx,0x89abcdef
diff --git a/gas/testsuite/gas/i386/ilp32/x86-64-addr32-intel.d b/gas/testsuite/gas/i386/ilp32/x86-64-addr32-intel.d
index 86e8cf8039..d6d2e1d8b4 100644
--- a/gas/testsuite/gas/i386/ilp32/x86-64-addr32-intel.d
+++ b/gas/testsuite/gas/i386/ilp32/x86-64-addr32-intel.d
@@ -2,28 +2,4 @@
 #objdump: -drwMintel
 #source: ../x86-64-addr32.s
 #name: x86-64 (ILP32) 32-bit addressing (Intel mode)
-
-.*: +file format .*
-
-Disassembly of section .text:
-
-0+ <.text>:
-[ 	]*[a-f0-9]+:	67 48 8d 80 00 00 00 00[ 	]+lea[ 	]+rax,\[eax\+0x0\].*
-[ 	]*[a-f0-9]+:	67 49 8d 80 00 00 00 00[ 	]+lea[ 	]+rax,\[r8d\+0x0\].*
-[ 	]*[a-f0-9]+:	67 48 8d 05 00 00 00 00[ 	]+lea[ 	]+rax,\[eip\+0x0\].*
-[ 	]*[a-f0-9]+:	67 48 8d 04 25 00 00 00 00 	lea[ ]+rax,\[eiz\*1\+0x0\].*
-[ 	]*[a-f0-9]+:	67 a0 98 08 60 00    	addr32 mov al,ds:0x600898
-[ 	]*[a-f0-9]+:	67 66 a1 98 08 60 00 	addr32 mov ax,ds:0x600898
-[ 	]*[a-f0-9]+:	67 a1 98 08 60 00    	addr32 mov eax,ds:0x600898
-[ 	]*[a-f0-9]+:	67 48 a1 98 08 60 00 	addr32 mov rax,ds:0x600898
-[ 	]*[a-f0-9]+:	67 48 a1 98 08 80 00 	addr32 mov rax,ds:0x800898
-[ 	]*[a-f0-9]+:	67 48 8b 1c 25 98 08 80 00 	mov[ ]+rbx,QWORD PTR \[eiz\*1\+0x800898\]
-[ 	]*[a-f0-9]+:	67 a2 98 08 60 00    	addr32 mov ds:0x600898,al
-[ 	]*[a-f0-9]+:	67 66 a3 98 08 60 00 	addr32 mov ds:0x600898,ax
-[ 	]*[a-f0-9]+:	67 a3 98 08 60 00    	addr32 mov ds:0x600898,eax
-[ 	]*[a-f0-9]+:	67 48 a3 98 08 60 00 	addr32 mov ds:0x600898,rax
-[ 	]*[a-f0-9]+:	67 48 a3 98 08 80 00 	addr32 mov ds:0x800898,rax
-[ 	]*[a-f0-9]+:	67 48 89 1c 25 98 08 80 00 	mov[ ]+QWORD PTR \[eiz\*1\+0x800898\],rbx
-[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+DWORD PTR \[eiz\*1-0xccddef\],eax
-[ 	]*[a-f0-9]+:	67 89 04 65 11 22 33 ff 	mov[ ]+DWORD PTR \[eiz\*2-0xccddef\],eax
-#pass
+#dump: ../x86-64-addr32-intel.d
diff --git a/gas/testsuite/gas/i386/ilp32/x86-64-addr32.d b/gas/testsuite/gas/i386/ilp32/x86-64-addr32.d
index b866473acc..9c4d3729ce 100644
--- a/gas/testsuite/gas/i386/ilp32/x86-64-addr32.d
+++ b/gas/testsuite/gas/i386/ilp32/x86-64-addr32.d
@@ -2,28 +2,4 @@
 #as: -J
 #objdump: -drw
 #name: x86-64 (ILP32) 32-bit addressing
-
-.*: +file format .*
-
-Disassembly of section .text:
-
-0+ <.text>:
-[ 	]*[a-f0-9]+:	67 48 8d 80 00 00 00 00[ 	]+lea[ 	]+0x0\(%eax\),%rax.*
-[ 	]*[a-f0-9]+:	67 49 8d 80 00 00 00 00[ 	]+lea[ 	]+0x0\(%r8d\),%rax.*
-[ 	]*[a-f0-9]+:	67 48 8d 05 00 00 00 00[ 	]+lea[ 	]+0x0\(%eip\),%rax.*
-[ 	]*[a-f0-9]+:	67 48 8d 04 25 00 00 00 00[ 	]+lea[ 	]+0x0\(,%eiz,1\),%rax.*
-[ 	]*[a-f0-9]+:	67 a0 98 08 60 00    	addr32 mov 0x600898,%al
-[ 	]*[a-f0-9]+:	67 66 a1 98 08 60 00 	addr32 mov 0x600898,%ax
-[ 	]*[a-f0-9]+:	67 a1 98 08 60 00    	addr32 mov 0x600898,%eax
-[ 	]*[a-f0-9]+:	67 48 a1 98 08 60 00 	addr32 mov 0x600898,%rax
-[ 	]*[a-f0-9]+:	67 48 a1 98 08 80 00 	addr32 mov 0x800898,%rax
-[ 	]*[a-f0-9]+:	67 48 8b 1c 25 98 08 80 00 	mov[ ]+0x800898\(,%eiz,1\),%rbx
-[ 	]*[a-f0-9]+:	67 a2 98 08 60 00    	addr32 mov %al,0x600898
-[ 	]*[a-f0-9]+:	67 66 a3 98 08 60 00 	addr32 mov %ax,0x600898
-[ 	]*[a-f0-9]+:	67 a3 98 08 60 00    	addr32 mov %eax,0x600898
-[ 	]*[a-f0-9]+:	67 48 a3 98 08 60 00 	addr32 mov %rax,0x600898
-[ 	]*[a-f0-9]+:	67 48 a3 98 08 80 00 	addr32 mov %rax,0x800898
-[ 	]*[a-f0-9]+:	67 48 89 1c 25 98 08 80 00 	mov[ ]+%rbx,0x800898\(,%eiz,1\)
-[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+%eax,-0xccddef\(,%eiz,1\)
-[ 	]*[a-f0-9]+:	67 89 04 65 11 22 33 ff 	mov[ ]+%eax,-0xccddef\(,%eiz,2\)
-#pass
+#dump: ../x86-64-addr32.d
diff --git a/gas/testsuite/gas/i386/x86-64-addr32-intel.d b/gas/testsuite/gas/i386/x86-64-addr32-intel.d
index ec0966caf4..0988457b34 100644
--- a/gas/testsuite/gas/i386/x86-64-addr32-intel.d
+++ b/gas/testsuite/gas/i386/x86-64-addr32-intel.d
@@ -18,12 +18,18 @@ Disassembly of section .text:
 [ 	]*[a-f0-9]+:	67 48 a1 98 08 60 00 	addr32 mov rax,ds:0x600898
 [ 	]*[a-f0-9]+:	67 48 a1 98 08 80 00 	addr32 mov rax,ds:0x800898
 [ 	]*[a-f0-9]+:	67 48 8b 1c 25 98 08 80 00 	mov[ ]+rbx,QWORD PTR \[eiz\*1\+0x800898\]
+[ 	]*[a-f0-9]+:	67 48 a1 ef cd ab 89 	addr32 mov rax,ds:0x89abcdef
+[ 	]*[a-f0-9]+:	67 48 8b 1c 25 ef cd ab 89 	mov[ ]+rbx,QWORD PTR \[eiz\*1\+0x89abcdef\]
+[ 	]*[a-f0-9]+:	67 48 b8 ef cd ab 89 00 00 00 00 	addr32 movabs rax,0x89abcdef
+[ 	]*[a-f0-9]+:	67 48 bb ef cd ab 89 00 00 00 00 	addr32 movabs rbx,0x89abcdef
 [ 	]*[a-f0-9]+:	67 a2 98 08 60 00    	addr32 mov ds:0x600898,al
 [ 	]*[a-f0-9]+:	67 66 a3 98 08 60 00 	addr32 mov ds:0x600898,ax
 [ 	]*[a-f0-9]+:	67 a3 98 08 60 00    	addr32 mov ds:0x600898,eax
 [ 	]*[a-f0-9]+:	67 48 a3 98 08 60 00 	addr32 mov ds:0x600898,rax
 [ 	]*[a-f0-9]+:	67 48 a3 98 08 80 00 	addr32 mov ds:0x800898,rax
 [ 	]*[a-f0-9]+:	67 48 89 1c 25 98 08 80 00 	mov[ ]+QWORD PTR \[eiz\*1\+0x800898\],rbx
-[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+DWORD PTR \[eiz\*1-0xccddef\],eax
-[ 	]*[a-f0-9]+:	67 89 04 65 11 22 33 ff 	mov[ ]+DWORD PTR \[eiz\*2-0xccddef\],eax
+[ 	]*[a-f0-9]+:	67 48 a3 ef cd ab 89 	addr32 mov ds:0x89abcdef,rax
+[ 	]*[a-f0-9]+:	67 48 89 1c 25 ef cd ab 89 	mov[ ]+QWORD PTR \[eiz\*1\+0x89abcdef\],rbx
+[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+DWORD PTR \[eiz\*1\+0xff332211\],eax
+[ 	]*[a-f0-9]+:	67 89 04 65 11 22 33 ff 	mov[ ]+DWORD PTR \[eiz\*2\+0xff332211\],eax
 #pass
diff --git a/gas/testsuite/gas/i386/x86-64-addr32.d b/gas/testsuite/gas/i386/x86-64-addr32.d
index 6b73de951f..d9481a7439 100644
--- a/gas/testsuite/gas/i386/x86-64-addr32.d
+++ b/gas/testsuite/gas/i386/x86-64-addr32.d
@@ -17,12 +17,18 @@ Disassembly of section .text:
 [ 	]*[a-f0-9]+:	67 48 a1 98 08 60 00 	addr32 mov 0x600898,%rax
 [ 	]*[a-f0-9]+:	67 48 a1 98 08 80 00 	addr32 mov 0x800898,%rax
 [ 	]*[a-f0-9]+:	67 48 8b 1c 25 98 08 80 00 	mov[ ]+0x800898\(,%eiz,1\),%rbx
+[ 	]*[a-f0-9]+:	67 48 a1 ef cd ab 89 	addr32 mov 0x89abcdef,%rax
+[ 	]*[a-f0-9]+:	67 48 8b 1c 25 ef cd ab 89 	mov[ ]+0x89abcdef\(,%eiz,1\),%rbx
+[ 	]*[a-f0-9]+:	67 48 b8 ef cd ab 89 00 00 00 00 	addr32 movabs \$0x89abcdef,%rax
+[ 	]*[a-f0-9]+:	67 48 bb ef cd ab 89 00 00 00 00 	addr32 movabs \$0x89abcdef,%rbx
 [ 	]*[a-f0-9]+:	67 a2 98 08 60 00    	addr32 mov %al,0x600898
 [ 	]*[a-f0-9]+:	67 66 a3 98 08 60 00 	addr32 mov %ax,0x600898
 [ 	]*[a-f0-9]+:	67 a3 98 08 60 00    	addr32 mov %eax,0x600898
 [ 	]*[a-f0-9]+:	67 48 a3 98 08 60 00 	addr32 mov %rax,0x600898
 [ 	]*[a-f0-9]+:	67 48 a3 98 08 80 00 	addr32 mov %rax,0x800898
 [ 	]*[a-f0-9]+:	67 48 89 1c 25 98 08 80 00 	mov[ ]+%rbx,0x800898\(,%eiz,1\)
-[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+%eax,-0xccddef\(,%eiz,1\)
-[ 	]*[a-f0-9]+:	67 89 04 65 11 22 33 ff 	mov[ ]+%eax,-0xccddef\(,%eiz,2\)
+[ 	]*[a-f0-9]+:	67 48 a3 ef cd ab 89 	addr32 mov %rax,0x89abcdef
+[ 	]*[a-f0-9]+:	67 48 89 1c 25 ef cd ab 89 	mov[ ]+%rbx,0x89abcdef\(,%eiz,1\)
+[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+%eax,0xff332211\(,%eiz,1\)
+[ 	]*[a-f0-9]+:	67 89 04 65 11 22 33 ff 	mov[ ]+%eax,0xff332211\(,%eiz,2\)
 #pass
diff --git a/gas/testsuite/gas/i386/x86-64-addr32.s b/gas/testsuite/gas/i386/x86-64-addr32.s
index 5acdce9dae..9d3aa6afaa 100644
--- a/gas/testsuite/gas/i386/x86-64-addr32.s
+++ b/gas/testsuite/gas/i386/x86-64-addr32.s
@@ -10,11 +10,17 @@
 	addr32 mov	0x600898,%rax
 	addr32 mov	0x800898,%rax
 	addr32 mov	0x800898,%rbx
+	addr32 mov	0x89abcdef,%rax
+	addr32 mov	0x89abcdef,%rbx
+	addr32 mov	$0x89abcdef,%rax
+	addr32 mov	$0x89abcdef,%rbx
 	addr32 mov	%al,0x600898
 	addr32 mov	%ax,0x600898
 	addr32 mov	%eax,0x600898
 	addr32 mov	%rax,0x600898
 	addr32 mov	%rax,0x800898
 	addr32 mov	%rbx,0x800898
+	addr32 mov	%rax,0x89abcdef
+	addr32 mov	%rbx,0x89abcdef
 	mov		%eax, -0xccddef(,%eiz,)
 	mov		%eax, -0xccddef(,%eiz,2)
diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index a6cc01381b..511e6321ce 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -11877,8 +11877,13 @@ OP_E_memory (int bytemode, int sizeflag)
 	{
 	  if (address_mode == mode_64bit)
 	    {
-	      /* Display eiz instead of addr32.  */
-	      needindex = addr32flag;
+	      if (addr32flag)
+		{
+		  /* Without base nor index registers, zero-extend the
+		     lower 32-bit displacement to 64 bits.  */
+		  disp = (unsigned int) disp;
+		  needindex = 1;
+		}
 	      needaddr32 = 1;
 	    }
 	  else
-- 
2.26.2

Re: [PATCH] x86-64: Zero-extend lower 32 bits displacement to 64 bits

[Jan Beulich]

On 14.07.2020 18:54, H.J. Lu wrote:
> On Tue, Jul 14, 2020 at 5:55 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Tue, Jul 14, 2020 at 5:51 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 14.07.2020 14:43, H.J. Lu wrote:
>>>> On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>> 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.
>>>
>>> Hmm, the former two lines there look correct to me, while the latter
>>> two lines look to have been translated with a gas that didn't have
>>> yesterday's change yet. IOW - I'm somewhat confused.
>>
>> The bug is against binutils 2.35, not master.
>>
> 
> Here is the patch for master branch.

Ah, so your issue was just with disassembly. Yet then why not go a step
further and (at least in 64-bit mode) print

[ 	]*[a-f0-9]+:	67 48 89 1c 25 ef cd ab 89 	mov[ ]+%rbx,0x89abcdef
[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+%eax,0xff332211

instead of

[ 	]*[a-f0-9]+:	67 48 89 1c 25 ef cd ab 89 	mov[ ]+%rbx,0x89abcdef\(,%eiz,1\)
[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+%eax,0xff332211\(,%eiz,1\)

and keep the () part only for

[ 	]*[a-f0-9]+:	67 89 04 65 11 22 33 ff 	mov[ ]+%eax,0xff332211\(,%eiz,2\)

, as there's no SIB-less way to express a base-and-index-less address?

Jan

[PATCH] x86: Don't display eiz with no scale

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On Tue, Jul 14, 2020 at 11:14 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 14.07.2020 18:54, H.J. Lu wrote:
> > On Tue, Jul 14, 2020 at 5:55 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>
> >> On Tue, Jul 14, 2020 at 5:51 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>
> >>> On 14.07.2020 14:43, H.J. Lu wrote:
> >>>> On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> 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.
> >>>
> >>> Hmm, the former two lines there look correct to me, while the latter
> >>> two lines look to have been translated with a gas that didn't have
> >>> yesterday's change yet. IOW - I'm somewhat confused.
> >>
> >> The bug is against binutils 2.35, not master.
> >>
> >
> > Here is the patch for master branch.
>
> Ah, so your issue was just with disassembly. Yet then why not go a step
> further and (at least in 64-bit mode) print
>
> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef
> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211
>
> instead of
>
> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef\(,%eiz,1\)
> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,1\)
>
> and keep the () part only for
>
> [       ]*[a-f0-9]+:    67 89 04 65 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,2\)
>
> , as there's no SIB-less way to express a base-and-index-less address?
>

Done.

I am checking in this.

-- 
H.J.

[-- Attachment #2: 0001-x86-Don-t-display-eiz-with-no-scale.patch --]
[-- Type: text/x-patch, Size: 8201 bytes --]

From 4114f54845b24c01cf4f14bd02233047569194d4 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 15 Jul 2020 06:49:45 -0700
Subject: [PATCH] x86: Don't display eiz with no scale

Change

67 48 8b 1c 25 ef cd ab 89 	mov    0x89abcdef(,%eiz,1),%rbx

to

67 48 8b 1c 25 ef cd ab 89 	mov    0x89abcdef,%rbx

in AT&T syntax and

67 48 8b 1c 25 ef cd ab 89 	mov    rbx,QWORD PTR [eiz*1+0x89abcdef]

to

67 48 8b 1c 25 ef cd ab 89 	mov    rbx,QWORD PTR ds:0x89abcdef

in Intel syntax.

gas/

	PR gas/26237
	* testsuite/gas/i386/evex-no-scale-64.d: Updated.
	* testsuite/gas/i386/addr32.d: Likewise.
	* testsuite/gas/i386/x86-64-addr32-intel.d: Likewise.
	* testsuite/gas/i386/x86-64-addr32.d: Likewise.

opcodes/

	PR gas/26237
	* i386-dis.c (OP_E_memory): Don't display eiz with no scale
	without base nor index registers.
---
 gas/ChangeLog                                |  8 ++++++++
 gas/testsuite/gas/i386/evex-no-scale-64.d    |  2 +-
 gas/testsuite/gas/i386/x86-64-addr32-intel.d | 12 ++++++------
 gas/testsuite/gas/i386/x86-64-addr32.d       | 12 ++++++------
 opcodes/ChangeLog                            |  6 ++++++
 opcodes/i386-dis.c                           |  2 +-
 6 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/gas/ChangeLog b/gas/ChangeLog
index 194819eecf..2336fb57e1 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,11 @@
+2020-07-15  H.J. Lu  <hongjiu.lu@intel.com>
+
+	PR gas/26237
+	* testsuite/gas/i386/evex-no-scale-64.d: Updated.
+	* testsuite/gas/i386/addr32.d: Likewise.
+	* testsuite/gas/i386/x86-64-addr32-intel.d: Likewise.
+	* testsuite/gas/i386/x86-64-addr32.d: Likewise.
+
 2020-07-15  Nick Clifton  <nickc@redhat.com>
 
 	* write.c (create_note_reloc): Add desc2_size parameter.  Zero out
diff --git a/gas/testsuite/gas/i386/evex-no-scale-64.d b/gas/testsuite/gas/i386/evex-no-scale-64.d
index 33623656ee..6c9f68faf2 100644
--- a/gas/testsuite/gas/i386/evex-no-scale-64.d
+++ b/gas/testsuite/gas/i386/evex-no-scale-64.d
@@ -10,5 +10,5 @@ Disassembly of section .text:
  +[a-f0-9]+:	62 f1 7c 48 28 04 05 40 00 00 00 	vmovaps 0x40\(,%rax,1\),%zmm0
  +[a-f0-9]+:	62 f1 7c 48 28 04 25 40 00 00 00 	vmovaps 0x40,%zmm0
  +[a-f0-9]+:	67 62 f1 7c 48 28 04 05 40 00 00 00 	vmovaps 0x40\(,%eax,1\),%zmm0
- +[a-f0-9]+:	67 62 f1 7c 48 28 04 25 40 00 00 00 	vmovaps 0x40\(,%eiz,1\),%zmm0
+ +[a-f0-9]+:	67 62 f1 7c 48 28 04 25 40 00 00 00 	vmovaps 0x40,%zmm0
  +[a-f0-9]+:	62 f1 7c 48 28 04 25 40 00 00 00 	vmovaps 0x40,%zmm0
diff --git a/gas/testsuite/gas/i386/x86-64-addr32-intel.d b/gas/testsuite/gas/i386/x86-64-addr32-intel.d
index 0988457b34..7a25d40162 100644
--- a/gas/testsuite/gas/i386/x86-64-addr32-intel.d
+++ b/gas/testsuite/gas/i386/x86-64-addr32-intel.d
@@ -11,15 +11,15 @@ Disassembly of section .text:
 [ 	]*[a-f0-9]+:	67 48 8d 80 00 00 00 00[ 	]+lea[ 	]+rax,\[eax\+0x0\].*
 [ 	]*[a-f0-9]+:	67 49 8d 80 00 00 00 00[ 	]+lea[ 	]+rax,\[r8d\+0x0\].*
 [ 	]*[a-f0-9]+:	67 48 8d 05 00 00 00 00[ 	]+lea[ 	]+rax,\[eip\+0x0\].*
-[ 	]*[a-f0-9]+:	67 48 8d 04 25 00 00 00 00 	lea[ ]+rax,\[eiz\*1\+0x0\].*
+[ 	]*[a-f0-9]+:	67 48 8d 04 25 00 00 00 00 	lea[ ]+rax,ds:0x0	.*
 [ 	]*[a-f0-9]+:	67 a0 98 08 60 00    	addr32 mov al,ds:0x600898
 [ 	]*[a-f0-9]+:	67 66 a1 98 08 60 00 	addr32 mov ax,ds:0x600898
 [ 	]*[a-f0-9]+:	67 a1 98 08 60 00    	addr32 mov eax,ds:0x600898
 [ 	]*[a-f0-9]+:	67 48 a1 98 08 60 00 	addr32 mov rax,ds:0x600898
 [ 	]*[a-f0-9]+:	67 48 a1 98 08 80 00 	addr32 mov rax,ds:0x800898
-[ 	]*[a-f0-9]+:	67 48 8b 1c 25 98 08 80 00 	mov[ ]+rbx,QWORD PTR \[eiz\*1\+0x800898\]
+[ 	]*[a-f0-9]+:	67 48 8b 1c 25 98 08 80 00 	mov[ ]+rbx,QWORD PTR ds:0x800898
 [ 	]*[a-f0-9]+:	67 48 a1 ef cd ab 89 	addr32 mov rax,ds:0x89abcdef
-[ 	]*[a-f0-9]+:	67 48 8b 1c 25 ef cd ab 89 	mov[ ]+rbx,QWORD PTR \[eiz\*1\+0x89abcdef\]
+[ 	]*[a-f0-9]+:	67 48 8b 1c 25 ef cd ab 89 	mov[ ]+rbx,QWORD PTR ds:0x89abcdef
 [ 	]*[a-f0-9]+:	67 48 b8 ef cd ab 89 00 00 00 00 	addr32 movabs rax,0x89abcdef
 [ 	]*[a-f0-9]+:	67 48 bb ef cd ab 89 00 00 00 00 	addr32 movabs rbx,0x89abcdef
 [ 	]*[a-f0-9]+:	67 a2 98 08 60 00    	addr32 mov ds:0x600898,al
@@ -27,9 +27,9 @@ Disassembly of section .text:
 [ 	]*[a-f0-9]+:	67 a3 98 08 60 00    	addr32 mov ds:0x600898,eax
 [ 	]*[a-f0-9]+:	67 48 a3 98 08 60 00 	addr32 mov ds:0x600898,rax
 [ 	]*[a-f0-9]+:	67 48 a3 98 08 80 00 	addr32 mov ds:0x800898,rax
-[ 	]*[a-f0-9]+:	67 48 89 1c 25 98 08 80 00 	mov[ ]+QWORD PTR \[eiz\*1\+0x800898\],rbx
+[ 	]*[a-f0-9]+:	67 48 89 1c 25 98 08 80 00 	mov[ ]+QWORD PTR ds:0x800898,rbx
 [ 	]*[a-f0-9]+:	67 48 a3 ef cd ab 89 	addr32 mov ds:0x89abcdef,rax
-[ 	]*[a-f0-9]+:	67 48 89 1c 25 ef cd ab 89 	mov[ ]+QWORD PTR \[eiz\*1\+0x89abcdef\],rbx
-[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+DWORD PTR \[eiz\*1\+0xff332211\],eax
+[ 	]*[a-f0-9]+:	67 48 89 1c 25 ef cd ab 89 	mov[ ]+QWORD PTR ds:0x89abcdef,rbx
+[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+DWORD PTR ds:0xff332211,eax
 [ 	]*[a-f0-9]+:	67 89 04 65 11 22 33 ff 	mov[ ]+DWORD PTR \[eiz\*2\+0xff332211\],eax
 #pass
diff --git a/gas/testsuite/gas/i386/x86-64-addr32.d b/gas/testsuite/gas/i386/x86-64-addr32.d
index d9481a7439..c513f0dd8e 100644
--- a/gas/testsuite/gas/i386/x86-64-addr32.d
+++ b/gas/testsuite/gas/i386/x86-64-addr32.d
@@ -10,15 +10,15 @@ Disassembly of section .text:
 [ 	]*[a-f0-9]+:	67 48 8d 80 00 00 00 00[ 	]+lea[ 	]+0x0\(%eax\),%rax.*
 [ 	]*[a-f0-9]+:	67 49 8d 80 00 00 00 00[ 	]+lea[ 	]+0x0\(%r8d\),%rax.*
 [ 	]*[a-f0-9]+:	67 48 8d 05 00 00 00 00[ 	]+lea[ 	]+0x0\(%eip\),%rax.*
-[ 	]*[a-f0-9]+:	67 48 8d 04 25 00 00 00 00[ 	]+lea[ ]+0x0\(,%eiz,1\),%rax.*
+[ 	]*[a-f0-9]+:	67 48 8d 04 25 00 00 00 00[ 	]+lea[ ]+0x0,%rax.*
 [ 	]*[a-f0-9]+:	67 a0 98 08 60 00    	addr32 mov 0x600898,%al
 [ 	]*[a-f0-9]+:	67 66 a1 98 08 60 00 	addr32 mov 0x600898,%ax
 [ 	]*[a-f0-9]+:	67 a1 98 08 60 00    	addr32 mov 0x600898,%eax
 [ 	]*[a-f0-9]+:	67 48 a1 98 08 60 00 	addr32 mov 0x600898,%rax
 [ 	]*[a-f0-9]+:	67 48 a1 98 08 80 00 	addr32 mov 0x800898,%rax
-[ 	]*[a-f0-9]+:	67 48 8b 1c 25 98 08 80 00 	mov[ ]+0x800898\(,%eiz,1\),%rbx
+[ 	]*[a-f0-9]+:	67 48 8b 1c 25 98 08 80 00 	mov[ ]+0x800898,%rbx
 [ 	]*[a-f0-9]+:	67 48 a1 ef cd ab 89 	addr32 mov 0x89abcdef,%rax
-[ 	]*[a-f0-9]+:	67 48 8b 1c 25 ef cd ab 89 	mov[ ]+0x89abcdef\(,%eiz,1\),%rbx
+[ 	]*[a-f0-9]+:	67 48 8b 1c 25 ef cd ab 89 	mov[ ]+0x89abcdef,%rbx
 [ 	]*[a-f0-9]+:	67 48 b8 ef cd ab 89 00 00 00 00 	addr32 movabs \$0x89abcdef,%rax
 [ 	]*[a-f0-9]+:	67 48 bb ef cd ab 89 00 00 00 00 	addr32 movabs \$0x89abcdef,%rbx
 [ 	]*[a-f0-9]+:	67 a2 98 08 60 00    	addr32 mov %al,0x600898
@@ -26,9 +26,9 @@ Disassembly of section .text:
 [ 	]*[a-f0-9]+:	67 a3 98 08 60 00    	addr32 mov %eax,0x600898
 [ 	]*[a-f0-9]+:	67 48 a3 98 08 60 00 	addr32 mov %rax,0x600898
 [ 	]*[a-f0-9]+:	67 48 a3 98 08 80 00 	addr32 mov %rax,0x800898
-[ 	]*[a-f0-9]+:	67 48 89 1c 25 98 08 80 00 	mov[ ]+%rbx,0x800898\(,%eiz,1\)
+[ 	]*[a-f0-9]+:	67 48 89 1c 25 98 08 80 00 	mov[ ]+%rbx,0x800898
 [ 	]*[a-f0-9]+:	67 48 a3 ef cd ab 89 	addr32 mov %rax,0x89abcdef
-[ 	]*[a-f0-9]+:	67 48 89 1c 25 ef cd ab 89 	mov[ ]+%rbx,0x89abcdef\(,%eiz,1\)
-[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+%eax,0xff332211\(,%eiz,1\)
+[ 	]*[a-f0-9]+:	67 48 89 1c 25 ef cd ab 89 	mov[ ]+%rbx,0x89abcdef
+[ 	]*[a-f0-9]+:	67 89 04 25 11 22 33 ff 	mov[ ]+%eax,0xff332211
 [ 	]*[a-f0-9]+:	67 89 04 65 11 22 33 ff 	mov[ ]+%eax,0xff332211\(,%eiz,2\)
 #pass
diff --git a/opcodes/ChangeLog b/opcodes/ChangeLog
index e4e26a18d6..91d7f5b674 100644
--- a/opcodes/ChangeLog
+++ b/opcodes/ChangeLog
@@ -1,3 +1,9 @@
+2020-07-15  H.J. Lu  <hongjiu.lu@intel.com>
+
+	PR gas/26237
+	* i386-dis.c (OP_E_memory): Don't display eiz with no scale
+	without base nor index registers.
+
 2020-07-15  Jan Beulich  <jbeulich@suse.com>
 
 	* i386-dis.c (putop): Move 'V' and 'W' handling.
diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index cd8a9a8d75..d1d7a7d94d 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -11818,7 +11818,7 @@ OP_E_memory (int bytemode, int sizeflag)
 		  /* Without base nor index registers, zero-extend the
 		     lower 32-bit displacement to 64 bits.  */
 		  disp = (unsigned int) disp;
-		  needindex = 1;
+		  needindex = scale;
 		}
 	      needaddr32 = 1;
 	    }
-- 
2.26.2

Re: [PATCH] x86: Don't display eiz with no scale

[Jan Beulich]

On 15.07.2020 15:57, H.J. Lu wrote:
> On Tue, Jul 14, 2020 at 11:14 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 14.07.2020 18:54, H.J. Lu wrote:
>>> On Tue, Jul 14, 2020 at 5:55 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>
>>>> On Tue, Jul 14, 2020 at 5:51 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> On 14.07.2020 14:43, H.J. Lu wrote:
>>>>>> On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>> 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.
>>>>>
>>>>> Hmm, the former two lines there look correct to me, while the latter
>>>>> two lines look to have been translated with a gas that didn't have
>>>>> yesterday's change yet. IOW - I'm somewhat confused.
>>>>
>>>> The bug is against binutils 2.35, not master.
>>>>
>>>
>>> Here is the patch for master branch.
>>
>> Ah, so your issue was just with disassembly. Yet then why not go a step
>> further and (at least in 64-bit mode) print
>>
>> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef
>> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211
>>
>> instead of
>>
>> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef\(,%eiz,1\)
>> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,1\)
>>
>> and keep the () part only for
>>
>> [       ]*[a-f0-9]+:    67 89 04 65 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,2\)
>>
>> , as there's no SIB-less way to express a base-and-index-less address?
>>
> 
> Done.
> 
> I am checking in this.

Having only now noticed the collision with my change to
evex-no-scale-64.d, I wonder whether we want to revert your change,
and hence whether my underlying suggestion was a bad one: There's
now no sign left that these insns use 32-bit address mode. The
alternative would be to ensure that an addr32 prefix gets emitted.
I'd personally favor this alternative, but I could live with the
revert. (When making the suggestion I didn't realize this is a
32-bit-addressing-specific output mode.)

Jan

Re: [PATCH] x86: Don't display eiz with no scale

[H.J. Lu]

On Tue, Jul 21, 2020 at 2:40 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.07.2020 15:57, H.J. Lu wrote:
> > On Tue, Jul 14, 2020 at 11:14 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 14.07.2020 18:54, H.J. Lu wrote:
> >>> On Tue, Jul 14, 2020 at 5:55 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>
> >>>> On Tue, Jul 14, 2020 at 5:51 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>
> >>>>> On 14.07.2020 14:43, H.J. Lu wrote:
> >>>>>> On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>> 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.
> >>>>>
> >>>>> Hmm, the former two lines there look correct to me, while the latter
> >>>>> two lines look to have been translated with a gas that didn't have
> >>>>> yesterday's change yet. IOW - I'm somewhat confused.
> >>>>
> >>>> The bug is against binutils 2.35, not master.
> >>>>
> >>>
> >>> Here is the patch for master branch.
> >>
> >> Ah, so your issue was just with disassembly. Yet then why not go a step
> >> further and (at least in 64-bit mode) print
> >>
> >> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef
> >> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211
> >>
> >> instead of
> >>
> >> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef\(,%eiz,1\)
> >> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,1\)
> >>
> >> and keep the () part only for
> >>
> >> [       ]*[a-f0-9]+:    67 89 04 65 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,2\)
> >>
> >> , as there's no SIB-less way to express a base-and-index-less address?
> >>
> >
> > Done.
> >
> > I am checking in this.
>
> Having only now noticed the collision with my change to
> evex-no-scale-64.d, I wonder whether we want to revert your change,
> and hence whether my underlying suggestion was a bad one: There's
> now no sign left that these insns use 32-bit address mode. The
> alternative would be to ensure that an addr32 prefix gets emitted.
> I'd personally favor this alternative, but I could live with the
> revert. (When making the suggestion I didn't realize this is a
> 32-bit-addressing-specific output mode.)
>

Can you revert it for me?

Thanks.

-- 
H.J.

Re: [PATCH] x86: Don't display eiz with no scale

[Jan Beulich]

On 21.07.2020 13:29, H.J. Lu wrote:
> On Tue, Jul 21, 2020 at 2:40 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 15.07.2020 15:57, H.J. Lu wrote:
>>> On Tue, Jul 14, 2020 at 11:14 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 14.07.2020 18:54, H.J. Lu wrote:
>>>>> On Tue, Jul 14, 2020 at 5:55 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Jul 14, 2020 at 5:51 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>
>>>>>>> On 14.07.2020 14:43, H.J. Lu wrote:
>>>>>>>> On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>> 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.
>>>>>>>
>>>>>>> Hmm, the former two lines there look correct to me, while the latter
>>>>>>> two lines look to have been translated with a gas that didn't have
>>>>>>> yesterday's change yet. IOW - I'm somewhat confused.
>>>>>>
>>>>>> The bug is against binutils 2.35, not master.
>>>>>>
>>>>>
>>>>> Here is the patch for master branch.
>>>>
>>>> Ah, so your issue was just with disassembly. Yet then why not go a step
>>>> further and (at least in 64-bit mode) print
>>>>
>>>> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef
>>>> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211
>>>>
>>>> instead of
>>>>
>>>> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef\(,%eiz,1\)
>>>> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,1\)
>>>>
>>>> and keep the () part only for
>>>>
>>>> [       ]*[a-f0-9]+:    67 89 04 65 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,2\)
>>>>
>>>> , as there's no SIB-less way to express a base-and-index-less address?
>>>>
>>>
>>> Done.
>>>
>>> I am checking in this.
>>
>> Having only now noticed the collision with my change to
>> evex-no-scale-64.d, I wonder whether we want to revert your change,
>> and hence whether my underlying suggestion was a bad one: There's
>> now no sign left that these insns use 32-bit address mode. The
>> alternative would be to ensure that an addr32 prefix gets emitted.
>> I'd personally favor this alternative, but I could live with the
>> revert. (When making the suggestion I didn't realize this is a
>> 32-bit-addressing-specific output mode.)
>>
> 
> Can you revert it for me?

Done.

Jan

Re: [PATCH] x86: Don't display eiz with no scale

[H.J. Lu]

On Tue, Jul 21, 2020 at 5:22 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 21.07.2020 13:29, H.J. Lu wrote:
> > On Tue, Jul 21, 2020 at 2:40 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 15.07.2020 15:57, H.J. Lu wrote:
> >>> On Tue, Jul 14, 2020 at 11:14 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 14.07.2020 18:54, H.J. Lu wrote:
> >>>>> On Tue, Jul 14, 2020 at 5:55 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>>>
> >>>>>> On Tue, Jul 14, 2020 at 5:51 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>
> >>>>>>> On 14.07.2020 14:43, H.J. Lu wrote:
> >>>>>>>> On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>> 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.
> >>>>>>>
> >>>>>>> Hmm, the former two lines there look correct to me, while the latter
> >>>>>>> two lines look to have been translated with a gas that didn't have
> >>>>>>> yesterday's change yet. IOW - I'm somewhat confused.
> >>>>>>
> >>>>>> The bug is against binutils 2.35, not master.
> >>>>>>
> >>>>>
> >>>>> Here is the patch for master branch.
> >>>>
> >>>> Ah, so your issue was just with disassembly. Yet then why not go a step
> >>>> further and (at least in 64-bit mode) print
> >>>>
> >>>> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef
> >>>> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211
> >>>>
> >>>> instead of
> >>>>
> >>>> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef\(,%eiz,1\)
> >>>> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,1\)
> >>>>
> >>>> and keep the () part only for
> >>>>
> >>>> [       ]*[a-f0-9]+:    67 89 04 65 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,2\)
> >>>>
> >>>> , as there's no SIB-less way to express a base-and-index-less address?
> >>>>
> >>>
> >>> Done.
> >>>
> >>> I am checking in this.
> >>
> >> Having only now noticed the collision with my change to
> >> evex-no-scale-64.d, I wonder whether we want to revert your change,
> >> and hence whether my underlying suggestion was a bad one: There's
> >> now no sign left that these insns use 32-bit address mode. The
> >> alternative would be to ensure that an addr32 prefix gets emitted.
> >> I'd personally favor this alternative, but I could live with the
> >> revert. (When making the suggestion I didn't realize this is a
> >> 32-bit-addressing-specific output mode.)
> >>
> >
> > Can you revert it for me?
>
> Done.

Thanks.

-- 
H.J.

Re: [PATCH] x86: Don't display eiz with no scale

[H.J. Lu]

On Tue, Jul 21, 2020 at 7:01 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jul 21, 2020 at 5:22 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 21.07.2020 13:29, H.J. Lu wrote:
> > > On Tue, Jul 21, 2020 at 2:40 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>
> > >> On 15.07.2020 15:57, H.J. Lu wrote:
> > >>> On Tue, Jul 14, 2020 at 11:14 PM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>
> > >>>> On 14.07.2020 18:54, H.J. Lu wrote:
> > >>>>> On Tue, Jul 14, 2020 at 5:55 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >>>>>>
> > >>>>>> On Tue, Jul 14, 2020 at 5:51 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>>>>
> > >>>>>>> On 14.07.2020 14:43, H.J. Lu wrote:
> > >>>>>>>> On Mon, Jul 13, 2020 at 11:12 PM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>>>>>> 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.
> > >>>>>>>
> > >>>>>>> Hmm, the former two lines there look correct to me, while the latter
> > >>>>>>> two lines look to have been translated with a gas that didn't have
> > >>>>>>> yesterday's change yet. IOW - I'm somewhat confused.
> > >>>>>>
> > >>>>>> The bug is against binutils 2.35, not master.
> > >>>>>>
> > >>>>>
> > >>>>> Here is the patch for master branch.
> > >>>>
> > >>>> Ah, so your issue was just with disassembly. Yet then why not go a step
> > >>>> further and (at least in 64-bit mode) print
> > >>>>
> > >>>> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef
> > >>>> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211
> > >>>>
> > >>>> instead of
> > >>>>
> > >>>> [       ]*[a-f0-9]+:    67 48 89 1c 25 ef cd ab 89      mov[ ]+%rbx,0x89abcdef\(,%eiz,1\)
> > >>>> [       ]*[a-f0-9]+:    67 89 04 25 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,1\)
> > >>>>
> > >>>> and keep the () part only for
> > >>>>
> > >>>> [       ]*[a-f0-9]+:    67 89 04 65 11 22 33 ff         mov[ ]+%eax,0xff332211\(,%eiz,2\)
> > >>>>
> > >>>> , as there's no SIB-less way to express a base-and-index-less address?
> > >>>>
> > >>>
> > >>> Done.
> > >>>
> > >>> I am checking in this.
> > >>
> > >> Having only now noticed the collision with my change to
> > >> evex-no-scale-64.d, I wonder whether we want to revert your change,
> > >> and hence whether my underlying suggestion was a bad one: There's
> > >> now no sign left that these insns use 32-bit address mode. The
> > >> alternative would be to ensure that an addr32 prefix gets emitted.
> > >> I'd personally favor this alternative, but I could live with the
> > >> revert. (When making the suggestion I didn't realize this is a
> > >> 32-bit-addressing-specific output mode.)
> > >>
> > >
> > > Can you revert it for me?
> >
> > Done.
>
> Thanks.

I am backporting it to 2.35 branch.


-- 
H.J.