[PATCH] D130315: [BOLT][DWARF] Implement new mechanism for DWARFRewriter

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo added a comment.

In D130315#4480361 <https://reviews.llvm.org/D130315#4480361>, @gribozavr2 wrote:

> Hi,
>
> This commit introduces a null pointer dereference in a unit test, so I'm going to revert it together with all dependent commits:
>
>   $ lldb -- ./unittests/DebugInfo/DWARF/DebugInfoDWARFTests --gtest_filter=DWARFDebugInfo.TestDwarfToFunctions
>   This is google-lldb.
>   Help: http://go/lldb. File a bug: http://go/lldb-bug.
>   (lldb) target create "./unittests/DebugInfo/DWARF/DebugInfoDWARFTests"
>   Current executable set to '/usr/local/google/home/dmitrig/llvm2/build-debug/unittests/DebugInfo/DWARF/DebugInfoDWARFTests' (x86_64).
>   (lldb) settings set -- target.run-args  "--gtest_filter=DWARFDebugInfo.TestDwarfToFunctions"
>   (lldb) r
>   Process 2771099 launched: '/usr/local/google/home/dmitrig/llvm2/build-debug/unittests/DebugInfo/DWARF/DebugInfoDWARFTests' (x86_64)
>   Note: Google Test filter = DWARFDebugInfo.TestDwarfToFunctions
>   [==========] Running 1 test from 1 test suite.
>   [----------] Global test environment set-up.
>   [----------] 1 test from DWARFDebugInfo
>   [ RUN      ] DWARFDebugInfo.TestDwarfToFunctions
>   Process 2771099 stopped
>   * thread #1, name = 'DebugInfoDWARFT', stop reason = signal SIGSEGV: address not mapped to object (fault address: 0x20)
>       frame #0: 0x0000555555c0d41c DebugInfoDWARFTests`llvm::DWARFUnitHeader::getVersion(this=0x0000000000000018) const at DWARFUnit.h:89:33
>      86     bool applyIndexEntry(const DWARFUnitIndex::Entry *Entry);
>      87     uint64_t getOffset() const { return Offset; }
>      88     const dwarf::FormParams &getFormParams() const { return FormParams; }
>   -> 89     uint16_t getVersion() const { return FormParams.Version; }
>      90     dwarf::DwarfFormat getFormat() const { return FormParams.Format; }
>      91     uint8_t getAddressByteSize() const { return FormParams.AddrSize; }
>      92     uint8_t getRefAddrByteSize() const { return FormParams.getRefAddrByteSize(); }
>   (lldb) up
>   frame #1: 0x000055555810bc59 DebugInfoDWARFTests`llvm::DWARFUnit::getVersion(this=0x0000000000000000) const at DWARFUnit.h:323:47
>      320    const dwarf::FormParams &getFormParams() const {
>      321      return Header.getFormParams();
>      322    }
>   -> 323    uint16_t getVersion() const { return Header.getVersion(); }
>      324    uint8_t getAddressByteSize() const { return Header.getAddressByteSize(); }
>      325    uint8_t getRefAddrByteSize() const { return Header.getRefAddrByteSize(); }
>      326    uint8_t getDwarfOffsetByteSize() const {
>   (lldb) up
>   frame #2: 0x00005555581953a3 DebugInfoDWARFTests`llvm::DWARFFormValue::getAsSectionedAddress(Value=0x00007fffffffd2e8, Form=0, U=0x0000000000000000) at DWARFFormValue.cpp:642:51
>      639
>      640  std::optional<object::SectionedAddress> DWARFFormValue::getAsSectionedAddress(
>      641      const ValueType &Value, const dwarf::Form Form, const DWARFUnit *U) {
>   -> 642    if (!doesFormBelongToClass(Form, FC_Address, U->getVersion()))
>      643      return std::nullopt;
>      644    bool AddrOffset = Form == dwarf::DW_FORM_LLVM_addrx_offset;
>      645    if (Form == DW_FORM_GNU_addr_index || Form == DW_FORM_addrx ||
>   (lldb) p U
>   (const llvm::DWARFUnit *) nullptr
>
> Please also note that https://github.com/llvm/llvm-project/commit/460a2244430fae192298a5fd9fa2a269e540e8c1 was landed with an incorrect Phabricator link.
>
> This commit also needed numerous fixup commits, which is a good signal that this commit chain needs better testing before landing.

That is why it's part of the stack. Original diff was done by someone else. I commandeered it. All the fixes are in subsequent commit to separate work done. Thus I didn't squash into one commit.
I tried it locally, and for me it passes. Either on full stack or on this diff.
F28160346: image.png <https://reviews.llvm.org/F28160346>

Although looking at stack trace I see where it goes wrong. 
I can change 
if (!doesFormBelongToClass(Form, FC_Address, U->getVersion()))
to
if (!U&&!isFormClass(FC_Address) || !doesFormBelongToClass(Form, FC_Address, U->getVersion()))
which should restore to old behavior in case U is nullptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130315