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

[Mathieu Desnoyers]

----- On May 8, 2019, at 12:08 PM, Simon Marchi simon.marchi at efficios.com wrote:

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

it fails here too... the executable test-bitfield was renamed to test_bitfield
and I had an old tree with the prior executable. Will investigate.

Thanks,

Mathieu

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

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