[Lldb-commits] [lldb] [lldb][Commands] image lookup: avoid double type lookup into current module (PR #146554)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

<details>
<summary>Changes</summary>

The `current_module` pointer here was never set, but we check it when looping over the `target_modules` list. Presumably the intention was to avoid calling `LookupInModule` if we already found the type in the current module. This only affects `image lookup --all`.

This patch sets `current_module` if we successfully completed a lookup into it.

Before:
```
(lldb) im loo -vt Foo --all
Best match found in /Users/jonas/Git/llvm-worktrees/llvm-project/a.out:
id = {0x00000037}, name = "Foo", byte-size = 1, decl = foo.cpp:1, compiler_type = "struct Foo {
}"

1 match found in /Users/jonas/Git/llvm-worktrees/llvm-project/a.out:
id = {0x00000037}, name = "Foo", byte-size = 1, decl = foo.cpp:1, compiler_type = "struct Foo {
}"
```

After:
```
(lldb) im loo -vt Foo --all
Best match found in /Users/jonas/Git/llvm-worktrees/llvm-project/a.out:
id = {0x00000037}, name = "Foo", byte-size = 1, decl = foo.cpp:1, compiler_type = "struct Foo {
}"
```

---
Full diff: https://github.com/llvm/llvm-project/pull/146554.diff


2 Files Affected:

- (modified) lldb/source/Commands/CommandObjectTarget.cpp (+12-12) 
- (added) lldb/test/Shell/Commands/command-image-lookup-current-module.test (+43) 


``````````diff
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index a4ced37649ea0..97ed2bab802c8 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -3946,8 +3946,8 @@ class CommandObjectTargetModulesLookup : public CommandObjectParsed {
 
   Options *GetOptions() override { return &m_options; }
 
-  bool LookupHere(CommandInterpreter &interpreter, CommandReturnObject &result,
-                  bool &syntax_error) {
+  ModuleSP LookupHere(CommandInterpreter &interpreter,
+                      CommandReturnObject &result, bool &syntax_error) {
     switch (m_options.m_type) {
     case eLookupTypeAddress:
     case eLookupTypeFileLine:
@@ -3955,7 +3955,7 @@ class CommandObjectTargetModulesLookup : public CommandObjectParsed {
     case eLookupTypeFunctionOrSymbol:
     case eLookupTypeSymbol:
     default:
-      return false;
+      return nullptr;
     case eLookupTypeType:
       break;
     }
@@ -3963,29 +3963,29 @@ class CommandObjectTargetModulesLookup : public CommandObjectParsed {
     StackFrameSP frame = m_exe_ctx.GetFrameSP();
 
     if (!frame)
-      return false;
+      return nullptr;
 
     const SymbolContext &sym_ctx(frame->GetSymbolContext(eSymbolContextModule));
 
     if (!sym_ctx.module_sp)
-      return false;
+      return nullptr;
 
     switch (m_options.m_type) {
     default:
-      return false;
+      return nullptr;
     case eLookupTypeType:
       if (!m_options.m_str.empty()) {
         if (LookupTypeHere(&GetTarget(), m_interpreter,
                            result.GetOutputStream(), *sym_ctx.module_sp,
                            m_options.m_str.c_str(), m_options.m_use_regex)) {
           result.SetStatus(eReturnStatusSuccessFinishResult);
-          return true;
+          return sym_ctx.module_sp;
         }
       }
       break;
     }
 
-    return false;
+    return nullptr;
   }
 
   bool LookupInModule(CommandInterpreter &interpreter, Module *module,
@@ -4086,12 +4086,12 @@ class CommandObjectTargetModulesLookup : public CommandObjectParsed {
     // Dump all sections for all modules images
 
     if (command.GetArgumentCount() == 0) {
-      ModuleSP current_module;
-
       // Where it is possible to look in the current symbol context first,
       // try that.  If this search was successful and --all was not passed,
       // don't print anything else.
-      if (LookupHere(m_interpreter, result, syntax_error)) {
+      ModuleSP current_module_sp =
+          LookupHere(m_interpreter, result, syntax_error);
+      if (current_module_sp) {
         result.GetOutputStream().EOL();
         num_successful_lookups++;
         if (!m_options.m_print_all) {
@@ -4110,7 +4110,7 @@ class CommandObjectTargetModulesLookup : public CommandObjectParsed {
       }
 
       for (ModuleSP module_sp : target_modules.ModulesNoLocking()) {
-        if (module_sp != current_module &&
+        if (module_sp != current_module_sp &&
             LookupInModule(m_interpreter, module_sp.get(), result,
                            syntax_error)) {
           result.GetOutputStream().EOL();
diff --git a/lldb/test/Shell/Commands/command-image-lookup-current-module.test b/lldb/test/Shell/Commands/command-image-lookup-current-module.test
new file mode 100644
index 0000000000000..52eec1c2b37f3
--- /dev/null
+++ b/lldb/test/Shell/Commands/command-image-lookup-current-module.test
@@ -0,0 +1,43 @@
+# REQUIRES: system-darwin
+
+# RUN: split-file %s %t
+# RUN: %clang_host -g -gdwarf %t/lib1.cpp -shared -o %t-lib1.dylib
+# RUN: %clang_host -g -gdwarf %t/lib2.cpp -shared -o %t-lib2.dylib
+# RUN: %clang_host -g -gdwarf %t/main.cpp %t-lib1.dylib %t-lib2.dylib -o %t.out
+# RUN: %lldb -x -b -s %t/commands.input %t.out -o exit 2>&1 \
+# RUN:       | FileCheck %s
+
+#--- main.cpp
+
+struct Foo {} f;
+
+int main() { __builtin_debugtrap(); }
+
+#--- lib1.cpp
+
+struct Foo {} f1;
+
+#--- lib2.cpp
+
+struct Foo {} f2;
+
+#--- commands.input
+
+run
+target modules lookup --type Foo
+
+# CHECK:      (lldb) target modules lookup --type Foo
+# CHECK-NEXT: Best match found in
+# CHECK-NEXT: name = "Foo"
+
+# Confirm we only dumped the match once.
+# CHECK-NOT:  name = "Foo"
+
+target modules lookup --type Foo --all
+
+# CHECK: (lldb) target modules lookup --type Foo --all
+# CHECK-NEXT: Best match found in
+# CHECK-NEXT: name = "Foo"
+
+# CHECK: 1 match found in {{.*}}lib1.dylib
+# CHECK: 1 match found in {{.*}}lib2.dylib

``````````

</details>


https://github.com/llvm/llvm-project/pull/146554