On 06/04/2024 13:15, Jørgen Kvalsvik wrote:
On 06/04/2024 07:50, Richard Biener wrote:
Am 05.04.2024 um 21:59 schrieb Jørgen Kvalsvik <j...@lambda.is>:
Hi,
I propose these fixes for the current issues with the condition
coverage.
Rainer, I propose to simply delete the test with __sigsetjmp. I don't
think it actually detects anything reasonable any more, I kept it around
to prevent a regression. Since then I have built a lot of programs (with
optimization enabled) and not really seen this problem.
H.J., the problem you found with -O2 was really a problem of
tree-inlining, which was actually caught earlier by Jan [1]. It probably
warrants some more testing, but I could reproduce by tuning your test
case to use always_inline and not -O2 and trigger the error.
[1] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/648785.html
Ok
Thanks, committed.
I am wondering if the fn->cond_uids access should always be guarded (in
tree-profile.cc) should always be guarded. Right now there is the
assumption that if condition coverage is requested the will exist and be
populated, but as this shows there may be other circumstances where this
is not true.
Or perhaps there should be a gcc_assert to (reliably) detect cases where
the map is not constructed properly?
Thanks,
Jørgen
I gave this some more thought, and realised I was too eager to fix the
segfault. While trunk no longer crashes (at least on my x86_64 linux)
the fix itself is bad. It copies the gcond -> uid mappings into the
caller, but the stmts are deep copied into the caller, so no gcond will
ever be a hit when we look up the condition_uids in tree-profile.cc.
I did a very quick prototype to confirm. By applying this patch:
@@ -2049,6 +2049,9 @@ copy_bb (copy_body_data *id, basic_block bb,
copy_gsi = gsi_start_bb (copy_basic_block);
+ if (!cfun->cond_uids && id->src_cfun->cond_uids)
+ cfun->cond_uids = new hash_map <gcond*, unsigned> ();
+
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
{
gimple_seq stmts;
@@ -2076,6 +2079,12 @@ copy_bb (copy_body_data *id, basic_block bb,
if (gimple_nop_p (stmt))
continue;
+ if (id->src_cfun->cond_uids && is_a <gcond*> (orig_stmt))
+ {
+ unsigned *v = id->src_cfun->cond_uids->get (as_a<gcond*>
(orig_stmt));
+ if (v) cfun->cond_uids->put (as_a <gcond*> (stmt), *v);
+ }
+
and this test program:
__attribute__((always_inline))
inline int
inlinefn (int a)
{
if (a > 5)
{
printf ("a > 5\n");
return a;
}
else
printf ("a < 5, was %d\n", a);
return a * a - 2;
}
int
mcdc027e (int a, int b)
{
int y = inlinefn (a);
return y + b;
}
gcov reports:
2: 18:mcdc027e (int a, int b)
condition outcomes covered 1/2
condition 0 not covered (true)
-: 19:{
2: 20: int y = inlinefn (a);
2: 21: return y + b;
-: 22:}
but without the patch, gcov prints nothing.
I am not sure if this approach is even ideal. Probably the most
problematic is the source line mapping which is all messed up. I checked
with gcov --branch-probabilities and it too reports the callee at the
top of the caller.
If you think it is a good strategy I can clean up the prototype and
submit a patch. I suppose the function _totals_ should be accurate, even
if the source mapping is a bit surprising.
What do you think? I am open to other strategies, too
Thanks,
Jørgen
Thanks,
Richard
Thanks,
Jørgen
Jørgen Kvalsvik (2):
Remove unecessary and broken MC/DC compile test
Copy condition->expr map when inlining [PR114599]
gcc/testsuite/gcc.misc-tests/gcov-19.c | 11 ---------
gcc/testsuite/gcc.misc-tests/gcov-pr114599.c | 25 ++++++++++++++++++++
gcc/tree-inline.cc | 20 +++++++++++++++-
3 files changed, 44 insertions(+), 12 deletions(-)
create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114599.c
--
2.30.2