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

[Simon Marchi]

On 2019-05-08 11:59 a.m., Mathieu Desnoyers wrote:
> What compiler do you use, and which compilation flags ?
> (it works here)

"gcc (Ubuntu 7.4.0-1ubuntu1~18.04) 7.4.0", which is the system compiler on Ubuntu 18.04.

I just built with

  ./configure 'CFLAGS=-g3 -O0 -fsanitize=address -Wall'

and got some failures, the first one being

not ok 8 - Writing and reading back 0xC07EBC7, signed
#     Failed test (test_bitfield.c:run_test_signed() at line 224)
# Failed reading value written "signed char"-wise, with start=0 and length=29. Read FFFFFFFFFFFFFFC7
# 0xFFFFFFC7 0xFFFFFFEB 0x7 0xC 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0 0x0

The test passes without the patch.  If you can't reproduce it easily, I can dig
on my side.

>>>  
>>>  /*
>>>   * 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.

Right, and I should have mentioned I didn't actually test that macro, so handle with care!

Simon