llvm
[LLDB] Optimize identifier lookup in DIL
Remove unused code and unnecessary function calls, optimize global variable search. Add more test cases.
@llvm/pr-subscribers-lldb
Author: Ilia Kuklin (kuilpd)
Changes
Remove unused code and unnecessary function calls, optimize global variable search. Add more test cases.
Full diff: https://github.com/llvm/llvm-project/pull/146094.diff
6 Files Affected:
- (modified) lldb/include/lldb/ValueObject/DILEval.h (+2-4)
- (modified) lldb/source/ValueObject/DILEval.cpp (+52-120)
- (modified) lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/Makefile (+1-1)
- (modified) lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/TestFrameVarDILGlobalVariableLookup.py (+11-11)
- (added) lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/extern.cpp (+10)
- (modified) lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/main.cpp (+3)
diff --git a/lldb/include/lldb/ValueObject/DILEval.h b/lldb/include/lldb/ValueObject/DILEval.h
index 2a0cb548a810f..45e29b3ddcd7b 100644
--- a/lldb/include/lldb/ValueObject/DILEval.h
+++ b/lldb/include/lldb/ValueObject/DILEval.h
@@ -25,8 +25,7 @@ namespace lldb_private::dil {
/// evaluating).
lldb::ValueObjectSP LookupIdentifier(llvm::StringRef name_ref,
std::shared_ptr<StackFrame> frame_sp,
- lldb::DynamicValueType use_dynamic,
- CompilerType *scope_ptr = nullptr);
+ lldb::DynamicValueType use_dynamic);
/// Given the name of an identifier, check to see if it matches the name of a
/// global variable. If so, find the ValueObject for that global variable, and
@@ -35,8 +34,7 @@ lldb::ValueObjectSP LookupIdentifier(llvm::StringRef name_ref,
lldb::ValueObjectSP LookupGlobalIdentifier(llvm::StringRef name_ref,
std::shared_ptr<StackFrame> frame_sp,
lldb::TargetSP target_sp,
- lldb::DynamicValueType use_dynamic,
- CompilerType *scope_ptr = nullptr);
+ lldb::DynamicValueType use_dynamic);
class Interpreter : Visitor {
public:
diff --git a/lldb/source/ValueObject/DILEval.cpp b/lldb/source/ValueObject/DILEval.cpp
index b2bb4e20ddc24..f75f104ba2bc5 100644
--- a/lldb/source/ValueObject/DILEval.cpp
+++ b/lldb/source/ValueObject/DILEval.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "lldb/ValueObject/DILEval.h"
+#include "lldb/Symbol/CompileUnit.h"
#include "lldb/Symbol/VariableList.h"
#include "lldb/Target/RegisterContext.h"
#include "lldb/ValueObject/DILAST.h"
@@ -18,70 +19,26 @@
namespace lldb_private::dil {
-static lldb::ValueObjectSP LookupStaticIdentifier(
- VariableList &variable_list, std::shared_ptr<StackFrame> exe_scope,
- llvm::StringRef name_ref, llvm::StringRef unqualified_name) {
- // First look for an exact match to the (possibly) qualified name.
- for (const lldb::VariableSP &var_sp : variable_list) {
- lldb::ValueObjectSP valobj_sp(
- ValueObjectVariable::Create(exe_scope.get(), var_sp));
- if (valobj_sp && valobj_sp->GetVariable() &&
- (valobj_sp->GetVariable()->NameMatches(ConstString(name_ref))))
- return valobj_sp;
- }
-
- // If the qualified name is the same as the unqualfied name, there's nothing
- // more to be done.
- if (name_ref == unqualified_name)
- return nullptr;
-
- // We didn't match the qualified name; try to match the unqualified name.
- for (const lldb::VariableSP &var_sp : variable_list) {
- lldb::ValueObjectSP valobj_sp(
- ValueObjectVariable::Create(exe_scope.get(), var_sp));
- if (valobj_sp && valobj_sp->GetVariable() &&
- (valobj_sp->GetVariable()->NameMatches(ConstString(unqualified_name))))
- return valobj_sp;
- }
-
- return nullptr;
-}
-
static lldb::VariableSP DILFindVariable(ConstString name,
- lldb::VariableListSP variable_list) {
+ VariableList &variable_list) {
lldb::VariableSP exact_match;
std::vector<lldb::VariableSP> possible_matches;
- for (lldb::VariableSP var_sp : *variable_list) {
+ for (lldb::VariableSP var_sp : variable_list) {
llvm::StringRef str_ref_name = var_sp->GetName().GetStringRef();
- // Check for global vars, which might start with '::'.
- str_ref_name.consume_front("::");
- if (str_ref_name == name.GetStringRef())
- possible_matches.push_back(var_sp);
- else if (var_sp->NameMatches(name))
- possible_matches.push_back(var_sp);
- }
-
- // Look for exact matches (favors local vars over global vars)
- auto exact_match_it =
- llvm::find_if(possible_matches, [&](lldb::VariableSP var_sp) {
- return var_sp->GetName() == name;
- });
-
- if (exact_match_it != possible_matches.end())
- return *exact_match_it;
-
- // Look for a global var exact match.
- for (auto var_sp : possible_matches) {
- llvm::StringRef str_ref_name = var_sp->GetName().GetStringRef();
str_ref_name.consume_front("::");
+ // Check for the exact same match
if (str_ref_name == name.GetStringRef())
return var_sp;
+
+ // Check for possible matches by base name
+ if (var_sp->NameMatches(name))
+ possible_matches.push_back(var_sp);
}
- // If there's a single non-exact match, take it.
- if (possible_matches.size() == 1)
+ // If there's a non-exact match, take it.
+ if (possible_matches.size() > 0)
return possible_matches[0];
return nullptr;
@@ -89,38 +46,23 @@ static lldb::VariableSP DILFindVariable(ConstString name,
lldb::ValueObjectSP LookupGlobalIdentifier(
llvm::StringRef name_ref, std::shared_ptr<StackFrame> stack_frame,
- lldb::TargetSP target_sp, lldb::DynamicValueType use_dynamic,
- CompilerType *scope_ptr) {
- // First look for match in "local" global variables.
- lldb::VariableListSP variable_list(stack_frame->GetInScopeVariableList(true));
- name_ref.consume_front("::");
+ lldb::TargetSP target_sp, lldb::DynamicValueType use_dynamic) {
+ // Get a global variables list without the locals from the current frame
+ SymbolContext symbol_context =
+ stack_frame->GetSymbolContext(lldb::eSymbolContextCompUnit);
+ lldb::VariableListSP variable_list =
+ symbol_context.comp_unit->GetVariableList(true);
+ name_ref.consume_front("::");
lldb::ValueObjectSP value_sp;
if (variable_list) {
lldb::VariableSP var_sp =
- DILFindVariable(ConstString(name_ref), variable_list);
+ DILFindVariable(ConstString(name_ref), *variable_list);
if (var_sp)
value_sp =
stack_frame->GetValueObjectForFrameVariable(var_sp, use_dynamic);
}
- if (value_sp)
- return value_sp;
-
- // Also check for static global vars.
- if (variable_list) {
- const char *type_name = "";
- if (scope_ptr)
- type_name = scope_ptr->GetCanonicalType().GetTypeName().AsCString();
- std::string name_with_type_prefix =
- llvm::formatv("{0}::{1}", type_name, name_ref).str();
- value_sp = LookupStaticIdentifier(*variable_list, stack_frame,
- name_with_type_prefix, name_ref);
- if (!value_sp)
- value_sp = LookupStaticIdentifier(*variable_list, stack_frame, name_ref,
- name_ref);
- }
-
if (value_sp)
return value_sp;
@@ -129,28 +71,22 @@ lldb::ValueObjectSP LookupGlobalIdentifier(
target_sp->GetImages().FindGlobalVariables(
ConstString(name_ref), std::numeric_limits<uint32_t>::max(),
modules_var_list);
- if (modules_var_list.Empty())
- return nullptr;
- for (const lldb::VariableSP &var_sp : modules_var_list) {
- std::string qualified_name = llvm::formatv("::{0}", name_ref).str();
- if (var_sp->NameMatches(ConstString(name_ref)) ||
- var_sp->NameMatches(ConstString(qualified_name))) {
+ if (!modules_var_list.Empty()) {
+ lldb::VariableSP var_sp =
+ DILFindVariable(ConstString(name_ref), modules_var_list);
+ if (var_sp)
value_sp = ValueObjectVariable::Create(stack_frame.get(), var_sp);
- break;
- }
- }
-
- if (value_sp)
- return value_sp;
+ if (value_sp)
+ return value_sp;
+ }
return nullptr;
}
lldb::ValueObjectSP LookupIdentifier(llvm::StringRef name_ref,
std::shared_ptr<StackFrame> stack_frame,
- lldb::DynamicValueType use_dynamic,
- CompilerType *scope_ptr) {
+ lldb::DynamicValueType use_dynamic) {
// Support $rax as a special syntax for accessing registers.
// Will return an invalid value in case the requested register doesn't exist.
if (name_ref.consume_front("$")) {
@@ -164,38 +100,34 @@ lldb::ValueObjectSP LookupIdentifier(llvm::StringRef name_ref,
return nullptr;
}
- lldb::VariableListSP variable_list(
- stack_frame->GetInScopeVariableList(false));
-
if (!name_ref.contains("::")) {
- if (!scope_ptr || !scope_ptr->IsValid()) {
- // Lookup in the current frame.
- // Try looking for a local variable in current scope.
- lldb::ValueObjectSP value_sp;
- if (variable_list) {
- lldb::VariableSP var_sp =
- DILFindVariable(ConstString(name_ref), variable_list);
- if (var_sp)
- value_sp =
- stack_frame->GetValueObjectForFrameVariable(var_sp, use_dynamic);
- }
- if (!value_sp)
- value_sp = stack_frame->FindVariable(ConstString(name_ref));
-
- if (value_sp)
- return value_sp;
-
- // Try looking for an instance variable (class member).
- SymbolContext sc = stack_frame->GetSymbolContext(
- lldb::eSymbolContextFunction | lldb::eSymbolContextBlock);
- llvm::StringRef ivar_name = sc.GetInstanceVariableName();
- value_sp = stack_frame->FindVariable(ConstString(ivar_name));
- if (value_sp)
- value_sp = value_sp->GetChildMemberWithName(name_ref);
-
- if (value_sp)
- return value_sp;
+ // Lookup in the current frame.
+ // Try looking for a local variable in current scope.
+ lldb::VariableListSP variable_list(
+ stack_frame->GetInScopeVariableList(false));
+
+ lldb::ValueObjectSP value_sp;
+ if (variable_list) {
+ lldb::VariableSP var_sp =
+ variable_list->FindVariable(ConstString(name_ref));
+ if (var_sp)
+ value_sp =
+ stack_frame->GetValueObjectForFrameVariable(var_sp, use_dynamic);
}
+
+ if (value_sp)
+ return value_sp;
+
+ // Try looking for an instance variable (class member).
+ SymbolContext sc = stack_frame->GetSymbolContext(
+ lldb::eSymbolContextFunction | lldb::eSymbolContextBlock);
+ llvm::StringRef ivar_name = sc.GetInstanceVariableName();
+ value_sp = stack_frame->FindVariable(ConstString(ivar_name));
+ if (value_sp)
+ value_sp = value_sp->GetChildMemberWithName(name_ref);
+
+ if (value_sp)
+ return value_sp;
}
return nullptr;
}
diff --git a/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/Makefile b/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/Makefile
index 99998b20bcb05..c39bce9a94dcf 100644
--- a/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/Makefile
+++ b/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/Makefile
@@ -1,3 +1,3 @@
-CXX_SOURCES := main.cpp
+CXX_SOURCES := main.cpp extern.cpp
include Makefile.rules
diff --git a/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/TestFrameVarDILGlobalVariableLookup.py b/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/TestFrameVarDILGlobalVariableLookup.py
index edb013c7b3526..8a8f068c19466 100644
--- a/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/TestFrameVarDILGlobalVariableLookup.py
+++ b/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/TestFrameVarDILGlobalVariableLookup.py
@@ -32,20 +32,20 @@ def test_frame_var(self):
self.expect_var_path("::globalPtr", type="int *")
self.expect_var_path("::globalRef", type="int &")
- self.expect(
- "frame variable 'externGlobalVar'",
- error=True,
- substrs=["use of undeclared identifier"],
- ) # 0x00C0FFEE
- self.expect(
- "frame variable '::externGlobalVar'",
- error=True,
- substrs=["use of undeclared identifier"],
- ) # ["12648430"])
-
self.expect_var_path("ns::globalVar", value="13")
self.expect_var_path("ns::globalPtr", type="int *")
self.expect_var_path("ns::globalRef", type="int &")
self.expect_var_path("::ns::globalVar", value="13")
self.expect_var_path("::ns::globalPtr", type="int *")
self.expect_var_path("::ns::globalRef", type="int &")
+
+ self.expect_var_path("externGlobalVar", value="2")
+ self.expect_var_path("::externGlobalVar", value="2")
+ self.expect_var_path("ext::externGlobalVar", value="4")
+ self.expect_var_path("::ext::externGlobalVar", value="4")
+
+ self.expect_var_path("ExtStruct::static_inline", value="16")
+
+ # Test local variable priority over global
+ self.expect_var_path("foo", value="1")
+ self.expect_var_path("::foo", value="2")
diff --git a/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/extern.cpp b/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/extern.cpp
new file mode 100644
index 0000000000000..c451191dafc57
--- /dev/null
+++ b/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/extern.cpp
@@ -0,0 +1,10 @@
+int externGlobalVar = 2;
+
+namespace ext {
+int externGlobalVar = 4;
+} // namespace ext
+
+struct ExtStruct {
+private:
+ static constexpr inline int static_inline = 16;
+} es;
diff --git a/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/main.cpp b/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/main.cpp
index 5bae4fd423e32..a8ecbe2d8fc44 100644
--- a/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/main.cpp
+++ b/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/main.cpp
@@ -10,6 +10,9 @@ int *globalPtr = &globalVar;
int &globalRef = globalVar;
} // namespace ns
+int foo = 2;
+
int main(int argc, char **argv) {
+ int foo = 1;
return 0; // Set a breakpoint here
}
I did some benchmarks of DIL on Unreal Engine 5 (thousands of variables to search through) and noticed that our current variable lookup implementation is ~10 times slower than it was in lldb-eval, especially when it comes to global variables in other files.
I looked into it and made some changes:
- Removed the code for lookups in the specific scope: we don't have an interface or any other code to pass the scope into variable lookup, so it is unused for now and cannot even be tested.
- Shortened the code in
DILFindVariableto stop at the first full match or return any partial match found. - Replaced the the call to
DILFindVariablewithvariable_list->FindVariablein local variable search: we don't need full/partial matching checks since there is no namespaces and::in the variables in the local scope. - Search through local+global variables is now through globals of the current file only, without locals: reduces the amount to search through and allows to search a global variable that has the same base name as a local one. I also wanted to completely remove this step, because the later search through all globals in all files includes the current file as well (and this was the logic in
lldb-eval, no search through current file globals). However, this slows down lookup if we search for a global in the current file, but also speeds up the lookup if we search for a global in a different file, and I'm not sure which tradeoff is better. Since currentframe vardoesn't have an ability to search through other files, I'm guessing the faster search in the current one is more important, at least for now. - Removed the entire LookupStaticIdentifier: I think there was a misunderstanding here while adapting the code from
lldb-eval, this was the function that used to search through all global variables, and now here in the upstream it iterated through the current file variable list again but via a different interface, which is much slower and caused most of the slowdown. Its original functionality is at the end ofLookupGlobalIdentifierthat searches through all global variables, now also doing partial matching viaDILFindVariable.
Because of the extra search in the current file globals, DIL is now somewhat faster in searching variables in the current file and somewhat slower in searching variables in other files compared to lldb-eval.
I'm not sure that's the right thing to do -- if there are multiple matches, how can we know we picked the one that the user wanted to see? What might matter for performance is, if returning false/nullptr here causes the implementation to perform the lookup at a larger (more expensive scope). If that's the case, then I'd say that the right thing is to not do the second lookup in case the first one is ambiguous (as that doesn't help in resolving the ambiguity). So, the function could return one of three results: found zero, found one, found more than one; and in the last case we would immediately bail out.
Well, my logic was that it's better to return something than nothing, similar to what current frame var does, it just outputs the first thing that matches the base name, regardless of namespace. Should I make DILFindVariable return a specific error message like "ambiguous name" or something?
That said, I think the search for the current CU has different performance characteristics (I believe it materializes all globals, and then searches for the right name), so while I'm not sure if it's necessary (this should be a one-shot thing), I can imagine implementing the search differently somehow, so that we can only return the "local globals" with the right name.
I tried doing symbol_context.module_sp->GetSymbolFile()->FindGlobalVariables, which in turn calls SymbolFileDWARF::FindGlobalVariables, but it searches through the entire module, not just current CU. CompileUnit class has only GetVariableList, no search. It looks like getting variable list there does something different, but I really can't tell if it's any faster.
Well, my logic was that it's better to return something than nothing,
For a simple variable query, it might be okay since you kind of see the full name in the command output. However, for expressions like a+b, it's kind of important to know which as and bs are you adding together. I think we should change that...
similar to what current
frame vardoes, it just outputs the first thing that matches the base name, regardless of namespace. Should I makeDILFindVariablereturn a specific error message like "ambiguous name" or something?
... but if the current implementation does the same thing, then I think it's good enough for now.
I tried doing symbol_context.module_sp->GetSymbolFile()->FindGlobalVariables, which in turn calls SymbolFileDWARF::FindGlobalVariables, but it searches through the entire module, not just current CU. CompileUnit class has only GetVariableList, no search. It looks like getting variable list there does something different, but I really can't tell if it's any faster.
That might depend on the exact use case (number of CUs, number of variables per CU, how many times you're calling this, etc.). If you think it's necessary, we can talk about optimizing this, but I don't think we need to do that now.
Looks like this is failing on macOS CI: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/28857/execution/node/106/log/?consoleFull
14:41:07 FAIL: test_frame_var (TestFrameVarDILGlobalVariableLookup.TestFrameVarDILGlobalVariableLookup)
14:41:07 ----------------------------------------------------------------------
14:41:07 Traceback (most recent call last):
14:41:07 File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/TestFrameVarDILGlobalVariableLookup.py", line 47, in test_frame_var
14:41:07 self.expect_var_path("ExtStruct::static_inline", value="16")
14:41:07 File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2589, in expect_var_path
14:41:07 value_check.check_value(self, eval_result, str(eval_result))
14:41:07 File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 301, in check_value
14:41:07 test_base.assertSuccess(val.GetError())
14:41:07 File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2597, in assertSuccess
14:41:07 self.fail(self._formatMessage(msg, "'{}' is not success".format(error)))
14:41:07 AssertionError: '<user expression 0>:1:1: use of undeclared identifier 'ExtStruct::static_inline'
14:41:07 1 | ExtStruct::static_inline
14:41:07 | ^' is not success
Could you take a look? And revert if the fix might need some time to land?
This might be isolated to dsyms (i remember dsymutil stripping some static inlines, but i forget exactly in which cases).
This might be isolated to dsyms (i remember dsymutil stripping some static inlines, but i forget exactly in which cases).
This is very likely the case, but I don't have a machine to debug this on. Could we just skip this test case?
Found this comment: https://github.com/kuilpd/llvm-project/blob/6440b1028220955c510c7325bb6e27dc293f711a/lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py#L105
@Michael137 If I add a non-static member to that struct, would dsymutil keep the static member debug info as well?
I think if you used the address of the static maybe it would force dsymutil to keep it? Let me check
Hmm weirdly doesn't repro on my machine. Also only happens on the AArch64 bot (but not x86_64). Bit confused.
Skipped for now: https://github.com/llvm/llvm-project/commit/d74c9ef8370c9310452859ff876a2a5d8b8f7ad5