aadsm added inline comments.
================
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;
};
----------------
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?
================
Comment at:
lldb/unittests/SymbolFile/DWARF/Inputs/test-invalid-addresses.yaml:203-254
+ - sectname: __apple_names
+ segname: __DWARF
+ addr: 0x000000010000223C
+ size: 116
+ offset: 0x0000223C
+ align: 0
+ reloff: 0x00000000
----------------
labath wrote:
> I guess these (and debug_pub(types/names), and possible debug_aranges) are
> not really needed for this test.
I changed the line table by hand but forgot to add comments there.
================
Comment at: lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp:349-369
+
+TEST_F(SymbolFileDWARFTests, EnsureLineEntriesExistInAfterFirstCodeSection) {
+ auto ExpectedFile = TestFile::fromYamlFile("test-invalid-addresses.yaml");
+ ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded());
+
+ lldb::ModuleSP module_sp =
+ std::make_shared<Module>(ExpectedFile->moduleSpec());
----------------
labath wrote:
> I think this would be better as a shell test (if for nothing else, then
> because other line table tests are written that way). `image lookup` is
> pretty much a direct equivalent to `ResolveSymbolContext`, but its output is
> more readable, and its easier to fiddle with these kinds of tests.
I think what you describe is an end to end test but here I explicitly want to
test the SymbolFileDWARF code because that's what I changed, it's more like a
unit test.
I would prefer to stay away from an end to end test for this particular change
because 1) knowing that `image lookup` calls `ResolveSymbolContext` is an
implementation detail 2) The output of `image lookup` could change over time,
3) Other bugs unrelated to this could be introduced that will make `image
lookup` show a different content as well and fail the test.
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
