[COMMITTED] gas: sframe: do not test whether offsetT exceeds INT64_MIN..INT64_MAX

[Jens Remus]

Hello Indu!

On 1/13/2026 5:10 PM, Jan Beulich wrote:
> On 13.01.2026 17:07, Jens Remus wrote:
>> On 1/13/2026 4:52 PM, Jan Beulich wrote:
>>> On 13.01.2026 16:46, Jens Remus wrote:
>>>> On 1/13/2026 4:37 PM, Jan Beulich wrote:
>>>>> On 13.01.2026 16:27, Jens Remus wrote:
>>>>>> On 1/13/2026 2:59 PM, Jan Beulich wrote:
>>>>>>> On 13.01.2026 14:24, Jens Remus wrote:
>>>>>>>> A value of type offsetT, which is either a signed 32-bit or 64-bit
>>>>>>>> integer, cannot exceed the range of INT64_MIN..INT64_MAX.  This
>>>>>>>> resolves the following compile error:
>>>>>>>>
>>>>>>>> ../../binutils-gdb/gas/gen-sframe.c: In function ‘get_offset_size_in_bytes’:
>>>>>>>> ../../binutils-gdb/gas/gen-sframe.c:213:45: error: comparison is always true due to limited range of data type [-Werror=type-limits]
>>>>>>>>   213 |   else if ((sizeof (offsetT) > 4) && (value <= INT64_MAX && value >= INT64_MIN))
>>>>>>>>       |                                             ^~
>>>>>>>> ../../binutils-gdb/gas/gen-sframe.c:213:67: error: comparison is always true due to limited range of data type [-Werror=type-limits]
>>>>>>>>   213 |   else if ((sizeof (offsetT) > 4) && (value <= INT64_MAX && value >= INT64_MIN))
>>>>>>>
>>>>>>> This same error ...
>>>>>>>
>>>>>>>> --- a/gas/gen-sframe.c
>>>>>>>> +++ b/gas/gen-sframe.c
>>>>>>>> @@ -210,7 +210,7 @@ get_offset_size_in_bytes (offsetT value)
>>>>>>>>      size = 2;
>>>>>>>>    else if (value <= INT32_MAX && value >= INT32_MIN)
>>>>>>>
>>>>>>> ... will trigger here on a !BFD64 build.
>>>>>>
>>>>>> Argh!  Good catch! Thanks for letting me know!  How do I resolve this
>>>>>> properly?  Shall I commit another fix "gas: sframe: do not test whether
>>>>>> 32-bit offsetT exceeds INT32_MIN..INT32_MAX" or shall I better revert
>>>>>> both commits and start fresh with a v3 review of the initial patch?
>>>>>
>>>>> Incrementally is fine with me.
>>>>>
>>>>>> I propose the following as solution:
>>>>>>
>>>>>>   if (value <= INT8_MAX && value >= INT8_MIN)
>>>>>>     size = 1;
>>>>>>   else if (value <= INT16_MAX && value >= INT16_MIN)
>>>>>>     size = 2;
>>>>>>   else if ((sizeof (offsetT) == 4) || (value <= INT32_MAX && value >= INT32_MIN))
>>>>>
>>>>> Depending on the internal workings of compilers, this may still trigger
>>>>> the same diagnostic. I.e. depending on whether the rhs of the || is
>>>>> eliminated ahead of or after when these kinds of warnings are issued.
>>>>> A pre-processor conditional involving BFD64 may be better.
>>>>
>>>> Is the following patch ok?
>>>
>>> In principle yes; the part I need to defer to Indu or you is whether ...
>>>
>>>> --- a/gas/gen-sframe.c
>>>> +++ b/gas/gen-sframe.c
>>>> @@ -208,10 +208,15 @@ get_offset_size_in_bytes (offsetT value)
>>>>      size = 1;
>>>>    else if (value <= INT16_MAX && value >= INT16_MIN)
>>>>      size = 2;
>>>> +#ifdef BFD64
>>>>    else if (value <= INT32_MAX && value >= INT32_MIN)
>>>>      size = 4;
>>>> -  else if (sizeof (offsetT) > 4)
>>>> +  else
>>>>      size = 8;
>>>> +#else
>>>> +  else
>>>> +    size = 4;
>>>> +#endif
>>>
>>> ... 8-byte sizes really are entirely impossible to use when !BFD64. Since
>>> it's an offsetT that is passed in, this would seem logical, but I'd rather
>>> have Indu / you double-check.
>>
>> Condensed excerpt from gas/as.h:
>>
>> #ifdef BFD64
>> typedef int64_t bfd_signed_vma;
>> #else /* not BFD64  */
>> typedef int32_t bfd_signed_vma;
>> #endif /* not BFD64  */
>>
>> typedef bfd_signed_vma offsetT;
>>
>> If BFD64, then offsetT is a signed 64-bit integer.  Otherwise it is a
>> signed 32-bit integer.  As a result it is impossible for !BFD64 to have
>> a 64-bit offsetT.
> 
> Sure, hence my reference to offsetT in my earlier reply.
> 
>> Or were you concerned whether SFrame would be broken for !BFD64 targets
>> in general?  AFAIK SFrame does only allow 8-bit, 16-bit, and 32-bit
>> SFrame offsets.  64-bit ones are not supported:
>>
>> Excerpt from gas/gen-sframe.c:
>>
>> #define SFRAME_FRE_OFFSET_FUNC_MAP_INDEX_1B  0 /* SFRAME_FRE_OFFSET_1B.  */
>> #define SFRAME_FRE_OFFSET_FUNC_MAP_INDEX_2B  1 /* SFRAME_FRE_OFFSET_2B.  */
>> #define SFRAME_FRE_OFFSET_FUNC_MAP_INDEX_4B  2 /* SFRAME_FRE_OFFSET_4B.  */
>> #define SFRAME_FRE_OFFSET_FUNC_MAP_INDEX_8B  3 /* Not supported in SFrame.  */
> 
> Then why is size 8 even a possible return value of the function?
Honestly I don't know.  Indu, could you please shed some light on this?

Given SFrame does not use offsetT for the respective offsets, would it
instead make sense to change the function to accept a "int value" or
"int32_t value" (instead of "offsetT value") and remove the 64-bit case?

static unsigned int
get_offset_size_in_bytes (int value)
{
  unsigned int size = 0;

  if (value <= INT8_MAX && value >= INT8_MIN)
    size = 1;
  else if (value <= INT16_MAX && value >= INT16_MIN)
    size = 2;
  else
    size = 4;

  return size;
}

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/