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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked an inline comment as done.
aardappel 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;
+        }
----------------
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.. 





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

https://reviews.llvm.org/D79428