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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

In D154638#4478742 <https://reviews.llvm.org/D154638#4478742>, @rastogishubham wrote:

> In D154638#4478628 <https://reviews.llvm.org/D154638#4478628>, @avl wrote:
>
>> Could you compare size of .debug_info, .debug_addr sections for clang binary with and without this patch, please?
>
> Of course!
>
> The size of a clang.dSYM debug_info section built with `-gdwarf-5` without this patch is 930,375,162 Bytes or 930 MB
>
> The size of a clang.dSYM debug_info + debug_addr section built with `-gdwarf-5` with this patch is 914,086,636 Bytes or 914 MB
>
> I would also like to note, that at the moment I am not optimizing for the addrx values, i.e. no work is done to sort the addrx values by frequency to optimize the size of the addrx values in the debug info section based on frequency, and we still get a modest 1.8% decrease in size

Thank you! yep, 1.8% is a good result.

How about changing DW_RLE_start_length/DW_LLE_start_length into DW_RLE_startx_length/DW_LLE_startx_length for rnglists/loclists? Or this is for another patch?



================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:100
 
+  virtual void emitAddrs(const std::vector<uint64_t> &Addrs,
+                         uint8_t AddrSize) = 0;
----------------
add a Doxygen 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;
----------------
llvm coding standard suggests to use SmallVector instead std::vector. https://llvm.org/docs/CodingStandards.html#c-standard-library


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1393
+      AddrPool.Addrs.push_back(*Addr);
+    }
   }
----------------
probably it would make sense to hide this code under method of DebugAddrPool?

```

uint64_t DebugAddrPool::getAddrIndex ( uint64_t Addr ) {
   DenseMap<uint64_t, uint64_t>::iterator It = AddrPool.AddrIndexMap.find(Addr);

    if (It == AddrPool.AddrIndexMap.end()) {
      AddrPool.Addrs.push_back(*Addr);
      It = AddrPool.AddrIndexMap.insert(std::make_pair(Addr, AddrPool.Addrs.size()).first;
    }

    return It.second;
}
```


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1400
+                              ? AddrPool.AddrIndexMap[AddrxIndex]
+                              : *Addr));
+
----------------
assuming DebugAddrPool::getAddrIndex exist:


```
               DIEInteger(AddrAttribute->getForm() == dwarf::DW_FORM_addr
                              ? *Addr 
                              :  DebugAddrPool::getAddrIndex(*Addr));

```


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2579
+      if (DwarfVersion >= 5)
+        Linker.emitDebugAddrSection(*CurrentUnit, AddrPool.Addrs);
+      AddrPool.clear();
----------------
probably, the dwarf::DW_AT_addr_base should be updated here with the proper offset to .debug_addr section.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2725
+  DenseMap<uint64_t, uint64_t> AddrIndexMap;
+  std::vector<uint64_t> Addrs;
 
----------------
It looks like AddrIndexMap&Addrs may be local for compile unit. i.e., not necessary to make them global.


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:278
+  for (auto Addr : Addrs) {
+    Asm->OutStreamer->emitIntValue(Addr, AddrSize);
+  }
----------------
it seems AddrSectionSize should be updated here.


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

https://reviews.llvm.org/D154638