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

Thu May 14 16:21:40 PDT 2020

aheejin 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:
> > 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.
> 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).

================
Comment at: llvm/test/CodeGen/WebAssembly/stackified-debug.ll:58
+  ret void, !dbg !22
+}
+
----------------
aheejin wrote:
> aheejin wrote:
> > Nit: We may not need `bitcast`s and `tail call`s. 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_addr`s 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.
> "(This does not include llvm.dbg.value; they have to be added)" was added by mistake, nvm
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.

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

https://reviews.llvm.org/D79428