-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[lldb] Fix TypeSystemClang::GetBasicTypeEnumeration for 128-bit int types #162278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[lldb] Fix TypeSystemClang::GetBasicTypeEnumeration for 128-bit int types #162278
Conversation
031ed28
to
3d32637
Compare
@llvm/pr-subscribers-lldb Author: Matej Košík (sedymrak) ChangesWhen we take the following C program:
and create a statically-linked executable from it:
Then we can observe the following
The value return by the The proposed changes make the When the above change is applied, the behavior of the
Full diff: https://github.com/llvm/llvm-project/pull/162278.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 21c265ede0bc5..1a574c97d9e46 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -849,8 +849,8 @@ lldb::BasicType TypeSystemClang::GetBasicTypeEnumeration(llvm::StringRef name) {
{"unsigned long long int", eBasicTypeUnsignedLongLong},
// "int128"
- {"__int128_t", eBasicTypeInt128},
- {"__uint128_t", eBasicTypeUnsignedInt128},
+ {"__int128", eBasicTypeInt128},
+ {"unsigned __int128", eBasicTypeUnsignedInt128},
// "bool"
{"bool", eBasicTypeBool},
diff --git a/lldb/unittests/Symbol/TestTypeSystemClang.cpp b/lldb/unittests/Symbol/TestTypeSystemClang.cpp
index f673cceae00dd..df507739d5c73 100644
--- a/lldb/unittests/Symbol/TestTypeSystemClang.cpp
+++ b/lldb/unittests/Symbol/TestTypeSystemClang.cpp
@@ -159,9 +159,9 @@ TEST_F(TestTypeSystemClang, TestGetBasicTypeFromName) {
GetBasicQualType("unsigned long long"));
EXPECT_EQ(GetBasicQualType(eBasicTypeUnsignedLongLong),
GetBasicQualType("unsigned long long int"));
- EXPECT_EQ(GetBasicQualType(eBasicTypeInt128), GetBasicQualType("__int128_t"));
+ EXPECT_EQ(GetBasicQualType(eBasicTypeInt128), GetBasicQualType("__int128"));
EXPECT_EQ(GetBasicQualType(eBasicTypeUnsignedInt128),
- GetBasicQualType("__uint128_t"));
+ GetBasicQualType("unsigned __uint128"));
EXPECT_EQ(GetBasicQualType(eBasicTypeVoid), GetBasicQualType("void"));
EXPECT_EQ(GetBasicQualType(eBasicTypeBool), GetBasicQualType("bool"));
EXPECT_EQ(GetBasicQualType(eBasicTypeFloat), GetBasicQualType("float"));
|
EXPECT_EQ(GetBasicQualType(eBasicTypeInt128), GetBasicQualType("__int128")); | ||
EXPECT_EQ(GetBasicQualType(eBasicTypeUnsignedInt128), | ||
GetBasicQualType("__uint128_t")); | ||
GetBasicQualType("unsigned __uint128")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetBasicQualType("unsigned __uint128")); | |
GetBasicQualType("unsigned __int128")); |
Hence the PR CI test failure
So IIUC, the issue is that Seems like this dates back quite a while:
And there weren't associated tests for it, so hard to say why
Could you add a test for this? E.g., you could add this to On the flip-side it looks like the following commands do currently work today:
Do they still work after your patch? |
3d32637
to
04fea01
Compare
Concerning this (particular) question:
Since neither
The behavior of LLDB will thus change in this respect, but the new behavior is the one that we expect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ok, but I think we should leave the old typename mappings in to make sure we don't break anyone's scripts that might be using the __int128_t
or __uint128_t
// "int128" | ||
{"__int128_t", eBasicTypeInt128}, | ||
{"__uint128_t", eBasicTypeUnsignedInt128}, | ||
{"__int128", eBasicTypeInt128}, | ||
{"unsigned __int128", eBasicTypeUnsignedInt128}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to leave in the old defines in to make sure it doesn't break anyone? So just add the new entries and leave the old ones?
EXPECT_EQ(GetBasicQualType(eBasicTypeInt128), GetBasicQualType("__int128_t")); | ||
EXPECT_EQ(GetBasicQualType(eBasicTypeInt128), GetBasicQualType("__int128")); | ||
EXPECT_EQ(GetBasicQualType(eBasicTypeUnsignedInt128), | ||
GetBasicQualType("__uint128_t")); | ||
GetBasicQualType("unsigned __int128")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave old mappings in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit that these tests back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just needs API tests for the FindFirstType(...).size
cases
That makes sense. I do not have much experience with this but I can try. |
Sure, feel free to ping us here if you have questions! |
In the I have, instead, pushed a commit that adds a new API test related to this issue. |
Thanks for adding the test. Left small suggestion. Otherwise LGTM |
self.assertEqual(c.GetType().GetBasicType(), int_basic_type) | ||
self.assertEqual(d.GetType().GetBasicType(), int_basic_type) | ||
|
||
def testBasicTypeSize(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to make this a separate test case. Just fold it into the existing case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit that joins those two tests back into a single test.
def testBasicTypeSize(self): | ||
"""Check the size of the chosen basic types.""" | ||
self.assertEqual(self.target().FindFirstType("__int128").size, 16) | ||
self.assertEqual(self.target().FindFirstType("unsigned __int128").size, 16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add the same assertions for __uint128_t
and __int128_t
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit that adds this kind of changes as well.
class TestCase(TestBase): | ||
def test(self): | ||
"""Test that SBType.GetBasicType unwraps typedefs.""" | ||
def setUp(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final nit: can we revert the changes of adding a setUp method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing this out. I've missed that. I've pushed a commit that inlines setUp
(back) to the test
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for addressing all the comments
When we take the following C program:
and create a statically-linked executable from it:
Then we can observe the following
lldb
behavior:The value return by the
SBTarget::FindFirstType
method is wrong for the__int128
andunsigned __int128
basic types.The proposed changes make the
TypeSystemClang::GetBasicTypeEnumeration
method consistent withgcc
andclang
C language extension related to 128-bit integer types as well as with theBuiltinType::getName
method in the LLVM codebase itself.When the above change is applied, the behavior of the
lldb
changes in the following (desired) way: