x86: Remove 32-bit sign extension in offset_in_range

Message ID CAMe9rOqM-gU5gT8WWZxK62HKJr44=QAJCO_jk+CVTsK-zP-Jsg@mail.gmail.com
State New
Headers show
Series
  • x86: Remove 32-bit sign extension in offset_in_range
Related show

Commit Message

David Faust via Binutils July 13, 2020, 5:23 p.m.
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.

Comments

Jan Beulich July 14, 2020, 6:12 a.m. | #1
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
David Faust via Binutils July 14, 2020, 12:43 p.m. | #2
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.
Jan Beulich July 14, 2020, 12:51 p.m. | #3
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
David Faust via Binutils July 14, 2020, 12:55 p.m. | #4
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

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