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

[António Afonso via Phabricator via lldb-commits]

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