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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
