[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:
> 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.


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

https://reviews.llvm.org/D79428