[Lldb-commits] [PATCH] D87172: Check if debug line sequences are starting after the first code segment

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:503-505
+  if (section_list) {
+    InitializeFirstCodeAddress(*section_list);
+  }
----------------
clayborg wrote:
> remove braces
or even fold the declaration into the if condition.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:520
   std::vector<uint32_t> m_lldb_cu_to_dwarf_unit;
+  lldb::addr_t m_first_code_address = LLDB_INVALID_ADDRESS;
 };
----------------
aadsm wrote:
> labath wrote:
> > clayborg wrote:
> > > A lengthy comment would be great here. Maybe something like:
> > > 
> > > ```
> > > /// Many linkers will concatenate all available DWARF, even if parts of that DWARF
> > > /// should have been dead stripped. Some linkers will place tombstone values in for
> > > /// addresses that should have been dead stripped, with a value like -1 or -2. But many
> > > /// linkers will apply a relocation and attempt to set the value to zero. This is problematic
> > > /// in that we can end up with many addresses that are zero or close to zero due to there
> > > /// being an addend on the relocation whose base is at zero. Here we attempt to avoid 
> > > /// addresses that are zero or close to zero by finding the first valid code address by looking
> > > /// at the sections and finding the first one that has read + execute permissions. This allows
> > > /// us to do a quick and cheap comparison against this address when parsing line tables and
> > > /// parsing functions where the DWARF should have been dead stripped.
> > > ```
> > To me, "should have been stripped" implies that the linkers are doing something wrong, as if they were required to parse and rewrite the dwarf in the input files. However, there is nothing in the DWARF or ELF specs that would support that, and the linkers are simply doing what the specs say, and what linkers have been doing since the dawn of time -- concatenate things.
> > 
> > It would be better to just state the facts here, instead of passing judgement (or make an appearance of doing that).
> @clayborg can you clarify this comment like @labath suggests?
How about this:
```
DWARF does not provide a good way for traditional (concatenating) linkers to invalidate debug info describing dead-stripped code. These linkers will keep the debug info but resolve any addresses referring to such code as zero (BFD), a small positive integer (zero + relocation addend -- GOLD), or -1/-2 (LLD). Try to filter out this debug info by comparing it to the lowest code address in the module.
```


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/line-entries-invalid-addresses.yaml:3
+# RUN: %lldb %t -b -o "image lookup -f main.cpp -l 2 -v" | FileCheck %s
+# CHECK: LineEntry: {{.*}}main.cpp:2:{{.*}}
+
----------------
aadsm wrote:
> labath wrote:
> > I think you also need to check for the "1 matches found", otherwise this will succeed even without the patch.
> This should be fine, this is what you get without the patch:
> 
> ```
> (lldb) image lookup -f main.cpp -l 2 -v
> 1 match found in main.cpp:2 in /Users/aadsm/Projects/test-bad.obj:
>         Address:  ()
>         Summary:
> ```
True, but adding the check still wouldn't hurt, as one can imagine a bug that would cause the second main.cpp:2 entry (at address 0xf) to make its way here -- and that's what we're trying to avoid.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87172/new/

https://reviews.llvm.org/D87172