[LLDB] Optimize identifier lookup in DIL #146094
Conversation
|
@llvm/pr-subscribers-lldb Author: Ilia Kuklin (kuilpd) ChangesRemove unused code and unnecessary function calls, optimize global variable search. Full diff: https://github.com/llvm/llvm-project/pull/146094.diff 6 Files Affected:
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
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 |
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
Looks mostly good, I'm just not sure about this part:
or return any partial match found
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?
It also doesn't seem like it matters for performance, as it's just a check on the size of the candidate list.
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.
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.
I think this is the right order at least (variables in the current CU should resolve first), and I definitely wouldn't want to search throught all (potentially thousands) of CUs and then try to pick out the variables from the current one. 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.
| 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()) { |
There was a problem hiding this comment.
I'd just delete this check. The function does the right thing when called with an empty list, and I doubt it's going to be faster.
Well, my logic was that it's better to return something than nothing, similar to what current
I tried doing |
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
... but if the current implementation does the same thing, then I think it's good enough for now.
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 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 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 |
|
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: d74c9ef |
|
Alright, thank you! |
4 participants
Remove unused code and unnecessary function calls, optimize global variable search.
Add more test cases.