Page MenuHomePhabricator

[Debuginfo][Instcombiner] Do not clone dbg.declare in TryToSinkInstruction()
ClosedPublic

Authored by avl on Thu, Sep 5, 2:32 AM.

Details

Summary

TryToSinkInstruction() has a bug: While updating debug info for sunk instruction, it could clone dbg.declare intrinsic. That is wrong. There could be only one dbg.declare. The fix is to not clone dbg.declare intrinsic and to update it`s arguments, to not to point to sunk instruction.

before instcombine :

entry:

%result = alloca i64, align 8
%tmpcast = bitcast i64* %result to %struct.S1*
%0 = bitcast i64* %result to i8*, !dbg !24
call void @llvm.dbg.declare(metadata %struct.S1* %tmpcast, metadata !12, metadata !DIExpression()), !dbg !24

%p1 = bitcast i64* %result to i32*, !dbg !35
store i32 2, i32* %p1, align 8, !dbg !35

after instcombine:

%result = alloca i64, align 8
%0 = bitcast i64* %result to i8*, !dbg !24
call void @llvm.dbg.declare(metadata i64* %result, metadata !12, metadata !DIExpression()), !dbg !24

%tmpcast = bitcast i64* %result to %struct.S1*
call void @llvm.dbg.declare(metadata %struct.S1* %tmpcast, metadata !12, metadata !DIExpression()), !dbg !24
^^^^^^^^^^^^^^^^^^^^^^^^^^^
cloned llvm.dbg.declare

Diff Detail

Repository
rL LLVM

Event Timeline

avl created this revision.Thu, Sep 5, 2:32 AM
jmorse added inline comments.Thu, Sep 5, 7:57 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3169 ↗(On Diff #218867)

"should" rather than "could"? Also, IMO "be left at original" should be "be left in the original"

3173–3175 ↗(On Diff #218867)

As I understand it, this works right now because the only sinkable instructions that dbg.declares point at are casts of other things (like arguments). This is probably fine -- but if dbg.declares operand rules ever change or someone writes IR that doesn't match that expectation, getOperand/setOperand could fail and it won't be clear why.

IMO it'd be good to add something like

if (!isa<Cast>(I))
  continue; // dbg.declare points at something it shouldn't

so that the failure mode is a debug use-before-def (which I don't believe is an error) rather than a crash.

avl updated this revision to Diff 218939.Thu, Sep 5, 9:37 AM

addressed comments.

aprantl accepted this revision.Mon, Sep 9, 2:03 PM

This is a strict improvement and as such is fine, but there's some general cleanups that are uncovered by this that we should also be doing.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3173–3175 ↗(On Diff #218867)

We could probably simplify the model by enforcing that dbg.declares always point at allocas or function arguments directly, That would make code like this one look less murky.

3169 ↗(On Diff #218939)

How about:

A dbg.declare instruction should not be cloned, since there can only be one per variable fragment.
3170 ↗(On Diff #218939)

space before (

3174 ↗(On Diff #218939)

This should really be a Verifier error.

This revision is now accepted and ready to land.Mon, Sep 9, 2:03 PM
aprantl added inline comments.Mon, Sep 9, 2:04 PM
llvm/test/Transforms/InstCombine/do-not-clone-dbg-declare.ll
149 ↗(On Diff #218939)

You could probably use this as a !dbg atachement in most instructions to simplify the testcase.

This revision was automatically updated to reflect the committed changes.
avl marked an inline comment as done.Wed, Sep 11, 6:59 AM
avl added inline comments.
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3173–3175 ↗(On Diff #218867)

@aprantl how should this be done in a procedural manner? I mean - Could I create a patch for verifier, which would check "that dbg.declares always point at allocas or function arguments directly" and then fix all places ? Or that question should be previously discussed somehow?

aprantl added inline comments.Wed, Sep 11, 8:46 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3173–3175 ↗(On Diff #218867)

It may be best to proceed even slower than that. Changing what we accept inside of dbg.declare potentially means that all sorts of producers of LLVM need to be updated, so we need to be mindful of that. I think it would be reasonable to only let dbg.declare point inside an alloca or inside a (stack-allocated) function argument/sret value. But it might be too strict to demand that there is a 1-1 correspondence between allocas and dbg.declares, I think it might be reasonable to also allow bitcast and getelementptr operations to support clients like ASAN — as long as it points into a stack slot.

So I'd start by writing a patch that

  • updates LangRef.rst / SourceLevelDebugging.rst
  • verifies that the argument of a dbg.declare fullfills these rules
  • see if we have any failures (running the llvm testsuite, compiling clang at -O0, -O3, -O1+asan):
  • - if they are obvious mistakes, fix them
  • - otherwise come back here to discuss what we found
  • from there we can gradually tighten the requirements.
avl marked an inline comment as done.Wed, Sep 11, 10:43 AM
avl added inline comments.
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3173–3175 ↗(On Diff #218867)

Ok, I would follow that plan. Thank you.