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

[Indu Bhagat]

On 1/13/26 8:18 AM, Jens Remus wrote:
> 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))
>>>>>>>>

Sorry for not detecting this in review.  I remembered adding those 
typecasts for addressing some warnings in the past, but for the life of 
me, I couldnt explain to myself why would I have seen any warnings...

>>>>>>>> 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?
> 

It was just to keep keep the function get_offset_size_in_bytes () usable 
for any offsetT value (and doing only what the function name hints).  We 
do error checking/failure in callers of get_offset_size_in_bytes ().

> 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;
> }
> 

Wouldnt the compiler then warn about the narrowing of arg value which is 
offsetT in the rest of gen-sframe.c ?

How about reverting back to typecasts to pacify Wtype-limits warnings:

   else if (value <= (offsetT) INT32_MAX && value >= (offsetT) INT32_MIN)
     size = 4;
   else if (sizeof (offsetT) > 4)
     size = 8;