Page MenuHomePhabricator

Fix line numbers for code inlined from __nodebug__ functions.

Authored by eugenis on Jun 2 2014, 4:01 AM.



This is a fix for PR19001.

Instructions from nodebug functions don't have file:line information even when inlined into no-nodebug functions.
As a result, intrinsics (SSE and other) from <*intrin.h> clang headers _never_ have file:line information.

With this change, an instruction without !dbg metadata gets one from the call instruction when inlined.

Diff Detail

Event Timeline

eugenis updated this revision to Diff 10010.Jun 2 2014, 4:01 AM
eugenis retitled this revision from to Fix line numbers for code inlined from __nodebug__ functions..
eugenis updated this object.
eugenis edited the test plan for this revision. (Show Details)
eugenis added a reviewer: chandlerc.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: Unknown Object (MLST).

What's the user experience today that this is fixing? Is this any different/worse than a whole (non-inlined) function having no line information?

Any idea what GCC does here?

dblaikie added inline comments.Jun 2 2014, 10:53 AM
2 ↗(On Diff #10010)

Do you have the original source for this test? I imagine these functions could be a little simpler, maybe? (do they need parameters/return values? It's unlikely the outer function (test2) does, I imagine - the inner function might be slightly simpler, while still having an un-elidable instruction, if it just assigned a constant to a global? Maybe there's some other simple operation it could perform)

chandlerc accepted this revision.Jun 2 2014, 11:16 AM
chandlerc edited edge metadata.

I'll defer to Dave for getting the actual test case into a form he's happy with, but I think this is the *exact* right change to behavior. always_inlin,nodebug functions should behave as-if their contents were written in the place they are called, and this gives us that behavior.

Among other things this should fix is stepping through a long sequence of _mm_...() calls.


Rather than mentioning *intrin.h functions, I would just say that this is important for ((always_inline, nodebug)) functions to at least have *some* location after inlining.

This revision is now accepted and ready to land.Jun 2 2014, 11:16 AM
eugenis updated this revision to Diff 10044.Jun 3 2014, 1:57 AM
eugenis edited edge metadata.

I've also included updates to tests in clang and compiler-rt that were earlier sent as a separate CL.

Btw, is there any reason not to commit CLs spanning multiple llvm projects as one? This seems to work perfectly well, but for some reason people often do multiple commits with inconsistent state in between.



2 ↗(On Diff #10010)

I've simplified the test and included the original source to simplify future modifications.

dblaikie added inline comments.Jun 3 2014, 2:54 PM
9 ↗(On Diff #10044)

This seems a strange way to test this - I would've expected just a single assignment in "callee" and the nodebug attribute on it?

Why two assignments and a manual modification to the metadata?


Does this need to be tested here (and for /every/ intrinsic)? Seems fair to just test this once. (I guess somewhere we test that the intrinsics have nodebug on them in some way already?)

eugenis added inline comments.Jun 4 2014, 1:34 AM
9 ↗(On Diff #10044)

Because this way I'm testing a function that has instructions both with and without debug metadata. This is not the exact case we are interested in, but a more general one.


Sounds reasonable, I'll add a separate test for this.

eugenis updated this revision to Diff 10085.Jun 4 2014, 1:57 AM


I've also moved the LLVM test under test/DebugInfo - that's where other inliner+!dbg tests are in.

dblaikie added inline comments.Jun 4 2014, 10:39 AM
9 ↗(On Diff #10044)

Why is that a scenario we need to test? When do we end up with a function both with and without debug metadata?

eugenis updated this revision to Diff 10126.Jun 5 2014, 1:51 AM

Updated the test to avoid manual IR editing.

dblaikie accepted this revision.Jun 5 2014, 8:42 AM
dblaikie added a reviewer: dblaikie.

Thanks - looks great. One optional comment.


It's probably worth having a similar check line for the value of A to demonstrate that they're both located at line 3, not line 2? (and if you like, you can omit everything after the field you care about (the first field, in this instance: "CHECK: [[X]] = metadata !{i32 42,") - but I don't feel strongly about it))

Thanks, committed as r210459.


I've left all metadata fields to verify that B is an inlined location, and A is not.

edward-san closed this revision.Jun 9 2014, 10:34 AM
edward-san added a subscriber: edward-san.

I suppose this can be closed safely, right?