include: sys: util_macro FIX BIT_MASK(32) #42226
Conversation
When generating BIT_MASK(32) on a arm 32 processor, the macro generate a warning: warning: left shift count >= width of type [-Wshift-count-overflow] 44 | #define BIT(n) (1UL << (n)) To generate a bit mask we set the n bit to 1 and then do minus -1. So we need an intermediate 33 bits variable to have a 32 bit mask. Signed-off-by: Julien Massot <julien.massot@iot.bzh>
julien-massot
requested review from
carlescufi,
galak,
MaureenHelm and
nashif
as code owners
|
fix #42163 |
| @@ -65,7 +65,7 @@ extern "C" { | |||
| * @brief Bit mask with bits 0 through <tt>n-1</tt> (inclusive) set, | |||
| * or 0 if @p n is 0. | |||
| */ | |||
| #define BIT_MASK(n) (BIT(n) - 1UL) | |||
| #define BIT_MASK(n) (BIT64(n) - 1UL) | |||
There was a problem hiding this comment.
This is going to produce an rvalue with a 64 bit type, which is very likely not what you want as it will widen the math of any expression it's placed in, likely produce extra instructions to compute, and potentially change the result vs. what we had before. You want to cast this back to "unsigned long"[1] to preserve compatibility with the existing API.
[1] And not to a fixed-side type! On 64 bit platforms obviously all the expanded constants are longs, and thus 64 bit.
There was a problem hiding this comment.
I completely agree, I do not think we should do this at all. Instead, please add a new BIT_MASK64() that uses BIT64
There was a problem hiding this comment.
I completely agree, I do not think we should do this at all. Instead, please add a new
BIT_MASK64()that usesBIT64
In fact, there is already BIT64_MASK right below that does just that:
zephyr/include/sys/util_macro.h
Line 74 in a92ca17
There was a problem hiding this comment.
Hi, @andyross @carlescufi @stephanosio
Not sure I was clear enough, what I'm trying to fix is this code sample
uint32_t mask = BIT_MASK(32); /* 0xffffffff */
Are you suggesting to use this instead ?
uint32_t mask = BIT64_MASK(32); /* 0xffffffff */
There was a problem hiding this comment.
This fix is a legitimate warning workaround for the 32 bit version of the macro. It just needs a cast on the final expression to prevent changing the API (effectively what the code as written does is, in fact, implement BIT64_MASK(), and that's not what you want.
There was a problem hiding this comment.
Ok thanks you all !
| @@ -65,7 +65,7 @@ extern "C" { | |||
| * @brief Bit mask with bits 0 through <tt>n-1</tt> (inclusive) set, | |||
| * or 0 if @p n is 0. | |||
| */ | |||
| #define BIT_MASK(n) (BIT(n) - 1UL) | |||
| #define BIT_MASK(n) (BIT64(n) - 1UL) | |||
There was a problem hiding this comment.
I completely agree, I do not think we should do this at all. Instead, please add a new BIT_MASK64() that uses BIT64
On 32bit compiler the BIT_MASK(32) generate a warning, after discussion on zephyrproject-rtos#42226 and zephyrproject-rtos#42163, advise was to use BIT64_MASK instead. Signed-off-by: Julien Massot <julien.massot@iot.bzh>
On 32bit compiler the BIT_MASK(32) generate a warning, after discussion on zephyrproject-rtos#42226 and zephyrproject-rtos#42163, advise was to use BIT64_MASK instead. Signed-off-by: Julien Massot <julien.massot@iot.bzh>
When using BIT_MASK(32) on a arm 32bits processor, the macro
generate a warning:
warning: left shift count >= width of type [-Wshift-count-overflow]
44 | #define BIT(n) (1UL << (n))
To generate a bit mask we set the n bit to 1 and then do minus -1.
So we need an intermediate 33 bits variable to have a 32 bit mask.
fixes #42163
Signed-off-by: Julien Massot julien.massot@iot.bzh