[lttng-dev] [PATCH babeltrace v2] Fix: bitfield: left shift undefined behavior

[Mathieu Desnoyers]

----- On May 8, 2019, at 11:49 AM, Simon Marchi simark at simark.ca wrote:

> On 2019-05-08 10:39 a.m., Mathieu Desnoyers wrote:
>> bitfield.h uses the left shift operator with a left operand which
>> may be negative. The C99 standard states that shifting a negative
>> value is undefined.
>> 
>> When building with -Wshift-negative-value, we get this gcc warning:
>> 
>> In file included from
>> /home/smarchi/src/babeltrace/include/babeltrace/ctfser-internal.h:44:0,
>>                  from /home/smarchi/src/babeltrace/ctfser/ctfser.c:42:
>> /home/smarchi/src/babeltrace/include/babeltrace/ctfser-internal.h: In function
>> ‘bt_ctfser_write_unsigned_int’:
>> /home/smarchi/src/babeltrace/include/babeltrace/bitfield-internal.h:116:24:
>> error: left shift of negative value [-Werror=shift-negative-value]
>>    mask = ~((~(type) 0) << (__start % ts));  \
>>                         ^
>> /home/smarchi/src/babeltrace/include/babeltrace/bitfield-internal.h:222:2: note:
>> in expansion of macro ‘_bt_bitfield_write_le’
>>   _bt_bitfield_write_le(ptr, type, _start, _length, _v)
>>   ^~~~~~~~~~~~~~~~~~~~~
>> /home/smarchi/src/babeltrace/include/babeltrace/ctfser-internal.h:418:3: note:
>> in expansion of macro ‘bt_bitfield_write_le’
>>    bt_bitfield_write_le(mmap_align_addr(ctfser->base_mma) +
>>    ^~~~~~~~~~~~~~~~~~~~
>> 
>> This boils down to the fact that the expression ~((uint8_t)0) has type
>> "signed int", which is used as an operand of the left shift.  This is due
>> to the integer promotion rules of C99 (6.3.3.1):
>> 
>>     If an int can represent all values of the original type, the value is
>>     converted to an int; otherwise, it is converted to an unsigned int.
>>     These are called the integer promotions. All other types are unchanged
>>     by the integer promotions.
>> 
>> We also need to cast the result explicitly into the left hand
>> side type to deal with:
>> 
>> warning: large integer implicitly truncated to unsigned type [-Woverflow]
>> 
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
>> ---
>> Changes since v1:
>> - Generate compile-time error if the type argument passed to
>>   _bt_unsigned_cast() is larger than sizeof(uint64_t), this
>>   allows removing _bt_check_max_64bit,
>> - Introduce _br_fill_mask, which replaces _bt_bitwise_not,
>> - Clarify _bt_unsigned_cast comment,
>> - Expand explanation of the issue within the patch commit message.
> 
> I didn't spot any mistake by inspecting the code, but I get some errors running
> tests/lib/test_bitfield, so I guess there are some :).  Does it pass on your
> side?

What compiler do you use, and which compilation flags ?
(it works here)

[...]
>>  
>>  /*
>>   * bt_bitfield_write - write integer to a bitfield in native endianness
>> @@ -108,15 +113,15 @@ do {									\
>>  									\
>>  	/* Trim v high bits */						\
>>  	if (__length < sizeof(__v) * CHAR_BIT)				\
>> -		__v &= ~((~(typeof(__v)) 0) << __length);		\
>> +		__v &= (typeof(__v)) ~(_bt_fill_mask(typeof(__v)) << __length); \
> 
> Not a blocker, but this operation is done enough times that it might be worth
> giving it a name and make a macro for it.  It could help make this big macro
> more readable, and we could test it on its own if needed.
> 
> /* Generate a mask of type `type` with the `length` least significant bits set.
> */
> 
> #define _bt_make_mask(type, length) \
>	((type) ~(_bt_fill_mask(type) << length))

Good point! Will do. Missing () around "length" though.

Thanks,

Mathieu

> 
> Simon

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com