This is an archive of the discontinued LLVM Phabricator instance.

[Debug] Annotate compiler generated range-for loop variables.
ClosedPublic

Authored by mattd on Feb 1 2018, 1:14 PM.

Details

Summary

This change aims to simplify debugging by annotating the range-for loop artificial variables (range, begin, end) with the scope depth.

Diff Detail

Event Timeline

mattd created this revision.Feb 1 2018, 1:14 PM
mattd edited the summary of this revision. (Show Details)
mattd updated this revision to Diff 132464.Feb 1 2018, 1:21 PM

Updating the diff, missed a few deltas that should have been in the original patch.

Seems plausible - maybe walking the scopes to count the range-for loops would produce a better number, but would be slower. :/ Dunno.

lib/Sema/SemaStmt.cpp
2346 ↗(On Diff #132464)

Why shifted right/divided by two? (I'd probably write this as "/ 2" rather thane ">> 1" if this is what's)

I guess this is because a range-for introduces two (actually 3, I think) scopes?

But what about if there are other random scopes that could be present? Does that adversely affect anything here?

mattd added inline comments.Feb 6 2018, 9:20 AM
lib/Sema/SemaStmt.cpp
2346 ↗(On Diff #132464)

Thanks for the comment. Honestly, I discovered the pattern after crafting up a tightly nested loop example, and realized that my Depths were 2,4,6... for each range loop respectively. I think it's two scopes and the range variables (begin, end, __range) are nestled inside each inner scope. Thus each range loop seems to be created of 2 scopes.

If there are other scopes between two range-loops, such as the 'if' in the example below, then the outer-most "for" is range1/begin1/end1, followed by the middle loop: range3/begin3/end3, and the innermost: range4/begin4/end4.

for (...) // range1
  if (...) 
    for(...) // range 3
      for (...) //range4
mattd updated this revision to Diff 134263.Feb 14 2018, 10:34 AM
  • Added a division by 2 instead of the shift, it reads clearer.
  • Updated the associated comment, reflecting that we divide by two because the variables we are annotating are within the inner scope of the ranged-based for loop.
dblaikie accepted this revision.Feb 14 2018, 10:58 AM

Seems good, thanks :)

test/CodeGenCXX/debug-for-range-scope-hints.cpp
1 ↗(On Diff #134263)

maybe this test should be called debug-info-range-for-var-names? Just a thought. (debug-info seems to be the usual prefix here, and range-for is a common shortening of "range-based-for" I think (maybe I'm wrong & other tests don't use that naming?)? & 'hints' seems a bit vague as to what the test is about - when it's specifically about naming the variables)

28–36 ↗(On Diff #134263)

I'd drop the ", {{.*}}, flags: DIFlagArtificial)" from these lines - since the name's the only bit that's really interesting to this test.

This revision is now accepted and ready to land.Feb 14 2018, 10:58 AM
mattd updated this revision to Diff 134272.Feb 14 2018, 11:25 AM

Thanks @dblaikie! I renamed the test, and cleaned up per your suggestion. I originally regex'd the debug-info lines so that the test would verify that the names were artificial; however, being that we already match them as metadata earlier, it's not really that necessary; we are only testing name strings anyways. Thanks again for the suggestion.

Thanks @dblaikie! I renamed the test, and cleaned up per your suggestion. I originally regex'd the debug-info lines so that the test would verify that the names were artificial; however, being that we already match them as metadata earlier, it's not really that necessary; we are only testing name strings anyways. Thanks again for the suggestion.

Great - can you commit this yourself or would you like me to do it for you?

Great - can you commit this yourself or would you like me to do it for you?

I've got it. Thanks!

This revision was automatically updated to reflect the committed changes.