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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This is fixing the same issue that I tried to fix with D84402 <https://reviews.llvm.org/D84402>, but then failed to get back to it when the discussion about the best approach started getting long.  While I do have some reservations about this approach, it is definitive improvement than the status quo, so I won't start bikeshedding the design (but do see the inline comments)..



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:437
+      m_supports_DW_AT_APPLE_objc_complete_type(eLazyBoolCalculate) {
+  auto first_code_section_sp =
+      m_objfile_sp->GetModule()->GetSectionList()->FindSectionByType(
----------------
This is really the _first_ text section (as listed in the section headers), is it not? But, I assume you really want the section with the lowest file address?

The linkers (and other producers) will normally output the sections in ascending address order (because it's the simplest thing to do). However, I don't believe that is actually required by anything, and I /think/ it should be possible (with a moderately funky linker script) to produce a fully functional executable which does not have the sections listed in this order.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:437-441
+  auto first_code_section_sp =
+      m_objfile_sp->GetModule()->GetSectionList()->FindSectionByType(
+          eSectionTypeCode, true, 0);
+  if (first_code_section_sp)
+    m_first_code_address = first_code_section_sp->GetFileAddress();
----------------
clayborg wrote:
> This should be fine. Any symbol files that might get added after the fact really need to have matching sections or nothing will make sense.
Doing this in `InitializeObject` would be better.


================
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;
 };
----------------
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).


================
Comment at: lldb/unittests/SymbolFile/DWARF/Inputs/test-invalid-addresses.yaml:1-8
+# int foo() {
+#     return 42;
+# }
+#
+# int main() {
+#     int x = foo();
+#     return x;
----------------
Is this the actual source code, or you've made some changes to it (like, to change the line table start address)?


================
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
----------------
I guess these (and debug_pub(types/names), and possible debug_aranges) are not really needed for this test.


================
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());
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87172