Page MenuHomePhabricator

[lldb] Fix type conversion in the Scalar getters
ClosedPublic

Authored by labath on Jun 29 2020, 8:01 AM.

Details

Summary

The Scalar class claims to follow the C type conversion rules. This is
true for the Promote function, but it is not true for the implicit
conversions done in the getter methods.

These functions had a subtle bug: when extending the type, they used the
signedness of the *target* type in order to determine whether to do
sign-extension or zero-extension. This is not how things work in C,
which uses the signedness of the *source* type. I.e., C does
(sign-)extension before it does signed->unsigned conversion, and not the
other way around.

This means that: (unsigned long)(int)-1

is equal to (unsigned long)0xffffffffffffffff
and not (unsigned long)0x00000000ffffffff

Unsurprisingly, we have accumulated code which depended on this
inconsistent behavior. It mainly manifested itself as code calling
"ULongLong/SLongLong" as a way to get the value of the Scalar object in
a primitive type that is "large enough". Previously, the ULongLong
conversion did not do sign-extension, but now it does.

This patch makes the Scalar getters consistent with the declared
semantics, and fixes the couple of call sites that were using it
incorrectly.

Diff Detail

Event Timeline

labath created this revision.Jun 29 2020, 8:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 8:01 AM

+ Ismail as he looked at this recently.

Another possibility would be to rename [US]LongLong to something like Get[SZ]ExtInteger (similar to APInt::get[SZ]ExtValue) to make it clear what it does, and ditch the other getter functions. I've mean meaning to move this class away from using host types at some point (it's not appropriate or useful for most of our uses), but I've thought it would be good to clean it up first. Now I think it may be better to do it the other way around....

mib added a comment.Jun 30 2020, 2:28 AM

Hey Pavel! Thanks for taking care of this. I started looking at it no long ago but I got sidetracked working on other stuff.
I don't see anything wrong with the patch, looks good to me.

Thanks for looking at this. Could you also tell me what you think of D82864? That's sort of a more safe/NFC-ish alternative to this patch, as it doesn't change behavior -- it just makes the current behavior more obvious. (It's technically possible for the removal of the [US]Short and co. accessors to change behavior, but that would require a pretty complex set of conditions).

teemperor accepted this revision.Jun 30 2020, 5:00 AM

Beside some minor things this LGTM.

lldb/source/Utility/Scalar.cpp
864

Seems unrelated to the patch? Also it would be inconsistent that this is removed here and not below. FWIW this is used to suppress -Wdouble-promotion warnings, so it does have a purpose.

lldb/unittests/Utility/ScalarTest.cpp
97

If you change this to CheckConversion<int>(0x87654321); then that's one C-style cast less (which will make me very happy) and if someone accidentally makes this a long literal or so we get a compiler warning (instead of the compiler just silently truncating the thing).

Also I guess short and char is missing?

This revision is now accepted and ready to land.Jun 30 2020, 5:00 AM
JDevlieghere accepted this revision.Jun 30 2020, 9:30 AM

LGTM with Raphael's and clang-format's comments addressed :-)

labath marked 3 inline comments as done.Jul 2 2020, 9:01 AM
labath added inline comments.
lldb/source/Utility/Scalar.cpp
864

Yeah, I think this ended up being copied from the function above it.

lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py
66

FTR, this was working correctly before already. I'm just creating a more explicit test for it.

lldb/unittests/Utility/ScalarTest.cpp
97

Changed to (T) to <T>. short and char aren't interesting to test because Scalar does not have first-class support for them. They would get promoted to int before they reach the Scalar constructor. Long double is also missing -- that's because the relevant constructor is currently a booby-trap.

This revision was automatically updated to reflect the committed changes.