[DebugInfo] Fix a misleading usage of DWARF forms with DIEExpr. NFC.

[ikudrin]

Summary

For now, DIEExpr is used only in two places:

1. in the debug info library unit test suite to emit a DW_AT_str_offsets_base attribute with the DW_FORM_sec_offset form, see dwarfgen::DIE::addStrOffsetsBaseAttribute();
2. in DwarfCompileUnit::addLocationAttribute() to generate the location attribute for a TLS variable.

The later case used an incorrect DWARF form of DW_FORM_udata, which implies storing an uleb128 value, not a 4/8 byte constant. The generated result was as expected because DIEExpr::SizeOf() did not handle the used form, but returned the size of the code pointer by default.

The patch fixes the issue by using more appropriate DWARF forms for the problematic case and making DIEExpr::SizeOf() more straightforward.

[dblaikie]

I guess this change is NFC? The observable behavior (the exact bits) produced by LLVM remains the same - because, even though the form changed, blocks don't actually have forms, it's just a way for us to internally communicate how things should be encoded?

(I guess if we had a really big TLS symbol location then the ULEB encoding would be a problem? Hmm - actually, if sizeof() does the right thing, but the code that actually wrote the value wrote it out as ULEB, wouldn't that throw off the whole encoding & not write enough bytes to the block? That sounds like it'd be testable in some form? (later ops wouldn't be at the right byte offset, or the whole DWARF emission would be a bit short & throw off everything?))

[ikudrin]

The change is NFC (NFCI?). The form is ignored in DIEExpr::emitValue() and only the result of SizeOf() is used, which returned the expected value because it used AP->getPointerSize() for unhandled forms. All the construction is quite fragile and worked only thanks to the fortune.

[dblaikie]

In D83958#2157396, @ikudrin wrote:

The change is NFC (NFCI?).

Yeah, always intent is the best we can do, really.

The form is ignored in DIEExpr::emitValue() and only the result of SizeOf() is used, which returned the expected value because it used AP->getPointerSize() for unhandled forms. All the construction is quite fragile and worked only thanks to the fortune.

Oh, form isn't ignored exactly - but it's used via SizeOf so the bug is symmetric: if SizeOf got the size wrong, at least it'd get it wrong both in the layout code that needed to know the size ahead of time, and in the bytes being emitted - so you wouldn't end up with the output being weirdly the wrong length compared to other lengths/offsets/etc used in the DWARF.

Makes sense. Thanks for the patch/explanation.

llvm/lib/CodeGen/AsmPrinter/DIE.cpp

 ///
 void DIEExpr::emitValue(const AsmPrinter *AP, dwarf::Form Form) const {
   AP->emitDebugValue(Expr, SizeOf(AP, Form));
 }
 
 /// SizeOf - Determine size of expression value in bytes.
 ///
 unsigned DIEExpr::SizeOf(const AsmPrinter *AP, dwarf::Form Form) const {
-  if (Form == dwarf::DW_FORM_data4) return 4;
-  if (Form == dwarf::DW_FORM_sec_offset) return 4;
-  if (Form == dwarf::DW_FORM_strp) return 4;
-  return AP->getPointerSize();
+  switch (Form) {
+  case dwarf::DW_FORM_data4:
+    return 4;
+  case dwarf::DW_FORM_data8:
+    return 8;
+  case dwarf::DW_FORM_sec_offset:
+    // FIXME: add support for DWARF64
+    return 4;
+  default:
+    llvm_unreachable("DIE Value form not supported yet");
+  }
 }
 
 LLVM_DUMP_METHOD
 void DIEExpr::print(raw_ostream &O) const { O << "Expr: " << *Expr; }
 
 //===----------------------------------------------------------------------===//
 // DIELabel Implementation
 //===----------------------------------------------------------------------===//

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (Global) {
           // Based on GCC's support for TLS:
           if (!DD->useSplitDwarf()) {
             // 1) Start with a constNu of the appropriate pointer size
             addUInt(*Loc, dwarf::DW_FORM_data1,
                     PointerSize == 4 ? dwarf::DW_OP_const4u
                                      : dwarf::DW_OP_const8u);
             // 2) containing the (relocated) offset of the TLS variable
             //    within the module's TLS block.
-            addExpr(*Loc, dwarf::DW_FORM_udata,
+            addExpr(*Loc,
+                    PointerSize == 4 ? dwarf::DW_FORM_data4
+                                     : dwarf::DW_FORM_data8,
                     Asm->getObjFileLowering().getDebugThreadLocalSymbol(Sym));
           } else {
             addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_GNU_const_index);
             addUInt(*Loc, dwarf::DW_FORM_udata,
                     DD->getAddressPool().getIndex(Sym, /* TLS */ true));
           }
           // 3) followed by an OP to make the debugger do a TLS lookup.
           addUInt(*Loc, dwarf::DW_FORM_data1,