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