[WebAssembly] Add DW_OP_WASM_location_int

[aardappel]

Summary

This to allow us to add reloctable global indices as a symbol.
See discussion in https://github.com/WebAssembly/debugging/issues/12

To accomplish this, we add a new DW_OP_WASM_location_int tag (to keep DW_OP_WASM_location unchanged for now for tools that rely on it), and a new R_WASM_GLOBAL_INDEX_I32 relocation type.

As discussed in the issue, the current implementation is a bit of a "hard-coded" version of what DwarfExpression does, with added support for symbols. Weaving this into DwarfExpression itself was deemed to invasive at this stage, but could be a good change in the future if use of this functionality expands. For now it serves our need of a relocatable stack pointer reference, and possibly other globals.

[yurydelendik]

I'm still curious if something like this will work:

diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
index c4dc53337c0..4bbff49606a 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
@@ -44,2 +44,3 @@ public:
       BaseTypeRef = 8,
+      WasmLocationArg = 30,
       SignBit = 0x80,
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp b/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
index e9463fd53d9..c51039c66ef 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
@@ -96,3 +96,3 @@ static DescVector getDescriptions() {
   Descriptions[DW_OP_WASM_location] =
-      Desc(Op::Dwarf4, Op::SizeLEB, Op::SignedSizeLEB);
+      Desc(Op::Dwarf4, Op::SizeLEB, Op::WasmLocationArg);
   Descriptions[DW_OP_GNU_push_tls_address] = Desc(Op::Dwarf3);
@@ -172,2 +172,15 @@ bool DWARFExpression::Operation::extract(DataExtractor Data,
       break;
+    case Operation::WasmLocationArg:
+      assert(Operand == 1);
+      switch (Operands.Op[0]) {
+        case 0: case 1: case 2:
+          Operands[Operand] = Data.getULEB128(&Offset);
+          break;
+        case 4: // global as uint32
+          Operands[Operand] = Data.getU32(&Offset);
+          break;
+        default:
+          return false; // Unknown Wasm location
+      }
+      break;
     case Operation::SizeBlock:
@@ -273,2 +286,10 @@ bool DWARFExpression::Operation::print(raw_ostream &OS,
         OS << format(" 0x%02x", Expr->Data.getU8(&Offset));
+    } else if (Size == Operation::WasmLocationArg) {
+      assert(Operand == 1);
+      switch (Operands[0]) {
+        case 0: case 1: case 2:
+        case 4: // global as uint32
+          OS << format(" 0x%" PRIx64, Operands[Operand]);
+        default: assert(false);
+      }
     } else {

[aardappel]

@yurydelendik that would just take care of the encoding, the bigger question would be how to plumb symbol support into DwarfExpression (including the buffering path).

[aardappel]

Fixed dwarfdump (see change to WasmObjectFile).
Fixed int32 value being unsigned (now signed, like existing tag).
Fixed accidental binary file added to patch.

[aardappel]

Added new relocation type: R_WASM_GLOBAL_INDEX_I32

[sbc100]

New relocation type lgtm. I'm surprised there wasn't a need to update any lld tests. Probably means we need to add one that tests DW_AT_frame_base

[aardappel]

Fixed global only used from debug section not getting flags set.
Fixed tests.

[aardappel]

Fixes for LLD

[dschuff]

In D77353#1959964, @aardappel wrote:

@yurydelendik that would just take care of the encoding, the bigger question would be how to plumb symbol support into DwarfExpression (including the buffering path).

Is this just because DwarfExpression doesn't have access to the AsmPrinter?

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

424–445

This should probably at least say which header this value comes from.

[aardappel]

Is this just because DwarfExpression doesn't have access to the AsmPrinter?

It is slightly more complicated, DwarfExpression has two paths, one that directly emits to a DIE structure as it goes, the other that caches into a list (of integers) whose size can be measured before its emitted. The latter is not really set up for metadata, like "this integer is really a symbol". Either way, I am sure it can be done, but it is in shared code, so I was a bit more hesitant to rework that, especially when @yurydelendik says it's outside the scope of this patch :) My conversations with @dblaikie also seemed to indicate it would be a much larger change, only worth it if we were going to have many more complex expressions making use of this functionality. Then again, this patch already grew larger than expected.. I'd say its something we can consider if my "hack" in this patch is needed in at least one more location?

[aardappel]

Updated comment to refer to original header

[aardappel]

If there are no further comments on improvements we can carry out right now, does anyone feel like approving this for merging? :)

[yurydelendik]

If there are no further comments on improvements we can carry out right now, does anyone feel like approving this for merging? :)

The adding of new OP code DW_OP_WASM_location_int seems driven by LLVM implementation detail. I feel that is not necessary. Is it possible to use the alternative encoding suggested above, since we solved plumbing symbol support?

[dschuff]

@yurydelendik is that actually true? Concretely you are proposing that the form of DW_OP_Wasm_location be variadic instead of fixed, right? i.e. it has different fields based on the value of the first field. I don't think there is any precedent for that in DWARF anywhere, is there? e.g. there are different ops for OP_plus and OP_plus_uconst; or OP_call2 and OP_call4 could be just one op with a similar discriminator, but they are not. You could argue that even having OP_Wasm_location as it is (if it didn't need different forms) is inefficient because instead of the discriminator we could just use different ops, and make it smaller.

[aardappel]

@yurydelendik how would you change Descriptions[DW_OP_WASM_location] = Desc(Op::Dwarf4, Op::SizeLEB, Op::SignedSizeLEB); to make it variadic? And there may be further locations that need to support this.
We didn't "solve" plumbing, we circumvented it for the moment :)
When a location is a local, it is still being output with your existing LEB code. So we could make your suggestion happen simply by using DW_OP_WASM_location instead of DW_OP_WASM_location_int.

@dschuff Yes, I could bake the cases of the wasm::TI_ enum into multiple DW_OP_WASM_ tags, be happy to do that as part of this patch. We could then deprecate the existing DW_OP_WASM_location tag. Consumers already need to deal with new tags showing up for globals, might as well do it for locals at the same time then?

[yurydelendik]

In D77353#1972706, @dschuff wrote:

Concretely you are proposing that the form of DW_OP_Wasm_location be variadic instead of fixed, right? i.e. it has different fields based on the value of the first field. I don't think there is any precedent for that in DWARF anywhere, is there?

Closest to variadic args I see is DW_OP_implicit_value. The DWARF sees DW_OP_Wasm_location is an operator by itself (which may have its custom arguments or none). FWIW I was planing to suggest to have something like DW_OP_Wasm_location wasm-local-0, DW_OP_Wasm_location wasm-local-1, .. similar to DWARF's reg0..reg31 to save extra byte.

I argue for this design because the space for the vendor extension is pretty limited (only 32 entries and some are already taken), so we can quickly run out of IDs for DW_OP_Wasm_xxx. I don't see any requirement on amount and kinds of arguments for vendor operators. The limitation on at most 2 arguments with strict types comes from the DWARFExpression.cpp implementation.

[yurydelendik]

In D77353#1972809, @aardappel wrote:

@yurydelendik how would you change Descriptions[DW_OP_WASM_location] = Desc(Op::Dwarf4, Op::SizeLEB, Op::SignedSizeLEB); to make it variadic?

I was hoping to test the code I suggested in the comment above at https://reviews.llvm.org/D77353#1959871 , which changes the above to Descriptions[DW_OP_WASM_location] = Desc(Op::Dwarf4, Op::SizeLEB, Op::WasmLocationArg);. I'll try to test my idea in couple of days.

[aardappel]

So far we only need 2 tags, for locals and globals, and maybe soon a third for stack-relative. I don't see us running out of 32 entries that fast :)

[yurydelendik]

In D77353#1972855, @aardappel wrote:

So far we only need 2 tags, for locals and globals, and maybe soon a third for stack-relative. I don't see us running out of 32 entries that fast :)

Per https://github.com/gcc-mirror/gcc/blob/master/include/dwarf2.def, 22 is already well-known.

Anyway suggested encoding patch can be found at https://gist.github.com/yurydelendik/de99a1f512cca6e685780f07aade416a

[aardappel]

@yurydelendik thanks! that appears to mostly be a merge of this patch and the changes you outlined above?

@dschuff @sbc100 do you prefer this over whats in this patch? (keeping the single WASM dwarf tag in exchange for implementing a variadic argument reader/writer?)

[aardappel]

@paolosev you may have an opinion on this, given that you're already working with this data as well?

[dschuff]

If there really are only 32 codes available for all the user targets, then maybe we should just try to get away with only using one.

Also cc @philip.pfaffe

[paolosev]

In D77353#1978576, @aardappel wrote:

@paolosev you may have an opinion on this, given that you're already working with this data as well?

I also think that if possible we should try to use only one code, since there are only 10 vendor extension codes left (9 if we count our DW_OP_WASM_location).

I am not an expert here, but looking at DW_OP_implicit_value as Yury says, it should be possible to have variable-length arguments (DW_OP_implicit_value expects a ULEB128 size followed by block of that size).

[aardappel]

@dschuff @paolosev thanks for the feedback, seems we have some consensus on going forward with the single variadic tag. I'll merge in Yury's changes.

[aardappel]

@yurydelendik one thing about your patch: the variadic argument was previously declared as SignedSizeLEB and emitted with emitSigned, yet your new code reads it as getULEB128 and stores it in an array of uint64_t (which we can't change since its shared code). It doesn't matter much for the current use case, but we might want to be consistent about this storing signed or unsigned values? I see value in it being signed for future use cases, but given the existing DWARFExpression code it may well have to be unsigned.

[yurydelendik]

In D77353#1987324, @aardappel wrote:

@yurydelendik one thing about your patch: the variadic argument was previously declared as SignedSizeLEB and emitted with emitSigned, yet your new code reads it as getULEB128 and stores it in an array of uint64_t (which we can't change since its shared code). It doesn't matter much for the current use case, but we might want to be consistent about this storing signed or unsigned values? I see value in it being signed for future use cases, but given the existing DWARFExpression code it may well have to be unsigned.

Previously the signed LEB was chosen as universal argument type. For wasm-local, -global, -stack we just need unsigned, so I decided to switch to be unsigned for these location. I have no preference of them be signed/unsigned.

[aardappel]

Changed encoding to use Yury's variadic WasmLocationArg type.

[aardappel]

Ok, will try to make it more consistently unsigned, given that down the line they will be stored as unsigned anyway. But its still messy, given that at least thru one path they arrive as this TargetIndexLocation which is signed.
Made it explicit in the comments for TargetIndex that these are meant to be unsigned.

[aardappel]

Made values more consistently unsigned thru-out.

[sbc100]

New relocation type and handling LGTM.

If one were being meticulous one might land this as three different changes:

1. Rename TargetIndex enum values. NFC.
2. Introduce the new relocation type (albeit with no uses)
3. The actual meat of the change.

Not necessary, I just noticed the logical split.

[aardappel]

@sbc100 I'm a fan of small and focussed commits, but not a fan of new functionality without any uses/tests. Given that this commit is not particularly big, I vote to keep it together.

[dschuff]

LGTM with one nit

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp

238–241

I'm not exactly sure what these strings are for, but they should probably be changed to reflect the new naming of the enums?

[aardappel]

updated target index string descriptions

[saugustine]

The comparison on lines 81 and 82 of InputChunks.cpp is always true.

if (rel.Type != R_WASM_GLOBAL_INDEX_LEB ||
    rel.Type != R_WASM_GLOBAL_INDEX_I32) {

This causes build failures:

InputChunks.cpp:81:45: error: overlapping comparisons always evaluate to true [-Werror,-Wtautological-overlap-compare]

[aardappel]

@saugustine thanks, pushed a fix.

The perils of WFH developing with Visual Studio :(

[saugustine]

Appreciate the quick fix. Thanks.

[aardappel]

This has already landed.

llvm/include/llvm/BinaryFormat/Dwarf.def

 HANDLE_DW_OP(0xa7, xderef_type, 5, DWARF)
 HANDLE_DW_OP(0xa8, convert, 5, DWARF)
 HANDLE_DW_OP(0xa9, reinterpret, 5, DWARF)
 // Vendor extensions:
 // Extensions for GNU-style thread-local storage.
 HANDLE_DW_OP(0xe0, GNU_push_tls_address, 0, GNU)
 // Extensions for WebAssembly.
 HANDLE_DW_OP(0xed, WASM_location, 0, WASM)
+HANDLE_DW_OP(0xee, WASM_location_int, 0, WASM)
 // The GNU entry value extension.
 HANDLE_DW_OP(0xf3, GNU_entry_value, 0, GNU)
 // Extensions for Fission proposal.
 HANDLE_DW_OP(0xfb, GNU_addr_index, 0, GNU)
 HANDLE_DW_OP(0xfc, GNU_const_index, 0, GNU)
 
 // DWARF languages.
 HANDLE_DW_LANG(0x0001, C89, 0, 2, DWARF)

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (!includeMinimalInlineScopes()) {
     }
     case TargetFrameLowering::DwarfFrameBase::CFA: {
       DIELoc *Loc = new (DIEValueAllocator) DIELoc;
       addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_call_frame_cfa);
       addBlock(*SPDie, dwarf::DW_AT_frame_base, Loc);
       break;
     }
     case TargetFrameLowering::DwarfFrameBase::WasmFrameBase: {
+      // FIXME: don't want to depend on target specific headers?
+      const unsigned TI_GLOBAL_RELOC = 3;
+      if (FrameBase.Location.WasmLoc.Kind == TI_GLOBAL_RELOC) {
+        // These need to be relocatable.
+        assert(FrameBase.Location.WasmLoc.Index == 0);  // Only SP so far.
+        auto SPSym = Asm->GetExternalSymbolSymbol("__stack_pointer");
+        DIELoc *Loc = new (DIEValueAllocator) DIELoc;
+        addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_WASM_location_int);
+        addUInt(*Loc, dwarf::DW_FORM_udata, FrameBase.Location.WasmLoc.Kind);
+        addLabel(*Loc, dwarf::DW_FORM_udata, SPSym);
+        DD->addArangeLabel(SymbolCU(this, SPSym));
+        addUInt(*Loc, dwarf::DW_FORM_data1, dwarf::DW_OP_stack_value);
+        addBlock(*SPDie, dwarf::DW_AT_frame_base, Loc);
+      } else {
         DIELoc *Loc = new (DIEValueAllocator) DIELoc;
         DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
         DIExpressionCursor Cursor({});
         DwarfExpr.addWasmLocation(FrameBase.Location.WasmLoc.Kind,
-                                FrameBase.Location.WasmLoc.Index);
         DwarfExpr.addExpression(std::move(Cursor));
         addBlock(*SPDie, dwarf::DW_AT_frame_base, DwarfExpr.finalize());
+      }
       break;
     }
     }
   }
 
   // Add name to the name table, we do this here because we're guaranteed
   // to have concrete versions of our DW_TAG_subprogram nodes.
   DD->addSubprogramNames(*CUNode, SP, *SPDie);

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

static DescVector getDescriptions() {
   Descriptions[DW_OP_form_tls_address] = Desc(Op::Dwarf3);
   Descriptions[DW_OP_call_frame_cfa] = Desc(Op::Dwarf3);
   Descriptions[DW_OP_bit_piece] = Desc(Op::Dwarf3, Op::SizeLEB, Op::SizeLEB);
   Descriptions[DW_OP_implicit_value] =
       Desc(Op::Dwarf3, Op::SizeLEB, Op::SizeBlock);
   Descriptions[DW_OP_stack_value] = Desc(Op::Dwarf3);
   Descriptions[DW_OP_WASM_location] =
       Desc(Op::Dwarf4, Op::SizeLEB, Op::SignedSizeLEB);
+  Descriptions[DW_OP_WASM_location_int] =
+      Desc(Op::Dwarf4, Op::SizeLEB, Op::SignedSize4);
   Descriptions[DW_OP_GNU_push_tls_address] = Desc(Op::Dwarf3);
   Descriptions[DW_OP_addrx] = Desc(Op::Dwarf4, Op::SizeLEB);
   Descriptions[DW_OP_GNU_addr_index] = Desc(Op::Dwarf4, Op::SizeLEB);
   Descriptions[DW_OP_GNU_const_index] = Desc(Op::Dwarf4, Op::SizeLEB);
   Descriptions[DW_OP_GNU_entry_value] = Desc(Op::Dwarf4, Op::SizeLEB);
 
   Descriptions[DW_OP_convert] = Desc(Op::Dwarf5, Op::BaseTypeRef);
   Descriptions[DW_OP_entry_value] = Desc(Op::Dwarf5, Op::SizeLEB);

llvm/lib/Target/WebAssembly/WebAssembly.h

 void initializeWebAssemblyCFGSortPass(PassRegistry &);
 void initializeWebAssemblyCFGStackifyPass(PassRegistry &);
 void initializeWebAssemblyExplicitLocalsPass(PassRegistry &);
 void initializeWebAssemblyLowerBrUnlessPass(PassRegistry &);
 void initializeWebAssemblyRegNumberingPass(PassRegistry &);
 void initializeWebAssemblyPeepholePass(PassRegistry &);
 
 namespace WebAssembly {
-enum TargetIndex { TI_LOCAL_START, TI_GLOBAL_START, TI_OPERAND_STACK_START };
+enum TargetIndex {
+  // Followed by a local index (LEB).
+  TI_LOCAL,
+  // Followed by an absolute global index (LEB). DEPRECATED.
+  TI_GLOBAL_FIXED,
+  TI_OPERAND_STACK,
+  // Followed by a compilation unit relative global index (int32_t)
+  // that will have an associated relocation.
+  TI_GLOBAL_RELOC
+};
 } // end namespace WebAssembly
 
 } // end namespace llvm
 
 #endif

llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp

for (MachineInstr *DBI : reverse(DbgValues)) {
     Clone->getOperand(0).setReg(NewReg);
     MBB->insert(Insert, Clone);
   }
 }
 
 void WebAssemblyDebugValueManager::replaceWithLocal(unsigned LocalId) {
   for (auto *DBI : DbgValues) {
     MachineOperand &Op = DBI->getOperand(0);
-    Op.ChangeToTargetIndex(llvm::WebAssembly::TI_LOCAL_START, LocalId);
+    Op.ChangeToTargetIndex(llvm::WebAssembly::TI_LOCAL, LocalId);
   }
 }

llvm/lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp

 
 TargetFrameLowering::DwarfFrameBase
 WebAssemblyFrameLowering::getDwarfFrameBase(const MachineFunction &MF) const {
   DwarfFrameBase Loc;
   Loc.Kind = DwarfFrameBase::WasmFrameBase;
   const WebAssemblyFunctionInfo &MFI = *MF.getInfo<WebAssemblyFunctionInfo>();
   if (needsSP(MF) && MFI.isFrameBaseVirtual()) {
     unsigned LocalNum = MFI.getFrameBaseLocal();
-    Loc.Location.WasmLoc = {WebAssembly::TI_LOCAL_START, LocalNum};
+    Loc.Location.WasmLoc = {WebAssembly::TI_LOCAL, LocalNum};
   } else {
     // TODO: This should work on a breakpoint at a function with no frame,
     // but probably won't work for traversing up the stack.
-    // TODO: This needs a relocation for correct __stack_pointer
-    Loc.Location.WasmLoc = {WebAssembly::TI_GLOBAL_START, 0};
+    Loc.Location.WasmLoc = {WebAssembly::TI_GLOBAL_RELOC, 0};
   }
   return Loc;
 }

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp

bool WebAssemblyInstrInfo::reverseBranchCondition(
 
   Cond.front() = MachineOperand::CreateImm(!Cond.front().getImm());
   return false;
 }
 
 ArrayRef<std::pair<int, const char *>>
 WebAssemblyInstrInfo::getSerializableTargetIndices() const {
   static const std::pair<int, const char *> TargetIndices[] = {
-      {WebAssembly::TI_LOCAL_START, "wasm-local-start"},
-      {WebAssembly::TI_GLOBAL_START, "wasm-global-start"},
-      {WebAssembly::TI_OPERAND_STACK_START, "wasm-operator-stack-start"}};
+      {WebAssembly::TI_LOCAL, "wasm-local-start"},
+      {WebAssembly::TI_GLOBAL_FIXED, "wasm-global-start"},
+      {WebAssembly::TI_OPERAND_STACK, "wasm-operator-stack-start"},
+      {WebAssembly::TI_GLOBAL_RELOC, "wasm-global-start-reloc"}};
   return makeArrayRef(TargetIndices);
 }

llvm/test/CodeGen/WebAssembly/debugtest-opt.ll

 ; RUN: llc < %s -filetype=obj -o - | llvm-dwarfdump -
 
 ; To regenerate this file, use approximately the following C code:
 ; int* globl;
 ; void baz(int arg) {
 ;   int locl;
 ;   globl = &locl;
 ; }
 
 ; CHECK: DW_TAG_subprogram
 ; CHECK-NEXT:                DW_AT_low_pc
 ; CHECK-NEXT:                DW_AT_high_pc
 ;; Check that we fall back to the default frame base (the global)
-; CHECK-NEXT:                DW_AT_frame_base (DW_OP_WASM_location 0x1 +0, DW_OP_stack_value)
+; CHECK-NEXT:                DW_AT_frame_base (DW_OP_WASM_location_int 0x3 +0, DW_OP_stack_value)
 
 ; TODO: Find a more-reduced test case for The fix in WebAssemblyRegColoring
 
 ; ModuleID = 'debugtest-opt.c'
 source_filename = "debugtest-opt.c"
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32"
 

llvm/test/MC/WebAssembly/dwarfdump.ll

 ; CHECK-NEXT:                DW_AT_type (0x00000059 "void()")
 
 ; CHECK: 0x00000059:   DW_TAG_subroutine_type
 ; CHECK-NEXT:                DW_AT_prototyped (true)
 
 ; CHECK: 0x0000005a:   DW_TAG_subprogram
 ; CHECK-NEXT:                DW_AT_low_pc (0x0000000000000002)
 ; CHECK-NEXT:                DW_AT_high_pc  (0x0000000000000004)
-; CHECK-NEXT:                DW_AT_frame_base (DW_OP_WASM_location 0x1 +0, DW_OP_stack_value)
+; CHECK-NEXT:                DW_AT_frame_base (DW_OP_WASM_location_int 0x3 +0, DW_OP_stack_value)
 ; CHECK-NEXT:                DW_AT_name ("f2")
 ; CHECK-NEXT:                DW_AT_decl_file  ("/usr/local/google/home/sbc/dev/wasm/simple{{[/\\]}}test.c")
 ; CHECK-NEXT:                DW_AT_decl_line  (2)
 ; CHECK-NEXT:                DW_AT_prototyped (true)
 ; CHECK-NEXT:                DW_AT_external   (true)
 
 ; CHECK: 0x0000006e:   NULL