Page MenuHomePhabricator

[WebAssembly] Added Debug Fixup pass
ClosedPublic

Authored by aardappel on May 5 2020, 9:33 AM.

Details

Summary

This pass changes debug_value instructions referring to stackified registers into TI_OPERAND_STACK with correct stack depth.

Diff Detail

Event Timeline

aardappel created this revision.May 5 2020, 9:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2020, 9:33 AM
dschuff added inline comments.May 5 2020, 1:56 PM
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
75

style nit:
LLVM style would be something like
for (auto MII = MBB.begin, MIE = MBB.end(); MII != MIE; ++MII)

98

So I guess this is building DBG_VALUE which sets the value to register 0, which clears it? Why advance the iterator by 2? Wouldn't we need to keep advancing until the use of this def? Or until it comes off the stack?

Nice that this approach works!

  • Please clang-format
  • Can we have some more test cases other than one-def/one-use case? Like, when we have multiple stackified values at a time and uses them in one or multiple instruction, or when we have multivalue calls. I guess some tests in reg-stackify.ll or multivalue.ll can be adapted here.
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
80

Nit: MO.getReg() can be MO.getReg().isValid() to be more readable

98

In a simple scenario that a register is used right after it is stackified (as in the test case in this CL), it can be iterator+2, but

  • Does this handle cases where multiple values are stackified and later used, i.e., does this handle when stack depth > 1?
  • Even in one-def / one-use scenario, is it guaranteed that the popping instruction is right after DBG_VALUE? There can be other instructions that do not consume stack values in between. Also there can be other debug instructions and label instructions.

I think you need to find the 'use' instruction of the given register here, using some functions like these in MachineRegisterInfo, I guess.

And there are overloaded BuildMI functions that are designed to be used to create DBG_VALUE.

119

I'm not sure we should exclude the condition MO.isDead() here.

There can be dead registers (and drops) in normal scenarios in which we don't run -wasm-disable-explicit-locals. Shouldn't we preserve DBG_VALUEs between the value-pushing instruction and the subsequent drop? For example, when @somefunction pushes one value onto the stack,

call @somefunction
                               <-Here we need to preserve debug values, no?
drop

In general we can't even guarantee that the drop will follow right after the value-pushing instruction. So it is possible:

call @somefunction
call @somefunction
drop
drop

The problem of -wasm-disable-explicit-locals is it does not insert necessary drops, and thus making the stack invalid, as you saw. How about not running this pass entirely when -wasm-disable-explicit-locals is given? I mean, in WebAssemblyTargetMachine.cpp, we can do something like

// Some comments about why -wasm-disable-explicit-locals matters
if (!WasmDisableExplicitLocals)
  // Fix debug_values whose defs have been stackified.                           
  addPass(createWebAssemblyDebugFixup());

To do this, we need to move WasmDisableExplicitLocals from WebAssemblyExplicitLocals.cpp to WebAssemblyTargetMachine.cpp, but it's no big deal.

llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
988 ↗(On Diff #262142)

I don't think we need to exclude MO.isDead() here, regardless of what we decide to do with ExplicitLocal pass above, because this code runs before ExplicitLocal pass, so this will not fail anyway.

llvm/test/CodeGen/WebAssembly/stackified-debug.ll
24

Is this the encoded stack depth? Then how about mentioning that in the comment, like

; CHECK:   .int8  2                       # TI_OPERAND_STACK (2)
; CHECK:   .int8  0                       # Stack offset
29

Nit: This can be deleted

31

Nit: No need of -wasm

33

Nit: No need of hidden

51

Do we need these attributes for debug value testing? If not we can remove all these I think (or leave only the necessary ones)

aardappel marked 9 inline comments as done.May 7 2020, 5:47 PM
aardappel added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
75

We are inserting new instructions, so depending on the container its better to be calling end() here every time?

98

You are both right, this should be at the use not at the def.
Will add comments to make it clearer too.
Will use DBG_VALUE variant.

119

That's what I had originally, disable the pass when Explicit Locals is off. But I thought this was more elegant since I didn't want to disable a whole bunch of tests unnecessarily. I can revert it, though.

Note that this only happens in the case of an unused variable, but yeah, I guess worth preserving.

aardappel updated this revision to Diff 262796.May 7 2020, 5:48 PM
aardappel marked 2 inline comments as done.

DBG_VALUE $noreg now on pop of stackified reg.

aardappel updated this revision to Diff 262929.May 8 2020, 12:10 PM
aardappel marked 5 inline comments as done.

Improved test

dschuff accepted this revision.May 12 2020, 11:19 AM
dschuff added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
37

Since this pass only handles stackified values (and other debug info is handled elsewhere) should we name it something more specific like WebAssemblyDebugStackify instead?

This revision is now accepted and ready to land.May 12 2020, 11:19 AM
aheejin added inline comments.May 13 2020, 5:40 AM
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
98

I might be mistaken, but I'm not sure it is guaranteed that dbg.value instruction follows right after the corresponding def instruction. For example, this seems possible, in which dbg.value does not follow right after its def:

r0 = ...
r1 = ...
...
dbg.value r0, ... ;; at this point stack size is 2 (depth is 1), but r0's depth should be 0
dbg.value r1, ...

In that case it seems we can't use the stack depth as a target index.? (We may need to search the whole stack to find the matching register.)

It would be good to add a test case for this kind of case as well.

Also typo in the comment: need it construct -> need it to construct

114

Nit: If we do

BuildMI(*Prev.DebugValue->getParent(), std::next(MII), ...

We don't need a separate TMII variable.

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
56

Nit: clang-format

llvm/test/CodeGen/WebAssembly/stackified-debug.ll
59

Nit: We may not need bitcasts and tail calls. How about making this simpler by something like

define void @foo() {                                                             
entry:                                                                           
  %call = call i32 @input()
  call void @llvm.dbg.value(metadata i32 %call, metadata !16, metadata !DIExpression()), !dbg !19                                                  
  %call1 = call i32 @input()
  call void @llvm.dbg.value(metadata i32 %call1, metadata !17, metadata !DIExpression()), !dbg !19                                            
  call void @output(i32 %call, i32 %call1)                                       
  ret void                                                                       
}                                                                                
                                                                                 
declare i32 @input()                                                             
declare void @output(i32, i32)

? (This does not include llvm.dbg.value; they have to be added)
Also it looks we don't need attributes and local_unnamed_addrs for this test.

Also it'd be better to add a test case when llvm.dbg.value is not right after its corresponding def instruction.

aheejin added inline comments.May 13 2020, 5:42 AM
llvm/test/CodeGen/WebAssembly/stackified-debug.ll
59

"(This does not include llvm.dbg.value; they have to be added)" was added by mistake, nvm

aardappel marked 5 inline comments as done.May 14 2020, 10:57 AM
aardappel added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
98

I don't believe this is the case. For example, https://llvm.org/docs/SourceLevelDebugging.html says "The dbg.value’s position in the IR defines where in the instruction stream the variable’s value changes.". Reading our existing code we seem to assume in quite a few places that this is the case (e.g. WebAssemblyDebugValueManager).

We could maybe add some kind of check/assert that this is indeed the case? But then again we'd need that in a few other places too.

addressed aheejin's last comments.

dschuff added inline comments.May 14 2020, 11:45 AM
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
98

Wrt the source-level debugging doc, it has an example of dbg.value in LLVM IR converting to DBG_VALUE in MIR:
https://llvm.org/docs/SourceLevelDebugging.html#variable-locations-in-instruction-selection-and-mir
it has and example where the DBG_VALUE for %2 is not even in the same BB as the def. It describes a case where the value is a compound DIE expression computed with a GEP and folded into a load instruction.

In the debugging meeting today, @yurydelendik also said he thought maybe there were cases where the dbg.value wasn't directly following the def, but didn't have any specific examples.

More generally we should probably not ship an emscripten with an assert if we aren't sure the invariant actually holds. But if this CL can cover a useful subset of cases it might make sense to get it in now, unless we think the algorithm is fundamentally broken or something.
It would be nice if we had a way other than an assert to discover these kinds of cases.
Is there a way we can make the output obviously wrong in those cases so we can figure that out? Or maybe a flag or something we could enable? That way we could turn it on, and compile a bunch of our tests in debug mode to find things, without having it on for everyone.

aardappel marked an inline comment as done.May 14 2020, 12:02 PM
aardappel added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
98

Note that the code in this pass only deals with DBG_VALUE that refers to a valid register that has been stackified, which would exclude the example above (no stackification across blocks, for starters).

If it is indeed possible to generate out-of-order DBG_VALUE instructions for stackified values, then likely one of the asserts we already have would hit:

assert(Depth > 0 &&
       "WebAssemblyDebugFixup: DBG_VALUE: No value on the stack!");
assert(MO.getReg() == Stack.back().Reg &&
       "WebAssemblyDebugFixup: DBG_VALUE: Register not matched!");

Those asserts haven't hit with any of the tests so far.

Do we want to replace these asserts with an if that.. just leaves the register? Or sets an obviously invalid stack offset? (unsigned)-1 ? Or introduce a TI _INVALID debug location, that clearly signals we had debug information we weren't able to track..

aardappel marked an inline comment as done.May 14 2020, 12:08 PM
aardappel added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
98

Actually, a more resilient algo would be: rather than assert that the Reg is on the top of the stack, I could simply search the stack for that Reg, and derive the Depth from it. This is nice because the stack should still be correct if a DBG_VALUE can still work if its moved anywhere between the push(def) and pop(use).

If its not found in the stack that means its moved outside of its def-use range alltogether, in which case it is probably fine to leave it untouched, and let it be removed as an orphaned Reg.

Of course, if a DBG_VALUE is moved further ahead from its def, then a debugger that is stopped right after the def can potentially not view it, but that is a different matter. We could also fix this with an algorithm that places DBG_VALUEs back where they belong, but I am not sure this complexity is warranted until we find common cases of this.

Allowed DBG_VALUE for stackified regs to appear anywhere between its def-use.

forgot 1 .back() reference + clang-format

dschuff added inline comments.May 14 2020, 12:50 PM
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
98

Ah, that's a good point that this only applies to stackified values.

Searching the stack seems like a good idea though, and simple enough to implement.
Also yeah you're probably right that moving the DBG_VALUEs back isn't worth it.

great, still LGTM

aheejin added inline comments.May 14 2020, 4:02 PM
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
98

Actually, a more resilient algo would be: rather than assert that the Reg is on the top of the stack, I could simply search the stack for that Reg, and derive the Depth from it.

This is what I suggested in the last comment. But I think it'd be better to do the reverse search, because the value is likely on top of the stack (or near the top of the stack).

llvm/test/CodeGen/WebAssembly/stackified-debug.ll
59

As I said in the last comment, it'd be good to have a test case when llvm.dbg.value is not right after the corresponding def instruction..? We now can handle this but don't have a test case for it.

I get assertion failures in debug builds:

assert(cast<DILocalVariable>(Variable)->isValidLocationForIntrinsic(DL) &&
       "Expected inlined-at fields to agree");

fails, when I compile a test program with -O3, with the following call stack, where MachineFunctionPass is a WebAssemblyDebugFixup:

>	clang++.exe!llvm::BuildMI(llvm::MachineFunction & MF, const llvm::DebugLoc & DL, const llvm::MCInstrDesc & MCID, bool IsIndirect, llvm::Register Reg, const llvm::MDNode * Variable, const llvm::MDNode * Expr) Line 2040	C++
 	clang++.exe!llvm::BuildMI(llvm::MachineBasicBlock & BB, llvm::MachineInstrBundleIterator<llvm::MachineInstr,0> I, const llvm::DebugLoc & DL, const llvm::MCInstrDesc & MCID, bool IsIndirect, llvm::Register Reg, const llvm::MDNode * Variable, const llvm::MDNode * Expr) Line 2073	C++
 	clang++.exe!`anonymous namespace'::WebAssemblyDebugFixup::runOnMachineFunction(llvm::MachineFunction & MF) Line 125	C++
 	clang++.exe!llvm::MachineFunctionPass::runOnFunction(llvm::Function & F) Line 73	C++
 	clang++.exe!llvm::FPPassManager::runOnFunction(llvm::Function & F) Line 1482	C++

The assertion fails because in llvm/include/llvm/IR/DebugInfoMetadata.h

 bool isValidLocationForIntrinsic(const DILocation *DL) const {
  return DL && getScope()->getSubprogram() == DL->getScope()->getSubprogram();
}

the DISubprogram* don't match:

0x0000020cdabea990 {Line=0x00000075 ScopeLine=0x00000076 VirtualIndex=0x00000000 ...}	const llvm::DISubprogram *
0x0000020cdabefe20 {Line=0x0000004b ScopeLine=0x0000004b VirtualIndex=0x00000000 ...}	const llvm::DISubprogram *

@sbc100 @paolosev that crash was just fixed here: https://reviews.llvm.org/D80019

@aheejin I didn't think such a test was necessary, since a) the current code is not explicitly dependent on this adjacency anymore, b) we have code elsewhere (e.g. WebAssemblyDebugValueManager) that does assume this adjacency, so if there's any def + dbg_value that is not adjacent, this likely will require RegStackify to be fixed, and more likely this pass will continue to work. And c) we don't have an example yet where this is even produced.

aheejin added a comment.EditedMay 15 2020, 1:25 PM

@aheejin I didn't think such a test was necessary, since a) the current code is not explicitly dependent on this adjacency anymore, b) we have code elsewhere (e.g. WebAssemblyDebugValueManager) that does assume this adjacency, so if there's any def + dbg_value that is not adjacent, this likely will require RegStackify to be fixed, and more likely this pass will continue to work. And c) we don't have an example yet where this is even produced.

WebAssemblyDebugValueManager does not assume this adjacency. It calls MachineInstr::collectDebugValues, which traverses the whole BB to collect debug values instructions for a matching register. But when I change the order of llvm.dbg.values in your example and run it with -print-machineinstrs, somehow instruction selection manages to emit DBG_VALUEs after their defs. Not sure it is always guaranteed though, as in the link @dschuff pasted. Anyway, to test this case we need an mir test case, which I think is an overkill too.

But I still think reverse traversing of Stack is worth it, given that the register will be most likely to be on top of the stack? (This is also a thing I suggested, but you didn't answer that one either)

And on the other note, in general, even when you think some of my suggestions are unnecessary, I'd greatly appreciate if you answer them by explaining why they are unnecessary, rather than just ignoring them.

@aheejin Apologies if you think I ignored your comments, I feel I responded to all of them, though maybe I could have responded earlier (I wanted to first bring up your point about adjacency in the debugging subgroup meeting yesterday).

WebAssemblyDebugValueManager: ah, didn't see that.. I guess it makes them adjacent when it clones/moves them.

As for the loop in reverse.. I did think of that, but given the average expected stack depth, I didn't think it was necessary.. would be happy to add it though.