[PATCH] D32618: DWARF: Implementation of v5 string offsets table (.debug_str_offsets[.dwo] section)

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Thanks! The patch may be large, but the changes are mostly straightforward and generally look good to me. I found a couple of nits inline.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:241
 
+  // DWARF v5
+  virtual const DWARFSection &getStringOffsetSection() = 0;
----------------
```
/// DWARF 5.
/// @{
...
/// @}
```


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:217
   const RelocAddrMap *getRelocMap() const { return &InfoSection.Relocs; }
+  const RelocAddrMap *getStringOffsetsRelocMap() const {
+    return &StringOffsetSection.Relocs;
----------------
`const RelocAddrMap &getStringOffsetsRelocMap() const`?


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:618
+
+/// EmitValue - Emit string value.
+///
----------------
http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
wants us to comment in on place only. This should be a doxygen comment and it should be on the function prototype inside DIEIndexedString.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:80
+      CUNode(Node), Asm(A), DD(DW), DU(DWU), IndexTyDie(nullptr),
+      StringOffsetsStartSym(nullptr) {}
 
----------------
= nullptr?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1592
 
+// Emit the unit's contribution to the string offsets table.
+void DwarfUnit::emitStringOffsets() {
----------------
Move comment into header.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.h:314
 
+  /// addSectionDelta - Add a label delta attribute data and value.
+  DIE::value_iterator addSectionDelta(DIE &Die, dwarf::Attribute Attribute,
----------------
`/// Add a label delta attribute data and value.`


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.h:318
+
+  /// addSectionLabel - Add a Dwarf section label attribute data and value.
+  ///
----------------
ditto


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:102
+    uint64_t SegmentSize = StrOffsetExt.getU32(&Offset);
+    if (SegmentSize > 0xfffffff0 && SegmentSize < 0xffffffff)
+      return;
----------------
comment why?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:104
+      return;
+    else if (SegmentSize == 0xffffffff) {
+      if (!StrOffsetExt.isValidOffsetForDataOfSize(Offset, 8))
----------------
drop the else


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:314
+  if (DumpType == DIDT_All || DumpType == DIDT_StrOffsetsDwo) {
+    // If we have at least one unit with DWARF Version 5 or greater, we assume
+    // that the DWO string offsets section is formatted like the regular string
----------------
"DWARF 5" vs "DWARF v5" vs "DWARF Version 5"?


https://reviews.llvm.org/D32618