----- On May 8, 2019, at 11:49 AM, Simon Marchi sim...@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.desnoy...@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
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
