From 96b1784ac8c52fb912150de1387117de62de13ad Mon Sep 17 00:00:00 2001 From: Walter Erquinigo Date: Wed, 20 Sep 2023 10:23:41 -0400 Subject: [PATCH] [lldb-vscode] Use auto summaries whenever variables don't have a summary (#66551) Auto summaries were only being used when non-pointer/reference variables didn't have values nor summaries. Greg pointed out that it should be better to simply use auto summaries when the variable doesn't have a summary of its own, regardless of other conditions. This led to code simplification and correct visualization of auto summaries for pointer/reference types, as seen in this screenshot. Screenshot 2023-09-19 at 7 04 55 PM --- .../evaluate/TestVSCode_evaluate.py | 3 +- .../API/tools/lldb-vscode/evaluate/main.cpp | 3 +- lldb/tools/lldb-vscode/JSONUtils.cpp | 80 +++++++------------ 3 files changed, 31 insertions(+), 55 deletions(-) diff --git a/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py b/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py index 3cfe02ef6aa157..d1b73e1a057e1d 100644 --- a/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py +++ b/lldb/test/API/tools/lldb-vscode/evaluate/TestVSCode_evaluate.py @@ -63,8 +63,9 @@ def run_test_evaluate_expressions( "struct1", "{foo:15}" if enableAutoVariableSummaries else "my_struct @ 0x" ) self.assertEvaluate( - "struct2", "{foo:16}" if enableAutoVariableSummaries else "0x.*" + "struct2", "0x.* {foo:16}" if enableAutoVariableSummaries else "0x.*" ) + self.assertEvaluate("struct3", "0x.*0") self.assertEvaluate("struct1.foo", "15") self.assertEvaluate("struct2->foo", "16") diff --git a/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp b/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp index 3a541b21b22082..f09d00e6444bb7 100644 --- a/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp +++ b/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp @@ -18,6 +18,7 @@ struct my_struct { int main(int argc, char const *argv[]) { my_struct struct1 = {15}; my_struct *struct2 = new my_struct{16}; + my_struct *struct3 = nullptr; int var1 = 20; int var2 = 21; int var3 = static_int; // breakpoint 1 @@ -43,6 +44,6 @@ int main(int argc, char const *argv[]) { my_bool_vec.push_back(true); my_bool_vec.push_back(false); // breakpoint 6 my_bool_vec.push_back(true); // breakpoint 7 - + return 0; } diff --git a/lldb/tools/lldb-vscode/JSONUtils.cpp b/lldb/tools/lldb-vscode/JSONUtils.cpp index c6b422e4d7a02e..6cf753170d8429 100644 --- a/lldb/tools/lldb-vscode/JSONUtils.cpp +++ b/lldb/tools/lldb-vscode/JSONUtils.cpp @@ -136,14 +136,14 @@ std::vector GetStrings(const llvm::json::Object *obj, /// first children, so that the user can get a glimpse of its contents at a /// glance. static std::optional -GetSyntheticSummaryForContainer(lldb::SBValue &v) { +TryCreateAutoSummaryForContainer(lldb::SBValue &v) { // We gate this feature because it performs GetNumChildren(), which can // cause performance issues because LLDB needs to complete possibly huge // types. if (!g_vsc.enable_auto_variable_summaries) return std::nullopt; - if (v.TypeIsPointerType() || !v.MightHaveChildren()) + if (!v.MightHaveChildren()) return std::nullopt; /// As this operation can be potentially slow, we limit the total time spent /// fetching children to a few ms. @@ -194,28 +194,18 @@ GetSyntheticSummaryForContainer(lldb::SBValue &v) { return summary; } -/// Return whether we should dereference an SBValue in order to generate a more -/// meaningful summary string. -static bool ShouldBeDereferencedForSummary(lldb::SBValue &v) { +/// Try to create a summary string for the given value that doesn't have a +/// summary of its own. +static std::optional TryCreateAutoSummary(lldb::SBValue value) { if (!g_vsc.enable_auto_variable_summaries) - return false; - - if (!v.GetType().IsPointerType() && !v.GetType().IsReferenceType()) - return false; - - // If we are referencing a pointer, we don't dereference to avoid confusing - // the user with the addresses that could shown in the summary. - if (v.Dereference().GetType().IsPointerType()) - return false; - - // If it's synthetic or a pointer to a basic type that provides a summary, we - // don't dereference. - if ((v.IsSynthetic() || v.GetType().GetPointeeType().GetBasicType() != - lldb::eBasicTypeInvalid) && - !llvm::StringRef(v.GetSummary()).empty()) { - return false; - } - return true; + return std::nullopt; + + // We use the dereferenced value for generating the summary. + if (value.GetType().IsPointerType() || value.GetType().IsReferenceType()) + value = value.Dereference(); + + // We only support auto summaries for containers. + return TryCreateAutoSummaryForContainer(value); } void SetValueForKey(lldb::SBValue &v, llvm::json::Object &object, @@ -227,36 +217,20 @@ void SetValueForKey(lldb::SBValue &v, llvm::json::Object &object, if (!error.Success()) { strm << ""; } else { - auto tryDumpSummaryAndValue = [&strm](lldb::SBValue value) { - llvm::StringRef val = value.GetValue(); - llvm::StringRef summary = value.GetSummary(); - if (!val.empty()) { - strm << val; - if (!summary.empty()) - strm << ' ' << summary; - return true; - } - if (!summary.empty()) { - strm << ' ' << summary; - return true; - } - if (auto container_summary = GetSyntheticSummaryForContainer(value)) { - strm << *container_summary; - return true; - } - return false; - }; - - // We first try to get the summary from its dereferenced value. - bool done = ShouldBeDereferencedForSummary(v) && - tryDumpSummaryAndValue(v.Dereference()); - - // We then try to get it from the current value itself. - if (!done) - done = tryDumpSummaryAndValue(v); - - // As last resort, we print its type and address if available. - if (!done) { + llvm::StringRef value = v.GetValue(); + llvm::StringRef nonAutoSummary = v.GetSummary(); + std::optional summary = !nonAutoSummary.empty() + ? nonAutoSummary.str() + : TryCreateAutoSummary(v); + if (!value.empty()) { + strm << value; + if (summary) + strm << ' ' << *summary; + } else if (summary) { + strm << *summary; + + // As last resort, we print its type and address if available. + } else { if (llvm::StringRef type_name = v.GetType().GetDisplayTypeName(); !type_name.empty()) { strm << type_name;