[COMMITTED] gas: sframe: do not test whether offsetT exceeds INT64_MIN..INT64_MAX
Jan Beulich
jbeulich@suse.com
Wed Jan 14 07:34:46 GMT 2026
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:
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)
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.
Jan
More information about the Binutils
mailing list