[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.

[GitBox <gi...@apache.org>]

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-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org