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

[Mathieu Desnoyers]

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 
---
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.
---
 include/babeltrace/bitfield-internal.h | 77 --
 1 file changed, 37 insertions(+), 40 deletions(-)

diff --git a/include/babeltrace/bitfield-internal.h 
b/include/babeltrace/bitfield-internal.h
index c5d5eccd..7eb36989 100644
--- a/include/babeltrace/bitfield-internal.h
+++ b/include/babeltrace/bitfield-internal.h
@@ -55,21 +55,26 @@
 #define _bt_is_signed_type(type)   ((type) -1 < (type) 0)
 
 /*
- * NOTE: The cast to (uint64_t) below ensures that we're not casting a
- * negative value, which is undefined in C. However, this limits the
- * maximum type size of `type` and `v` to 64-bit. The
- * _bt_check_max_64bit() is used to check that the users of this header
- * do not use types with a size greater than 64-bit.
+ * Cast value `v` to an unsigned integer type of the size of type `type`.
+ *
+ * The unsigned cast ensures that we're not shifting a negative value,
+ * which is undefined in C. However, this limits the maximum type size
+ * of `type` to 64-bit. Generate a compile-time error if the size of
+ * `type` is larger than 64-bit.
  */
 #define _bt_unsigned_cast(type, v) \
-({ \
-   (sizeof(v) < sizeof(type)) ?\
-   ((type) (v)) & ((type) (~(~(uint64_t) 0 << (sizeof(v) * 
CHAR_BIT : \
-   (type) (v); \
-})
+   (sizeof(type) == sizeof(uint8_t) ? (uint8_t) (v) :  \
+   sizeof(type) == sizeof(uint16_t) ? (uint16_t) (v) : \
+   sizeof(type) == sizeof(uint32_t) ? (uint32_t) (v) : \
+   sizeof(type) == sizeof(uint64_t) ? (uint64_t) (v) : \
+   sizeof(struct { int f:(sizeof(type) > sizeof(uint64_t) ? -1 : 1); }))
 
-#define _bt_check_max_64bit(type)  \
-   char _max_64bit_assertion[sizeof(type) <= sizeof(uint64_t) ? 1 : -1] 
__attribute__((unused))
+/*
+ * _bt_fill_mask evaluates to an unsigned integer with the size of
+ * "type" with all bits set. It is meant to be used as a left operand to
+ * the shift-left operator to create bit masks.
+ */
+#define _bt_fill_mask(type)_bt_unsigned_cast(type, ~(type) 0)
 
 /*
  * 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); 
\

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

[Mathieu Desnoyers]

- On May 7, 2019, at 6:13 PM, Simon Marchi sim...@simark.ca wrote:

> On 2019-05-07 4:40 p.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.

Hi Simon,

Thanks a ton for your input! I've taken care of all your comments and
will post an updated patch for babeltrace.

Once we all agree on the content, I'll work on porting this to
lttng-ust, lttng-modules, and barectf.

Let's keep in mind that bitfield.h needs to stay ANSI-C compatible
as much as possible so the barectf implementation don't diverge too
much.

Thanks,

Mathieu


> 
> Please add a reference to the original problem that prompted you to
> do this change in the commit message.  For example:
> 
> --- 8< ---
> 
> 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.
> 
> --- >8 ---
> 
>> 
>> 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 
>> ---
>>  include/babeltrace/bitfield-internal.h | 61 
>> ++
>>  1 file changed, 33 insertions(+), 28 deletions(-)
>> 
>> diff --git a/include/babeltrace/bitfield-internal.h
>> b/include/babeltrace/bitfield-internal.h
>> index c5d5eccd..da56f089 100644
>> --- a/include/babeltrace/bitfield-internal.h
>> +++ b/include/babeltrace/bitfield-internal.h
>> @@ -55,19 +55,24 @@
>>  #define _bt_is_signed_type(type)((type) -1 < (type) 0)
>>  
>>  /*
>> - * NOTE: The cast to (uint64_t) below ensures that we're not casting a
>> - * negative value, which is undefined in C. However, this limits the
>> - * maximum type size of `type` and `v` to 64-bit. The
>> - * _bt_check_max_64bit() is used to check that the users of this header
>> - * do not use types with a size greater than 64-bit.
>> + * NOTE: The unsigned cast ensures that we're not shifting a negative
>> + * value, which is undefined in C. However, this limits the maximum
>> + * type size of `type` and `v` to 64-bit. The _bt_check_max_64bit() is
>> + * used to check that the users of this header do not use types with a
>> + * size greater than 64-bit.
>>   */
> 
> The comment explains "why" this macro is needed, but doesn't explain what
> it does.  I would suggest starting the comment with:
> 
> Cast value `v` to an unsigned integer type of the size of type `type`.
> 
>>
>>  #define _bt_unsigned_cast(type, v)  \
>>  ({  \
>> -(sizeof(v) < sizeof(type)) ?\
>> -((type) (v)) & ((type) (~(~(uint64_t) 0 << (sizeof(v) * 
>> CHAR_BIT : \
>> -(type) (v); \
>> +__builtin_types_compatible_p(type, int8_t) ? (uint8_t) (v) :\
>> +__builtin_types_compatible_p(type, int16_t) ? (uint16_t) (v) :  \
>> +__builtin_types_compatible_p(type, int32_t) ? (uint32_t) (v) :  \
>> +__builtin_types_compatible_p(type, int64_t) ? (uint64_t) (v) :  \
>> +(type) (v); \
>>  })
> 
> What should we do in the "else" case.  The macro claims to cast to an unsigned
> type, but in the case where the "else" branch would be taken, the value is 
> cast
> to the original type (possible signed).  I think this can be very misleading.
> 
> Knowing that all of this is supposed to get resolved at compiled time, wou

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

[Simon Marchi]

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

> ---
>  include/babeltrace/bitfield-internal.h | 77 
> --
>  1 file changed, 37 insertions(+), 40 deletions(-)
> 
> diff --git a/include/babeltrace/bitfield-internal.h 
> b/include/babeltrace/bitfield-internal.h
> index c5d5eccd..7eb36989 100644
> --- a/include/babeltrace/bitfield-internal.h
> +++ b/include/babeltrace/bitfield-internal.h
> @@ -55,21 +55,26 @@
>  #define _bt_is_signed_type(type) ((type) -1 < (type) 0)
>  
>  /*
> - * NOTE: The cast to (uint64_t) below ensures that we're not casting a
> - * negative value, which is undefined in C. However, this limits the
> - * maximum type size of `type` and `v` to 64-bit. The
> - * _bt_check_max_64bit() is used to check that the users of this header
> - * do not use types with a size greater than 64-bit.
> + * Cast value `v` to an unsigned integer type of the size of type `type`.
> + *
> + * The unsigned cast ensures that we're not shifting a negative value,
> + * which is undefined in C. However, this limits the maximum type size
> + * of `type` to 64-bit. Generate a compile-time error if the size of
> + * `type` is larger than 64-bit.
>   */
>  #define _bt_unsigned_cast(type, v)   \
> -({   \
> - (sizeof(v) < sizeof(type)) ?\
> - ((type) (v)) & ((type) (~(~(uint64_t) 0 << (sizeof(v) * 
> CHAR_BIT : \
> - (type) (v); \
> -})
> + (sizeof(type) == sizeof(uint8_t) ? (uint8_t) (v) :  \
> + sizeof(type) == sizeof(uint16_t) ? (uint16_t) (v) : \
> + sizeof(type) == sizeof(uint32_t) ? (uint32_t) (v) : \
> + sizeof(type) == sizeof(uint64_t) ? (uint64_t) (v) : \
> + sizeof(struct { int f:(sizeof(type) > sizeof(uint64_t) ? -1 : 1); }))
>  
> -#define _bt_check_max_64bit(type)\
> - char _max_64bit_assertion[sizeof(type) <= sizeof(uint64_t) ? 1 : -1] 
> __attribute__((unused))
> +/*
> + * _bt_fill_mask evaluates to an unsigned integer with the size of
> + * "type" with all bits set. It is meant to be used as a left operand to
> + * the shift-left operator to create bit masks.
> + */
> +#define _bt_fill_mask(type)  _bt_unsigned_cast(type, ~(type) 0)

Thanks for the comment, it makes what this macro does very clear.

>  
>  /*
>   * bt_bitfield_write - write integer to a bitfield in native endianness
> @@ -108,15 +113,15 @@ do {

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

[Mathieu Desnoyers]

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

Re: [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 FFC7
# 0xFFC7 0xFFEB 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

___
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Mathieu Desnoyers]

- On May 8, 2019, at 12:08 PM, Simon Marchi simon.mar...@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 FFC7
> # 0xFFC7 0xFFEB 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
___
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [lttng-dev] default sub-buf size and count

[Jonathan Rajotte-Julien]

Hi,

On Tue, May 07, 2019 at 05:08:33PM -0400, Mosleh Uddin wrote:
> Hello,
> 
> I was just wondering what the default values for the sub-buffer size and
> the number of sub-buffers are. I attempted to print them out after setting
> the channel with default attr, however both values appear 0.

This information is normally found in the man pages.

man lttng enable-channel, search for --subbuf-size=SIZE

Could you outline the session configuration used so we can see if the fact that
both value are zero is a bug?

Cheers

> 
> Thanks for any insight,
> Mosleh

> ___
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev


-- 
Jonathan Rajotte-Julien
EfficiOS
___
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Mathieu Desnoyers]

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

Changes since v2:
- Fix unwanted sign extension when generating masks,
- Introduce macro helpers to clarify code:
  - _bt_cast_value_to_unsigned()
  - _bt_cast_value_to_unsigned_type(),
  - _bt_make_mask_complement(),
  - _bt_make_mask().
---
 include/babeltrace/bitfield-internal.h | 128 -
 1 file changed, 77 insertions(+), 51 deletions(-)

diff --git a/include/babeltrace/bitfield-internal.h 
b/include/babeltrace/bitfield-internal.h
index c5d5eccd..27e0fdfd 100644
--- a/include/babeltrace/bitfield-internal.h
+++ b/include/babeltrace/bitfield-internal.h
@@ -55,21 +55,55 @@
 #define _bt_is_signed_type(type)   ((type) -1 < (type) 0)
 
 /*
- * NOTE: The cast to (uint64_t) below ensures that we're not casting a
- * negative value, which is undefined in C. However, this limits the
- * maximum type size of `type` and `v` to 64-bit. The
- * _bt_check_max_64bit() is used to check that the users of this header
- * do not use types with a size greater than 64-bit.
+ * Cast value `v` to an unsigned integer of the same size as `v`.
  */
-#define _bt_unsigned_cast(type, v) \
-({ \
-   (sizeof(v) < sizeof(type)) ?\
-   ((type) (v)) & ((type) (~(~(uint64_t) 0 << (sizeof(v) * 
CHAR_BIT : \
-   (type) (v); \
-})
+#define _bt_cast_value_to_unsigned(v)  \
+   (sizeof(v) == sizeof(uint8_t) ? (uint8_t) (v) : \
+   sizeof(v) == sizeof(uint16_t) ? (uint16_t) (v) :\
+   sizeof(v) == sizeof(uint32_t) ? (uint32_t) (v) :\
+   sizeof(v) == sizeof(uint64_t) ? (uint64_t) (v) :\
+   sizeof(struct { int f:(sizeof(v) > sizeof(uint64_t) ? -1 : 1); }))
+
+/*
+ * Cast value `v` to an unsigned integer type of the size of type `type`
+ * *without* sign-extension.
+ *
+ * The unsigned cast ensures that we're not shifting a negative value,
+ * which is undefined in C. However, this limits the maximum type size
+ * of `type` to 64-bit. Generate a compile-time error if the size of
+ * `type` is larger than 64-bit.
+ */
+#define _bt_cast_value_to_unsigned_type(type, v)   \
+   (sizeof(type) == sizeof(uint8_t) ?  \
+   (uint8_t) _bt_cast_value_to_unsigned(v) :   \
+   sizeof(type) == sizeof(uint16_t) ?  \
+   (uint16_t) _bt_cast_value_to_unsigned(v) :  \
+   sizeof(type) == sizeof(uint32_t) ?  \
+   (uint32_t) _bt_cast_value_to_unsigned(v) :  \
+   sizeof(type) == sizeof(uint64_t) ?  \
+