[WebAssembly] locals can now be indirect in DWARF

[aardappel]

Summary

This for example to indicate that byval args are represented by a pointer to a struct.
We could have used DW_OP_deref for this, but we needed to change TargetIndex
anyway to be able to pass on indirect-ness thru LLVM, so leaving it target specific
seemed cleaner.
Followup to https://reviews.llvm.org/D94140

[RReverser]

Note: adding this representation will require changing the spec at https://yurydelendik.github.io/webassembly-dwarf/ as some non-LLVM based tooling can also rely on it.

[RReverser]

Actually, instead of using DW_AT_location, I think the better approach here would be to add DW_AT_calling_convention to the struct entry itself. Normally DWARF relies on consumers knowing the ABI of any given type, but that attribute is explicitly designed to indicate whether a composite type is passed by reference or by value.

[aardappel]

@RReverser is one struct guaranteed to always be passed the same? Here structs are passed by pointer, sometimes they can get passed in registers/locals..

[RReverser]

is one struct guaranteed to always be passed the same?

I think it is, at least with today's ABIs.

[dschuff]

This approach definitely makes for simple implementation. It does seem a little unfortunate (at the dwarf level) that we are inventing something target-specific for a case that could be covered by something generic.
Is it possible just to modify DwarfExpression::addWasmLocation() to slip an extra DW_OP_deref into the expression when the TI kind is indirect? or does that have to be done elsewhere, e.g. in the caller?

In D94347#2488099, @RReverser wrote:

Actually, instead of using DW_AT_location, I think the better approach here would be to add DW_AT_calling_convention to the struct entry itself. Normally DWARF relies on consumers knowing the ABI of any given type, but that attribute is explicitly designed to indicate whether a composite type is passed by reference or by value.

That could maybe work. One potential downside is that it doesn't really match up with how LLVM decides to pass the structure. The calling_convention and other struct attributes are generated by the frontend but it's actually the use of 'byval' in the IR that makes the backend pass it indirectly. Although thinking about that a little more, maybe that doesn't really matter. Another frontend would have to generate its own type descriptors and also independently decide whether it wanted to use byval or not.

Other things to consider on that:

1. does lldb or the devtools debugger already use the calling_convention description on types, or would this require more work there? (relatedly, does clang generate this attribute already?)
2. we still intend to try a new ABI that would pass some structs by value in register(s). In this case the frontend would presumably just not use 'byval' for those, but it would also need to generate composite debug info. so it's not obvious right now (to me) whether that would be easier or harder with either option.

[RReverser]

does lldb or the devtools debugger already use the calling_convention description on types, or would this require more work there? (relatedly, does clang generate this attribute already?)

I'm not sure. Now that I check it, this attribute was only added to type definitions in DWARF 5 (before that, only functions had a DW_AT_calling_convention), so it's possible that tools need to catch up. However, my understanding is that either of those attributes would require adding support to the debugger, so they should be equal.

In this case the frontend would presumably just not use 'byval' for those, but it would also need to generate composite debug info.

Yeah, regardless of ABI, the composite type debug entry always has to be generated, and AFAIK the suggested ABI changes also would fit into "attribute on the type itself" model.

so it's not obvious right now (to me) whether that would be easier or harder with either option.

I agree in terms of complexity there is no clear winner. I'm mostly hesitant against adding custom target-specific encodings where an underlying standard attribute can be used - once we add a custom location variant, other tools might start to rely on it and it will be hard to remove.

For now it was mostly LLVM + Emscripten + DevTools, but I know at least few other projects working with Wasm DWARF info now, so just want to make sure we try to stay forward-compatible and use underlying standard where possible. Maybe I'm overthinking this?

[aardappel]

This solution is certainly driven by what fits well in the current way LLVM does things, though all the different steps of lowering.

I can "slip in" a DW_OP_deref, but then the bool that indicates that this needs to happen needs to be passed through all the way from ExplicitLocals to the DWARF emitter, for which the likeliest storage is the TargetIndex like we are doing currently. So at least code-wise this solution would be less elegant, since now we're duplicating information at the end.

I am sure there are other structures possible, but I would suggest that is potential future improvement.

I think it is, at least with today's ABIs.

Do these ABIs need to be adhered to for static functions calling each other internally to a compilation unit (in -O2)?

[dschuff]

I looked a little at DW_AT_calling_convention and it looks like clang does use it (e.g. DW_CC_pass_by_reference), even for dwarf 4, for e.g. nontrivially-copyable classes (see e.g. https://github.com/llvm/llvm-project/blob/master/clang/test/CodeGenCXX/debug-info-composite-cc.cpp). In this case though, a function that takes this value is also generated in the IR as a pointer rather than a byval.
In order to make this work we'd probably need to make the contract between a frontend that uses byval and the backend even more obscure (i.e. it would need to mark all the type infos for structs with this attribute in addition to making them passed byval). In other words since it's the backend that decides exactly how a byval IR parameter is passed it makes sense that the backend also sets the debug info accordingly. Assuming that's possible or reasonable, of course; that's why I was asking about what it would take to pass the information through to DwarfExpression::addWasmLocation.

I can "slip in" a DW_OP_deref, but then the bool that indicates that this needs to happen needs to be passed through all the way from ExplicitLocals to the DWARF emitter, for which the likeliest storage is the TargetIndex like we are doing currently. So at least code-wise this solution would be less elegant, since now we're duplicating information at the end.

Yeah I guess to be more clear, that's basically what I had in mind; i.e. we'd still need to pass the information from ExplicitLocals to the dwarf emitter, and using the TargetIndex just as this CL does makes sense for that. the only difference would be that instead of encoding TI_LOCAL_INDIRECT directly into first argument of DW_OP_WASM_location, that the Dwarf writer would instead change the final argument of DW_AT_location to DW_OP_deref. That change is less elegant in the LLVM code but makes the standardized interface simpler.

Do these ABIs need to be adhered to for static functions calling each other internally to a compilation unit (in -O2)?

No, the midlevel optimizer can and does change the calling convention (including dropping unused arguments) for internal functions. This is especially powerful when using LTO. In order to have correct debug info it would need to change the formal parameter information of the function according to whatever optimization it does (i don't know if it actually does that; but presumably that might be easier than changing the type info for the struct type, which then would no longer match other uses of that type?)

[aardappel]

the only difference would be that instead of encoding TI_LOCAL_INDIRECT directly into first argument of DW_OP_WASM_location, that the Dwarf writer would instead change the final argument of DW_AT_location to DW_OP_deref. That change is less elegant in the LLVM code but makes the standardized interface simpler.

Ok, and it would change TI_LOCAL_INDIRECT back to TI_LOCAL as it emits DW_OP_deref ? And TI_LOCAL_INDIRECT is now marked as being internal to LLVM only?

[dschuff]

Ok, and it would change TI_LOCAL_INDIRECT back to TI_LOCAL as it emits DW_OP_deref ?

Yes... or maybe not? i.e. does it actually matter after that what value the TargetIndex MachineOperand has?

And TI_LOCAL_INDIRECT is now marked as being internal to LLVM only?

or maybe another way to say it is that the TargetIndex MachineOperand is itself an internal LLVM data structure, and it's only by happenstance that one of its members has values that can match up with DW_OP_WASM_location values?

[aardappel]

i.e. does it actually matter after that what value the TargetIndex MachineOperand has?

I meant in the sense that it also gets emitted as-is in DWARF data. So consumers currently have to check if the value is 0 or 4 to know if something is a local. That could be simplified by rewriting it back to TI_LOCAL.

[aardappel]

Made it emit a DW_OP_deref instead.

[dschuff]

@pfaffe does this make sense? (i.e. when a structure is passed by value, it is passed indirectly, and its DW_AT_location ends with a DW_OP_deref to indicate this)

llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp

64

what does the immediate operand of the DBG_VALUE represent here?

[aardappel]

llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp

64

it's the second operand which is typically either 0 (indirect) or $noreg. See use of $noreg here: https://llvm.org/docs/SourceLevelDebugging.html

[pfaffe]

DW_OP_deref loads 32bit (64bit if the address size is 8), is that what's intended here? The struct passed by value could be bigger than that, couldn't it?

Currently, DW_OP_WASM_location is always followed by a DW_OP_stack_value. Instead of emitting DW_OP_deref, what if you didn't emit the stack value op? DW_OP_stack_value indicates that the top-stack isn't an andress but is in fact the actual value. Which seems is exactly the opposite of what we need for the by-val case.

[dschuff]

Hm, that's a good point, I missed the size bit when reading the spec.
DW_OP_stack_value seems to be just sort of the generic terminator; i'm not sure what exactly it would mean if we omitted it? Really I guess it would mean whatever the debugger thinks it does.
I'm not sure whether it would be better to try to shoehorn something in here that almost fits but not quite, or to try to change what we do in the frontend (which could maybe be either changing the type of the parameter to some_struct*, or marking all struct descriptors with DW_AT_calling_convention, or perhaps even just not using byval at all and making the pointer parameter explicit in the frontend.
I'm not sure yet how hard it would be to do those. Do you have a sense of how lldb would react to having OP_deref, or leaving off OP_stack_value? (i.e. would it actually solve the problem, or would it just require a bunch of difficult work on the debugger side?)

[pfaffe]

OP_deref would work if the structs are small enough. Leaving out OP_stack_value should just work. It's a terminator of sorts for values that aren't in memory, like registers on native or locals or globals in wasm land.

Edit: remove bogus comparison to &* operators

[dblaikie]

(haven't followed this whole conversation - but I will say that DW_AT_calling_convention is used/needed for knowing how to call a function to meet the ABI (do you stuff the member variables in certain registers then call, or do you make a copy on the stack and stuff a pointer to that copy in a register then call) - the DW_AT_locations of variables aren't used for calling (they may not be present - perhaps there's a member function you have a declaration for - or even a function you have no DWARF for, but you can demangle it and find the parameter types in the DWARF) as they may not. describe the location of variables at the start of the function/as-the-ABI-specifies them. so probably best not to use that feature as a property for determining DW_AT_locations of variables)

[aardappel]

Removed the redundant DW_OP_stack_value after DW_OP_deref.

[aardappel]

Please see DwarfExpression.cpp if you think that Memory is indeed the correct LocationKind in this case. It is this kind not being Implicit that suppresses the DW_OP_stack_value from being emitted at the end of DwarfExpression::addExpression.

[pfaffe]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

671

If I understand correctly what TI_LOCAL_INDIRECT refers to then this leads to a double dereference.

Say you have a function void foo(T t) where t needs to be passed in memory as per ABI. Does TI_LOCAL_INDIRECT mean that when foo is lowered as .functype foo (i32) -> (), the value of that i32 TI_LOCAL_INDIRECT local holds the address of t?

The DW_OP_deref is superfluous then. DWARF expressions by default compute addresses. So once you've loaded that local's value via DW_OP_WASM_location 0x0 onto the DWARF stack, you're done! Top-stack now contains the local's value, which is the address of the variable t.

[aardappel]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

671

The TI_LOCAL_INDIRECT is turned back into a TI_LOCAL at this point, so how is then still a double deref?

Previous conversations had indicated that the indirectness of these byval args is best indicated with a DW_OP_deref, not with TI_LOCAL_INDIRECT, are you saying you'd prefer the reverse?

[aardappel]

To @pfaffe suggestion changed representation such that TI_LOCAL_INDIRECT emits neither a DW_OP_deref nor DW_OP_stack_value since the result of the expression is already a pointer to the value.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

if (!DwarfExpr.addMachineRegExpression(TRI, Cursor, Location.getReg()))
       return;
     return DwarfExpr.addExpression(std::move(Cursor));
   } else if (Value.isTargetIndexLocation()) {
     TargetIndexLocation Loc = Value.getTargetIndexLocation();
     // TODO TargetIndexLocation is a target-independent. Currently only the WebAssembly-specific
     // encoding is supported.
     assert(AP.TM.getTargetTriple().isWasm());
     DwarfExpr.addWasmLocation(Loc.Index, static_cast<uint64_t>(Loc.Offset));
-      DwarfExpr.addExpression(std::move(ExprCursor));
-      return;
   } else if (Value.isConstantFP()) {
     if (AP.getDwarfVersion() >= 4 && !AP.getDwarfDebug()->tuneForSCE() &&
         !ExprCursor) {
       DwarfExpr.addConstantFP(Value.getConstantFP()->getValueAPF(), AP);
       return;
     }
     if (Value.getConstantFP()->getValueAPF().bitcastToAPInt().getBitWidth() <=
         64 /*bits*/)

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

 void DwarfExpression::emitLegacyZExt(unsigned FromBits) {
   // (X & (1 << FromBits - 1))
   emitOp(dwarf::DW_OP_constu);
   emitUnsigned((1ULL << FromBits) - 1);
   emitOp(dwarf::DW_OP_and);
 }
 
 void DwarfExpression::addWasmLocation(unsigned Index, uint64_t Offset) {
-  assert(LocationKind == Implicit || LocationKind == Unknown);
-  LocationKind = Implicit;
   emitOp(dwarf::DW_OP_WASM_location);
-  emitUnsigned(Index);
+  emitUnsigned(Index == 4/*TI_LOCAL_INDIRECT*/ ? 0/*TI_LOCAL*/ : Index);
   emitUnsigned(Offset);
+  if (Index == 4 /*TI_LOCAL_INDIRECT*/) {
+    assert(LocationKind == Unknown);
+    LocationKind = Memory;
+  } else {
+    assert(LocationKind == Implicit || LocationKind == Unknown);
+    LocationKind = Implicit;
+  }
 }

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

case Operation::SizeLEB:
         Operands[Operand] = Data.getULEB128(&Offset);
       break;
     case Operation::BaseTypeRef:
       Operands[Operand] = Data.getULEB128(&Offset);
       break;
     case Operation::WasmLocationArg:
       assert(Operand == 1);
       switch (Operands[0]) {
-      case 0: case 1: case 2:
+      case 0:
+      case 1:
+      case 2:
+      case 4:
         Operands[Operand] = Data.getULEB128(&Offset);
         break;
       case 3: // global as uint32
          Operands[Operand] = Data.getU32(&Offset);
          break;
       default:
         return false; // Unknown Wasm location
       }
if (Size == Operation::BaseTypeRef && U) {
       // which is currently not supported.
       if (Opcode == DW_OP_convert && Operands[Operand] == 0)
         OS << " 0x0";
       else
         prettyPrintBaseTypeRef(U, OS, DumpOpts, Operands, Operand);
     } else if (Size == Operation::WasmLocationArg) {
       assert(Operand == 1);
       switch (Operands[0]) {
-      case 0: case 1: case 2:
+      case 0:
+      case 1:
+      case 2:
       case 3: // global as uint32
+      case 4:
         OS << format(" 0x%" PRIx64, Operands[Operand]);
         break;
       default: assert(false);
       }
     } else if (Size == Operation::SizeBlock) {
       uint64_t Offset = Operands[Operand];
       for (unsigned i = 0; i < Operands[Operand - 1]; ++i)
         OS << format(" 0x%02x", Expr->Data.getU8(&Offset));

llvm/lib/Target/WebAssembly/WebAssembly.h

 void initializeWebAssemblyPeepholePass(PassRegistry &);
 
 namespace WebAssembly {
 enum TargetIndex {
   // Followed by a local index (ULEB).
   TI_LOCAL,
   // Followed by an absolute global index (ULEB). DEPRECATED.
   TI_GLOBAL_FIXED,
+  // Followed by the index from the bottom of the Wasm stack.
   TI_OPERAND_STACK,
   // Followed by a compilation unit relative global index (uint32_t)
   // that will have an associated relocation.
-  TI_GLOBAL_RELOC
+  TI_GLOBAL_RELOC,
+  // Like TI_LOCAL, but indicates an indirect value (e.g. byval arg
+  // passed by pointer).
+  TI_LOCAL_INDIRECT
 };
 } // end namespace WebAssembly
 
 } // end namespace llvm
 
 #endif

llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp

for (MachineInstr *DBI : reverse(DbgValues)) {
     MachineInstr *Clone = MF->CloneMachineInstr(DBI);
     Clone->getDebugOperand(0).setReg(NewReg);
     MBB->insert(Insert, Clone);
   }
 }
 
 void WebAssemblyDebugValueManager::replaceWithLocal(unsigned LocalId) {
   for (auto *DBI : DbgValues) {
-    MachineOperand &Op = DBI->getDebugOperand(0);
-    Op.ChangeToTargetIndex(llvm::WebAssembly::TI_LOCAL, LocalId);
+    MachineOperand &Op0 = DBI->getDebugOperand(0);
+    MachineOperand &Op1 = DBI->getOperand(1);
+    bool Indirect = Op1.isImm() && Op1.getImm() == 0;
+    Op0.ChangeToTargetIndex(Indirect ? llvm::WebAssembly::TI_LOCAL_INDIRECT
+                                     : llvm::WebAssembly::TI_LOCAL,
+                            LocalId);
   }
 }

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.cpp

 }
 
 ArrayRef<std::pair<int, const char *>>
 WebAssemblyInstrInfo::getSerializableTargetIndices() const {
   static const std::pair<int, const char *> TargetIndices[] = {
       {WebAssembly::TI_LOCAL, "wasm-local"},
       {WebAssembly::TI_GLOBAL_FIXED, "wasm-global-fixed"},
       {WebAssembly::TI_OPERAND_STACK, "wasm-operand-stack"},
-      {WebAssembly::TI_GLOBAL_RELOC, "wasm-global-reloc"}};
+      {WebAssembly::TI_GLOBAL_RELOC, "wasm-global-reloc"},
+      {WebAssembly::TI_LOCAL_INDIRECT, "wasm-local-indirect"}};
   return makeArrayRef(TargetIndices);
 }

llvm/test/MC/WebAssembly/debug-byval-struct.ll

 
 
 
 ; CHECK-LABEL:  DW_TAG_formal_parameter
 ; CHECK-NEXT:     DW_AT_location        (DW_OP_fbreg +12)
 ; CHECK-NEXT:     DW_AT_name    ("x")
 
 ; CHECK-LABEL:  DW_TAG_formal_parameter
-; CHECK-NEXT:     DW_AT_location        (DW_OP_WASM_location 0x0 0x1, DW_OP_stack_value)
+; CHECK-NEXT:     DW_AT_location        (DW_OP_WASM_location 0x0 0x1)
 ; CHECK-NEXT:     DW_AT_name    ("some_union")
 
 ; CHECK-LABEL:  DW_TAG_formal_parameter
-; CHECK-NEXT:     DW_AT_location        (DW_OP_WASM_location 0x0 0x2, DW_OP_stack_value)
+; CHECK-NEXT:     DW_AT_location        (DW_OP_WASM_location 0x0 0x2)
 ; CHECK-NEXT:     DW_AT_name    ("some_struct")
 
 ; CHECK-LABEL:  DW_TAG_formal_parameter
 ; CHECK-NEXT:     DW_AT_location        (DW_OP_fbreg +8)
 ; CHECK-NEXT:     DW_AT_name    ("a")