[PATCH] D79428: [WebAssembly] Added Debug Fixup pass

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp:97
+          // variable since later we need it construct another one on pop.
+          Stack.back().DebugValue = &MI;
+        }
----------------
aardappel wrote:
> aardappel wrote:
> > dschuff wrote:
> > > aardappel wrote:
> > > > aheejin wrote:
> > > > > 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
> > > > 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.
> > > 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.
> > 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.. 
> > 
> > 
> > 
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79428/new/

https://reviews.llvm.org/D79428