[DebugInfo] Avoid triggering global location assert for 2-byte pointer sizes.

[jackoalan]

Summary

D111404 moved a 4/8 byte check assert into a block taken by 2-byte platforms.
Since these platforms do not take the branches where the pointer size is used,
sink the assert accordingly.

[jackoalan]

Fix test for windows

[dblaikie]

Could you add some details about where the assert came from/went to, link to the patch that made the change?

If this codepath that's being changed that relies on the pointer size variations isn't reachable by a 2 byte pointer platform, it might be better to move the assert & then it can be changed if/when the codoepath is used on a 2 byte pointer platform and the code changes can be tested.

[jackoalan]

In D116480#3220038, @dblaikie wrote:

Could you add some details about where the assert came from/went to, link to the patch that made the change?

The assert was moved by D111404 in order to cover an additional branch that uses the pointer size. Neither the MSP430 nor AVR targets make use of TLS or RWPI globals.

The solutions are one of the following:

• Duplicate the assert to each nested block instead
• Add (!TLS && !RWPI) || ... logic to the assert
• Add support for 2-byte data forms like this patch (which strikes me as the most general solution in a target-independent place like this)

[dblaikie]

Perhaps the right thing to do then would be to sink the assertion back down in some way - either just duplicating it in the two places it's used or using a lambda, perhaps?

[jackoalan]

In D116480#3220066, @dblaikie wrote:

Perhaps the right thing to do then would be to sink the assertion back down in some way - either just duplicating it in the two places it's used or using a lambda, perhaps?

Oh yeah, lambda would be a concise way to check that.

[dblaikie]

The lamdbda could assert and compute the pointer size - so the assertion stays close to the code that then uses the asserted property - and remove some of the duplication of computing the pointer size form in several places.

[jackoalan]

Sink assert using lambda

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

263–307

Maybe like this? (open to better naming)

265–270

Could this instead be "GetPointerSizedForm" and encapsulate the PointerSize == 4 ? dwarf::DW_OP_const4u : dwarf::DW_OP_const8u logic too?

Oh, I see there's both OP and FORM needed, not just FORM... maybe still nice to share?

[jackoalan]

Return resolved DWARF enums from lambda.

[dblaikie]

Awesome, thanks!

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (Expr) {
           NVPTXAddressSpace = LocalNVPTXAddressSpace;
         }
       }
       DwarfExpr->addFragmentOffset(Expr);
     }
 
     if (Global) {
       const MCSymbol *Sym = Asm->getSymbol(Global);
+      // 16-bit platforms like MSP430 and AVR take this path, so sink this
+      // assert to platforms that use it.
+      auto GetPointerSizedFormAndOp = [this]() {
         unsigned PointerSize = Asm->getDataLayout().getPointerSize();
         assert((PointerSize == 4 || PointerSize == 8) &&
                "Add support for other sizes if necessary");
+        struct FormAndOp {
+          dwarf::Form Form;
+          dwarf::LocationAtom Op;
+        };
+        return PointerSize == 4
+                   ? FormAndOp{dwarf::DW_FORM_data4, dwarf::DW_OP_const4u}
+                   : FormAndOp{dwarf::DW_FORM_data8, dwarf::DW_OP_const8u};
+      };
       if (Global->isThreadLocal()) {
         if (Asm->TM.useEmulatedTLS()) {
           // TODO: add debug info for emulated thread local mode.
         } else {
           // FIXME: Make this work with -gsplit-dwarf.
           // Based on GCC's support for TLS:
           if (!DD->useSplitDwarf()) {
+            auto FormAndOp = GetPointerSizedFormAndOp();
             // 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);
+            addUInt(*Loc, dwarf::DW_FORM_data1, FormAndOp.Op);
             // 2) containing the (relocated) offset of the TLS variable
             //    within the module's TLS block.
-            addExpr(*Loc,
-                    PointerSize == 4 ? dwarf::DW_FORM_data4
-                                     : dwarf::DW_FORM_data8,
+            addExpr(*Loc, FormAndOp.Form,
                     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,
                   DD->useGNUTLSOpcode() ? dwarf::DW_OP_GNU_push_tls_address
                                         : dwarf::DW_OP_form_tls_address);
         }
       } else if (Asm->TM.getRelocationModel() == Reloc::RWPI ||
                  Asm->TM.getRelocationModel() == Reloc::ROPI_RWPI) {
+        auto FormAndOp = GetPointerSizedFormAndOp();
         // Constant
-        addUInt(*Loc, dwarf::DW_FORM_data1,
-                PointerSize == 4 ? dwarf::DW_OP_const4u
-                                 : dwarf::DW_OP_const8u);
+        addUInt(*Loc, dwarf::DW_FORM_data1, FormAndOp.Op);
         // Relocation offset
-        addExpr(*Loc, PointerSize == 4 ? dwarf::DW_FORM_data4
-                                       : dwarf::DW_FORM_data8,
+        addExpr(*Loc, FormAndOp.Form,
                 Asm->getObjFileLowering().getIndirectSymViaRWPI(Sym));
         // Base register
         Register BaseReg = Asm->getObjFileLowering().getStaticBase();
         BaseReg = Asm->TM.getMCRegisterInfo()->getDwarfRegNum(BaseReg, false);
         addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_breg0 + BaseReg);
         // Offset from base register
         addSInt(*Loc, dwarf::DW_FORM_sdata, 0);
         // Operation

llvm/test/DebugInfo/MSP430/global-var.ll

+; RUN: llc --filetype=obj -o %t < %s
+; RUN: llvm-dwarfdump --debug-info %t | FileCheck %s
+; RUN: llvm-dwarfdump --verify %t
+
+; CHECK:      DW_TAG_variable
+; CHECK-NEXT:   DW_AT_name      ("global_var")
+; CHECK-NEXT:   DW_AT_type      ({{0x[0-9]+}} "char")
+; CHECK-NEXT:   DW_AT_external  (true)
+; CHECK-NEXT:   DW_AT_decl_file ("/tmp{{[/\\]}}global-var.c")
+; CHECK-NEXT:   DW_AT_decl_line (1)
+; CHECK-NEXT:   DW_AT_location  (DW_OP_addr 0x0)
+
+; ModuleID = 'global-var.c'
+source_filename = "global-var.c"
+target datalayout = "e-m:e-p:16:16-i32:16-i64:16-f32:16-f64:16-a:8-n8:16-S16"
+target triple = "msp430"
+
+@global_var = dso_local global i8 42, align 1, !dbg !0
+
+; Function Attrs: noinline nounwind optnone
+define dso_local i16 @main() #0 !dbg !10 {
+entry:
+  ret i16 0, !dbg !15
+}
+
+attributes #0 = { noinline nounwind optnone "frame-pointer"="none" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!6, !7, !8}
+!llvm.ident = !{!9}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "global_var", scope: !2, file: !3, line: 1, type: !5, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 14.0.0 (https://github.com/llvm/llvm-project ...)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, globals: !4, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "global-var.c", directory: "/tmp")
+!4 = !{!0}
+!5 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!6 = !{i32 7, !"Dwarf Version", i32 4}
+!7 = !{i32 2, !"Debug Info Version", i32 3}
+!8 = !{i32 1, !"wchar_size", i32 2}
+!9 = !{!"clang version 14.0.0 (https://github.com/llvm/llvm-project ...)"}
+!10 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 2, type: !11, scopeLine: 2, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !14)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13}
+!13 = !DIBasicType(name: "int", size: 16, encoding: DW_ATE_signed)
+!14 = !{}
+!15 = !DILocation(line: 2, column: 13, scope: !10)