[COMMITTED] gas: sframe: do not test whether offsetT exceeds INT64_MIN..INT64_MAX
Jens Remus
jremus@linux.ibm.com
Wed Jan 14 10:18:16 GMT 2026
On 1/14/2026 8:34 AM, Jan Beulich wrote:
> On 13.01.2026 20:12, Indu Bhagat wrote:
>> On 1/13/26 8:18 AM, Jens Remus wrote:
>>> 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 ?
>
> Not unless that kind of warning is explicitly enabled, afaik. Such "silent"
> truncation occurs all over the place if I'm not mistaken. Question is whether
> callers may actually pass values that fit in offsetT but not in int32_t
> (implying BFD64). In that case maybe take Jens's suggestion above but keep
> the parameter at offsetT and add
>
> int32_t val = value;
>
> gas_assert(val == value);
>
> (provided passing of such arguments really is a bug in the caller).
>
> Yet another possible alternative:
I like that! Simple and easy to understand. I'll send a new patch for review.
>
> static unsigned int
> get_offset_size_in_bytes (offsetT value)
> {
> unsigned int size = 0;
>
> if (value == (int8_t)value)
> size = 1;
> else if (value == (int16_t)value)
> size = 2;
> else if (value == (int32_t)value)
> size = 4;
> else if (sizeof (value) > 4)
Can't this be simply:
else
If offsetT is a signed 32-bit integer, then (value == (int32_t)value)
must be true.
> size = 8;
>
> return size;
> }
>
>> 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;
>
> But adding these casts makes no difference when offsetT is a typedef of int32_t.
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/
More information about the Binutils
mailing list