Page MenuHomePhabricator

[CGBlocks] Improve line info in backtraces containing *_helper_block

Authored by vsk on Oct 25 2017, 5:11 PM.



Instead of only setting a non-zero debug location on the return
instruction in *_helper_block functions, set a proper location on all
instructions within these functions. Pick the start location of the
block literal expr for maximum clarity.

The debugger does not step into *_helper_block functions during normal
single-stepping because we mark their parameters as artificial. This is
what we want (the functions are implicitly generated and uninteresting
to most users). The stepping behavior is unchanged by this patch.


Diff Detail


Event Timeline

vsk created this revision.Oct 25 2017, 5:11 PM
vsk updated this revision to Diff 120342.Oct 25 2017, 5:23 PM
  • Simplify the checks by not hardcoding a line number.
JDevlieghere added inline comments.Oct 25 2017, 6:04 PM
1651 ↗(On Diff #120342)

You probably have a good reason for doing so, but why do we need {}-initialization?

vsk added inline comments.Oct 25 2017, 6:21 PM
1651 ↗(On Diff #120342)

It's just a personal idiosyncrasy. I like braced-init because it doesn't have a vexing parse issue, it works with list/aggregate init, and it makes it clear that operator= isn't being called.

aprantl edited edge metadata.Oct 26 2017, 9:03 AM

Thanks! This looks generally good, I have one pending question inline.

13 ↗(On Diff #120342)

We nowadays usually try to avoid rdar links in anything but commit messages because it is not helpful information for the LLVM community. It's better to just describe the issue at hand (which you are doing!) or create a PR with that contents and link to it.

20 ↗(On Diff #120342)

What's the location used for the ret? I think it should also be` ![[DBG_LINE]]` since we are not actually executing the block.

JDevlieghere added inline comments.Oct 26 2017, 9:13 AM
1651 ↗(On Diff #120342)

Fair enough. Thanks for clarifying!

vsk added inline comments.Oct 26 2017, 10:55 AM
13 ↗(On Diff #120342)

Got it.

20 ↗(On Diff #120342)

We're using COPY_LINE, which is the same location used for the load instruction below.

What's the semantic difference between DBG_LINE (line 0) and COPY_LINE (line 68) anyway? Why do we have two different locations for the arguments to this function?

aprantl added inline comments.Oct 26 2017, 11:14 AM
20 ↗(On Diff #120342)

The debugger will skip over line 0 locations when single-stepping or when setting breakpoints. I can't tell without reading the code why we decide to put a line 0 on the call.

20 ↗(On Diff #120342)

The important thing is that the testcase should check that the ret has either COPY_LINE or line 0 on it and not line 71.

vsk updated this revision to Diff 120461.Oct 26 2017, 11:42 AM
vsk marked 8 inline comments as done.
  • Tighten test case to show the dbg loc on return instructions.
20 ↗(On Diff #120342)

I'll fix up the test case.

It looks like the zero location is an artifact of CodeGenFunction::StartFunction:

1128   // Emit a location at the end of the prologue.                                                                                                                                                            
1129   if (CGDebugInfo *DI = getDebugInfo())                                                                                                                                                                     
1130     DI->EmitLocation(Builder, StartLoc);

I think it's unnecessary, but maybe we can look into that separately?

aprantl accepted this revision.Oct 26 2017, 1:12 PM
aprantl added inline comments.
20 ↗(On Diff #120342)

... and StartLoc is empty for this function? (which would make sense)
I think I'm fine with leaving this as is unless you feel strongly about it.

This revision is now accepted and ready to land.Oct 26 2017, 1:12 PM
vsk added a comment.Oct 26 2017, 2:09 PM


20 ↗(On Diff #120342)

Yes, StartLoc is empty for the function, but I misunderstood the source of the zero location. I think it actually comes from CGDebugInfo::EmitFunctionStart, in the same logic that sets the artificial flag on parameters. It looks like this is intentional so I won't touch it. I'll go ahead and land this fix.

This revision was automatically updated to reflect the committed changes.