Improved Macro emission in dwarf

[aaboud]

Summary

Followed David feedback on emitting offset of macinfo entry into compiler unit DIE.
New code is using delta between labels rather than explicitly calculating size/offset of macro entry.

[aaboud]

Reverted the new structures AsmStreamerBase, ...
As they are not needed anymore.

[aprantl]

Thanks for going through this again! I'll defer to dblaikie who is more familiar with this code than I am.

[dblaikie]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

619

This seems wrong - a delta isn't a section offset. Once this DWARF is relocated (eg: linked with another object file containing DWARF) these deltas would become incorrect.

A label delta is constant, not a dynamic/link time relocation - representing the distance between two labels. That doesn't change when the debug info is linked and multiple macinfo sections are concatenated together.

But DWARF requires the sec_offset to represent the final offset within the section of the linked debug info. So I assume, much like DW_AT_stmt_list and DW_AT_ranges. Like those two attributes, I assume this one should be emitted using "addSectionOffset" to ensure it has the right kind of relocation, etc. (& on MachO it doesn't use a reloc - addSectionOffset handles that case too)

lib/CodeGen/AsmPrinter/DwarfDebug.h

211

Rather than adding a map, the MCSymbol should probably just be part of the DwarfCompileUnit itself?

(you can check DwarfCompileUnit::attachRangesOrLowHighPC, used to add DW_AT_ranges to the DW_TAG_compile_unit (& elsewhere) for some inspiration - though it won't match that exactly (that one builds a range list, keeps a label for it, attaches a reloc to that label, later on emits the range list with the label - some of that should be similar to what you need/want, without the need to build an intermediate data structure like the range list))

[aaboud]

Applied changes according to David comments.

[aaboud]

Thanks David for the comments.
I uploaded a new patch.

Please, let me know if this looks right now.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

619

I should check more for similar cases before highjack the addLabelDelta. Thanks for pointing that out.

Also, I had a bug there, because I wanted to calculate the offset from the beginning of the section and not from the previous section!

I reverted the change in addLabelDelta function and now is using addSectionDelta function, hope that it is the right one.

do you agree?

[dblaikie]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

564

We don't support non-existence of other DWARF sections (we don't generally check for null return from TLOF.getDwarf* functions elsewhere, so far as I can see/recall) - is there any reason we need to do it in this particular case?

565

I think this is the wrong API (looking at how setBeginSymbol is called elsewhere) to create the section start symbol - but in any case, again, we don't create section start symbols anywhere in the DWARF codegen, so I don't think we need to do it for Macro info either. We just rely on their existence in all other cases, it seems?

It's probably worth doing a little test of your own to build two separate objects with macro info, link them together & check that dumping/values all make sense - that the two CU's references to their macinfo actually get relocated during linking, etc. (no need to commit a test like this - we could have a test that checks that there is a relocation for the macinfo attribute, but I'm not /too/ fussed about that, it's awkward and probably annoyingly platform specific, etc)

623

addSectionDelta isn't quite right - because it doesn't account for the MachO case where relocations are not used.

DwarfCompileUnit::addSectionLabel handles this case and wraps addSectionDelta.

1505

Please commit these unrelated comment fixes separately. No need for further review on that, you can just commit them directly yourself.

[aaboud]

Thanks David for the comments.
I will upload a new patch soon.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

564

No specific reason.
It is just that I used an old implementation used to be in LLVM in the past that tries to emit macros.
That implementation used to check the existence of DWARF sections.
I was not sure if I should keep it or not.

Anyway, I removed the checks.

565

Sorry for my lack of understanding how the section handling works.
I debugged it a little and understand what was missing, I should have initialized the macinfo section with a name for the begin label.

I will do the manual test to verify that the generated object file is accepted by the linker.

623

Now, I understand what you mean. I modified the code to use "addSectionLabel".
In your first comment you mentioned "addSectionOffset" and I could not figure out how I can use it with labels.

[aaboud]

Modified patch according to David comments.

[dblaikie]

Looks good - thanks!

include/llvm/CodeGen/DIE.h

 
 namespace llvm {
 class AsmPrinter;
 class MCExpr;
 class MCSymbol;
 class raw_ostream;
 class DwarfTypeUnit;
 
-// AsmStreamerBase - A base abstract interface class defines methods that
-// can be implemented to stream objects or can be implemented to
-// calculate the size of the streamed objects.
-// The derived classes will use an AsmPrinter to implement the methods.
-//
-// TODO: complete this interface and use it to merge EmitValue and SizeOf
-//       methods in the DIE classes below.
-class AsmStreamerBase {
-protected:
-  const AsmPrinter *AP;
-  AsmStreamerBase(const AsmPrinter *AP) : AP(AP) {}
-
-public:
-  virtual ~AsmStreamerBase() {}
-  virtual unsigned emitULEB128(uint64_t Value, const char *Desc = nullptr,
-                               unsigned PadTo = 0) = 0;
-  virtual unsigned emitInt8(unsigned char Value) = 0;
-  virtual unsigned emitBytes(StringRef Data) = 0;
-};
-
-/// EmittingAsmStreamer - Implements AbstractAsmStreamer to stream objects.
-/// Notice that the return value is not the actual size of the streamed object.
-/// For size calculation use SizeReporterAsmStreamer.
-class EmittingAsmStreamer : public AsmStreamerBase {
-public:
-  EmittingAsmStreamer(const AsmPrinter *AP) : AsmStreamerBase(AP) {}
-  unsigned emitULEB128(uint64_t Value, const char *Desc = nullptr,
-                       unsigned PadTo = 0) override;
-  unsigned emitInt8(unsigned char Value) override;
-  unsigned emitBytes(StringRef Data) override;
-};
-
-/// SizeReporterAsmStreamer - Only reports the size of the streamed objects.
-class SizeReporterAsmStreamer : public AsmStreamerBase {
-public:
-  SizeReporterAsmStreamer(const AsmPrinter *AP) : AsmStreamerBase(AP) {}
-  unsigned emitULEB128(uint64_t Value, const char *Desc = nullptr,
-                       unsigned PadTo = 0) override;
-  unsigned emitInt8(unsigned char Value) override;
-  unsigned emitBytes(StringRef Data) override;
-};
-
 //===--------------------------------------------------------------------===//
 /// DIEAbbrevData - Dwarf abbreviation data, describes one attribute of a
 /// Dwarf abbreviation.
 class DIEAbbrevData {
   /// Attribute - Dwarf attribute code.
   ///
   dwarf::Attribute Attribute;
 

lib/CodeGen/AsmPrinter/DIE.cpp

 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
-// EmittingAsmStreamer Implementation
-//===----------------------------------------------------------------------===//
-unsigned EmittingAsmStreamer::emitULEB128(uint64_t Value, const char *Desc,
-                                          unsigned PadTo) {
-  AP->EmitULEB128(Value, Desc, PadTo);
-  return 0;
-}
-
-unsigned EmittingAsmStreamer::emitInt8(unsigned char Value) {
-  AP->EmitInt8(Value);
-  return 0;
-}
-
-unsigned EmittingAsmStreamer::emitBytes(StringRef Data) {
-  AP->OutStreamer->EmitBytes(Data);
-  return 0;
-}
-
-//===----------------------------------------------------------------------===//
-// SizeReporterAsmStreamer Implementation
-//===----------------------------------------------------------------------===//
-unsigned SizeReporterAsmStreamer::emitULEB128(uint64_t Value, const char *Desc,
-                                              unsigned PadTo) {
-  return getULEB128Size(Value);
-}
-
-unsigned SizeReporterAsmStreamer::emitInt8(unsigned char Value) { return 1; }
-
-unsigned SizeReporterAsmStreamer::emitBytes(StringRef Data) {
-  return Data.size();
-}
-
-//===----------------------------------------------------------------------===//
 // DIEAbbrevData Implementation
 //===----------------------------------------------------------------------===//
 
 /// Profile - Used to gather unique data for the abbreviation folding set.
 ///
 void DIEAbbrevData::Profile(FoldingSetNodeID &ID) const {
   // Explicitly cast to an integer type for which FoldingSetNodeID has
   // overloads.  Otherwise MSVC 2010 thinks this call is ambiguous.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.h

class DwarfCompileUnit : public DwarfUnit {
   DIE::value_iterator StmtListValue;
 
   /// Skeleton unit associated with this unit.
   DwarfCompileUnit *Skeleton;
 
   /// The start of the unit within its section.
   MCSymbol *LabelBegin;
 
+  /// The start of the unit macro info within macro section.
+  MCSymbol *MacroLabelBegin;
+
   typedef llvm::SmallVector<const MDNode *, 8> ImportedEntityList;
   typedef llvm::DenseMap<const MDNode *, ImportedEntityList>
   ImportedEntityMap;
 
   ImportedEntityMap ImportedEntities;
 
   /// GlobalNames - A map of globally visible named entities for this unit.
   StringMap<const DIE *> GlobalNames;
public:
 
   void emitHeader(bool UseOffsets) override;
 
   MCSymbol *getLabelBegin() const {
     assert(Section);
     return LabelBegin;
   }
 
+  MCSymbol *getMacroLabelBegin() const {
+    return MacroLabelBegin;
+  }
+
   /// Add a new global name to the compile unit.
   void addGlobalName(StringRef Name, DIE &Die, const DIScope *Context) override;
 
   /// Add a new global type to the compile unit.
   void addGlobalType(const DIType *Ty, const DIE &Die,
                      const DIScope *Context) override;
 
   const StringMap<const DIE *> &getGlobalNames() const { return GlobalNames; }

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

 namespace llvm {
 
 DwarfCompileUnit::DwarfCompileUnit(unsigned UID, const DICompileUnit *Node,
                                    AsmPrinter *A, DwarfDebug *DW,
                                    DwarfFile *DWU)
     : DwarfUnit(UID, dwarf::DW_TAG_compile_unit, Node, A, DW, DWU),
       Skeleton(nullptr), BaseAddress(nullptr) {
   insertDIE(Node, &getUnitDie());
+  MacroLabelBegin = Asm->createTempSymbol("cu_macro_begin");
 }
 
 /// addLabelAddress - Add a dwarf label attribute data and value using
 /// DW_FORM_addr or DW_FORM_GNU_addr_index.
 ///
 void DwarfCompileUnit::addLabelAddress(DIE &Die, dwarf::Attribute Attribute,
                                        const MCSymbol *Label) {
 

lib/CodeGen/AsmPrinter/DwarfDebug.h

class DwarfDebug : public AsmPrinterHandler {
 
   /// Maps subprogram MDNode with its corresponding DwarfCompileUnit.
   MapVector<const MDNode *, DwarfCompileUnit *> SPMap;
 
   /// Maps a CU DIE with its corresponding DwarfCompileUnit.
   DenseMap<const DIE *, DwarfCompileUnit *> CUDieMap;
 
   /// List of all labels used in aranges generation.
   std::vector<SymbolCU> ArangeLabels;
 
   /// Size of each symbol emitted (for those symbols that have a specific size).
   DenseMap<const MCSymbol *, uint64_t> SymSize;
 
   LexicalScopes LScopes;
 
   /// Collection of abstract variables.
   DenseMap<const MDNode *, std::unique_ptr<DbgVariable>> AbstractVariables;
class DwarfDebug : public AsmPrinterHandler {
   /// Emit address ranges into a debug aranges section.
   void emitDebugARanges();
 
   /// Emit address ranges into a debug ranges section.
   void emitDebugRanges();
 
   /// Emit macros into a debug macinfo section.
   void emitDebugMacinfo();
-  unsigned emitMacro(AsmStreamerBase *AS, DIMacro &M);
-  unsigned emitMacroFile(AsmStreamerBase *AS, DIMacroFile &F,
-                         DwarfCompileUnit &U);
-  unsigned handleMacroNodes(AsmStreamerBase *AS, DIMacroNodeArray Nodes,
-                            DwarfCompileUnit &U);
+  void emitMacro(DIMacro &M);
+  void emitMacroFile(DIMacroFile &F, DwarfCompileUnit &U);
+  void handleMacroNodes(DIMacroNodeArray Nodes, DwarfCompileUnit &U);
 
   /// DWARF 5 Experimental Split Dwarf Emitters
 
   /// Initialize common features of skeleton units.
   void initSkeletonUnit(const DwarfUnit &U, DIE &Die,
                         std::unique_ptr<DwarfUnit> NewU);
 
   /// Construct the split debug info compile unit for the debug info

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::finalizeModuleInfo() {
 
   finishSubprogramDefinitions();
 
   finishVariableDefinitions();
 
   // Collect info for variables that were optimized out.
   collectDeadVariables();
 
-  unsigned MacroOffset = 0;
-  std::unique_ptr<AsmStreamerBase> AS(new SizeReporterAsmStreamer(Asm));
   // Handle anything that needs to be done on a per-unit basis after
   // all other generation.
   for (const auto &P : CUMap) {
     auto &TheCU = *P.second;
     // Emit DW_AT_containing_type attribute to connect types with their
     // vtable holding type.
     TheCU.constructContainingTypeDIEs();
 
     // Add CU specific attributes if we need to add any.
     // If we're splitting the dwarf out now that we've got the entire
if (unsigned NumRanges = TheCU.getRanges().size()) {
         // 2.17.3).
         U.addUInt(U.getUnitDie(), dwarf::DW_AT_low_pc, dwarf::DW_FORM_addr, 0);
       else
         U.setBaseAddress(TheCU.getRanges().front().getStart());
       U.attachRangesOrLowHighPC(U.getUnitDie(), TheCU.takeRanges());
     }
 
     auto *CUNode = cast<DICompileUnit>(P.first);
-    if (CUNode->getMacros()) {
-      // Compile Unit has macros, emit "DW_AT_macro_info" attribute.
-      U.addUInt(U.getUnitDie(), dwarf::DW_AT_macro_info,
-                dwarf::DW_FORM_sec_offset, MacroOffset);
-      // Update macro section offset
-      MacroOffset += handleMacroNodes(AS.get(), CUNode->getMacros(), U);
-    }
+    // If compile Unit has macros, emit "DW_AT_macro_info" attribute.
+    if (CUNode->getMacros())
+      U.addSectionDelta(U.getUnitDie(), dwarf::DW_AT_macro_info,
+                        U.getMacroLabelBegin(),
+                        TLOF.getDwarfMacinfoSection()->getBeginSymbol());
   }
 
   // Compute DIE offsets and sizes.
   InfoHolder.computeSizeAndOffsets();
   if (useSplitDwarf())
     SkeletonHolder.computeSizeAndOffsets();
 }
 
 // Emit all Dwarf sections that should come after the content.
void DwarfDebug::emitDebugPubTypes(bool GnuStyle) {
   MCSection *PSec = GnuStyle
                         ? Asm->getObjFileLowering().getDwarfGnuPubTypesSection()
                         : Asm->getObjFileLowering().getDwarfPubTypesSection();
 
   emitDebugPubSection(GnuStyle, PSec, "Types",
                       &DwarfCompileUnit::getGlobalTypes);
 }
 
 // Emit visible names into a debug str section.
 void DwarfDebug::emitDebugStr() {
   DwarfFile &Holder = useSplitDwarf() ? SkeletonHolder : InfoHolder;
   Holder.emitStrings(Asm->getObjFileLowering().getDwarfStrSection());
 }
 
 void DwarfDebug::emitDebugLocEntry(ByteStreamer &Streamer,
                                    const DebugLocStream::Entry &Entry) {
   auto &&Comments = DebugLocs.getComments(Entry);
for (const RangeSpanList &List : TheCU->getRangeLists()) {
 
       // And terminate the list with two 0 values.
       Asm->OutStreamer->EmitIntValue(0, Size);
       Asm->OutStreamer->EmitIntValue(0, Size);
     }
   }
 }
 
-unsigned DwarfDebug::handleMacroNodes(AsmStreamerBase *AS,
-                                      DIMacroNodeArray Nodes,
-                                      DwarfCompileUnit &U) {
-  unsigned Size = 0;
+void DwarfDebug::handleMacroNodes(DIMacroNodeArray Nodes, DwarfCompileUnit &U) {
   for (auto *MN : Nodes) {
     if (auto *M = dyn_cast<DIMacro>(MN))
-      Size += emitMacro(AS, *M);
+      emitMacro(*M);
     else if (auto *F = dyn_cast<DIMacroFile>(MN))
-      Size += emitMacroFile(AS, *F, U);
+      emitMacroFile(*F, U);
     else
       llvm_unreachable("Unexpected DI type!");
   }
-  return Size;
 }
 
-unsigned DwarfDebug::emitMacro(AsmStreamerBase *AS, DIMacro &M) {
-  int Size = 0;
-  Size += AS->emitULEB128(M.getMacinfoType());
-  Size += AS->emitULEB128(M.getLine());
+void DwarfDebug::emitMacro(DIMacro &M) {
+  Asm->EmitULEB128(M.getMacinfoType());
+  Asm->EmitULEB128(M.getLine());
   StringRef Name = M.getName();
   StringRef Value = M.getValue();
-  Size += AS->emitBytes(Name);
+  Asm->OutStreamer->EmitBytes(Name);
   if (!Value.empty()) {
     // There should be one space between macro name and macro value.
-    Size += AS->emitInt8(' ');
-    Size += AS->emitBytes(Value);
+    Asm->EmitInt8(' ');
+    Asm->OutStreamer->EmitBytes(Value);
   }
-  Size += AS->emitInt8('\0');
-  return Size;
+  Asm->EmitInt8('\0');
 }
 
-unsigned DwarfDebug::emitMacroFile(AsmStreamerBase *AS, DIMacroFile &F,
-                                   DwarfCompileUnit &U) {
-  int Size = 0;
+void DwarfDebug::emitMacroFile(DIMacroFile &F, DwarfCompileUnit &U) {
   assert(F.getMacinfoType() == dwarf::DW_MACINFO_start_file);
-  Size += AS->emitULEB128(dwarf::DW_MACINFO_start_file);
-  Size += AS->emitULEB128(F.getLine());
+  Asm->EmitULEB128(dwarf::DW_MACINFO_start_file);
+  Asm->EmitULEB128(F.getLine());
   DIFile *File = F.getFile();
   unsigned FID =
       U.getOrCreateSourceID(File->getFilename(), File->getDirectory());
-  Size += AS->emitULEB128(FID);
-  Size += handleMacroNodes(AS, F.getElements(), U);
-  Size += AS->emitULEB128(dwarf::DW_MACINFO_end_file);
-  return Size;
+  Asm->EmitULEB128(FID);
+  handleMacroNodes(F.getElements(), U);
+  Asm->EmitULEB128(dwarf::DW_MACINFO_end_file);
 }
 
 // Emit visible names into a debug macinfo section.
 void DwarfDebug::emitDebugMacinfo() {
-  if (MCSection *Macinfo = Asm->getObjFileLowering().getDwarfMacinfoSection()) {
-    // Start the dwarf macinfo section.
-    Asm->OutStreamer->SwitchSection(Macinfo);
-  }
-  std::unique_ptr<AsmStreamerBase> AS(new EmittingAsmStreamer(Asm));
+  Asm->OutStreamer->SwitchSection(
+      Asm->getObjFileLowering().getDwarfMacinfoSection());
+
   for (const auto &P : CUMap) {
     auto &TheCU = *P.second;
     auto *SkCU = TheCU.getSkeleton();
     DwarfCompileUnit &U = SkCU ? *SkCU : TheCU;
     auto *CUNode = cast<DICompileUnit>(P.first);
-    handleMacroNodes(AS.get(), CUNode->getMacros(), U);
+    Asm->OutStreamer->EmitLabel(U.getMacroLabelBegin());
+    handleMacroNodes(CUNode->getMacros(), U);
   }
   Asm->OutStreamer->AddComment("End Of Macro List Mark");
   Asm->EmitInt8(0);
 }
 
 // DWARF5 Experimental Separate Dwarf emitters.
 
 void DwarfDebug::initSkeletonUnit(const DwarfUnit &U, DIE &Die,

lib/MC/MCObjectFileInfo.cpp

DwarfRangesSection = Ctx->getCOFFSection(
       ".debug_ranges",
       COFF::IMAGE_SCN_MEM_DISCARDABLE | COFF::IMAGE_SCN_CNT_INITIALIZED_DATA |
           COFF::IMAGE_SCN_MEM_READ,
       SectionKind::getMetadata(), "debug_range");
   DwarfMacinfoSection = Ctx->getCOFFSection(
       ".debug_macinfo",
       COFF::IMAGE_SCN_MEM_DISCARDABLE | COFF::IMAGE_SCN_CNT_INITIALIZED_DATA |
           COFF::IMAGE_SCN_MEM_READ,
-      SectionKind::getMetadata());
+      SectionKind::getMetadata(), "debug_macinfo");
   DwarfInfoDWOSection = Ctx->getCOFFSection(
       ".debug_info.dwo",
       COFF::IMAGE_SCN_MEM_DISCARDABLE | COFF::IMAGE_SCN_CNT_INITIALIZED_DATA |
           COFF::IMAGE_SCN_MEM_READ,
       SectionKind::getMetadata(), "section_info_dwo");
   DwarfTypesDWOSection = Ctx->getCOFFSection(
       ".debug_types.dwo",
       COFF::IMAGE_SCN_MEM_DISCARDABLE | COFF::IMAGE_SCN_CNT_INITIALIZED_DATA |