This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Corrections for salvageDebugInfo
ClosedPublic

Authored by bjope on Jul 2 2018, 9:25 AM.

Details

Summary

When salvaging a dbg.declare/dbg.addr we should not add
DW_OP_stack_value to the DIExpression
(see test/Transforms/InstCombine/salvage-dbg-declare.ll).

Consider this example

%vla = alloca i32, i64 2
call void @llvm.dbg.declare(metadata i32* %vla, metadata !1, metadata !DIExpression())

Instcombine will turn it into

%vla1 = alloca [2 x i32]
%vla1.sub = getelementptr inbounds [2 x i32], [2 x i32]* %vla, i64 0, i64 0
call void @llvm.dbg.declare(metadata [2 x i32]* %vla1.sub, metadata !19, metadata !DIExpression())

If the GEP can be eliminated, then the dbg.declare will be salvaged
and we should get

%vla1 = alloca [2 x i32]
call void @llvm.dbg.declare(metadata [2 x i32]* %vla1, metadata !19, metadata !DIExpression())

The problem was that salvageDebugInfo did not recognize dbg.declare
as being indirect (%vla1 points to the value, it does not hold the
value), so we incorrectly got

call void @llvm.dbg.declare(metadata [2 x i32]* %vla1, metadata !19, metadata !DIExpression(DW_OP_stack_value))

I also made sure that llvm::salvageDebugInfo and
DIExpression::prependOpcodes do not add DW_OP_stack_value to
the DIExpression in case no new operands are added to the
DIExpression. That way we avoid to, unneccessarily, turn a
register location expression into an implicit location expression
in some situations (see test11 in test/Transforms/LICM/sinking.ll).

Diff Detail

Event Timeline

bjope created this revision.Jul 2 2018, 9:25 AM
aprantl accepted this revision.Jul 2 2018, 9:36 AM

Thanks. This is an obvious improvement. It begs the question whether there is a better / more principled condition rather than "the expression is empty" to decide whether the stack_value should be added or not. Let me know if you have any ideas.

This revision is now accepted and ready to land.Jul 2 2018, 9:36 AM
vsk accepted this revision.Jul 2 2018, 9:45 AM

Thanks, LGTM, just one inline question.

lib/IR/DebugInfoMetadata.cpp
808

Is there a reason to allow callers to convert an expression into a stack value without adding any opcodes? Why not assert this can't happen?

bjope added inline comments.Jul 2 2018, 9:54 AM
lib/IR/DebugInfoMetadata.cpp
808

I was actually thinking about using an assert here. I did "git grep DIExpression::prepend" and found out that some users of DIExpression::prepend/DIExpression::prependOpcodes explicitly avoids calling those function with a zero Offset (or empty Ops vector), but some don't (an example is SelectionDAG::salvageDebugInfo). So I did not dare to add an assert.

vsk added inline comments.Jul 2 2018, 10:09 AM
lib/IR/DebugInfoMetadata.cpp
808

Thanks for pointing out that example. We can revisit tightening assertions in DIExpression::prepend* and see if that helps make it clearer whether or not some call turns an expression into a stack value.

This revision was automatically updated to reflect the committed changes.