[lldb] Print empty enums as if they were unrecognised normal enums #97553
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: David Spickett (DavidSpickett) ChangesFixes #97514 Given this example: lldb used to print nothing for At first this seemed like the 0 case needed fixing but the real issue here is that en enum with no enumerators was being detected as a "bitfield like enum". Which is an enum where all enumerators are a single bit value, or the sum of previous single bit values. For these we do not print anything for a value of 0, as we assume it must be the remainder after we've printed the other bits that were set (I think this is also unfortunate, but I'm not addressing that here). Clearly an enum with no enumerators cannot be being used as a bitfield, so check that up front and print it as if it's a normal enum where we didn't match any of the enumerators. This means you now get: Which is a change to decimal from hex, but I think it's overall more consistent. Printing hex here was never a concious decision. Full diff: https://github.com/llvm/llvm-project/pull/97553.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 093d27a92d718..960c9f6acde57 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -8649,19 +8649,24 @@ static bool DumpEnumValue(const clang::QualType &qual_type, Stream &s,
// At the same time, we're applying a heuristic to determine whether we want
// to print this enum as a bitfield. We're likely dealing with a bitfield if
// every enumerator is either a one bit value or a superset of the previous
- // enumerators. Also 0 doesn't make sense when the enumerators are used as
- // flags.
- for (auto *enumerator : enum_decl->enumerators()) {
- uint64_t val = enumerator->getInitVal().getSExtValue();
- val = llvm::SignExtend64(val, 8*byte_size);
- if (llvm::popcount(val) != 1 && (val & ~covered_bits) != 0)
- can_be_bitfield = false;
- covered_bits |= val;
- ++num_enumerators;
- if (val == enum_svalue) {
- // Found an exact match, that's all we need to do.
- s.PutCString(enumerator->getNameAsString());
- return true;
+ // enumerators. Also an enumerator of 0 doesn't make sense when the
+ // enumerators are used as flags.
+ clang::EnumDecl::enumerator_range enumerators = enum_decl->enumerators();
+ if (enumerators.empty())
+ can_be_bitfield = false;
+ else {
+ for (auto *enumerator : enum_decl->enumerators()) {
+ uint64_t val = enumerator->getInitVal().getSExtValue();
+ val = llvm::SignExtend64(val, 8 * byte_size);
+ if (llvm::popcount(val) != 1 && (val & ~covered_bits) != 0)
+ can_be_bitfield = false;
+ covered_bits |= val;
+ ++num_enumerators;
+ if (val == enum_svalue) {
+ // Found an exact match, that's all we need to do.
+ s.PutCString(enumerator->getNameAsString());
+ return true;
+ }
}
}
diff --git a/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp b/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp
index 6cb982d7f5980..767f19872f858 100644
--- a/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp
+++ b/lldb/unittests/ValueObject/DumpValueObjectOptionsTests.cpp
@@ -71,12 +71,13 @@ class ValueObjectMockProcessTest : public ::testing::Test {
}
CompilerType
- MakeEnumType(const std::vector<std::pair<const char *, int>> enumerators) {
- CompilerType uint_type = m_type_system->GetBuiltinTypeForEncodingAndBitSize(
- lldb::eEncodingUint, 32);
+ MakeEnumType(const std::vector<std::pair<const char *, int>> enumerators,
+ bool is_signed) {
+ CompilerType int_type = m_type_system->GetBuiltinTypeForEncodingAndBitSize(
+ is_signed ? lldb::eEncodingSint : lldb::eEncodingUint, 32);
CompilerType enum_type = m_type_system->CreateEnumerationType(
"TestEnum", m_type_system->GetTranslationUnitDecl(),
- OptionalClangModuleID(), Declaration(), uint_type, false);
+ OptionalClangModuleID(), Declaration(), int_type, false);
m_type_system->StartTagDeclarationDefinition(enum_type);
Declaration decl;
@@ -123,12 +124,27 @@ class ValueObjectMockProcessTest : public ::testing::Test {
lldb::ProcessSP m_process_sp;
};
+TEST_F(ValueObjectMockProcessTest, EmptyEnum) {
+ // All values of an empty enum should be shown as plain numbers.
+ TestDumpValueObject(MakeEnumType({}, false),
+ {{0, {}, "(TestEnum) test_var = 0\n"},
+ {1, {}, "(TestEnum) test_var = 1\n"},
+ {2, {}, "(TestEnum) test_var = 2\n"}});
+
+ TestDumpValueObject(MakeEnumType({}, true),
+ {{-2, {}, "(TestEnum) test_var = -2\n"},
+ {-1, {}, "(TestEnum) test_var = -1\n"},
+ {0, {}, "(TestEnum) test_var = 0\n"},
+ {1, {}, "(TestEnum) test_var = 1\n"},
+ {2, {}, "(TestEnum) test_var = 2\n"}});
+}
+
TEST_F(ValueObjectMockProcessTest, Enum) {
// This is not a bitfield-like enum, so values are printed as decimal by
// default. Also we only show the enumerator name if the value is an
// exact match.
TestDumpValueObject(
- MakeEnumType({{"test_2", 2}, {"test_3", 3}}),
+ MakeEnumType({{"test_2", 2}, {"test_3", 3}}, false),
{{0, {}, "(TestEnum) test_var = 0\n"},
{1, {}, "(TestEnum) test_var = 1\n"},
{2, {}, "(TestEnum) test_var = test_2\n"},
@@ -152,7 +168,7 @@ TEST_F(ValueObjectMockProcessTest, BitFieldLikeEnum) {
// as hex, a value of 0 shows nothing, and values with no exact enumerator are
// shown as combinations of the other values.
TestDumpValueObject(
- MakeEnumType({{"test_2", 2}, {"test_4", 4}}),
+ MakeEnumType({{"test_2", 2}, {"test_4", 4}}, false),
{
{0, {}, "(TestEnum) test_var =\n"},
{1, {}, "(TestEnum) test_var = 0x1\n"},
|
DavidSpickett
commented
Jul 3, 2024
| // Found an exact match, that's all we need to do. | ||
| s.PutCString(enumerator->getNameAsString()); | ||
| return true; | ||
| // enumerators. Also an enumerator of 0 doesn't make sense when the |
There was a problem hiding this comment.
I clarified this to "an enumerator of 0 doesn't make sense", which is my understanding of it. As opposed to "a value of 0 doesn't make sense".
Fixes llvm#97514 Given this example: ``` enum E {}; int main() { E x = E(0); E y = E(1); E z = E(2); return 0; } ``` lldb used to print nothing for `x`, but `0x1` for `y` and `0x2` for `z`. At first this seemed like the 0 case needed fixing but the real issue here is that en enum with no enumerators was being detected as a "bitfield like enum". Which is an enum where all enumerators are a single bit value, or the sum of previous single bit values. For these we do not print anything for a value of 0, as we assume it must be the remainder after we've printed the other bits that were set (I think this is also unfortunate, but I'm not addressing that here). Clearly an enum with no enumerators cannot be being used as a bitfield, so check that up front and print it as if it's a normal enum where we didn't match any of the enumerators. This means you now get: ``` (lldb) p x (E) 0 (lldb) p y (E) 1 (lldb) p z (E) 2 ``` Which is a change to decimal from hex, but I think it's overall more consistent. Printing hex here was never a concious decision.
DavidSpickett
force-pushed
the
lldb-emptyenum
branch
from
110562a to
1a170e9
Compare
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
…lvm#97553) Fixes llvm#97514 Given this example: ``` enum E {}; int main() { E x = E(0); E y = E(1); E z = E(2); return 0; } ``` lldb used to print nothing for `x`, but `0x1` for `y` and `0x2` for `z`. At first this seemed like the 0 case needed fixing but the real issue here is that en enum with no enumerators was being detected as a "bitfield like enum". Which is an enum where all enumerators are a single bit value, or the sum of previous single bit values. For these we do not print anything for a value of 0, as we assume it must be the remainder after we've printed the other bits that were set (I think this is also unfortunate, but I'm not addressing that here). Clearly an enum with no enumerators cannot be being used as a bitfield, so check that up front and print it as if it's a normal enum where we didn't match any of the enumerators. This means you now get: ``` (lldb) p x (E) 0 (lldb) p y (E) 1 (lldb) p z (E) 2 ``` Which is a change to decimal from hex, but I think it's overall more consistent. Printing hex here was never a concious decision.
|
This causes build errors with GCC (11): |
|
We are seeing the same failure in LLDB tests. Can we please revert this change? |
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
…lvm#97553) Fixes llvm#97514 Given this example: ``` enum E {}; int main() { E x = E(0); E y = E(1); E z = E(2); return 0; } ``` lldb used to print nothing for `x`, but `0x1` for `y` and `0x2` for `z`. At first this seemed like the 0 case needed fixing but the real issue here is that en enum with no enumerators was being detected as a "bitfield like enum". Which is an enum where all enumerators are a single bit value, or the sum of previous single bit values. For these we do not print anything for a value of 0, as we assume it must be the remainder after we've printed the other bits that were set (I think this is also unfortunate, but I'm not addressing that here). Clearly an enum with no enumerators cannot be being used as a bitfield, so check that up front and print it as if it's a normal enum where we didn't match any of the enumerators. This means you now get: ``` (lldb) p x (E) 0 (lldb) p y (E) 1 (lldb) p z (E) 2 ``` Which is a change to decimal from hex, but I think it's overall more consistent. Printing hex here was never a concious decision.
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
…enums (llvm#97553)" This reverts commit 41fddc4. Due to build errors with gcc passing signed ints to unsigned ints.
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
…enums (llvm#97553)" This reverts commit 927def4. I've refactored the tests so that we're explicit about whether the enum is signed or not. Which means we use the proper types throughout.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #97514
Given this example:
lldb used to print nothing for
x, but0x1foryand0x2forz.At first this seemed like the 0 case needed fixing but the real issue here is that en enum with no enumerators was being detected as a "bitfield like enum".
Which is an enum where all enumerators are a single bit value, or the sum of previous single bit values.
For these we do not print anything for a value of 0, as we assume it must be the remainder after we've printed the other bits that were set (I think this is also unfortunate, but I'm not addressing that here).
Clearly an enum with no enumerators cannot be being used as a bitfield, so check that up front and print it as if it's a normal enum where we didn't match any of the enumerators. This means you now get:
Which is a change to decimal from hex, but I think it's overall more consistent. Printing hex here was never a concious decision.