From fbc32ba76e51c75fd13ccfb63de77ca8977b5d46 Mon Sep 17 00:00:00 2001 From: Michael Buch Date: Tue, 1 Jul 2025 16:42:01 +0100 Subject: [PATCH 1/3] [lldb][Commands] image lookup: avoid double type lookup into current module 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 { }" ``` --- lldb/source/Commands/CommandObjectTarget.cpp | 24 +++++------ .../command-image-lookup-current-module.test | 43 +++++++++++++++++++ 2 files changed, 55 insertions(+), 12 deletions(-) create mode 100644 lldb/test/Shell/Commands/command-image-lookup-current-module.test 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 From 44e27c413adfbaa04c82bc50aa1a9a4d73e46cff Mon Sep 17 00:00:00 2001 From: Michael Buch Date: Wed, 2 Jul 2025 09:42:54 +0100 Subject: [PATCH 2/3] fixup! remove current_module logic --- lldb/source/Commands/CommandObjectTarget.cpp | 23 ++++++++----------- .../command-image-lookup-current-module.test | 1 + 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp index 97ed2bab802c8..fe421adfe8709 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; } - ModuleSP LookupHere(CommandInterpreter &interpreter, - CommandReturnObject &result, bool &syntax_error) { + bool 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 nullptr; + return false; case eLookupTypeType: break; } @@ -3963,29 +3963,29 @@ class CommandObjectTargetModulesLookup : public CommandObjectParsed { StackFrameSP frame = m_exe_ctx.GetFrameSP(); if (!frame) - return nullptr; + return false; const SymbolContext &sym_ctx(frame->GetSymbolContext(eSymbolContextModule)); if (!sym_ctx.module_sp) - return nullptr; + return false; switch (m_options.m_type) { default: - return nullptr; + return false; 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 sym_ctx.module_sp; + return true; } } break; } - return nullptr; + return false; } bool LookupInModule(CommandInterpreter &interpreter, Module *module, @@ -4089,9 +4089,7 @@ class CommandObjectTargetModulesLookup : public CommandObjectParsed { // 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. - ModuleSP current_module_sp = - LookupHere(m_interpreter, result, syntax_error); - if (current_module_sp) { + if (LookupHere(m_interpreter, result, syntax_error)) { result.GetOutputStream().EOL(); num_successful_lookups++; if (!m_options.m_print_all) { @@ -4110,8 +4108,7 @@ class CommandObjectTargetModulesLookup : public CommandObjectParsed { } for (ModuleSP module_sp : target_modules.ModulesNoLocking()) { - if (module_sp != current_module_sp && - LookupInModule(m_interpreter, module_sp.get(), result, + if (LookupInModule(m_interpreter, module_sp.get(), result, syntax_error)) { result.GetOutputStream().EOL(); num_successful_lookups++; diff --git a/lldb/test/Shell/Commands/command-image-lookup-current-module.test b/lldb/test/Shell/Commands/command-image-lookup-current-module.test index 52eec1c2b37f3..a6f6cbd3a594a 100644 --- a/lldb/test/Shell/Commands/command-image-lookup-current-module.test +++ b/lldb/test/Shell/Commands/command-image-lookup-current-module.test @@ -39,5 +39,6 @@ target modules lookup --type Foo --all # CHECK-NEXT: Best match found in # CHECK-NEXT: name = "Foo" +# CHECK: 1 match found in {{.*}}.out # CHECK: 1 match found in {{.*}}lib1.dylib # CHECK: 1 match found in {{.*}}lib2.dylib From 6cec208cb3f48d8233eb055d6f073c58b8690255 Mon Sep 17 00:00:00 2001 From: Michael Buch Date: Wed, 2 Jul 2025 09:44:01 +0100 Subject: [PATCH 3/3] fixup! remove test --- .../command-image-lookup-current-module.test | 44 ------------------- 1 file changed, 44 deletions(-) delete mode 100644 lldb/test/Shell/Commands/command-image-lookup-current-module.test diff --git a/lldb/test/Shell/Commands/command-image-lookup-current-module.test b/lldb/test/Shell/Commands/command-image-lookup-current-module.test deleted file mode 100644 index a6f6cbd3a594a..0000000000000 --- a/lldb/test/Shell/Commands/command-image-lookup-current-module.test +++ /dev/null @@ -1,44 +0,0 @@ -# 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 {{.*}}.out -# CHECK: 1 match found in {{.*}}lib1.dylib -# CHECK: 1 match found in {{.*}}lib2.dylib