Re: [PATCH v4] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
On Tue, 14 Mar 2023, Xionghu Luo wrote:
>
>
> On 2023/3/9 20:02, Richard Biener wrote:
> > On Wed, 8 Mar 2023, Xionghu Luo wrote:
> >
> >>
> >>
> >> On 2023/3/7 19:25, Richard Biener wrote:
> > It would be nice to avoid creating blocks / preserving labels we'll
> > immediately remove again. For that we do need some analysis
> > before creating basic-blocks that determines whether a label is
> > possibly reached by a non-falltru edge.
> >
>
> :
> p = 0;
> switch (s) , case 0: , case 1: >
>
> :
> : <= prev_stmt
> : <= stmt
> p = p + 1;
> n = n + -1;
> if (n != 0) goto ; else goto ;
>
> Check if is a case label and is a goto target then return
> true
> in stmt_starts_bb_p to start a new basic block? This would avoid
> creating
> and
> removing blocks, but cleanup_dead_labels has all bbs setup while
> stmt_starts_bb_p
> does't yet to iterate bbs/labels to establish label_for_bb[] map?
> >>
> >>> Yes. I think we'd need something more pragmatic before make_blocks (),
> >>> like re-computing TREE_USED of the label decls or computing a bitmap
> >>> of targeted labels (targeted by goto, switch or any other means).
> >>>
> >>> I'll note that doing a cleanup_dead_labels () like optimization before
> >>> we create blocks will help keeping LABEL_DECL_UID and thus
> >>> label_to_block_map dense. But it does look like a bit of
> >>> an chicken-and-egg problem and the question is how effective the
> >>> dead label removal is in practice.
> >>
> >> Tried to add function compute_target_labels(not sure whether the function
> >> name is suitable) in the front of make_blocks_1, now the fortran case
> >> doesn't
> >> create/removing blocks now, but I still have several questions:
> >>
> >> 1. I used hash_set to save the target labels instead of bitmap, as
> >> labels
> >> are tree type value instead of block index so bitmap is not good for it
> >> since
> >> we don't have LABEL_DECL_UID now?
> >
> > We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
> > hash_set vs. bitmap is somewhat arbitrary here. The real cost is
> > the extra walk over all stmts.
> >
> >> 2. Is the compute_target_labels still only for !optimize? And if we
> >> compute
> >> the target labels before create bbs, it is unnessary to guard the first
> >> cleanup_dead_labels under !optimize now, because the switch-case-do-while
> >> case already create new block for CASE_LABEL already.
> >
> > OK.
> >
> >> 3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
> >> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
> >> labels_eh?
> >
> > I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
> > coverage already?
>
> Added in patch v4.
>
> >
> >> PS1: The v3 patch will cause one test case fail:
> >>
> >> Number of regressions in total: 1
> >>> FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
> >>> errors)
> >>
> >> due to this exausting case has labels from L0 to L11, they won't be
> >> optimized
> >> to a simple if-else expression like before...
> >
> > Hmm, that's somewhat unexpected.
>
> It could be fixed by not start a new block if two locus are on same line as
> the
> labels are expanded by MACRO with same location info. BTW, I found that two
> UNKOWN_LOCATION variable may have different value but return true in
> same_line_p?
Yes, the raw location value also encodes other info so only
LOCATION_LOCUS (loc) will be equal to UNKNOWN_LOCATION. There's some
existing inconsistency in whether LOCATION_LOCUS or raw locus is
compared.
> 2: locus1 = 2147483670
> 3: locus2 = 2147483652
> (gdb) pel locus1
> {file = 0x0, line = 0, column = 0, data = 0x76bdc300, sysp = false}
> (gdb) pel locus2
> {file = 0x0, line = 0, column = 0, data = 0x76bdc4e0, sysp = false}
> (gdb) p LOCATION_LOCUS (locus1)
> $16 = 0
> (gdb) p LOCATION_LOCUS (locus2)
> $17 = 0
>
> So fix the function like this?
>
> @@ -1152,6 +1218,10 @@ same_line_p (location_t locus1, expanded_location
> *from, location_t locus2)
> {
>expanded_location to;
>
> + if (LOCATION_LOCUS (locus1) == UNKNOWN_LOCATION
> + && LOCATION_LOCUS (locus2) == UNKNOWN_LOCATION)
> +return false;
> +
I think we want to treat two unknown locations as the same line, but for
consistency I'd change the following test to use LOCATION_LOCUS
>if (locus1 == locus2)
> return true;
>
> >
> >>
> >> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail
> >> due
> >> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into
> >> it
> >> so
> >> just skip these label...
> >
> > Please investigate, we might be missing a corner case here.
>
> Yes. Take the case pointer_array_1.f90 as example, it has an UNUSED label
> "L.7"
> with locus info in it, not sure why it exists even since .original.
>
>
>
Re: [PATCH v4] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
On 2023/3/9 20:02, Richard Biener wrote:
On Wed, 8 Mar 2023, Xionghu Luo wrote:
On 2023/3/7 19:25, Richard Biener wrote:
It would be nice to avoid creating blocks / preserving labels we'll
immediately remove again. For that we do need some analysis
before creating basic-blocks that determines whether a label is
possibly reached by a non-falltru edge.
:
p = 0;
switch (s) , case 0: , case 1: >
:
: <= prev_stmt
: <= stmt
p = p + 1;
n = n + -1;
if (n != 0) goto ; else goto ;
Check if is a case label and is a goto target then return
true
in stmt_starts_bb_p to start a new basic block? This would avoid creating
and
removing blocks, but cleanup_dead_labels has all bbs setup while
stmt_starts_bb_p
does't yet to iterate bbs/labels to establish label_for_bb[] map?
Yes. I think we'd need something more pragmatic before make_blocks (),
like re-computing TREE_USED of the label decls or computing a bitmap
of targeted labels (targeted by goto, switch or any other means).
I'll note that doing a cleanup_dead_labels () like optimization before
we create blocks will help keeping LABEL_DECL_UID and thus
label_to_block_map dense. But it does look like a bit of
an chicken-and-egg problem and the question is how effective the
dead label removal is in practice.
Tried to add function compute_target_labels(not sure whether the function
name is suitable) in the front of make_blocks_1, now the fortran case doesn't
create/removing blocks now, but I still have several questions:
1. I used hash_set to save the target labels instead of bitmap, as
labels
are tree type value instead of block index so bitmap is not good for it since
we don't have LABEL_DECL_UID now?
We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
hash_set vs. bitmap is somewhat arbitrary here. The real cost is
the extra walk over all stmts.
2. Is the compute_target_labels still only for !optimize? And if we compute
the target labels before create bbs, it is unnessary to guard the first
cleanup_dead_labels under !optimize now, because the switch-case-do-while
case already create new block for CASE_LABEL already.
OK.
3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
labels_eh?
I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
coverage already?
Added in patch v4.
PS1: The v3 patch will cause one test case fail:
Number of regressions in total: 1
FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
errors)
due to this exausting case has labels from L0 to L11, they won't be
optimized
to a simple if-else expression like before...
Hmm, that's somewhat unexpected.
It could be fixed by not start a new block if two locus are on same line as the
labels are expanded by MACRO with same location info. BTW, I found that two
UNKOWN_LOCATION variable may have different value but return true in
same_line_p?
2: locus1 = 2147483670
3: locus2 = 2147483652
(gdb) pel locus1
{file = 0x0, line = 0, column = 0, data = 0x76bdc300, sysp = false}
(gdb) pel locus2
{file = 0x0, line = 0, column = 0, data = 0x76bdc4e0, sysp = false}
(gdb) p LOCATION_LOCUS (locus1)
$16 = 0
(gdb) p LOCATION_LOCUS (locus2)
$17 = 0
So fix the function like this?
@@ -1152,6 +1218,10 @@ same_line_p (location_t locus1, expanded_location *from,
location_t locus2)
{
expanded_location to;
+ if (LOCATION_LOCUS (locus1) == UNKNOWN_LOCATION
+ && LOCATION_LOCUS (locus2) == UNKNOWN_LOCATION)
+return false;
+
if (locus1 == locus2)
return true;
PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it
so
just skip these label...
Please investigate, we might be missing a corner case here.
Yes. Take the case pointer_array_1.f90 as example, it has an UNUSED label "L.7"
with locus info in it, not sure why it exists even since .original.
[pointer_array_1.f90:39:10] if (test.14 != 0) goto ; els
e goto ;
:
[pointer_array_1.f90:39:52] _gfortran_stop_numeric (3, 0);
:
parm.16 = {CLOBBER(eol)};
[pointer_array_1.f90:39:52] L.7: <= UNUSED label
:
[pointer_array_1.f90:39:52] L.3:
atmp.0 = {CLOBBER(eol)};
A.1 = {CLOBBER(eol)};
atmp.5 = {CLOBBER(eol)};
A.6 = {CLOBBER(eol)};
d = {CLOBBER(eol)};
[pointer_array_1.f90:41:14] return;
stmt_starts_bb_p will return true for L.7 as the prev_stmt "parm.16 =
{CLOBBER(eol)};"
is not a label statement, then will also return true in
stmt_starts_bb_p as
the label_stmt and prev_stmt are NOT on same line.
:
L.9:
L.8:
if (test.14 != 0) goto ; else goto ;
:
:
_gfortran_stop_numeric (3, 0);
:
:
parm.16 = {CLOBBER(eol)};
: <= empty block
L.7:
:
:
L.3:
atmp.0 = {CLOBBER(eol)};
A.1 = {CLOBBER(eol)};
atmp.5 = {CLOBBER(eol)};
A.6 = {CLOBBER(eol)};
d = {CLOBBER(eol)};
return;
So I have to fix
