Differential D8822
Proposed fix for PR23076 (conditional branch debug line info) Authored by wolfgangp on Apr 3 2015, 11:51 AM.
Details
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 TimelineComment Actions 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? Comment Actions 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’. Comment Actions 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. Comment Actions Apologies for the massively delayed review. I think this is probably good, Given this code:
G++-4.8: Clang before the change: Clang after the change: The lack of stepping between the evaluation of the LHS and the RHS to the Comment Actions 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: Clang before the change: Clang after the change: Comment Actions 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. 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? Comment Actions 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) 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: Clang ToT: 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... ? Comment Actions
Well, if op&& is overloaded, wouldn't we lose the short-circuit property? If so it makes sense to visit 11.
There is short-circuit in the primitive value version, so we wouldn't stop there.
I agree, The close paren of the if() would be better. Comment Actions Richard Smith: Is there a reasonable way to get the ')' of an "if ()" |