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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1393-1405
   Die.addValue(DIEAlloc, static_cast<dwarf::Attribute>(AttrSpec.Attr),
-               AttrSpec.Form, DIEInteger(*Addr));
-  return Unit.getOrigUnit().getAddressByteSize();
+               AttrSpec.Form, DIEInteger(AddrPool.getAddrIndex(*Addr)));
+
+  assert((AttrSpec.Form == dwarf::DW_FORM_addrx) ||
+         (AttrSpec.Form == dwarf::DW_FORM_addrx1 &&
+          AddrPool.getAddrIndex(*Addr) <= UINT8_MAX) ||
+         (AttrSpec.Form == dwarf::DW_FORM_addrx2 &&
----------------
JDevlieghere wrote:
> Let's extract `AddrPool.getAddrIndex(*Addr)` and assign it to a variable that you can reuse in the assert. Also, is the assert actually checking internal consistency or catching invalid DWARF? If it's the latter we should emit an error/warning instead. 
The purpose for the assert is to check internal consistency - newly generated addresses set should fit within the used form. We use original form for the newly generated addresses set as we assume that we do not add addresses and then original form should be enough. The assert proves this assumption.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2011
+
+  llvm_unreachable("Didn't find DW_AT_addr_base in cloned DIE!");
+}
----------------
JDevlieghere wrote:
> Similar question as for the assert: is this checking internal consistency (i.e. that we have created a `DW_AT_addr_base` in the cloned DIE) or can this trip for invalid input?
This is also should be check for internal consistency: We should not miss DW_AT_addr_base when we generate .debug_addr. The patchAddrBase is called from DIECloner::emitDebugAddrSection, thus we check for DW_AT_addr_base exactly when we are going to generate .debug_addr. The only problem with current implementation is this check will fail when original DWARF does not have .debug_addr but we call emitDebugAddrSection for generation of empty .debug_addr. The check for empty addresses set needs to be added into emitDebugAddrSection.

For the case of invalid DWARF(DW_AT_addr_base missed in input file) - we should not generate any addresses as we could not correctly read original addresses. DWARFLinker should report warning here and do not generate any address:

DIECloner::cloneAddressAttribute
```
  std::optional<uint64_t> Addr = AddrAttribute->getAsAddress();
  if (!Addr) {
    Linker.reportWarning("Cann't read address attribute value.", ObjFile);
    Addr = 0;
  }

```

Though, this place looks a bit incorrect. Instead of using zero Addr, the attribute should probably be skipped.


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

https://reviews.llvm.org/D154638