Page MenuHomePhabricator

[IRBuilder][DebugInfo] Don't pick DebugLocs for new instructions from debug intrinsics
AbandonedPublic

Authored by jmorse on Apr 26 2019, 9:46 AM.

Details

Summary

As discussed in D59272, LLVM sometimes uses the DebugLoc of a debug intrinsic as the DebugLoc for newly created instructions. This is undesirable, as the line number information for debug intrinsics is at best meaningless and worst misleading -- D59272 has a couple of examples of variable declaration line numbers gratuitously appearing in unrelated code.

This patch has the IRBuilder insertion-point-setting methods skip over debug intrinsics, and find a "Real" instruction to set the DebugLoc from. As this isn't related to a particular pass or transformation, I've added a unit test to check that this behaviour is preserved.

Advice on additional reviewers would be appreciated, IRBuilder seems pretty central, it's not clear who else should review, aside from "Everyone".

Diff Detail

Event Timeline

jmorse created this revision.Apr 26 2019, 9:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2019, 9:46 AM
jmorse updated this revision to Diff 196875.Apr 26 2019, 9:57 AM

Add the test change I forgot to stage (whoops) -- the AMDGPU test here doesn't have any non-meta instructions with line numbers. Prior to this patch the debug intrinsic line numbers leak in, which the test depends on. Fix this by giving the real instructions line numbers. (The test is for placement of dbg.values rather than line numbers anyway).

bjope added inline comments.Apr 26 2019, 10:27 AM
include/llvm/IR/IRBuilder.h
139

This could end up returning the end() iterator (so the assert above is now out-of-play).

150

You probably want to do this before checking for end() (to avoid reading debug loc from end()).

Another interesting thing here is that when setting the insertion point to the end of the BB (or rather after the last non-debug-intrinsic in the BB) the current debug location won't be updated. Is some user of this method relying on that. Should perhaps the current debug location be invalidated instead?

My thoughts are:

  • Should we simply skip setting debug location when insertion point is a debug intrinsic?
  • Or should we invalidate the debug location when insertion point is a debug intrinsic?
  • Should perhaps these SetInsertPoint methods take a second argument, providing the debug location to set (only ~800 call sites to update...)?
  • Or should we never do the side effect of updating debug location (just as many call sites to potentially update with explicit settings)? Notice that the SetInsertPoint(BasicBlock *TheBB) version does not set the current debug location. So at the moment it isn't obvious that SetInsertPoint always updates the current debug location.

Haven't really thought about what I'd prefer myself.

vsk added a comment.EditedApr 26 2019, 11:03 AM

There's a tension here between finding a narrow solution to the invalid-location bug and having SetInsertPoint() do something principled with debug locations. See D39982 for a more detailed explanation of this. That previous attempt to change the IRBuilder API stalled because we couldn't find an automated way to fix up in-tree/out-of-tree uses (imho this is a hard problem).

I think it'd make sense to opt for a narrow solution here, but am not sure building on top of SetInsertPoint() is the way to go. What do folks think about this (possibly naive) alternative: if locations on debug intrinsics aren't meaningful, can we get rid of them (or use line 0 locations which just preserve scope info)? This would prevent SetInsertPoint from propagating misleading debug locations.

Edit: I think this alternative amounts to a version of D59272 that doesn't special-case stores.

jmorse abandoned this revision.May 8 2019, 6:51 AM

For some reason I hadn't clocked that the IRBuilder deals with blocks in a state of partial construction/correctness (duh), and had assumed there would always be a terminator in each block, whoops.

In D61198#1480767, @vsk wrote:

There's a tension here between finding a narrow solution to the invalid-location bug and having SetInsertPoint() do something principled with debug locations. See D39982 for a more detailed explanation of this.

Ouch, I have now been enlightened,

I think it'd make sense to opt for a narrow solution here, but am not sure building on top of SetInsertPoint() is the way to go. What do folks think about this (possibly naive) alternative: if locations on debug intrinsics aren't meaningful, can we get rid of them (or use line 0 locations which just preserve scope info)? This would prevent SetInsertPoint from propagating misleading debug locations.

That sounds like a plan -- this patch was supposed to just reduce the circumstances where D59272 was necessary, but as it seems to be a harder problem to solve than expected, we'll just have to rely on zero-line-nos-for-debug-intrinsics more. I've updated D59272 accordingly.