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

* Re: [PATCH babeltrace] Fix: bitfield: left shift undefined behavior
       [not found] <20190507204039.10496-1-mathieu.desnoyers@efficios.com>
@ 2019-05-07 22:13 ` Simon Marchi
       [not found] ` <af860b96-0b30-0e7e-3668-53f0e76297c3@simark.ca>
  1 sibling, 0 replies; 3+ messages in thread
From: Simon Marchi @ 2019-05-07 22:13 UTC (permalink / raw)
  To: Mathieu Desnoyers, jgalar, joraj; +Cc: lttng-dev

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.

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 <mathieu.desnoyers@efficios.com>
> ---
>  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, would
there be a way to generate compilation error in the "else" branch?  If we ever
reach it, it means we need to add something here.

>  
> +/* Unsigned bitwise complement. */
> +#define _bt_bitwise_not(type, v)	_bt_unsigned_cast(type, ~(type) v)

I was trying to put words on what this macro does, to document it a bit better,
but then realized it iss always used with v = 0.  I don't know if you want to
simplify and remove the v parameter to hard-code 0?  In the end, all you want is
something that yields a value of the same size as type, but unsigned, and filled
with ones.

Since you said that you would change the code, I'll wait for the next version to
look at the hairy details.

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

^ permalink raw reply	[flat|nested] 3+ messages in thread