[PATCH] D154638: Emit a .debug_addr section with dsymutil

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

yep, thank you for the update.

Would you mind adding test for DW_AT_addr_base, please ? i.e. test having f.e. 3 compile units and testing that each DW_AT_addr_base attribute points to the proper part of .debug_addr section. Also, it looks like all(or probably most of) existing dwarf5 tests need adding checks for DW_AT_addr_base.



================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:149
+
+  /// Emit the address described by \p Addrs into the .debug_addr section.
+  virtual void emitDwarfDebugAddrs(const SmallVector<uint64_t> &Addrs,
----------------



================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:646
+      SmallVector<uint64_t> Addrs;
+      uint64_t Index;
+
----------------
This field is not used. we can safely remove it.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:650
+
+      void clear() {
+        AddrIndexMap.clear();
----------------
We do not need to use this method it can be removed.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:713
+    void emitDebugAddrSection(CompileUnit &Unit, SmallVector<uint64_t> &Addrs,
+                              uint64_t &AddrSecSize,
+                              const uint16_t DwarfVersion) const;
----------------
No need to pass AddrSecSize as a parameter. We already have it inside.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFStreamer.h:113
+
+  /// Emit the addresses described by \p Pool into .debug_addr table.
+  void emitDwarfDebugAddrs(const SmallVector<uint64_t> &Addrs,
----------------



================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1402
+         (AttrSpec.Form == dwarf::DW_FORM_addrx3 &&
+          AddrPool.getAddrIndex(*Addr) <= (UINT16_MAX + UINT8_MAX)) ||
+         (AttrSpec.Form == dwarf::DW_FORM_addrx4 &&
----------------
it looks like this constant is not correct:

UINT16_MAX + UINT8_MAX

The proper one would be:

(UINT32_MAX >> 8)


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2027
+}
+
 /// Insert the new line info sequence \p Seq into the current
----------------
Original version of patch had a check for "-u" option. I think we still need it here. Also no need to remember and store AddrSecSize:


```
  if (LLVM_UNLIKELY(Options.Update))
    return;

  if (DwarfVersion < 5)
    return;

  MCSymbol *EndLabel = Emitter->emitDwarfDebugAddrsHeader(Unit);

  patchAddrBase(*Unit.getOutputUnitDIE(), DIEInteger(Emitter->getDebugAddrSectionSize()));

  Emitter->emitDwarfDebugAddrs(Addrs, Unit.getOrigUnit().getAddressByteSize());
  Emitter->emitDwarfDebugAddrsFooter(Unit, EndLabel);
```


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2548
   const uint64_t StartOutputDebugInfoSize = OutputDebugInfoSize;
+  uint64_t AddrSectionSize = 0;
 
----------------
AddrSectionSize could be removed.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2596
+                           DwarfVersion);
+      AddrPool.clear();
     }
----------------
No need to clear AddrPool as it is local to DIECloner.


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:612
+  if (Unit.getOrigUnit().getVersion() < 5)
+    return nullptr;
+
----------------
No need to check the version again. It is already checked in DIECloner::emitDebugAddrSection(). 


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:641
+
+/// Emit the debug_addr section stored in \p Pool.
+void DwarfStreamer::emitDwarfDebugAddrs(const SmallVector<uint64_t> &Addrs,
----------------



================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:655
+  if (Unit.getOrigUnit().getVersion() < 5)
+    return;
+
----------------
No need to check the version again. It is already checked in DIECloner::emitDebugAddrSection().


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

https://reviews.llvm.org/D154638