Inlining corrupts debug locations if call-site's debug location is unknown

[Quuxplusone]

Bugzilla Link	PR20907
Status	RESOLVED INVALID
Importance	P normal
Reported by	Michael Woerister (michaelwoerister@posteo.net)
Reported on	2014-09-11 10:27:56 -0700
Last modified on	2014-09-15 15:27:41 -0700
Version	trunk
Hardware	All All
CC	dblaikie@gmail.com, llvm-bugs@lists.llvm.org, paul_robinson@playstation.sony.com, rnk@google.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

In the Rust compiler we have consistently run into an assertion in
LexicalScopes::getOrCreateRegularScope() at:

assert(DISubprogram(Scope).describes(MF->getFunction()));

This would only occur when function inlining was activated. After debugging
into this, it turns out the inlining pass leaves the debug source location info
in a state the is not properly handled by the LexicalScopes class:

The fixupLineNumbers() function in lib/Transforms/Utils/InlineFunction.cpp will
not modify debug locations *at all* if the source location of the call-site is
unknown. Because of this the instructions copied from the callee into the
caller are still associated to the callee source location, but without having
the additional InlinedAt data. The LexicalScopes class will then not know that
this is actually valid code it is dealing with.

As a quick fix, I modified the fixupLineNumbers() function to clear all debug
location information from inlined instructions if the call-site has no debug
location info attached. This solved the problem but is a bit of a crude
solution. I'm sure you people can come up with something better!

[Quuxplusone]

IIRC David is the one adding assertions in this area.

[Quuxplusone]

Are there cases where this is not a bug in IR generation?

I added this assertion and found nothing but bugs where location information was missing from calls where it should've been present - it was great for fixing bugs and thus improving debug source fidelity.

I forget how this works if you have a nodebug function in between two debug functions and you inline the debug function into the nodebug function, then inline that into the outer debug function - that could/should still trigger the assertion, but I think there's something in place to address that.

[Quuxplusone]

Thanks for the quick response!

One case where this occurs in the Rust compiler is for destructors. The compiler generates "glue" function (which has no source locations attached) which in turn calls the actual destructor function (which has source location). When the actual destructor function gets inlined into the glue function, we trigger the assertion.

I just saw that the assertion is gone in a newer LLVM revision. The LLVM version we are using is from August 4, so the problem won't occur after we update.

Still, it would be great to know that it is actually supported to create inlineable calls from functions without debuginfo to functions with debuginfo, which seems to be a valid case from my point of view.

[Quuxplusone]

(In reply to comment #3)
> Thanks for the quick response!
>
> One case where this occurs in the Rust compiler is for destructors. The
> compiler generates "glue" function (which has no source locations attached)
> which in turn calls the actual destructor function (which has source
> location). When the actual destructor function gets inlined into the glue
> function, we trigger the assertion.

But the glue function doesn't have debug info, right? So why does it trigger
the assertion (without debug info on the glue function, we should never try to
build its scopes, etc)

> I just saw that the assertion is gone in a newer LLVM revision. The LLVM
> version we are using is from August 4, so the problem won't occur after we
> update.

I believe it was reverted here: http://llvm.org/viewvc/llvm-
project?rev=214999&view=rev due to issues on Windows -gmlt build which I
haven't been able to reproduce.

Once I pester Reid enough to get a reproduction, I'll try to address that and
recommit the change. So any solid examples we should handle I would love to see
so I can keep them in mind/include tests for them.

> Still, it would be great to know that it is actually supported to create
> inlineable calls from functions without debuginfo to functions with
> debuginfo, which seems to be a valid case from my point of view.

The kicker is whether that function without debug info can be inlined into a
function with debug info where the call site has no debug info.

[Quuxplusone]

> But the glue function doesn't have debug info, right? So why does it trigger
> the assertion (without debug info on the glue function, we should never try
> to build its scopes, etc)

This probably occurs because the call to the destructor glue function does not
have a source location attached.

> I believe it was reverted here:
> http://llvm.org/viewvc/llvm-project?rev=214999&view=rev due to issues on
> Windows -gmlt build which I haven't been able to reproduce.

Yes that looks right, two days after the revision we are using.

> > Still, it would be great to know that it is actually supported to create
> > inlineable calls from functions without debuginfo to functions with
> > debuginfo, which seems to be a valid case from my point of view.
>
> The kicker is whether that function without debug info can be inlined into a
> function with debug info where the call site has no debug info.

Yes, that's true. Our "glue" functions are not really the culprit but that the
calls them are generated differently than normal functions calls (as mentioned
above). We can treat that as a IR generation issue on our side.

However, it would be nice to at least provide an earlier assertion in the
inlining pass that makes sure that this kind of thing doesn't occur. It was
rather time-consuming to find out what the actual problem was. Just some kind
of hint that call-sites must have debug info if their call is inlined and the
inlined instructions *do* have debuginfo.

[Quuxplusone]

(In reply to comment #5)
> > But the glue function doesn't have debug info, right? So why does it trigger
> > the assertion (without debug info on the glue function, we should never try
> > to build its scopes, etc)
>
> This probably occurs because the call to the destructor glue function does
> not have a source location attached.
>
> > I believe it was reverted here:
> > http://llvm.org/viewvc/llvm-project?rev=214999&view=rev due to issues on
> > Windows -gmlt build which I haven't been able to reproduce.
>
> Yes that looks right, two days after the revision we are using.
>
> > > Still, it would be great to know that it is actually supported to create
> > > inlineable calls from functions without debuginfo to functions with
> > > debuginfo, which seems to be a valid case from my point of view.
> >
> > The kicker is whether that function without debug info can be inlined into a
> > function with debug info where the call site has no debug info.
>
> Yes, that's true. Our "glue" functions are not really the culprit but that
> the calls them are generated differently than normal functions calls (as
> mentioned above). We can treat that as a IR generation issue on our side.

That would be ideal - for you and for me. Once you add a debug location to the
call site then you won't have to strip all the debug info out of the debug
function that's called from the glue - and if the glue is inlined (along with
the debug function already inlined into the glue) you should get an inlined
scope for the inner debug function in the outer debug function - the glue will
just be un-attributed instructions in the outer function.

> However, it would be nice to at least provide an earlier assertion in the
> inlining pass that makes sure that this kind of thing doesn't occur. It was
> rather time-consuming to find out what the actual problem was. Just some
> kind of hint that call-sites must have debug info if their call is inlined
> and the inlined instructions *do* have debuginfo.

Agreed - I debugged several of these issue & they were painful to varying
degrees.

You'll see that in the patch that was reverted, there's a change to the debug
info verifier. This can be enabled (I forget how, exactly) in LLVM and run
between each optimization pass - so it should diagnose the pass that violates
the desired invariant (that any instruction in a debug-having function, has a
scope chain that leads back to that function, not any other functioN)

[Quuxplusone]

(In reply to comment #6)
> That would be ideal - for you and for me. Once you add a debug location to
> the call site then you won't have to strip all the debug info out of the
> debug function that's called from the glue - and if the glue is inlined
> (along with the debug function already inlined into the glue) you should get
> an inlined scope for the inner debug function in the outer debug function -
> the glue will just be un-attributed instructions in the outer function.

OK, we'll do that then.

> > However, it would be nice to at least provide an earlier assertion in the
> > inlining pass that makes sure that this kind of thing doesn't occur. It was
> > rather time-consuming to find out what the actual problem was. Just some
> > kind of hint that call-sites must have debug info if their call is inlined
> > and the inlined instructions *do* have debuginfo.
>
> Agreed - I debugged several of these issue & they were painful to varying
> degrees.
>
> You'll see that in the patch that was reverted, there's a change to the
> debug info verifier. This can be enabled (I forget how, exactly) in LLVM and
> run between each optimization pass - so it should diagnose the pass that
> violates the desired invariant (that any instruction in a debug-having
> function, has a scope chain that leads back to that function, not any other
> functioN)

That sounds incredibly useful :)

[Quuxplusone]

(In reply to comment #7)
> (In reply to comment #6)
> > That would be ideal - for you and for me. Once you add a debug location to
> > the call site then you won't have to strip all the debug info out of the
> > debug function that's called from the glue - and if the glue is inlined
> > (along with the debug function already inlined into the glue) you should get
> > an inlined scope for the inner debug function in the outer debug function -
> > the glue will just be un-attributed instructions in the outer function.
>
> OK, we'll do that then.

Great!

> > > However, it would be nice to at least provide an earlier assertion in the
> > > inlining pass that makes sure that this kind of thing doesn't occur. It
was
> > > rather time-consuming to find out what the actual problem was. Just some
> > > kind of hint that call-sites must have debug info if their call is inlined
> > > and the inlined instructions *do* have debuginfo.
> >
> > Agreed - I debugged several of these issue & they were painful to varying
> > degrees.
> >
> > You'll see that in the patch that was reverted, there's a change to the
> > debug info verifier. This can be enabled (I forget how, exactly) in LLVM and
> > run between each optimization pass - so it should diagnose the pass that
> > violates the desired invariant (that any instruction in a debug-having
> > function, has a scope chain that leads back to that function, not any other
> > functioN)
>
> That sounds incredibly useful :)

Yep - it should be accessible, I think, via "-mllvm -verify-debug-info" (you
could try locally unreverting my patch and using those flags to see if they
diagnose the problem you have)