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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 5 inline comments 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;
+        }
----------------
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.


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

https://reviews.llvm.org/D79428