[patch] improve sloc assignment on bind_expr entry/exit code

[patch] improve sloc assignment on bind_expr entry/exit code

[Olivier Hainque]

[-- Attachment #1: Type: text/plain, Size: 3912 bytes --]

Hello,

For blocks requiring it, the gimplifier generates stack pointer
save/restore operations on entry/exit, per:

 gimplify_bind_expr (...)

  if (gimplify_ctxp->save_stack)
    {
      gimple stack_restore;

      /* Save stack on entry and restore it on exit.  Add a try_finally
	 block to achieve this.  */
      build_stack_save_restore (&stack_save, &stack_restore);

      gimplify_seq_add_stmt (&cleanup, stack_restore);
    }

  /* Add clobbers for all variables that go out of scope.  */
  ...

There is no specific location assigned to these entry/exit statements
so they eventually inherits slocs coming from preceding statements.

This is problematic for tools relying on debug info to infer
which statements were executed out of execution traces (allowing
coverage analysis without code instrumentation).

An example of problematic scenario is provided below.

The attached patch is a proposal to improve this by propagating
start and end of block locations from the block structure to the
few gimple statements we generate. It adds an "end_locus" to the
block structure for this purpose, which the Ada front-end knows
how to fill already.

I verified that it does inserts proper .loc directives before the
entry/exit code on the example. The patch also bootstraps and regtests
fine for languages=all,ada on x86_64-pc-linux-gnu.

OK to commit ?

Thanks in advance for your feedback,

With Kind Regards,

Olivier

--

2014-06-11  Olivier Hainque  <hainque@adacore.com>

	* tree-core.h (tree_block): Add an "end_locus" field, allowing
	memorization of the end of block source location.
	* tree.h (BLOCK_SOURCE_END_LOCATION): New accessor.
	* gimplify.c (gimplify_bind_expr): Propagate the block start and
	end source location info we have on the block entry/exit code we
	generate.

--

Here is an Ada example to illustrate:

-- p.adb
     1	procedure P (Choice : Integer; N : in out Integer) is
     2	begin
     3	   if Choice > 0 then
     4	      declare
     5	         S : String (1 .. N * 2);
     6	         pragma Volatile (S);
     7	      begin
     8		 S := (others => 'B');
     9	      end;
    10	   else
    11	      declare
    12		 S : String (1 .. N );
    13		 pragma Volatile (S);
    14	      begin
    15		 S := (others => '1');
    16	      end;
    17	   end if;
    18	end;

The two if/else blocks allocate a local string of dynamic size and call for
stack save/restore operations. A very recent mainline on x86_64-linux produces
the following code (gcc -c -g -save-temps -fverbose-asm p.adb):

     .loc 1 3 0
     cmpl    $0, -84(%rbp)   #, choice
     jle     .L2             <-------- branch to the else block if choice <= 0
     [...]
     [code for the if block here]
     ...
     .loc 1 8 0 discriminator 3
     addl    $1, %eax        #, D.3124
     jmp     .L5     #
     ...
   .L2:                      <------- branch lands here
   .LBB4:
     movq    %rsp, %rax      #, tmp136         <--- stack save on entry of the
     movq    %rax, %rdi      # tmp136, D.3125  <--- else block.
     .loc 1 12 0 is_stmt 1
     movl    -88(%rbp), %ecx # n, D.3129


The stack save code has no specific sloc assigned, so inherits
what happens to be current at this point, here the .loc 1 8 corresponding
to the code last emitted for the string assignment on line 8.

This is inaccurate because the stack save code really has nothing to do with
the statement on line 8. And this is problematic when infering coverage
information from execution traces, as it incorrectly appears as if part of
line 8 was executed as soon as we reach here.

The proposed change ends up inserting a .loc 1 11 0 just before the first move
insn, curing the problem, as well as a .loc 1 4, a .loc 1 9 and a .loc 1 16
before the other start and end of blocks for similar reasons.


[-- Attachment #2: bind_expr_locus.diff --]
[-- Type: application/octet-stream, Size: 3012 bytes --]

diff --git gcc/gimplify.c gcc/gimplify.c
index 654b05c..0c0af10 100644
--- gcc/gimplify.c
+++ gcc/gimplify.c
@@ -1046,6 +1046,7 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
   gimple gimple_bind;
   gimple_seq body, cleanup;
   gimple stack_save;
+  location_t start_locus = 0, end_locus = 0;
 
   tree temp = voidify_wrapper_expr (bind_expr, NULL);
 
@@ -1098,6 +1099,19 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
   gimplify_stmt (&BIND_EXPR_BODY (bind_expr), &body);
   gimple_bind_set_body (gimple_bind, body);
 
+  /* Source location wise, the cleanup code (stack_restore and clobbers)
+     belongs to the end of the block, so propagate what we have.  The
+     stack_save operation belongs to the beginning of block, which we can
+     infer from the bind_expr directly if the block has no explicit
+     assignment.  */
+  if (BIND_EXPR_BLOCK (bind_expr))
+    {
+      end_locus = BLOCK_SOURCE_END_LOCATION (BIND_EXPR_BLOCK (bind_expr));
+      start_locus = BLOCK_SOURCE_LOCATION (BIND_EXPR_BLOCK (bind_expr));
+    }
+  if (start_locus == 0)
+    start_locus = EXPR_LOCATION (bind_expr);
+
   cleanup = NULL;
   stack_save = NULL;
   if (gimplify_ctxp->save_stack)
@@ -1108,6 +1122,9 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
 	 block to achieve this.  */
       build_stack_save_restore (&stack_save, &stack_restore);
 
+      gimple_set_location (stack_save, start_locus);
+      gimple_set_location (stack_restore, end_locus);
+
       gimplify_seq_add_stmt (&cleanup, stack_restore);
     }
 
@@ -1125,10 +1142,12 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
 	  && !is_gimple_reg (t)
 	  && flag_stack_reuse != SR_NONE)
 	{
-	  tree clobber = build_constructor (TREE_TYPE (t),
-					    NULL);
+	  tree clobber = build_constructor (TREE_TYPE (t), NULL);
+	  gimple clobber_stmt;
 	  TREE_THIS_VOLATILE (clobber) = 1;
-	  gimplify_seq_add_stmt (&cleanup, gimple_build_assign (t, clobber));
+	  clobber_stmt = gimple_build_assign (t, clobber);
+	  gimple_set_location (clobber_stmt, end_locus);
+	  gimplify_seq_add_stmt (&cleanup, clobber_stmt);
 	}
     }
 
diff --git gcc/tree-core.h gcc/tree-core.h
index ebe5849..13be2c1 100644
--- gcc/tree-core.h
+++ gcc/tree-core.h
@@ -1248,6 +1248,7 @@ struct GTY(()) tree_block {
   unsigned block_num : 31;
 
   location_t locus;
+  location_t end_locus;
 
   tree vars;
   vec<tree, va_gc> *nonlocalized_vars;
diff --git gcc/tree.h gcc/tree.h
index 1382c78..ceb4464 100644
--- gcc/tree.h
+++ gcc/tree.h
@@ -1500,6 +1500,11 @@ extern void protected_set_expr_location (tree, location_t);
 
 #define BLOCK_SOURCE_LOCATION(NODE) (BLOCK_CHECK (NODE)->block.locus)
 
+/* This gives the location of the end of the block, useful to attach
+   code implicitly generated for outgoing paths.  */
+
+#define BLOCK_SOURCE_END_LOCATION(NODE) (BLOCK_CHECK (NODE)->block.end_locus)
+
 /* Define fields and accessors for nodes representing data types.  */
 
 /* See tree.def for documentation of the use of these fields.

[-- Attachment #3: Type: text/plain, Size: 1 bytes --]

Re: [patch] improve sloc assignment on bind_expr entry/exit code

[Jeff Law]

On 06/11/14 09:02, Olivier Hainque wrote:
> Hello,
>
> For blocks requiring it, the gimplifier generates stack pointer
> save/restore operations on entry/exit, per:
>
>   gimplify_bind_expr (...)
>
>    if (gimplify_ctxp->save_stack)
>      {
>        gimple stack_restore;
>
>        /* Save stack on entry and restore it on exit.  Add a try_finally
> 	 block to achieve this.  */
>        build_stack_save_restore (&stack_save, &stack_restore);
>
>        gimplify_seq_add_stmt (&cleanup, stack_restore);
>      }
>
>    /* Add clobbers for all variables that go out of scope.  */
>    ...
>
> There is no specific location assigned to these entry/exit statements
> so they eventually inherits slocs coming from preceding statements.
>
> This is problematic for tools relying on debug info to infer
> which statements were executed out of execution traces (allowing
> coverage analysis without code instrumentation).
>
> An example of problematic scenario is provided below.
>
> The attached patch is a proposal to improve this by propagating
> start and end of block locations from the block structure to the
> few gimple statements we generate. It adds an "end_locus" to the
> block structure for this purpose, which the Ada front-end knows
> how to fill already.
>
> I verified that it does inserts proper .loc directives before the
> entry/exit code on the example. The patch also bootstraps and regtests
> fine for languages=all,ada on x86_64-pc-linux-gnu.
>
> OK to commit ?
>
> Thanks in advance for your feedback,
>
> With Kind Regards,
>
> Olivier
>
> --
>
> 2014-06-11  Olivier Hainque  <hainque@adacore.com>
>
> 	* tree-core.h (tree_block): Add an "end_locus" field, allowing
> 	memorization of the end of block source location.
> 	* tree.h (BLOCK_SOURCE_END_LOCATION): New accessor.
> 	* gimplify.c (gimplify_bind_expr): Propagate the block start and
> 	end source location info we have on the block entry/exit code we
> 	generate.
OK.  I assume y'all will add a suitable test to the Ada testsuite and 
propagate it into the GCC testsuite in due course?

jeff

Re: [patch] improve sloc assignment on bind_expr entry/exit code

[Olivier Hainque]

Hi Jeff,

On Jun 17, 2014, at 22:42 , Jeff Law <law@redhat.com> wrote:

>> 	* tree-core.h (tree_block): Add an "end_locus" field, allowing
>> 	memorization of the end of block source location.
>> 	* tree.h (BLOCK_SOURCE_END_LOCATION): New accessor.
>> 	* gimplify.c (gimplify_bind_expr): Propagate the block start and
>> 	end source location info we have on the block entry/exit code we
>> 	generate.
> OK.

 Great, thanks! :-)

>  I assume y'all will add a suitable test to the Ada testsuite and propagate it into the GCC testsuite in due course?

 Yes, I will. At the patch submission time, I was unclear on what dejagnu
 device was available to setup a reliable testing protocol for this kind of
 issue and I was interested in getting feedback on the patch contents first.

 ISTM that dg-scan-asm for the expected extra .loc's would work, maybe
 restricted to some target we know produces .loc directives.

 Sounds appropriate ?

 Thanks again for your feedback,

 Olivier
 

Re: [patch] improve sloc assignment on bind_expr entry/exit code

[Olivier Hainque]

On Jun 18, 2014, at 09:42 , Olivier Hainque <hainque@adacore.com> wrote:
>> I assume y'all will add a suitable test to the Ada testsuite and propagate it into the GCC testsuite in due course?

> ISTM that dg-scan-asm for the expected extra .loc's would work, maybe
> restricted to some target we know produces .loc directives.
> 
> Sounds appropriate ?

 Ah, we already have one test doing exactly that (return3.adb).
 I'll just add one.

 With Kind Regards,

 Olivier

Re: [patch] improve sloc assignment on bind_expr entry/exit code

[Jeff Law]

On 06/18/14 01:42, Olivier Hainque wrote:
> Hi Jeff,
>
> On Jun 17, 2014, at 22:42 , Jeff Law <law@redhat.com> wrote:
>
>>> 	* tree-core.h (tree_block): Add an "end_locus" field, allowing
>>> 	memorization of the end of block source location.
>>> 	* tree.h (BLOCK_SOURCE_END_LOCATION): New accessor.
>>> 	* gimplify.c (gimplify_bind_expr): Propagate the block start and
>>> 	end source location info we have on the block entry/exit code we
>>> 	generate.
>> OK.
>
>   Great, thanks! :-)
>
>>   I assume y'all will add a suitable test to the Ada testsuite and propagate it into the GCC testsuite in due course?
>
>   Yes, I will. At the patch submission time, I was unclear on what dejagnu
>   device was available to setup a reliable testing protocol for this kind of
>   issue and I was interested in getting feedback on the patch contents first.
>
>   ISTM that dg-scan-asm for the expected extra .loc's would work, maybe
>   restricted to some target we know produces .loc directives.
>
>   Sounds appropriate ?
Yea, that should be fine.  Most folks test x86-64 linux, so that's going 
to get you the widest net for coverage.

jeff

Re: [patch] improve sloc assignment on bind_expr entry/exit code

[Olivier Hainque]

On Jun 18, 2014, at 15:48 , Jeff Law <law@redhat.com> wrote:

>>  ISTM that dg-scan-asm for the expected extra .loc's would work, maybe
>>  restricted to some target we know produces .loc directives.
>> 
>>  Sounds appropriate ?

> Yea, that should be fine.  Most folks test x86-64 linux, so that's going to get you the widest net for coverage.


 OK, patch & test checked in. Thanks again for your feedback.

 Cheers,

 Olivier