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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:101
+  /// Emit the address described by \p Addrs into the .debug_addr section.
+  virtual void emitAddrs(const SmallVector<uint64_t> &Addrs,
+                         uint8_t AddrSize) = 0;
----------------
nit: move this definition close to emitDwarfDebugAddrFooter so that methods emitting DebugAddr were defined together, please. 

probably name them using the same pattern:

emitDwarfDebugAddrsHeader
emitDwarfDebugAddrs
emitDwarfDebugAddrsFooter


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:142
+  virtual MCSymbol *emitDwarfDebugAddrHeader(const CompileUnit &Unit) = 0;
+
   /// Emit debug locations (.debug_loc, .debug_loclists) fragment.
----------------
nit: move this definition close to emitDwarfDebugAddrFooter so that methods emitting DebugAddr were defined together, please.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:831
 
+  void emitDebugAddrSection(CompileUnit &Unit,
+                            SmallVector<uint64_t> &Addrs) const;
----------------
add Doxigen comment please.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:642
+      DenseMap<uint64_t, uint64_t> &AddrIndexMap;
+      std::vector<uint64_t> &Addrs;
+      uint64_t Index;
----------------
rastogishubham wrote:
> avl wrote:
> > llvm coding standard suggests to use SmallVector instead std::vector. https://llvm.org/docs/CodingStandards.html#c-standard-library
> I was just being consistent because `DIECloner::DIECloner()` takes a `std::vector<std::unique_ptr<CompileUnit>> &CompileUnits,` maybe that should be changed to? 
yes, I think it(replacing std::vector with SmallVector for CompileUnits) would be good. But with separate cleanup patch.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1396
+             : AttrSize;
 }
 
----------------
probably, to not check for AttrSpec.Form == dwarf::DW_FORM_addr twice we can use early return pattern?


```
if (AttrSpec.Form == dwarf::DW_FORM_addr) {
  Die.addValue(DIEAlloc, AttrSpec.Attr, AttrSpec.Form, DIEInteger(*Addr));
  return Unit.getOrigUnit().getAddressByteSize();
}

Die.addValue(DIEAlloc, AttrSpec.Attr, AttrSpec.Form, DIEInteger(AddrPool.getAddrIndex(*Addr)));
return AttrSize;
```

Another thing, as we generate a new .debug_addr section, its size may be changed and original strictly sized forms (f.e. DW_FORM_addrx1) may become insufficient. On the other side, as we usually only remove addresses everything should be fine. It is probably worth adding an assertion here checking that AddrPool.getAddrIndex(*Addr) fits into the original form. f.e.


```
assert ((AttrSpec.Form == dwarf::DW_FORM_addrx1 && AddrPool.getAddrIndex(*Addr) <= UINT8_MAX)
            || other cases
```


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1659
     return !Update && SkipPC;
   case dwarf::DW_AT_addr_base:
+    return false;
----------------
as it matches with default we can just remove this case.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2571
+        Linker.emitDebugAddrSection(*CurrentUnit, AddrPool.Addrs);
+      AddrPool.clear();
     }
----------------
No need to do clear() as AddrPool is local for DIECloner.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2579
+      if (DwarfVersion >= 5)
+        Linker.emitDebugAddrSection(*CurrentUnit, AddrPool.Addrs);
+      AddrPool.clear();
----------------
rastogishubham wrote:
> avl wrote:
> > probably, the dwarf::DW_AT_addr_base should be updated here with the proper offset to .debug_addr section.
> Correct me if I am wrong, but there doesn't seem to be a way to modify the value of an attribute in the die after it has already been set right?
The cloned value of DW_AT_addr_base may be incorrect as we generate new .debug_addr, so it should be updated. It may be changed inside DWARFLinker::emitDebugAddrSection() the same way as DW_AT_stmt_list is updated inside DIECloner::generateLineTableForUnit():


```
static void patchStmtList(DIE &Die, DIEInteger Offset) {
  for (auto &V : Die.values())
    if (V.getAttribute() == dwarf::DW_AT_stmt_list) {
      V = DIEValue(V.getAttribute(), V.getForm(), Offset);
      return;
    }

  llvm_unreachable("Didn't find DW_AT_stmt_list in cloned DIE!");
}

```


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

https://reviews.llvm.org/D154638