[lldb] Add value to enumerator dump #69815
Conversation
|
@llvm/pr-subscribers-lldb Author: Vlad Serebrennikov (Endilll) ChangesThis patch adds the value to enumerator dump, e.g. Full diff: https://github.com/llvm/llvm-project/pull/69815.diff 6 Files Affected:
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index f1353db2631ddc6..b1ec1cf9a322907 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -8550,7 +8550,7 @@ static bool DumpEnumValue(const clang::QualType &qual_type, Stream &s,
++num_enumerators;
if (val == enum_svalue) {
// Found an exact match, that's all we need to do.
- s.PutCString(enumerator->getNameAsString());
+ s.Printf("%s(%" PRIi64 ")", enumerator->getNameAsString().c_str(), enum_svalue);
return true;
}
}
diff --git a/lldb/test/API/commands/expression/cast_int_to_anonymous_enum/TestCastIntToAnonymousEnum.py b/lldb/test/API/commands/expression/cast_int_to_anonymous_enum/TestCastIntToAnonymousEnum.py
index 923f021d53399cb..00610a29353423f 100644
--- a/lldb/test/API/commands/expression/cast_int_to_anonymous_enum/TestCastIntToAnonymousEnum.py
+++ b/lldb/test/API/commands/expression/cast_int_to_anonymous_enum/TestCastIntToAnonymousEnum.py
@@ -18,4 +18,4 @@ def test_cast_int_to_anonymous_enum(self):
self, "// break here", lldb.SBFileSpec("main.cpp", False)
)
- self.expect_expr("(flow_e)0", result_type="flow_e", result_value="A")
+ self.expect_expr("(flow_e)0", result_type="flow_e", result_value="A(0)")
diff --git a/lldb/test/API/lang/c/enum_types/TestEnumTypes.py b/lldb/test/API/lang/c/enum_types/TestEnumTypes.py
index 33a846c50d7def3..e69cefee2540d47 100644
--- a/lldb/test/API/lang/c/enum_types/TestEnumTypes.py
+++ b/lldb/test/API/lang/c/enum_types/TestEnumTypes.py
@@ -22,12 +22,12 @@ def test_command_line(self):
self, "// Breakpoint for bitfield", lldb.SBFileSpec("main.c")
)
- self.expect("fr var a", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = A$"])
- self.expect("fr var b", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = B$"])
- self.expect("fr var c", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = C$"])
- self.expect("fr var ab", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = AB$"])
+ self.expect("fr var a", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = A\\(1\\)$"])
+ self.expect("fr var b", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = B\\(2\\)$"])
+ self.expect("fr var c", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = C\\(4\\)$"])
+ self.expect("fr var ab", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = AB\\(3\\)$"])
self.expect("fr var ac", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = A | C$"])
- self.expect("fr var all", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = ALL$"])
+ self.expect("fr var all", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = ALL\\(7\\)$"])
# Test that an enum that doesn't match the heuristic we use in
# TypeSystemClang::DumpEnumValue, gets printed as a raw integer.
self.expect("fr var omega", DATA_TYPES_DISPLAYED_CORRECTLY, patterns=[" = 7$"])
diff --git a/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py b/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
index 530191e8a37ba1b..80e81c0df8a5bfd 100644
--- a/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ b/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -57,12 +57,12 @@ def test(self):
self.expect_expr("A::wchar_min == wchar_min", result_value="true")
# Test an unscoped enum.
- self.expect_expr("A::enum_val", result_value="enum_case2")
+ self.expect_expr("A::enum_val", result_value="enum_case2(2)")
# Test an unscoped enum with bool as the underlying type.
- self.expect_expr("A::enum_bool_val", result_value="enum_bool_case1")
+ self.expect_expr("A::enum_bool_val", result_value="enum_bool_case1(0)")
# Test a scoped enum.
- self.expect_expr("A::scoped_enum_val", result_value="scoped_enum_case2")
+ self.expect_expr("A::scoped_enum_val", result_value="scoped_enum_case2(2)")
# Test an scoped enum with a value that isn't an enumerator.
self.expect_expr(
"A::not_enumerator_scoped_enum_val", result_value="scoped_enum_case1 | 0x4"
@@ -74,9 +74,9 @@ def test(self):
)
# Test an enum with fixed underlying type.
- self.expect_expr("A::scoped_char_enum_val", result_value="case2")
- self.expect_expr("A::scoped_ll_enum_val_neg", result_value="case0")
- self.expect_expr("A::scoped_ll_enum_val", result_value="case2")
+ self.expect_expr("A::scoped_char_enum_val", result_value="case2(2)")
+ self.expect_expr("A::scoped_ll_enum_val_neg", result_value="case0(-9223372036854775808)")
+ self.expect_expr("A::scoped_ll_enum_val", result_value="case2(9223372036854775807)")
# Test taking address.
if lldbplatformutil.getPlatform() == "windows":
@@ -126,15 +126,15 @@ def test_class_with_only_constexpr_static(self):
# Test `constexpr static`.
self.expect_expr("ClassWithConstexprs::member", result_value="2")
- self.expect_expr("ClassWithConstexprs::enum_val", result_value="enum_case2")
+ self.expect_expr("ClassWithConstexprs::enum_val", result_value="enum_case2(2)")
self.expect_expr(
- "ClassWithConstexprs::scoped_enum_val", result_value="scoped_enum_case2"
+ "ClassWithConstexprs::scoped_enum_val", result_value="scoped_enum_case2(2)"
)
# Test an aliased enum with fixed underlying type.
self.expect_expr(
- "ClassWithEnumAlias::enum_alias", result_value="scoped_enum_case2"
+ "ClassWithEnumAlias::enum_alias", result_value="scoped_enum_case2(2)"
)
self.expect_expr(
- "ClassWithEnumAlias::enum_alias_alias", result_value="scoped_enum_case1"
+ "ClassWithEnumAlias::enum_alias_alias", result_value="scoped_enum_case1(1)"
)
diff --git a/lldb/test/API/lang/cpp/enum_types/TestCPP11EnumTypes.py b/lldb/test/API/lang/cpp/enum_types/TestCPP11EnumTypes.py
index 09b8967a443b39d..bb8b145d672b7ee 100644
--- a/lldb/test/API/lang/cpp/enum_types/TestCPP11EnumTypes.py
+++ b/lldb/test/API/lang/cpp/enum_types/TestCPP11EnumTypes.py
@@ -23,9 +23,9 @@ def check_enum(self, suffix):
substrs=["Case1", "Case2", "Case3"],
)
# Test each case in the enum.
- self.expect_expr("var1_" + suffix, result_type=enum_name, result_value="Case1")
- self.expect_expr("var2_" + suffix, result_type=enum_name, result_value="Case2")
- self.expect_expr("var3_" + suffix, result_type=enum_name, result_value="Case3")
+ self.expect("expr var1_" + suffix, patterns=[f"\\({enum_name}\\) \\$\\d+ = Case1\\(-?\\d+\\)"])
+ self.expect("expr var2_" + suffix, patterns=[f"\\({enum_name}\\) \\$\\d+ = Case2\\(-?\\d+\\)"])
+ self.expect("expr var3_" + suffix, patterns=[f"\\({enum_name}\\) \\$\\d+ = Case3\\(-?\\d+\\)"])
if unsigned:
self.expect_expr(
diff --git a/lldb/test/API/lang/rust/enum-structs/TestRustEnumStructs.py b/lldb/test/API/lang/rust/enum-structs/TestRustEnumStructs.py
index 46f96d89221d596..ab95e63422c3725 100644
--- a/lldb/test/API/lang/rust/enum-structs/TestRustEnumStructs.py
+++ b/lldb/test/API/lang/rust/enum-structs/TestRustEnumStructs.py
@@ -32,7 +32,7 @@ def test_clike_enums_are_represented_correctly(self):
self.target().FindFirstGlobalVariable("CLIKE_U32_B").GetValue(),
]
self.assertEqual(
- all_values, ["A", "B", "VariantA", "VariantC", "VariantA", "VariantB"]
+ all_values, ["A(2)", "B(10)", "VariantA(0)", "VariantC(2)", "VariantA(1)", "VariantB(2)"]
)
def test_enum_with_tuples_has_all_variants(self):
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I don't have the experience to know if enabling this globally makes sense, but I do have a future use case for it. At a some point I want to be able to describe register fields as enums, and having the value as well as the name means you can match it up with the code you're debugging and create new code to match the value you're seeing. It's also useful for old debuggers on new CPUs where new enum values have been added, but we don't know the names yet. What happens with values that don't have a name, do we already have a fallback for that? |
I think we do: llvm-project/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp Lines 8563 to 8571 in 2ad9fde Later there is a fallback for llvm-project/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp Lines 8597 to 8599 in 2ad9fde |
I commented those out and got a bunch of test failures, so we're good on coverage there. |
|
I'm not qualified to provide much of a review, but I'm in support of the change, thank you for the patch! |
There was a problem hiding this comment.
I am not sure about always showing the signed or unsigned value by default. Enum values by default get displayed as the string value if we can find it. If you want to see the value as signed or unsigned you can change the display format to decimal or unsigned. With this change, your value would appear to be A(0), so if the user tried to edit this value, we don't want them typing B(1), we just want them to type "B".
I would be ok if we change the summary of an enum value to be the signed or unsigned value though. That might be a better option.
|
To clarify a bit: each value (SBValue in our public API, and ValueObject in our private API) has the ability to supply a value string and a summary string. The value string can depend on what the current format is set to. For an enum value, the format defaults to |
|
Making sure I'm following along properly. For context, this is the debug experience I'm most used to: It sounds like you're saying we shouldn't change the enum formatter to display |
The summary should just display "1" (no parens). If we have the following code in C/C++: And we stop after the last line, we can do: And if you set the summary to be the signed unsigned value then we would have this: Where right now it returns If the user changes the format and we did the patch the way it was currently coded we would get: Since the normal value would listen to the format value (which defaults to |
|
What IDE are you using for the screenshot above? The issue you might run into is that IDE might only be showing the value string and not the summary string. XCode and Visual Studio code will show the value if there is one and the summary if there is one. If there is no value (structs and class instances have no values), then they show the summary only. |
|
If we have a "Foo *", the value will show up as the pointer value and the summary for the Pointee (if |
|
Another thing other IDEs do is show something like "{...}" as the value for a struct/union/class. This is what Visual Studio used to do. |
|
This is a mostly orthogonal comment, but at some point, we really should switch all the summaries that pretend to be the value of a variable to value providing Synthetic Children.
Summaries are currently playing two fairly different roles, one some free-form text that describes the value but has no semantic meaning and the other to represent some structure that acts as a scalar "value" when used, so it's supposed to stand in for the value. That makes life hard for clients, since you have no good way to know whether you should use GetValueAsCString or GetSummary in the value field for a variable.
Using a synthetic child provider as first conceived was awkward for this, because the a synthetic child was always ValueObject child of the top level value, which means you have to disclose an opaque toplevel to see what is supposed to be the value of the data type.
When we started doing swift, this came up a lot so we added "value providing synthetic child providers" where you can reply 0 to num_children, and you be asked get_value instead, and the SBValue that returns will act much like the dynamic type does, and replace the original ValueObject with the one you returned. That makes a good presentation for this "formal scalar value".
Then summary strings could go back to always being "side notes" on the variable's value, which should make client's lives easier...
C-strings are still tricky, since to users they really always have two values, the memory address, and the contents of that memory address up to the next null...
Jim
… On Oct 23, 2023, at 3:05 PM, Greg Clayton ***@***.***> wrote:
Another thing other IDEs do is show something like "{...}" as the value for a struct/union/class. This is what Visual Studio used to do.
—
Reply to this email directly, view it on GitHub <#69815 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWZDUQ6DSJA6JBHOCOLYA3SY3AVCNFSM6AAAAAA6J5ZRRCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZWGA4TIOBQGA>.
You are receiving this because you are on a team that was mentioned.
|
|
I agree with what Jim said as well. I would rather see the IDEs add the ability to change the format for the values like Xcode can so that people can see their values as needed. The summary here isn't a perfect option for this because if you switch the format to |
|
@clayborg Thank you for extensive feedback! Can you clarify where do you want me to put the changes? You expected the following:
But my patch doesn't seem to alter the behavior of enum E {
E1 = 0
};
int main()
{
E e;
return 0;
}I'm also wondering if refactoring this patch to use summaries is going to affect the way we dump enum types (with enumerators included). Apparently, this is covered by |
Visual Studio
Ah that could explain it. I'm used to seeing the value and the summary when both are available. |
|
Is there a way to have Visual Studio change the display format of the enum values? I currently like the way that enum values are displayed without any changes where we try to show the enum value as an integer because I can change the format if that is needed, or use the command line API to get the lldb::SBValue and get the value and signed or unsigned manually if it is needed. Does anyone else have any input for or against showing the signed/unsigned value as a summary? I would vote to leave things as is, but if others support showing the signed/unsigned value, then we can make that happen. The needed change in that case would be to show the signed/unsigned as a value as the summary. |
Sort of. You can specify you want to view values in hex and then you'll get |
How is Visual Studio getting access to LLDB when debugging? Is it using the lldb-vscode debug adaptor protocol from the VS Code stuff? |
I'm sorry for the confusion! I didn't mean to imply that I was using lldb through Visual Studio, only that I'm used to my debugger showing me both pieces of information at the same time instead of making me ask it twice, and I support lldb doing something similar. |
|
I would be interested in at least having a way to opt into this style, it would be useful for registers for example here where I am experimenting with defining Having the numerical value is really useful and saves a trip to the manual (or in this case, kernel sources). For this use case I'm setting options in Assuming a formatter has the same sort of control, would it be viable to add an option that a formatter could pass when it knows it wants the name plus the value? Instead of changing the global behaviour. @Endilll would that work for the types you're working with? |
@DavidSpickett What exactly do you set or unset there to get the output you're showing? This approach seems novel to me, so I'd like to investigate it. |
|
Yeah sorry I dropped the context there, I copied over your change to So what I meant to put across was that we are aiming for the same result. |
|
The existing code is . Register contents is described as a packed struct with bitfields as members. Now I am looking at making some of those enums so we can say what each value means. |
|
@DavidSpickett There is |
Sure, I can always set it to on when printing registers so that would work for me. |
This patch adds the value to enumerator dump, e.g.
Enumeratornow dumped asEnumerator(0). There are not-so-uncommon cases when value of enumerator is no less important than its name. One example can be found inllvm-project/clang/include/clang/AST/DeclarationName.h
Line 183 in 4aae538
llvm-project/llvm/include/llvm/ADT/PointerIntPair.h
Line 181 in 4aae538