[lldb] Use Function::GetAddress in Module::FindFunctions

[labath]

The original code resulted in a misfire in the symtab vs. debug info deduplication code, which caused us to return the same function twice when searching via a regex (for functions whose entry point is also not the lowest address).

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

The original code resulted in a misfire in the symtab vs. debug info deduplication code, which caused us to return the same function twice when searching via a regex (for functions whose entry point is also not the lowest address).

---

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

2 Files Affected:

• (modified) lldb/source/Core/Module.cpp (+2-3)
• (modified) lldb/test/Shell/SymbolFile/DWARF/x86/discontinuous-function.s (+16-4)

diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 9601c834d9b8fe..33668c5d20dde4 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -919,9 +919,8 @@ void Module::FindFunctions(const RegularExpression &regex,
               const SymbolContext &sc = sc_list[i];
               if (sc.block)
                 continue;
-              file_addr_to_index[sc.function->GetAddressRange()
-                                     .GetBaseAddress()
-                                     .GetFileAddress()] = i;
+              file_addr_to_index[sc.function->GetAddress().GetFileAddress()] =
+                  i;
             }
 
             FileAddrToIndexMap::const_iterator end = file_addr_to_index.end();
diff --git a/lldb/test/Shell/SymbolFile/DWARF/x86/discontinuous-function.s b/lldb/test/Shell/SymbolFile/DWARF/x86/discontinuous-function.s
index 93ea9f33e762df..197ab9aa14910e 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/x86/discontinuous-function.s
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/discontinuous-function.s
@@ -6,17 +6,29 @@
 # The function bar has been placed "in the middle" of foo, and the function
 # entry point is deliberately not its lowest address.
 
-# RUN: llvm-mc -triple x86_64-pc-linux -filetype=obj %s -o %t
-# RUN: %lldb %t -o "image lookup -v -n foo" -o "expr -- &foo" -o exit | FileCheck %s
+# RUN: split-file %s %t
+# RUN: llvm-mc -triple x86_64-pc-linux -filetype=obj %t/input.s -o %t/input.o
+# RUN: %lldb %t/input.o -s %t/commands -o exit | FileCheck %s
 
-# CHECK-LABEL: image lookup
+#--- commands
+
+image lookup -v -n foo
+# CHECK-LABEL: image lookup -v -n foo
+# CHECK: 1 match found in {{.*}}
+# CHECK: Summary: input.o`foo
+# CHECK: Function: id = {{.*}}, name = "foo", ranges = [0x0000000000000000-0x000000000000000e)[0x0000000000000014-0x000000000000001c)
+
+image lookup -v --regex -n '^foo$'
+# CHECK-LABEL: image lookup -v --regex -n '^foo$'
 # CHECK: 1 match found in {{.*}}
-# CHECK: Summary: {{.*}}`foo
+# CHECK: Summary: input.o`foo
 # CHECK: Function: id = {{.*}}, name = "foo", ranges = [0x0000000000000000-0x000000000000000e)[0x0000000000000014-0x000000000000001c)
 
+expr -- &foo
 # CHECK-LABEL: expr -- &foo
 # CHECK: (void (*)()) $0 = 0x0000000000000007
 
+#--- input.s
         .text
 
 foo.__part.1:

[jimingham]

This looks fine, but more generally, is there any good reason to ever use Function::GetAddressRange? Shouldn't that just be removed and all the instances replaced with either GetAddress or get the ranges and iterate over them looking for whatever they wanted?

[labath]

There isn't a good reason for using GetAddressRange, and removing it is what I'm trying to do. But rather than trying to do it all at once, I'm doing it in pieces as some of the changes (particularly those that need to iterate over a set of ranges) are not trivial. This way I can also verify that each change does what it's supposed to do.

I changed most of the trivial call sites when I introduced GetAddress (in #115836). In retrospect, this is probably trivial enough that it could have been a part of that, but I skipped it then because the mapping part looked scary.

[jimingham]

LGTM. You might add a comment to GetAddressRange saying "I'm in the process of getting rid of this, don't use it in the meantime...". But presumably you are also watching PR's to make sure nobody reintroduces its use.

[labath]

I added a "DEPRECATED" comment, which isn't quite as strong, but sort of conveys the same thing. The function is called from a bunch of places, but I don't think there's that much churn around it for backsliding to be a problem.