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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


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


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1049
   for (const llvm::DWARFDebugLine::Sequence &seq : line_table->Sequences) {
+    if (seq.LowPC < m_first_code_address)
+      continue;
----------------
Add a comment here about attempting to avoid line tables that should have been dead stripped.


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


================
Comment at: lldb/unittests/SymbolFile/DWARF/Inputs/test-invalid-offsets.yaml:520
+          Data:            0
+...
----------------
aadsm wrote:
> I generated this yaml with the code that is at the top, and then changed it to replicate the bug ,here's what the line table looks like:
> 
> 
> ```
> Address            Line   Column File   ISA Discriminator Flags
> ------------------ ------ ------ ------ --- ------------- -------------
> 0x0000000100000f80      1      0      1   0             0  is_stmt
> 0x0000000100000f84      2      5      1   0             0  is_stmt prologue_end
> 0x0000000100000f8b      2      5      1   0             0  is_stmt end_sequence
> 0x0000000100000f90      5      0      1   0             0  is_stmt
> 0x0000000100000f90      5      0      1   0             0  is_stmt end_sequence
> 0x000000000000000f      2     13      1   0             0  is_stmt prologue_end
> 0x0000000000000014      2      9      1   0             0
> 0x0000000000000017      3     12      1   0             0  is_stmt
> 0x000000000000001d      3     12      1   0             0  end_sequence
> ```
> 
> Now that I think about it maybe I should also copy this into this file as a comment.
Would line table work just fine without your fix since the good address comes first? Or do we end up with multiple locations? I thought I remembered you thinking that it would pick the first one. Am I remembering correctly?


================
Comment at: lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp:74
   // codes that start at 1, that we get O(1) access.
-  
+
   const auto byte_order = eByteOrderLittle;
----------------
remove whitespace only changes please. Ditto for all whitespace only changes below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87172