pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r992642956
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor
*type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
I'm a bit confused with this change. The signed vs unsigned shift has
essential difference in populating a sign bit, so `0xffffffff << 1 >> 1` (for
32 bits) will be `0x7fffffff` for unsigned and `0xffffffff` for signed. What
`get_signed_val` should return in case if `is_inline_int(type)` is evaluated to
`true`? For 64 bits target it is obvious since `uintptr_t` has the same size as
`int64_t` so `return (int64_t)(ulong_val << extra_bits >> extra_bits);` is
equivalent to `*(FAR int64_t *)val`, but if the value is `int32_t` then this
change will break the logic because sign bit will not be populated and
`int32_t` `-1` will become `4294967295` `int64_t` that seems to be completely
wrong.
Maybe it is better to keep code here as is and change `is_inline_int` to
`return bits < inline_bits;` and not equal, so equal condition will be
evaluated by `return *(FAR int64_t *)val;` in `get_signed_val`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
