[DwarfDebug] Restore code that make comments stay aligned in DwarfDebug::emitDebugLocEntry

[bjope]

Summary

Commit 2bddab25dba8d4b0 removed a piece of code from
DwarfDebug::emitDebugLocEntry that according to code comments
"Make sure comments stay aligned".

This patch restores that piece of code, together with the addition
of some extra checks in an existing lit test to work as a regression
test. Without this patch we incorrectly get

.byte   159                             # 0

instead of

.byte   159                             # DW_OP_stack_value

[dblaikie]

This seems like a fairly subtle/long distance dependence now, that "emitDIERef" will use the same ULEB128PadSize as this code uses? Which isn't necessarily true/could become divergent relatively easily.

[bjope]

In D117441#3249162, @dblaikie wrote:

This seems like a fairly subtle/long distance dependence now, that "emitDIERef" will use the same ULEB128PadSize as this code uses? Which isn't necessarily true/could become divergent relatively easily.

Yes, that could be true.

Honestly, I do not really know how it works with these comments. For example:

• How comes that we should skip (up-to) ULEB128PadSize comments for Encoding::BaseTypeRef? Why is it like that?
• Why are we just skipping comments and not printing them?

I was thinking that I could add a Twine argument to emitDIERef similarly to those found for emitInt8. But that felt weird since we just skip comments, not printing them. And I would need to pass the Iterator in order to step it multiple times.

Well, this patch simply restores the code needed to make our downstream test cases working again (we had some lit tests that used FileCheck lines to verify the debug section, including the comments, and those tests have been failing after commit 2bddab25dba8d4b0 due to the comments being totally messed up).

[dblaikie]

Perhaps a reasonable intermediate solution would be to return the pad size from emitDIERef? (or have an exposed constant in ByteStreamer that's used in emitDIERef and can be used from the DwarfDebug code) and use that value in the DwarfDebug code so the values can't diverge, at least...

[bjope]

Utilize a return value from APByteStreamer::emitDIERef to determine how many comments to skip.

[dblaikie]

Looks OK, thanks!

llvm/lib/CodeGen/AsmPrinter/ByteStreamer.h

protected:
   ByteStreamer() = default;
 
  public:
   // For now we're just handling the calls we need for dwarf emission/hashing.
   virtual void emitInt8(uint8_t Byte, const Twine &Comment = "") = 0;
   virtual void emitSLEB128(uint64_t DWord, const Twine &Comment = "") = 0;
   virtual void emitULEB128(uint64_t DWord, const Twine &Comment = "",
                            unsigned PadTo = 0) = 0;
-  virtual void emitDIERef(const DIE &D) = 0;
+  virtual unsigned emitDIERef(const DIE &D) = 0;
 };
 
 class APByteStreamer final : public ByteStreamer {
 private:
   AsmPrinter ≈
 
 public:
   APByteStreamer(AsmPrinter &Asm) : AP(Asm) {}
   void emitInt8(uint8_t Byte, const Twine &Comment) override {
     AP.OutStreamer->AddComment(Comment);
     AP.emitInt8(Byte);
   }
   void emitSLEB128(uint64_t DWord, const Twine &Comment) override {
     AP.OutStreamer->AddComment(Comment);
     AP.emitSLEB128(DWord);
   }
   void emitULEB128(uint64_t DWord, const Twine &Comment,
                    unsigned PadTo) override {
     AP.OutStreamer->AddComment(Comment);
     AP.emitULEB128(DWord, nullptr, PadTo);
   }
-  void emitDIERef(const DIE &D) override {
+  unsigned emitDIERef(const DIE &D) override {
     uint64_t Offset = D.getOffset();
     static constexpr unsigned ULEB128PadSize = 4;
     assert(Offset < (1ULL << (ULEB128PadSize * 7)) && "Offset wont fit");
     emitULEB128(Offset, "", ULEB128PadSize);
+    // Return how many comments to skip in DwarfDebug::emitDebugLocEntry to keep
+    // comments aligned with debug loc entries.
+    return ULEB128PadSize;
   }
 };
 
 class HashingByteStreamer final : public ByteStreamer {
  private:
   DIEHash &Hash;
  public:
    HashingByteStreamer(DIEHash &H) : Hash(H) {}
    void emitInt8(uint8_t Byte, const Twine &Comment) override {
      Hash.update(Byte);
   }
   void emitSLEB128(uint64_t DWord, const Twine &Comment) override {
     Hash.addSLEB128(DWord);
   }
   void emitULEB128(uint64_t DWord, const Twine &Comment,
                    unsigned PadTo) override {
     Hash.addULEB128(DWord);
   }
-  void emitDIERef(const DIE &D) override { Hash.hashRawTypeReference(D); }
+  unsigned emitDIERef(const DIE &D) override {
+    Hash.hashRawTypeReference(D);
+    return 0; // Only used together with the APByteStreamer.
+  }
 };
 
 class BufferByteStreamer final : public ByteStreamer {
 private:
   SmallVectorImpl<char> &Buffer;
   std::vector<std::string> &Comments;
 
 public:
void emitULEB128(uint64_t DWord, const Twine &Comment,
     if (GenerateComments) {
       Comments.push_back(Comment.str());
       // Add some empty comments to keep the Buffer and Comments vectors aligned
       // with each other.
       for (size_t i = 1; i < Length; ++i)
         Comments.push_back("");
     }
   }
-  void emitDIERef(const DIE &D) override {
+  unsigned emitDIERef(const DIE &D) override {
     uint64_t Offset = D.getOffset();
     static constexpr unsigned ULEB128PadSize = 4;
     assert(Offset < (1ULL << (ULEB128PadSize * 7)) && "Offset wont fit");
     emitULEB128(Offset, "", ULEB128PadSize);
+    return 0; // Only used together with the APByteStreamer.
   }
 };
 
 }
 
 #endif

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (auto &Op : Expr) {
     assert(Op.getCode() != dwarf::DW_OP_const_type &&
            "3 operand ops not yet supported");
     Streamer.emitInt8(Op.getCode(), Comment != End ? *(Comment++) : "");
     Offset++;
     for (unsigned I = 0; I < 2; ++I) {
       if (Op.getDescription().Op[I] == Encoding::SizeNA)
         continue;
       if (Op.getDescription().Op[I] == Encoding::BaseTypeRef) {
+        unsigned Length =
           Streamer.emitDIERef(*CU->ExprRefedBaseTypes[Op.getRawOperand(I)].Die);
+        // Make sure comments stay aligned.
+        for (unsigned J = 0; J < Length; ++J)
+          if (Comment != End)
+            Comment++;
       } else {
         for (uint64_t J = Offset; J < Op.getOperandEndOffset(I); ++J)
           Streamer.emitInt8(Data.getData()[J], Comment != End ? *(Comment++) : "");
       }
       Offset = Op.getOperandEndOffset(I);
     }
     assert(Offset == Op.getEndOffset());
   }

llvm/test/DebugInfo/X86/convert-loclist.ll

 ; identical and demonstrate that they have different DWO IDs.
 
 ; SPLIT: 0x00000000: Compile Unit: {{.*}} DWO_id = 0xa6edbf487b0a7acf
 
 ; Regression testing a fairly quirky bug where instead of hashing (see above),
 ; extra bytes would be emitted into the output assembly in no
 ; particular/intentional section - so let's check they don't show up at all:
 ; ASM-NOT: .asciz  "\200\200\200"
+; ASM:      .byte   10                              # Loc expr size
+; ASM-NEXT: .byte   17                              # DW_OP_consts
+; ASM-NEXT: .byte   7                               # 7
+; ASM-NEXT: .byte   48                              # DW_OP_lit0
+; ASM-NEXT: .byte   34                              # DW_OP_plus
+; ASM-NEXT: .byte   168                             # DW_OP_convert
+; ASM-NEXT: .asciz  "\232\200\200"                  #
+; ASM-NEXT: .byte   159                             # DW_OP_stack_value
+; ASM-NEXT: .byte   0                               # DW_LLE_end_of_list
+; ASM-NOT: .asciz  "\200\200\200"
 
 ; CHECK: 0x{{0*}}[[TYPE:.*]]: DW_TAG_base_type
 ; CHECK-NEXT:                   DW_AT_name ("DW_ATE_unsigned_32")
 
 ; CHECK: DW_LLE_offset_pair ({{.*}}): DW_OP_consts +7, DW_OP_lit0, DW_OP_plus, DW_OP_convert 0x[[TYPE]], DW_OP_stack_value
 
 ; Function Attrs: uwtable
 define dso_local void @_Z2f2v() local_unnamed_addr #0 !dbg !11 {