Re: [patch] improve sloc assignment on bind_expr entry/exit code
On Jun 18, 2014, at 15:48 , Jeff Law 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
Re: [patch] improve sloc assignment on bind_expr entry/exit code
On 06/18/14 01:42, Olivier Hainque wrote: Hi Jeff, On Jun 17, 2014, at 22:42 , Jeff Law 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
On Jun 18, 2014, at 09:42 , Olivier Hainque 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
Hi Jeff, On Jun 17, 2014, at 22:42 , Jeff Law 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
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
* 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
[patch] improve sloc assignment on bind_expr entry/exit code
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
* 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
4declare
5 S : String (1 .. N * 2);
6 pragma Volatile (S);
7begin
8 S := (others => 'B');
9end;
10 else
11declare
12 S : String (1 .. N );
13 pragma Volatile (S);
14begin
15 S := (others => '1');
16end;
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.
bind_expr_locus.diff
Description: Binary data
