Page MenuHomePhabricator

Proposed fix for PR23076 (conditional branch debug line info)
Needs ReviewPublic

Authored by wolfgangp on Apr 3 2015, 11:51 AM.

Details

Reviewers
dblaikie
Summary

When generating the IR for branch-on-boolean the branch instruction gets a debug location that is inherited from the condition’s context. It may get the same location as the instructions that precede the condition. This can lead to poor stepping behavior when the condition spans multiple lines, e.g.

If (a

&&
b)


;

In gdb we step back to the if from b’s line before continuing on.

The patch proposes to attach the condition’s debug location to the branch instruction.

Diff Detail

Event Timeline

wolfgangp updated this revision to Diff 23226.Apr 3 2015, 11:51 AM
wolfgangp retitled this revision from to Proposed fix for PR23076 (conditional branch debug line info).
wolfgangp updated this object.
wolfgangp edited the test plan for this revision. (Show Details)
wolfgangp added a reviewer: dblaikie.
wolfgangp added a subscriber: Unknown Object (MLST).
dblaikie edited edge metadata.Apr 10 2015, 9:20 AM

Neither of the test case updates seem to relate to the example given in the bug/patch description - perhaps you need a test case for that?

Also, the test case updates seem to be incorrect - those branches are for the branch to/over the second operand to the && and || and should be attributed to the && and || (lines 2001 and 2101) not to the same line as the first expression, I think?

wolfgangp updated this revision to Diff 23807.Apr 15 2015, 3:59 PM
wolfgangp edited edge metadata.

Sorry for the delay. David's feedback made me realize that by associating ALL conditional branches with the same line number as the condition my previous proposal would have degraded the stepping experience for conditional expressions. We would no longer step to the line with the ‘&&’ in the above example after evaluating ‘a’.

Instead, we associate only the conditional expression’s final branch with the condition’s end location to avoid stepping back to the ‘if’.

Hi David, did you have a chance to look at the new patch? There are a couple of new tests piggy-backing on the existing test case.

Apologies for the massively delayed review. I think this is probably good,
but here are some test cases to discuss (& I've cc'd Paul Robinson and
Adrian Prantl, both who work on debug info in LLVM as well, to get their
thoughts)

Given this code:

    1. int main() {
    2. if (
    3. tr()
    4. &&
    5. tr()
    6. )
    7. func(); 8.
    8. if (
  1. fa()
  2. &&
  3. tr()
  4. )
  5. func();
  6. }

G++-4.8:
3, 4, 5, 4, 2, 7
10, 11, 9, 15

Clang before the change:
3, 4, 5, 3, 7
10, 11, 15

Clang after the change:
3, 5, 7
10, 15

The lack of stepping between the evaluation of the LHS and the RHS to the
&& expression seems like a loss, but not a great one. How's everyone else
feel about it? (I imagine we can, with some work, preserve that step while
avoiding the weird backwards step to the start of the expression before
continuing)

Reposting dblaikie's example hoping that phabricator doesn't mangle it this time:

 1: int main() {
 2: if (
 3: tr()
 4: &&
 5: tr()
 6: )
 7: func();
 8: if (
 9: fa()
10: &&
11: tr()
12: )
13: func();
14: }
15:

G++-4.8:
3, 4, 5, 4, 2, 7
10, 11, 9, 15

Clang before the change:
3, 4, 5, 3, 7
10, 11, 15

Clang after the change:
3, 5, 7
10, 15

The linetable produced by GCC appears to be the most accurate one (stopping at the && for the short-circuit evaluation and then returning there for the actual & and the branch). I would not have a problem with clang implementing it.
The downside of it is that it breaks the flow of stepping when the cursor goes back up one line even if that is what is actually happening.

I don't think that getting rid of the short-circuit breakpoint would be a very big loss, and not returning to the operator after returning from the RHS also shouldn't break any useful workflow.

What happens if && is an overloaded C++ operator — would still be a breakpoint before the call or would it also disappear?

Fair question on overloaded operators... Taking the original code:

 1: int main() {
 2: if (
 3: tr()
 4: &&
 5: tr()
 6: )
 7: func();
 8: if (
 9: fa()
10: &&
11: tr()
12: )
13: func();
14: }

& making tr/fa return a user defined type with a boolean member and provide op&& and op|| overloads that test the bool member in the obvious way.

Clang (with patch)
3, 5, 4, 5, 7
9, 11, 10, 11, 14

So that's somewhat problematic - we shouldn't visit 11 at all. (but we are today, as is GCC... so maybe NBD?)

GCC 4.8:
4, 2, 7
10, 8, 14

Clang ToT:
3, 5, 4, 3, 7
9, 11, 10, 9, 14

I think using the end of the condition is problematic/confusing. I'm not sure why this doesn't show up in the primitive value version, but it seems like it should (& we should end up stepping to the end of the condition (which would be the close paren of the function call, not the close paren of the 'if ()'))

Perhaps we should use the close paren of the 'if ()' but tehre's no source location for that readily available - I guess the way to get there is to navigate to the next token from the end of the condition expression... ?

Clang (with patch)
3, 5, 4, 5, 7
9, 11, 10, 11, 14

So that's somewhat problematic - we shouldn't visit 11 at all. (but we are today, as is GCC... so maybe NBD?)

Well, if op&& is overloaded, wouldn't we lose the short-circuit property? If so it makes sense to visit 11.

I think using the end of the condition is problematic/confusing. I'm not sure why this doesn't show up in the primitive value version, but it seems like it should (& we should end up stepping to the end of the condition (which would be the close paren of the function call, not the close paren of the 'if ()'))

There is short-circuit in the primitive value version, so we wouldn't stop there.

Perhaps we should use the close paren of the 'if ()' but tehre's no source location for that readily available - I guess the way to get there is to navigate to the next token from the end of the condition expression... ?

I agree, The close paren of the if() would be better.

Richard Smith: Is there a reasonable way to get the ')' of an "if ()"
expression in the AST? Is it just a matter of going down and playing games
with source locations/lexing to get the token after the end location of the
condition expression?