[PATCH] D37078: [DebugInfo] Handle memory locations in LiveDebugValues

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In https://reviews.llvm.org/D37078#851603, @zturner wrote:

> Can you explain this in terms of an actual debugger scenario that this improves?


No, but this is a necessary pre-requisite to http://llvm.org/pr34136, starting from the bottom up. Clang doesn't generate the dbg.values used in these test cases yet because LLVM doesn't handle them, but it will soon.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5173
+    // node map.
+    // FIXME: Handle byval/inalloca params as above.
+    if (const AllocaInst *AI = dyn_cast<AllocaInst>(V)) {
----------------
aprantl wrote:
> rnk wrote:
> > rnk wrote:
> > > aprantl wrote:
> > > > I think this should be a separate commit?
> > > Ugh, this doesn't actually do the right thing. Here's my test that comes here:
> > > ```
> > > void useptr(int *);
> > > void use_alloca(int cond) {
> > >   int x = 0;
> > >   int *px = &x;
> > >   if (cond)
> > >     useptr(px);
> > > }
> > > ```
> > > 
> > > After optimization, `px` will be described by a dbg.value of x's alloca. This code in InstrEmitter.cpp incorrectly builds an "indirect in memory" DBG_VALUE for px:
> > > ```
> > >   if (SD->getKind() == SDDbgValue::FRAMEIX) {
> > >     // Stack address; this needs to be lowered in target-dependent fashion.
> > >     // EmitTargetCodeForFrameDebugValue is responsible for allocation.
> > >     return BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE))
> > >         .addFrameIndex(SD->getFrameIx())
> > >         .addImm(0)
> > >         .addMetadata(Var)
> > >         .addMetadata(Expr);
> > >   }
> > > ```
> > > 
> > > I guess I'll need to fix that to look for DW_OP_deref.
> > Alternatively, you could say I misunderstood `DAG.getFrameIndexDbgValue`.
> I think it would be best to split the SelectionDAG change out into a separate review and have a MIR-only test for LiveDebugValues. The LiveDebugValues change should be generally good and uncontroversial (apart from performance considerations) and this change looks like it will generate a longer discussion.
Agreed.


https://reviews.llvm.org/D37078