diff mbox series

[barectf,1/2] Fix: bitfield: shiftundefined/implementation defined behaviors (v3)

Message ID 20190517143537.13182-1-mathieu.desnoyers@efficios.com
State New
Headers
Series [barectf,1/2] Fix: bitfield: shiftundefined/implementation defined behaviors (v3) |

Commit Message

Mathieu Desnoyers May 17, 2019, 2:35 p.m. UTC 
  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]

The C99 standard states that a right shift has implementation-defined
behavior when shifting a signed negative value. Add a preprocessor check
that the compiler provides the expected behavior, else provide an
alternative implementation which guarantees the intended behavior.

A preprocessor check is also added to ensure that the compiler
representation for signed values is two's complement, which is expected
by this header.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
---
Changes since v1:
- Fix wrong use of _$prefix$bt_make_mask() in _$prefix$bt_bitfield_write_be.
  Should be _$prefix$bt_make_mask_complement().

Changes since v2:
- Documentation and style updates.
---
 barectf/templates.py | 193 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 162 insertions(+), 31 deletions(-)
diff mbox series

Patch

diff --git a/barectf/templates.py b/barectf/templates.py
index eec677b..6444da4 100644
--- a/barectf/templates.py
+++ b/barectf/templates.py
@@ -596,11 +596,9 @@  _BITFIELD = '''#ifndef _$PREFIX$BITFIELD_H
 #define _$PREFIX$BITFIELD_H
 
 /*
- * BabelTrace
- *
  * Bitfields read/write functions.
  *
- * Copyright 2010 - Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
+ * Copyright 2010-2019 - Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -632,16 +630,149 @@  _BITFIELD = '''#ifndef _$PREFIX$BITFIELD_H
 
 #define $PREFIX$BYTE_ORDER $ENDIAN_DEF$
 
-/* We can't shift a int from 32 bit, >> 32 and << 32 on int is undefined */
-#define _$prefix$bt_piecewise_rshift(_vtype, _v, _shift) \\
+/*
+ * This bitfield header requires the compiler representation of signed
+ * integers to be two's complement.
+ */
+#if (-1 != ~0)
+#error "bitfield.h requires the compiler representation of signed integers to be two's complement."
+#endif
+
+/*
+ * Produce a build-time error if the condition `cond` is non-zero.
+ * Evaluates as a size_t expression.
+ */
+#define _$prefix$BT_BUILD_ASSERT(cond)					\\
+	sizeof(struct { int f:(2 * !!(cond) - 1); })
+
+/*
+ * Cast value `v` to an unsigned integer of the same size as `v`.
+ */
+#define _$prefix$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) :		\\
+	_$prefix$BT_BUILD_ASSERT(sizeof(v) <= sizeof(uint64_t)))
+
+/*
+ * 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 _$prefix$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) ?				\\
+		(uint64_t) _bt_cast_value_to_unsigned(v) :		\\
+	_$prefix$BT_BUILD_ASSERT(sizeof(v) <= sizeof(uint64_t)))
+
+/*
+ * _$prefix$bt_fill_mask evaluates to a "type" integer with all bits set.
+ */
+#define _$prefix$bt_fill_mask(type)	((type) ~(type) 0)
+
+/*
+ * Left shift a value `v` of `shift` bits.
+ *
+ * The type of `v` can be signed or unsigned integer.
+ * The value of `shift` must be less than the size of `v` (in bits),
+ * otherwise the behavior is undefined.
+ * Evaluates to the result of the shift operation.
+ *
+ * According to the C99 standard, left shift of a left hand-side signed
+ * type is undefined if it has a negative value or if the result cannot
+ * be represented in the result type. This bitfield header discards the
+ * bits that are left-shifted beyond the result type representation,
+ * which is the behavior of an unsigned type left shift operation.
+ * Therefore, always perform left shift on an unsigned type.
+ *
+ * This macro should not be used if `shift` can be greater or equal than
+ * the bitwidth of `v`. See `_bt_safe_lshift`.
+ */
+#define _$prefix$bt_lshift(_vtype, v, shift)				\\
+	((_vtype) (_$prefix$bt_cast_value_to_unsigned(v) << (shift)))
+
+/*
+ * Generate a mask of type `type` with the `length` least significant bits
+ * cleared, and the most significant bits set.
+ */
+#define _$prefix$bt_make_mask_complement(type, length)			\\
+	_$prefix$bt_lshift(type, _$prefix$bt_fill_mask(type), length)
+
+/*
+ * Generate a mask of type `type` with the `length` least significant bits
+ * set, and the most significant bits cleared.
+ */
+#define _$prefix$bt_make_mask(type, length)				\\
+	((type) ~_$prefix$bt_make_mask_complement(type, length))
+
+/*
+ * Right shift a value `v` of `shift` bits.
+ *
+ * The type of `v` can be signed or unsigned integer.
+ * The value of `shift` must be less than the size of `v` (in bits),
+ * otherwise the behavior is undefined.
+ * Evaluates to the result of the shift operation.
+ *
+ * According to the C99 standard, right shift of a left hand-side signed
+ * type which has a negative value is implementation defined. This
+ * bitfield header relies on the right shift implementation carrying the
+ * sign bit. If the compiler implementation has a different behavior,
+ * emulate carrying the sign bit.
+ *
+ * This macro should not be used if `shift` can be greater or equal than
+ * the bitwidth of `v`. See `_bt_safe_rshift`.
+ */
+#if ((-1 >> 1) == -1)
+#define _$prefix$bt_rshift(_vtype, v, shift)	((v) >> (shift))
+#else
+#define _$prefix$bt_rshift(_vtype, v, shift)				\\
+	((_vtype) ((_$prefix$bt_cast_value_to_unsigned(v) >> (shift)) |	\\
+		((v) < 0 ? _$prefix$bt_make_mask_complement(_vtype,	\\
+			sizeof(v) * CHAR_BIT - (shift)) : 0)))
+#endif
+
+/*
+ * Right shift a signed or unsigned integer with `shift` value being an
+ * arbitrary number of bits. `v` is modified by this macro. The shift
+ * is transformed into a sequence of `_nr_partial_shifts` consecutive
+ * shift operations, each of a number of bits smaller than the bitwidth
+ * of `v`, ending with a shift of the number of left over bits.
+ */
+#define _$prefix$bt_safe_rshift(vtype, v, shift)			\\
+do {									\\
+	unsigned long _nr_partial_shifts = (shift) / (sizeof(v) * CHAR_BIT - 1); \\
+	unsigned long _leftover_bits = (shift) % (sizeof(v) * CHAR_BIT - 1); \\
+									\\
+	for (; _nr_partial_shifts; _nr_partial_shifts--)		\\
+		(v) = _$prefix$bt_rshift(vtype, v, sizeof(v) * CHAR_BIT - 1); \\
+	(v) = _$prefix$bt_rshift(vtype, v, _leftover_bits);		\\
+} while (0)
+
+/*
+ * Left shift a signed or unsigned integer with `shift` value being an
+ * arbitrary number of bits. `v` is modified by this macro. The shift
+ * is transformed into a sequence of `_nr_partial_shifts` consecutive
+ * shift operations, each of a number of bits smaller than the bitwidth
+ * of `v`, ending with a shift of the number of left over bits.
+ */
+#define _$prefix$bt_safe_lshift(vtype, v, shift)			\\
 do {									\\
-	unsigned long ___shift = (_shift);				\\
-	unsigned long sb = (___shift) / (sizeof(_v) * CHAR_BIT - 1);	\\
-	unsigned long final = (___shift) % (sizeof(_v) * CHAR_BIT - 1); \\
+	unsigned long _nr_partial_shifts = (shift) / (sizeof(v) * CHAR_BIT - 1); \\
+	unsigned long _leftover_bits = (shift) % (sizeof(v) * CHAR_BIT - 1); \\
 									\\
-	for (; sb; sb--)						\\
-		_v >>= sizeof(_v) * CHAR_BIT - 1;			\\
-	_v >>= final;							\\
+	for (; _nr_partial_shifts; _nr_partial_shifts--)		\\
+		(v) = _$prefix$bt_lshift(vtype, v, sizeof(v) * CHAR_BIT - 1); \\
+	(v) = _$prefix$bt_lshift(vtype, v, _leftover_bits);		\\
 } while (0)
 
 /*
@@ -664,7 +795,7 @@  do {									\\
 
 #define _$prefix$bt_bitfield_write_le(_ptr, type, _start, _length, _vtype, _v) \\
 do {									\\
-	_vtype __v = (_v);					\\
+	_vtype __v = (_v);						\\
 	type *__ptr = CAST_PTR(type *, _ptr);				\\
 	unsigned long __start = (_start), __length = (_length);		\\
 	type mask, cmask;						\\
@@ -681,15 +812,15 @@  do {									\\
 									\\
 	/* Trim v high bits */						\\
 	if (__length < sizeof(__v) * CHAR_BIT)				\\
-		__v &= ~((~(_vtype) 0) << __length);		\\
+		__v &= _$prefix$bt_make_mask(_vtype, __length);		\\
 									\\
 	/* We can now append v with a simple "or", shift it piece-wise */ \\
 	this_unit = start_unit;						\\
 	if (start_unit == end_unit - 1) {				\\
-		mask = ~((~(type) 0) << (__start % ts));		\\
+		mask = _$prefix$bt_make_mask(type, __start % ts);	\\
 		if (end % ts)						\\
-			mask |= (~(type) 0) << (end % ts);		\\
-		cmask = (type) __v << (__start % ts);			\\
+			mask |= _$prefix$bt_make_mask_complement(type, end % ts); \\
+		cmask = _$prefix$bt_lshift(type, (type) (__v), __start % ts); \\
 		cmask &= ~mask;						\\
 		__ptr[this_unit] &= mask;				\\
 		__ptr[this_unit] |= cmask;				\\
@@ -697,22 +828,22 @@  do {									\\
 	}								\\
 	if (__start % ts) {						\\
 		cshift = __start % ts;					\\
-		mask = ~((~(type) 0) << cshift);			\\
-		cmask = (type) __v << cshift;				\\
+		mask = _$prefix$bt_make_mask(type, cshift);		\\
+		cmask = _$prefix$bt_lshift(type, (type) (__v), cshift);	\\
 		cmask &= ~mask;						\\
 		__ptr[this_unit] &= mask;				\\
 		__ptr[this_unit] |= cmask;				\\
-		_$prefix$bt_piecewise_rshift(_vtype, __v, ts - cshift); \\
+		_$prefix$bt_safe_rshift(_vtype, __v, ts - cshift); \\
 		__start += ts - cshift;					\\
 		this_unit++;						\\
 	}								\\
 	for (; this_unit < end_unit - 1; this_unit++) {			\\
 		__ptr[this_unit] = (type) __v;				\\
-		_$prefix$bt_piecewise_rshift(_vtype, __v, ts); 		\\
+		_$prefix$bt_safe_rshift(_vtype, __v, ts); 		\\
 		__start += ts;						\\
 	}								\\
 	if (end % ts) {							\\
-		mask = (~(type) 0) << (end % ts);			\\
+		mask = _$prefix$bt_make_mask_complement(type, end % ts); \\
 		cmask = (type) __v;					\\
 		cmask &= ~mask;						\\
 		__ptr[this_unit] &= mask;				\\
@@ -723,7 +854,7 @@  do {									\\
 
 #define _$prefix$bt_bitfield_write_be(_ptr, type, _start, _length, _vtype, _v) \\
 do {									\\
-	_vtype __v = (_v);					\\
+	_vtype __v = (_v);						\\
 	type *__ptr = CAST_PTR(type *, _ptr);				\\
 	unsigned long __start = (_start), __length = (_length);		\\
 	type mask, cmask;						\\
@@ -740,15 +871,15 @@  do {									\\
 									\\
 	/* Trim v high bits */						\\
 	if (__length < sizeof(__v) * CHAR_BIT)				\\
-		__v &= ~((~(_vtype) 0) << __length);			\\
+		__v &= _$prefix$bt_make_mask(_vtype, __length);		\\
 									\\
 	/* We can now append v with a simple "or", shift it piece-wise */ \\
 	this_unit = end_unit - 1;					\\
 	if (start_unit == end_unit - 1) {				\\
-		mask = ~((~(type) 0) << ((ts - (end % ts)) % ts));	\\
+		mask = _$prefix$bt_make_mask(type, (ts - (end % ts)) % ts); \\
 		if (__start % ts)					\\
-			mask |= (~((type) 0)) << (ts - (__start % ts));	\\
-		cmask = (type) __v << ((ts - (end % ts)) % ts);		\\
+			mask |= _$prefix$bt_make_mask_complement(type, ts - (__start % ts)); \\
+		cmask = _$prefix$bt_lshift(type, (type) (__v), (ts - (end % ts)) % ts); \\
 		cmask &= ~mask;						\\
 		__ptr[this_unit] &= mask;				\\
 		__ptr[this_unit] |= cmask;				\\
@@ -756,22 +887,22 @@  do {									\\
 	}								\\
 	if (end % ts) {							\\
 		cshift = end % ts;					\\
-		mask = ~((~(type) 0) << (ts - cshift));			\\
-		cmask = (type) __v << (ts - cshift);			\\
+		mask = _$prefix$bt_make_mask(type, ts - cshift);	\\
+		cmask = _$prefix$bt_lshift(type, (type) (__v), ts - cshift); \\
 		cmask &= ~mask;						\\
 		__ptr[this_unit] &= mask;				\\
 		__ptr[this_unit] |= cmask;				\\
-		_$prefix$bt_piecewise_rshift(_vtype, __v, cshift); \\
+		_$prefix$bt_safe_rshift(_vtype, __v, cshift); \\
 		end -= cshift;						\\
 		this_unit--;						\\
 	}								\\
 	for (; (long) this_unit >= (long) start_unit + 1; this_unit--) { \\
 		__ptr[this_unit] = (type) __v;				\\
-		_$prefix$bt_piecewise_rshift(_vtype, __v, ts); \\
+		_$prefix$bt_safe_rshift(_vtype, __v, ts); \\
 		end -= ts;						\\
 	}								\\
 	if (__start % ts) {						\\
-		mask = (~(type) 0) << (ts - (__start % ts));		\\
+		mask = _$prefix$bt_make_mask_complement(type, ts - (__start % ts)); \\
 		cmask = (type) __v;					\\
 		cmask &= ~mask;						\\
 		__ptr[this_unit] &= mask;				\\