[2/2] gcov: Fix incorrect gimple line LOCATION [PR97923]

  For case like belowi test.c:

1:int foo(char c)
2:{
3:  return ((c >= 'A' && c <= 'Z')
4:       || (c >= 'a' && c <= 'z')
5:       || (c >= '0' && c <='0'));}

the generated line number is incorrect for condition c>='A' of block 2:
Thus correct the condition op0 location.

gcno diff before and with this patch:

test.gcno:  575:                  block 11: 1:0001(tree)
test.gcno:  583:    01450000:  35:LINES
-test.gcno:  595:                  block 2:`test.c':1, 5
+test.gcno:  595:                  block 2:`test.c':1, 3
test.gcno:  626:    01450000:  31:LINES
test.gcno:  638:                  block 3:`test.c':3
test.gcno:  665:    01450000:  31:LINES
test.gcno:  677:                  block 4:`test.c':4
test.gcno:  704:    01450000:  31:LINES
test.gcno:  716:                  block 5:`test.c':4
test.gcno:  743:    01450000:  31:LINES
test.gcno:  755:                  block 6:`test.c':5

Also save line id in line vector for gcov debug use.

Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
master?

gcc/ChangeLog:

	PR gcov/97923
	* gcov.cc (line_info::line_info): Init id.
	(solve_flow_graph): Fix typo.
	(add_line_counts): Set line->id.
	* gimplify.cc (shortcut_cond_r): Correct cond expr op0 location.

gcc/testsuite/ChangeLog:

	PR gcov/97923
	* gcc.misc-tests/gcov-pr97923.c: New test.

Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
---
 gcc/gcov.cc                                 |  9 ++++++---
 gcc/gimplify.cc                             |  6 ++++--
 gcc/testsuite/gcc.misc-tests/gcov-pr97923.c | 13 +++++++++++++
 3 files changed, 23 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr97923.c
  

On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> For case like belowi test.c:
>
> 1:int foo(char c)
> 2:{
> 3:  return ((c >= 'A' && c <= 'Z')
> 4:       || (c >= 'a' && c <= 'z')
> 5:       || (c >= '0' && c <='0'));}
>
> the generated line number is incorrect for condition c>='A' of block 2:
> Thus correct the condition op0 location.
>
> gcno diff before and with this patch:
>
> test.gcno:  575:                  block 11: 1:0001(tree)
> test.gcno:  583:    01450000:  35:LINES
> -test.gcno:  595:                  block 2:`test.c':1, 5
> +test.gcno:  595:                  block 2:`test.c':1, 3
> test.gcno:  626:    01450000:  31:LINES
> test.gcno:  638:                  block 3:`test.c':3
> test.gcno:  665:    01450000:  31:LINES
> test.gcno:  677:                  block 4:`test.c':4
> test.gcno:  704:    01450000:  31:LINES
> test.gcno:  716:                  block 5:`test.c':4
> test.gcno:  743:    01450000:  31:LINES
> test.gcno:  755:                  block 6:`test.c':5
>
> Also save line id in line vector for gcov debug use.
>
> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
> master?
>
> gcc/ChangeLog:
>
>         PR gcov/97923
>         * gcov.cc (line_info::line_info): Init id.
>         (solve_flow_graph): Fix typo.
>         (add_line_counts): Set line->id.
>         * gimplify.cc (shortcut_cond_r): Correct cond expr op0 location.
>
> gcc/testsuite/ChangeLog:
>
>         PR gcov/97923
>         * gcc.misc-tests/gcov-pr97923.c: New test.
>
> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
> ---
>  gcc/gcov.cc                                 |  9 ++++++---
>  gcc/gimplify.cc                             |  6 ++++--
>  gcc/testsuite/gcc.misc-tests/gcov-pr97923.c | 13 +++++++++++++
>  3 files changed, 23 insertions(+), 5 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr97923.c
>
> diff --git a/gcc/gcov.cc b/gcc/gcov.cc
> index 2ec7248cc0e..77ca94c71c4 100644
> --- a/gcc/gcov.cc
> +++ b/gcc/gcov.cc
> @@ -205,6 +205,8 @@ public:
>    /* Execution count.  */
>    gcov_type count;
>
> +  unsigned id;
> +
>    /* Branches from blocks that end on this line.  */
>    vector<arc_info *> branches;
>
> @@ -216,8 +218,8 @@ public:
>    unsigned has_unexecuted_block : 1;
>  };
>
> -line_info::line_info (): count (0), branches (), blocks (), exists (false),
> -  unexceptional (0), has_unexecuted_block (0)
> +line_info::line_info (): count (0), id (0), branches (), blocks (),
> +  exists (false), unexceptional (0), has_unexecuted_block (0)
>  {
>  }
>
> @@ -2370,7 +2372,7 @@ solve_flow_graph (function_info *fn)
>
>    /* If the graph has been correctly solved, every block will have a
>       valid count.  */
> -  for (unsigned i = 0; ix < fn->blocks.size (); i++)
> +  for (unsigned i = 0; i < fn->blocks.size (); i++)
>      if (!fn->blocks[i].count_valid)
>        {
>         fnotice (stderr, "%s:graph is unsolvable for '%s'\n",
> @@ -2730,6 +2732,7 @@ add_line_counts (coverage_info *coverage, function_info *fn)
>                     }
>                   line->count += block->count;
>                 }
> +             line->id = ln;
>             }
>
>           has_any_line = true;
> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> index ade6e335da7..341a27b033e 100644
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -3915,7 +3915,8 @@ shortcut_cond_r (tree pred, tree *true_label_p, tree *false_label_p,
>         false_label_p = &local_label;
>
>        /* Keep the original source location on the first 'if'.  */
> -      t = shortcut_cond_r (TREE_OPERAND (pred, 0), NULL, false_label_p, locus);
> +      tree op0 = TREE_OPERAND (pred, 0);
> +      t = shortcut_cond_r (op0, NULL, false_label_p, EXPR_LOCATION (op0));
>        append_to_statement_list (t, &expr);

The comment now no longer is true?  For the else arm we use
rexpr_location, why not
here as well?  To quote the following lines:

      /* Set the source location of the && on the second 'if'.  */
      new_locus = rexpr_location (pred, locus);
      t = shortcut_cond_r (TREE_OPERAND (pred, 1), true_label_p, false_label_p,
                           new_locus);
      append_to_statement_list (t, &expr);

with your change the location of the outer COND_EXPR is lost?  Can we guarantee
that it's used for the first operand of a if (a && b && c)?  It would
be nice to expand
the leading comment for such a three operand case and explain how it's supposed
to work.

I didn't look at the gcov changes, leaving those to the gcov maintainer(s).

Richard.

>
>        /* Set the source location of the && on the second 'if'.  */
> @@ -3938,7 +3939,8 @@ shortcut_cond_r (tree pred, tree *true_label_p, tree *false_label_p,
>         true_label_p = &local_label;
>
>        /* Keep the original source location on the first 'if'.  */
> -      t = shortcut_cond_r (TREE_OPERAND (pred, 0), true_label_p, NULL, locus);
> +      tree op0 = TREE_OPERAND (pred, 0);
> +      t = shortcut_cond_r (op0, true_label_p, NULL, EXPR_LOCATION (op0));
>        append_to_statement_list (t, &expr);
>
>        /* Set the source location of the || on the second 'if'.  */
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr97923.c b/gcc/testsuite/gcc.misc-tests/gcov-pr97923.c
> new file mode 100644
> index 00000000000..ad4f7d40817
> --- /dev/null
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr97923.c
> @@ -0,0 +1,13 @@
> +/* { dg-options "-fprofile-arcs -ftest-coverage" } */
> +/* { dg-do run { target native } } */
> +
> +int foo(int c)
> +{
> +  return ((c >= 'A' && c <= 'Z') /* count(1*) */
> +      || (c >= 'a' && c <= 'z') /* count(1*) */
> +      || (c >= '0' && c <= '0')); /* count(1*) */
> +}
> +
> +int main() { foo(0); }
> +
> +/* { dg-final { run-gcov gcov-pr97923-1.c } } */
> --
> 2.27.0
>
  

On 2023/3/2 16:16, Richard Biener wrote:
> On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> For case like belowi test.c:
>>
>> 1:int foo(char c)
>> 2:{
>> 3:  return ((c >= 'A' && c <= 'Z')
>> 4:       || (c >= 'a' && c <= 'z')
>> 5:       || (c >= '0' && c <='0'));}
>>
>> the generated line number is incorrect for condition c>='A' of block 2:
>> Thus correct the condition op0 location.
>>
>> gcno diff before and with this patch:
>>
>> test.gcno:  575:                  block 11: 1:0001(tree)
>> test.gcno:  583:    01450000:  35:LINES
>> -test.gcno:  595:                  block 2:`test.c':1, 5
>> +test.gcno:  595:                  block 2:`test.c':1, 3
>> test.gcno:  626:    01450000:  31:LINES
>> test.gcno:  638:                  block 3:`test.c':3
>> test.gcno:  665:    01450000:  31:LINES
>> test.gcno:  677:                  block 4:`test.c':4
>> test.gcno:  704:    01450000:  31:LINES
>> test.gcno:  716:                  block 5:`test.c':4
>> test.gcno:  743:    01450000:  31:LINES
>> test.gcno:  755:                  block 6:`test.c':5
>>
>> Also save line id in line vector for gcov debug use.
>>
>> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
>> master?
>>
>> gcc/ChangeLog:
>>
>>          PR gcov/97923
>>          * gcov.cc (line_info::line_info): Init id.
>>          (solve_flow_graph): Fix typo.
>>          (add_line_counts): Set line->id.
>>          * gimplify.cc (shortcut_cond_r): Correct cond expr op0 location.
>>
>> gcc/testsuite/ChangeLog:
>>
>>          PR gcov/97923
>>          * gcc.misc-tests/gcov-pr97923.c: New test.
>>
>> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
>> ---
>>   gcc/gcov.cc                                 |  9 ++++++---
>>   gcc/gimplify.cc                             |  6 ++++--
>>   gcc/testsuite/gcc.misc-tests/gcov-pr97923.c | 13 +++++++++++++
>>   3 files changed, 23 insertions(+), 5 deletions(-)
>>   create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr97923.c
>>
>> diff --git a/gcc/gcov.cc b/gcc/gcov.cc
>> index 2ec7248cc0e..77ca94c71c4 100644
>> --- a/gcc/gcov.cc
>> +++ b/gcc/gcov.cc
>> @@ -205,6 +205,8 @@ public:
>>     /* Execution count.  */
>>     gcov_type count;
>>
>> +  unsigned id;
>> +
>>     /* Branches from blocks that end on this line.  */
>>     vector<arc_info *> branches;
>>
>> @@ -216,8 +218,8 @@ public:
>>     unsigned has_unexecuted_block : 1;
>>   };
>>
>> -line_info::line_info (): count (0), branches (), blocks (), exists (false),
>> -  unexceptional (0), has_unexecuted_block (0)
>> +line_info::line_info (): count (0), id (0), branches (), blocks (),
>> +  exists (false), unexceptional (0), has_unexecuted_block (0)
>>   {
>>   }
>>
>> @@ -2370,7 +2372,7 @@ solve_flow_graph (function_info *fn)
>>
>>     /* If the graph has been correctly solved, every block will have a
>>        valid count.  */
>> -  for (unsigned i = 0; ix < fn->blocks.size (); i++)
>> +  for (unsigned i = 0; i < fn->blocks.size (); i++)
>>       if (!fn->blocks[i].count_valid)
>>         {
>>          fnotice (stderr, "%s:graph is unsolvable for '%s'\n",
>> @@ -2730,6 +2732,7 @@ add_line_counts (coverage_info *coverage, function_info *fn)
>>                      }
>>                    line->count += block->count;
>>                  }
>> +             line->id = ln;
>>              }
>>
>>            has_any_line = true;
>> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
>> index ade6e335da7..341a27b033e 100644
>> --- a/gcc/gimplify.cc
>> +++ b/gcc/gimplify.cc
>> @@ -3915,7 +3915,8 @@ shortcut_cond_r (tree pred, tree *true_label_p, tree *false_label_p,
>>          false_label_p = &local_label;
>>
>>         /* Keep the original source location on the first 'if'.  */
>> -      t = shortcut_cond_r (TREE_OPERAND (pred, 0), NULL, false_label_p, locus);
>> +      tree op0 = TREE_OPERAND (pred, 0);
>> +      t = shortcut_cond_r (op0, NULL, false_label_p, EXPR_LOCATION (op0));
>>         append_to_statement_list (t, &expr);
> 
> The comment now no longer is true?  For the else arm we use
> rexpr_location, why not
> here as well?  To quote the following lines:
> 
>        /* Set the source location of the && on the second 'if'.  */
>        new_locus = rexpr_location (pred, locus);
>        t = shortcut_cond_r (TREE_OPERAND (pred, 1), true_label_p, false_label_p,
>                             new_locus);
>        append_to_statement_list (t, &expr);

Thanks, should use rexpr_location with each operand like below.


> 
> with your change the location of the outer COND_EXPR is lost?  Can we guarantee
> that it's used for the first operand of a if (a && b && c)?  It would
> be nice to expand
> the leading comment for such a three operand case and explain how it's supposed
> to work.

I tested the three operand case, it will iteratively call shortcut_cond_r and
also works as expected.  Seems the outer COND_EXPR is useless if we do the
followed conversion?


    if (TREE_CODE (pred) == TRUTH_ANDIF_EXPR)
      {
        location_t new_locus;

        /* Turn if (a && b) into

          if (a); else goto no;
          if (b) goto yes; else goto no;
          (no:) */

        if (false_label_p == NULL)
         false_label_p = &local_label;

-      /* Keep the original source location on the first 'if'.  */
-      tree op0 = TREE_OPERAND (pred, 0);
-      t = shortcut_cond_r (op0, NULL, false_label_p, EXPR_LOCATION (op0));
+      /* Set the original source location on the first 'if'.  */
+      tree op0 = TREE_OPERAND(pred, 0);
+      new_locus = rexpr_location (op0, locus);
+      t = shortcut_cond_r (op0, NULL, false_label_p, new_locus);      // <=
        append_to_statement_list (t, &expr);

        /* Set the source location of the && on the second 'if'.  */
-      new_locus = rexpr_location (pred, locus);
-      t = shortcut_cond_r (TREE_OPERAND (pred, 1), true_label_p, false_label_p,
-                          new_locus);
+      tree op1 = TREE_OPERAND(pred, 1);
+      new_locus = rexpr_location (op1, locus);
+      t = shortcut_cond_r (op1, true_label_p, false_label_p, new_locus);
        append_to_statement_list (t, &expr);
      }


Breakpoint 5, shortcut_cond_r (pred=0x7ffff6f78fa0, true_label_p=0x0, false_label_p=0x7fffffffbef0, locus=2147483654) at ../.
./tgcc-master/gcc/gimplify.cc:3918
(gdb) pel locus
{file = 0x3e3e940 "test.c", line = 5, column = 19, data = 0x0, sysp = false}
(gdb) n
(gdb)
(gdb) pel new_locus
{file = 0x3e3e940 "test.c", line = 4, column = 18, data = 0x0, sysp = false}
(gdb) ptree op0
  <truth_andif_expr 0x7ffff6f78f50
     type <boolean_type 0x7ffff6df2b28 _Bool public unsigned QI
         size <integer_cst 0x7ffff6dd3e88 constant 8>
         unit-size <integer_cst 0x7ffff6dd3ea0 constant 1>
         align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff6df2b28 precision:1 min <integer_cst 0x7ffff6
df60f0 0> max <integer_cst 0x7ffff6df6120 1>>
     arg:0 <gt_expr 0x7ffff6f78eb0 type <boolean_type 0x7ffff6df2b28 _Bool>
         arg:0 <parm_decl 0x7ffff7ff7080 c type <integer_type 0x7ffff6df23f0 char>
             used read QI test.c:1:15 size <integer_cst 0x7ffff6dd3e88 8> unit-size <integer_cst 0x7ffff6dd3ea0 1>
             align:8 warn_if_not_align:0 context <function_decl 0x7ffff6f7e800 test> arg-type <integer_type 0x7ffff6df25e8 int>>
         arg:1 <integer_cst 0x7ffff6f88a08 constant 64>
         test.c:4:11 start: test.c:4:9 finish: test.c:4:16>
     arg:1 <gt_expr 0x7ffff6f78ed8 type <boolean_type 0x7ffff6df2b28 _Bool>
         arg:0 <parm_decl 0x7ffff7ff7080 c>
         arg:1 <integer_cst 0x7ffff6f88a38 constant 76>
         test.c:5:12 start: test.c:5:10 finish: test.c:5:17>
     test.c:4:18 start: test.c:4:9 finish: test.c:5:17>


1int test(char c)
2{
3  return (
4        c >= 'A' &&
5         c >= 'M' &&
6          c <= 'Z');
7}


bbs:

   <bb 2> :
   if (c_2(D) > 64)
     goto <bb 3>; [INV]
   else
     goto <bb 6>; [INV]

   <bb 3> :
   if (c_2(D) > 76)
     goto <bb 4>; [INV]
   else
     goto <bb 6>; [INV]

   <bb 4> :
   if (c_2(D) <= 90)
     goto <bb 5>; [INV]
   else
     goto <bb 6>; [INV]


gcno diff before and with this patch:

-test.gcno: 1312:                  block 2:`test.c':1, 5
+test.gcno: 1312:                  block 2:`test.c':1, 4
  test.gcno: 1343:    01450000:  31:LINES
-test.gcno: 1355:                  block 3:`test.c':4
+test.gcno: 1355:                  block 3:`test.c':5
  test.gcno: 1382:    01450000:  31:LINES
-test.gcno: 1394:                  block 4:`test.c':5
+test.gcno: 1394:                  block 4:`test.c':6
  test.gcno: 1421:    01450000:  31:LINES
  test.gcno: 1433:                  block 5:`test.c':5
  test.gcno: 1460:    01450000:  31:LINES
  test.gcno: 1472:                  block 6:`test.c':5
  test.gcno: 1499:    01450000:  31:LINES
  test.gcno: 1511:                  block 7:`test.c':5
  test.gcno: 1538:    01450000:  31:LINES
  test.gcno: 1550:                  block 8:`test.c':5

<bb 2>, <bb 3> and <bb 4> are  mapped to line 4, 5, 6 correctly now...
  

On Thu, 2 Mar 2023, Xionghu Luo wrote:

> 
> 
> On 2023/3/2 16:16, Richard Biener wrote:
> > On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> For case like belowi test.c:
> >>
> >> 1:int foo(char c)
> >> 2:{
> >> 3:  return ((c >= 'A' && c <= 'Z')
> >> 4:       || (c >= 'a' && c <= 'z')
> >> 5:       || (c >= '0' && c <='0'));}
> >>
> >> the generated line number is incorrect for condition c>='A' of block 2:
> >> Thus correct the condition op0 location.
> >>
> >> gcno diff before and with this patch:
> >>
> >> test.gcno:  575:                  block 11: 1:0001(tree)
> >> test.gcno:  583:    01450000:  35:LINES
> >> -test.gcno:  595:                  block 2:`test.c':1, 5
> >> +test.gcno:  595:                  block 2:`test.c':1, 3
> >> test.gcno:  626:    01450000:  31:LINES
> >> test.gcno:  638:                  block 3:`test.c':3
> >> test.gcno:  665:    01450000:  31:LINES
> >> test.gcno:  677:                  block 4:`test.c':4
> >> test.gcno:  704:    01450000:  31:LINES
> >> test.gcno:  716:                  block 5:`test.c':4
> >> test.gcno:  743:    01450000:  31:LINES
> >> test.gcno:  755:                  block 6:`test.c':5
> >>
> >> Also save line id in line vector for gcov debug use.
> >>
> >> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
> >> master?
> >>
> >> gcc/ChangeLog:
> >>
> >>          PR gcov/97923
> >>          * gcov.cc (line_info::line_info): Init id.
> >>          (solve_flow_graph): Fix typo.
> >>          (add_line_counts): Set line->id.
> >>          * gimplify.cc (shortcut_cond_r): Correct cond expr op0 location.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >>          PR gcov/97923
> >>          * gcc.misc-tests/gcov-pr97923.c: New test.
> >>
> >> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
> >> ---
> >>   gcc/gcov.cc                                 |  9 ++++++---
> >>   gcc/gimplify.cc                             |  6 ++++--
> >>   gcc/testsuite/gcc.misc-tests/gcov-pr97923.c | 13 +++++++++++++
> >>   3 files changed, 23 insertions(+), 5 deletions(-)
> >>   create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr97923.c
> >>
> >> diff --git a/gcc/gcov.cc b/gcc/gcov.cc
> >> index 2ec7248cc0e..77ca94c71c4 100644
> >> --- a/gcc/gcov.cc
> >> +++ b/gcc/gcov.cc
> >> @@ -205,6 +205,8 @@ public:
> >>     /* Execution count.  */
> >>     gcov_type count;
> >>
> >> +  unsigned id;
> >> +
> >>     /* Branches from blocks that end on this line.  */
> >>     vector<arc_info *> branches;
> >>
> >> @@ -216,8 +218,8 @@ public:
> >>     unsigned has_unexecuted_block : 1;
> >>   };
> >>
> >> -line_info::line_info (): count (0), branches (), blocks (), exists
> >> (false),
> >> -  unexceptional (0), has_unexecuted_block (0)
> >> +line_info::line_info (): count (0), id (0), branches (), blocks (),
> >> +  exists (false), unexceptional (0), has_unexecuted_block (0)
> >>   {
> >>   }
> >>
> >> @@ -2370,7 +2372,7 @@ solve_flow_graph (function_info *fn)
> >>
> >>     /* If the graph has been correctly solved, every block will have a
> >>        valid count.  */
> >> -  for (unsigned i = 0; ix < fn->blocks.size (); i++)
> >> +  for (unsigned i = 0; i < fn->blocks.size (); i++)
> >>       if (!fn->blocks[i].count_valid)
> >>         {
> >>          fnotice (stderr, "%s:graph is unsolvable for '%s'\n",
> >> @@ -2730,6 +2732,7 @@ add_line_counts (coverage_info *coverage,
> >> function_info *fn)
> >>                      }
> >>                    line->count += block->count;
> >>                  }
> >> +             line->id = ln;
> >>              }
> >>
> >>            has_any_line = true;
> >> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> >> index ade6e335da7..341a27b033e 100644
> >> --- a/gcc/gimplify.cc
> >> +++ b/gcc/gimplify.cc
> >> @@ -3915,7 +3915,8 @@ shortcut_cond_r (tree pred, tree *true_label_p, tree
> >> *false_label_p,
> >>          false_label_p = &local_label;
> >>
> >>         /* Keep the original source location on the first 'if'.  */
> >> -      t = shortcut_cond_r (TREE_OPERAND (pred, 0), NULL, false_label_p,
> >> locus);
> >> +      tree op0 = TREE_OPERAND (pred, 0);
> >> +      t = shortcut_cond_r (op0, NULL, false_label_p, EXPR_LOCATION (op0));
> >>         append_to_statement_list (t, &expr);
> > 
> > The comment now no longer is true?  For the else arm we use
> > rexpr_location, why not
> > here as well?  To quote the following lines:
> > 
> >        /* Set the source location of the && on the second 'if'.  */
> >        new_locus = rexpr_location (pred, locus);
> >        t = shortcut_cond_r (TREE_OPERAND (pred, 1), true_label_p,
> >        false_label_p,
> >                             new_locus);
> >        append_to_statement_list (t, &expr);
> 
> Thanks, should use rexpr_location with each operand like below.
> 
> 
> > 
> > with your change the location of the outer COND_EXPR is lost?  Can we
> > guarantee
> > that it's used for the first operand of a if (a && b && c)?  It would
> > be nice to expand
> > the leading comment for such a three operand case and explain how it's
> > supposed
> > to work.
> 
> I tested the three operand case, it will iteratively call shortcut_cond_r and
> also works as expected.  Seems the outer COND_EXPR is useless if we do the
> followed conversion?
> 
> 
>    if (TREE_CODE (pred) == TRUTH_ANDIF_EXPR)
>      {
>        location_t new_locus;
> 
>        /* Turn if (a && b) into
> 
>          if (a); else goto no;
>          if (b) goto yes; else goto no;
>          (no:) */
> 
>        if (false_label_p == NULL)
>         false_label_p = &local_label;
> 
> -      /* Keep the original source location on the first 'if'.  */
> -      tree op0 = TREE_OPERAND (pred, 0);
> -      t = shortcut_cond_r (op0, NULL, false_label_p, EXPR_LOCATION (op0));
> +      /* Set the original source location on the first 'if'.  */
> +      tree op0 = TREE_OPERAND(pred, 0);
> +      new_locus = rexpr_location (op0, locus);
> +      t = shortcut_cond_r (op0, NULL, false_label_p, new_locus);      // <=
>        append_to_statement_list (t, &expr);
> 
>        /* Set the source location of the && on the second 'if'.  */
> -      new_locus = rexpr_location (pred, locus);
> -      t = shortcut_cond_r (TREE_OPERAND (pred, 1), true_label_p,
> false_label_p,
> -                          new_locus);
> +      tree op1 = TREE_OPERAND(pred, 1);
> +      new_locus = rexpr_location (op1, locus);
> +      t = shortcut_cond_r (op1, true_label_p, false_label_p, new_locus);
>        append_to_statement_list (t, &expr);
>      }

OK, so I think the original code did, for

 if (a && b)

use the location of 'if (a && b)' for the emitted

 if (a); else goto no;

and the location of 'a && b' for the emitted

 if (b) goto yes; else goto no;

that kind of makes sense to me to some extent - the operands
themselves keep their location but using the operands location
for the generated 'if's doesn't make much sense to me?

So I think the original code is correct.

Richard.

> 
> Breakpoint 5, shortcut_cond_r (pred=0x7ffff6f78fa0, true_label_p=0x0,
> false_label_p=0x7fffffffbef0, locus=2147483654) at ../.
> ./tgcc-master/gcc/gimplify.cc:3918
> (gdb) pel locus
> {file = 0x3e3e940 "test.c", line = 5, column = 19, data = 0x0, sysp = false}
> (gdb) n
> (gdb)
> (gdb) pel new_locus
> {file = 0x3e3e940 "test.c", line = 4, column = 18, data = 0x0, sysp = false}
> (gdb) ptree op0
>  <truth_andif_expr 0x7ffff6f78f50
>     type <boolean_type 0x7ffff6df2b28 _Bool public unsigned QI
>         size <integer_cst 0x7ffff6dd3e88 constant 8>
>         unit-size <integer_cst 0x7ffff6dd3ea0 constant 1>
>         align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
>         0x7ffff6df2b28 precision:1 min <integer_cst 0x7ffff6
> df60f0 0> max <integer_cst 0x7ffff6df6120 1>>
>     arg:0 <gt_expr 0x7ffff6f78eb0 type <boolean_type 0x7ffff6df2b28 _Bool>
>         arg:0 <parm_decl 0x7ffff7ff7080 c type <integer_type 0x7ffff6df23f0
>         char>
>             used read QI test.c:1:15 size <integer_cst 0x7ffff6dd3e88 8>
>             unit-size <integer_cst 0x7ffff6dd3ea0 1>
>             align:8 warn_if_not_align:0 context <function_decl 0x7ffff6f7e800
>             test> arg-type <integer_type 0x7ffff6df25e8 int>>
>         arg:1 <integer_cst 0x7ffff6f88a08 constant 64>
>         test.c:4:11 start: test.c:4:9 finish: test.c:4:16>
>     arg:1 <gt_expr 0x7ffff6f78ed8 type <boolean_type 0x7ffff6df2b28 _Bool>
>         arg:0 <parm_decl 0x7ffff7ff7080 c>
>         arg:1 <integer_cst 0x7ffff6f88a38 constant 76>
>         test.c:5:12 start: test.c:5:10 finish: test.c:5:17>
>     test.c:4:18 start: test.c:4:9 finish: test.c:5:17>
> 
> 
> 1int test(char c)
> 2{
> 3  return (
> 4        c >= 'A' &&
> 5         c >= 'M' &&
> 6          c <= 'Z');
> 7}
> 
> 
> bbs:
> 
>   <bb 2> :
>   if (c_2(D) > 64)
>     goto <bb 3>; [INV]
>   else
>     goto <bb 6>; [INV]
> 
>   <bb 3> :
>   if (c_2(D) > 76)
>     goto <bb 4>; [INV]
>   else
>     goto <bb 6>; [INV]
> 
>   <bb 4> :
>   if (c_2(D) <= 90)
>     goto <bb 5>; [INV]
>   else
>     goto <bb 6>; [INV]
> 
> 
> gcno diff before and with this patch:
> 
> -test.gcno: 1312:                  block 2:`test.c':1, 5
> +test.gcno: 1312:                  block 2:`test.c':1, 4
>  test.gcno: 1343:    01450000:  31:LINES
> -test.gcno: 1355:                  block 3:`test.c':4
> +test.gcno: 1355:                  block 3:`test.c':5
>  test.gcno: 1382:    01450000:  31:LINES
> -test.gcno: 1394:                  block 4:`test.c':5
> +test.gcno: 1394:                  block 4:`test.c':6
>  test.gcno: 1421:    01450000:  31:LINES
>  test.gcno: 1433:                  block 5:`test.c':5
>  test.gcno: 1460:    01450000:  31:LINES
>  test.gcno: 1472:                  block 6:`test.c':5
>  test.gcno: 1499:    01450000:  31:LINES
>  test.gcno: 1511:                  block 7:`test.c':5
>  test.gcno: 1538:    01450000:  31:LINES
>  test.gcno: 1550:                  block 8:`test.c':5
> 
> <bb 2>, <bb 3> and <bb 4> are  mapped to line 4, 5, 6 correctly now...
> 
>