[lldb] [ObjC runtime] Don't cast to signed when left shifting #86605
Conversation
This is fixing a report from ubsan which I don't think is super high value, but our testsuite hits it on TestDataFormatterObjCNSContainer.py so I'd like to work around it. We are getting ``` runtime error: left shift of negative value -8827055269646171913 3159 int64_t data_payload_signed = 3160 ((int64_t)((int64_t)unobfuscated -> 3161 << m_objc_debug_taggedpointer_ext_payload_lshift) >> 3162 m_objc_debug_taggedpointer_ext_payload_rshift); ``` At this point `unobfuscated` is 0x85800000000000f7 and `m_objc_debug_taggedpointer_ext_payload_lshift` is 9, so `(int64_t)0x85800000000000f7<<9` shifts off the "sign" bit and then some zeroes etc, and that's how we get this error. We're only trying to extract some bits in the middle of the doubleword, so the fact that we're "losing" the sign is not a bug. Change the inner cast to (uint64_t).
|
@llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) ChangesThis is fixing a report from ubsan which I don't think is super high value, but our testsuite hits it on At this point We're only trying to extract some bits in the middle of the doubleword, so the fact that we're "losing" the sign is not a bug. Change the inner cast to (uint64_t). Full diff: https://github.com/llvm/llvm-project/pull/86605.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
index 3e5ee6f6637303..d3fc487aed4333 100644
--- a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -3154,7 +3154,7 @@ AppleObjCRuntimeV2::TaggedPointerVendorExtended::GetClassDescriptor(
<< m_objc_debug_taggedpointer_ext_payload_lshift) >>
m_objc_debug_taggedpointer_ext_payload_rshift);
int64_t data_payload_signed =
- ((int64_t)((int64_t)unobfuscated
+ ((int64_t)((uint64_t)unobfuscated
<< m_objc_debug_taggedpointer_ext_payload_lshift) >>
m_objc_debug_taggedpointer_ext_payload_rshift);
|
|
I've seen this ubsan error every time I run the testsuite for years, i just finally sat down and figured out what was going on. |
| @@ -3154,7 +3154,7 @@ AppleObjCRuntimeV2::TaggedPointerVendorExtended::GetClassDescriptor( | |||
| << m_objc_debug_taggedpointer_ext_payload_lshift) >> | |||
| m_objc_debug_taggedpointer_ext_payload_rshift); | |||
| int64_t data_payload_signed = | |||
| ((int64_t)((int64_t)unobfuscated | |||
| ((int64_t)((uint64_t)unobfuscated | |||
| << m_objc_debug_taggedpointer_ext_payload_lshift) >> | |||
There was a problem hiding this comment.
Since the goal is to extract some bits from the middle of this 64-bit value, is there a way we could produce a mask instead of shifting left and then shifting right?
There was a problem hiding this comment.
We could just shift right and then mask.
There was a problem hiding this comment.
I suspect the runtime gave us these "left & right shift" values, and the person who wrote this used the obvious implementation. It could be expressed as a bit slice with a little subtraction, true. Just to be clear, we're not fixing a real bug here, we were saying a UInt64 was signed and then ubsan got all shirty when we shifted away some bits that would indicate sign.
There was a problem hiding this comment.
Probably the one interesting thing is that the value we're slicing out is, apparently, signed, so we want it to sign extend to Int64 when it is done. That would take a little more care with a bit slice approach.
There was a problem hiding this comment.
e.g.
(lldb) p (-5LL <<60) >> 60
(long long) -5
if the bit slice we're pulling out has its high bit set, that'll shfit down and be sign extended.
There was a problem hiding this comment.
Makes sense to me. I think this is fine then.
This is fixing a report from ubsan which I don't think is super high value, but our testsuite hits it on
TestDataFormatterObjCNSContainer.py so I'd like to work around it. We are getting
At this point
unobfuscatedis 0x85800000000000f7 andm_objc_debug_taggedpointer_ext_payload_lshiftis 9, so(int64_t)0x85800000000000f7<<9shifts off the "sign" bit and then some zeroes etc, and that's how we get this error.We're only trying to extract some bits in the middle of the doubleword, so the fact that we're "losing" the sign is not a bug. Change the inner cast to (uint64_t).