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