cfgbuild: Fix DEBUG_INSN handling in find_bb_boundaries [PR106719]
Checks
Commit Message
Hi!
The following testcase FAILs on aarch64-linux. We have some atomic
instruction followed by 2 DEBUG_INSNs (if -g only of course) followed
by NOTE_INSN_EPILOGUE_BEG followed by some USE insn.
Now, split3 pass replaces the atomic instruction with a code sequence
which ends with a conditional jump and the split3 pass calls
find_many_sub_basic_blocks.
For -g0, find_bb_boundaries sees the flow_transfer_insn (the new conditional
jump), then NOTE_INSN_EPILOGUE_BEG which can live in between basic blocks
and then the USE insn, so splits block after the NOTE_INSN_EPILOGUE_BEG
and puts the NOTE in between the blocks.
For -g, if sees a DEBUG_INSN after the flow_transfer_insn, so sets
debug_insn to it, then walks over another DEBUG_INSN, NOTE_INSN_EPILOGUE_BEG
until it finally sees the USE insn, and triggers the:
rtx_insn *prev = PREV_INSN (insn);
/* If the first non-debug inside_basic_block_p insn after a control
flow transfer is not a label, split the block before the debug
insn instead of before the non-debug insn, so that the debug
insns are not lost. */
if (debug_insn && code != CODE_LABEL && code != BARRIER)
prev = PREV_INSN (debug_insn);
code I've added for PR81325. If there are only DEBUG_INSNs, that is
the right thing to do, but if in between debug_insn and insn there are
notes which can stay in between basic blocks or simnilarly JUMP_TABLE_DATA
or their associated CODE_LABELs, it causes -fcompare-debug differences.
The following patch fixes it by clearing debug_insn if JUMP_TABLE_DATA
or associated CODE_LABEL is seen (I'm afraid there is no good answer
what to do with DEBUG_INSNs before those; the code then removes them:
/* Clean up the bb field for the insns between the blocks. */
for (x = NEXT_INSN (flow_transfer_insn);
x != BB_HEAD (fallthru->dest);
x = next)
{
next = NEXT_INSN (x);
/* Debug insns should not be in between basic blocks,
drop them on the floor. */
if (DEBUG_INSN_P (x))
delete_insn (x);
else if (!BARRIER_P (x))
set_block_for_insn (x, NULL);
}
but if there are NOTEs, the patch just reorders the NOTEs and DEBUG_INSNs,
such that the NOTEs come first (so that they stay in between basic blocks
like with -g0) and DEBUG_INSNs after those (so that bb is split before
them, so they will be in the basic block after NOTE_INSN_BASIC_BLOCK).
Bootstrapped/regtested on x86_64-linux and i686-linux plus tested on
the testcase in a cross to aarch64-linux, ok for trunk?
2022-12-08 Jakub Jelinek <jakub@redhat.com>
PR debug/106719
* cfgbuild.cc (find_bb_boundaries): If there are NOTEs in between
debug_insn (seen after flow_transfer_insn) and insn, move NOTEs
before all the DEBUG_INSNs and split after NOTEs. If there are
other insns like jump table data, clear debug_insn.
* gcc.dg/pr106719.c: New test.
Jakub
Comments
On Thu, Dec 8, 2022 at 11:12 AM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi!
>
> The following testcase FAILs on aarch64-linux. We have some atomic
> instruction followed by 2 DEBUG_INSNs (if -g only of course) followed
> by NOTE_INSN_EPILOGUE_BEG followed by some USE insn.
> Now, split3 pass replaces the atomic instruction with a code sequence
> which ends with a conditional jump and the split3 pass calls
> find_many_sub_basic_blocks.
> For -g0, find_bb_boundaries sees the flow_transfer_insn (the new conditional
> jump), then NOTE_INSN_EPILOGUE_BEG which can live in between basic blocks
> and then the USE insn, so splits block after the NOTE_INSN_EPILOGUE_BEG
> and puts the NOTE in between the blocks.
> For -g, if sees a DEBUG_INSN after the flow_transfer_insn, so sets
> debug_insn to it, then walks over another DEBUG_INSN, NOTE_INSN_EPILOGUE_BEG
> until it finally sees the USE insn, and triggers the:
> rtx_insn *prev = PREV_INSN (insn);
>
> /* If the first non-debug inside_basic_block_p insn after a control
> flow transfer is not a label, split the block before the debug
> insn instead of before the non-debug insn, so that the debug
> insns are not lost. */
> if (debug_insn && code != CODE_LABEL && code != BARRIER)
> prev = PREV_INSN (debug_insn);
> code I've added for PR81325. If there are only DEBUG_INSNs, that is
> the right thing to do, but if in between debug_insn and insn there are
> notes which can stay in between basic blocks or simnilarly JUMP_TABLE_DATA
> or their associated CODE_LABELs, it causes -fcompare-debug differences.
>
> The following patch fixes it by clearing debug_insn if JUMP_TABLE_DATA
> or associated CODE_LABEL is seen (I'm afraid there is no good answer
> what to do with DEBUG_INSNs before those; the code then removes them:
> /* Clean up the bb field for the insns between the blocks. */
> for (x = NEXT_INSN (flow_transfer_insn);
> x != BB_HEAD (fallthru->dest);
> x = next)
> {
> next = NEXT_INSN (x);
> /* Debug insns should not be in between basic blocks,
> drop them on the floor. */
> if (DEBUG_INSN_P (x))
> delete_insn (x);
> else if (!BARRIER_P (x))
> set_block_for_insn (x, NULL);
> }
> but if there are NOTEs, the patch just reorders the NOTEs and DEBUG_INSNs,
> such that the NOTEs come first (so that they stay in between basic blocks
> like with -g0) and DEBUG_INSNs after those (so that bb is split before
> them, so they will be in the basic block after NOTE_INSN_BASIC_BLOCK).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux plus tested on
> the testcase in a cross to aarch64-linux, ok for trunk?
OK.
Thanks,
Richard.
> 2022-12-08 Jakub Jelinek <jakub@redhat.com>
>
> PR debug/106719
> * cfgbuild.cc (find_bb_boundaries): If there are NOTEs in between
> debug_insn (seen after flow_transfer_insn) and insn, move NOTEs
> before all the DEBUG_INSNs and split after NOTEs. If there are
> other insns like jump table data, clear debug_insn.
>
> * gcc.dg/pr106719.c: New test.
>
> --- gcc/cfgbuild.cc.jj 2022-01-18 11:58:58.944991171 +0100
> +++ gcc/cfgbuild.cc 2022-12-07 21:36:27.493363173 +0100
> @@ -445,6 +445,7 @@ find_bb_boundaries (basic_block bb)
> rtx_insn *debug_insn = NULL;
> edge fallthru = NULL;
> bool skip_purge;
> + bool seen_note_after_debug = false;
>
> if (insn == end)
> return;
> @@ -492,7 +493,10 @@ find_bb_boundaries (basic_block bb)
> if (code == DEBUG_INSN)
> {
> if (flow_transfer_insn && !debug_insn)
> - debug_insn = insn;
> + {
> + debug_insn = insn;
> + seen_note_after_debug = false;
> + }
> }
> /* In case we've previously seen an insn that effects a control
> flow transfer, split the block. */
> @@ -506,7 +510,40 @@ find_bb_boundaries (basic_block bb)
> insn instead of before the non-debug insn, so that the debug
> insns are not lost. */
> if (debug_insn && code != CODE_LABEL && code != BARRIER)
> - prev = PREV_INSN (debug_insn);
> + {
> + prev = PREV_INSN (debug_insn);
> + if (seen_note_after_debug)
> + {
> + /* Though, if there are NOTEs intermixed with DEBUG_INSNs,
> + move the NOTEs before the DEBUG_INSNs and split after
> + the last NOTE. */
> + rtx_insn *first = NULL, *last = NULL;
> + for (x = debug_insn; x != insn; x = NEXT_INSN (x))
> + {
> + if (NOTE_P (x))
> + {
> + if (first == NULL)
> + first = x;
> + last = x;
> + }
> + else
> + {
> + gcc_assert (DEBUG_INSN_P (x));
> + if (first)
> + {
> + reorder_insns_nobb (first, last, prev);
> + prev = last;
> + first = last = NULL;
> + }
> + }
> + }
> + if (first)
> + {
> + reorder_insns_nobb (first, last, prev);
> + prev = last;
> + }
> + }
> + }
> fallthru = split_block (bb, prev);
> if (flow_transfer_insn)
> {
> @@ -547,6 +584,14 @@ find_bb_boundaries (basic_block bb)
> flow_transfer_insn = prev_nonnote_nondebug_insn_bb (insn);
> debug_insn = NULL;
> }
> + else if (debug_insn)
> + {
> + if (code == NOTE)
> + seen_note_after_debug = true;
> + else
> + /* Jump tables. */
> + debug_insn = NULL;
> + }
>
> if (control_flow_insn_p (insn))
> flow_transfer_insn = insn;
> --- gcc/testsuite/gcc.dg/pr106719.c.jj 2022-12-07 21:35:56.523810192 +0100
> +++ gcc/testsuite/gcc.dg/pr106719.c 2022-12-07 21:35:40.964034788 +0100
> @@ -0,0 +1,13 @@
> +/* PR debug/106719 */
> +/* { dg-do compile { target sync_char_short } } */
> +/* { dg-options "-O2 -fcompare-debug" } */
> +
> +extern short int esi, easi[2];
> +
> +void
> +foo (void)
> +{
> + short int *psi = &easi[1];
> + __atomic_nand_fetch (psi, esi, 0);
> + psi = &easi[1];
> +}
>
> Jakub
>
@@ -445,6 +445,7 @@ find_bb_boundaries (basic_block bb)
rtx_insn *debug_insn = NULL;
edge fallthru = NULL;
bool skip_purge;
+ bool seen_note_after_debug = false;
if (insn == end)
return;
@@ -492,7 +493,10 @@ find_bb_boundaries (basic_block bb)
if (code == DEBUG_INSN)
{
if (flow_transfer_insn && !debug_insn)
- debug_insn = insn;
+ {
+ debug_insn = insn;
+ seen_note_after_debug = false;
+ }
}
/* In case we've previously seen an insn that effects a control
flow transfer, split the block. */
@@ -506,7 +510,40 @@ find_bb_boundaries (basic_block bb)
insn instead of before the non-debug insn, so that the debug
insns are not lost. */
if (debug_insn && code != CODE_LABEL && code != BARRIER)
- prev = PREV_INSN (debug_insn);
+ {
+ prev = PREV_INSN (debug_insn);
+ if (seen_note_after_debug)
+ {
+ /* Though, if there are NOTEs intermixed with DEBUG_INSNs,
+ move the NOTEs before the DEBUG_INSNs and split after
+ the last NOTE. */
+ rtx_insn *first = NULL, *last = NULL;
+ for (x = debug_insn; x != insn; x = NEXT_INSN (x))
+ {
+ if (NOTE_P (x))
+ {
+ if (first == NULL)
+ first = x;
+ last = x;
+ }
+ else
+ {
+ gcc_assert (DEBUG_INSN_P (x));
+ if (first)
+ {
+ reorder_insns_nobb (first, last, prev);
+ prev = last;
+ first = last = NULL;
+ }
+ }
+ }
+ if (first)
+ {
+ reorder_insns_nobb (first, last, prev);
+ prev = last;
+ }
+ }
+ }
fallthru = split_block (bb, prev);
if (flow_transfer_insn)
{
@@ -547,6 +584,14 @@ find_bb_boundaries (basic_block bb)
flow_transfer_insn = prev_nonnote_nondebug_insn_bb (insn);
debug_insn = NULL;
}
+ else if (debug_insn)
+ {
+ if (code == NOTE)
+ seen_note_after_debug = true;
+ else
+ /* Jump tables. */
+ debug_insn = NULL;
+ }
if (control_flow_insn_p (insn))
flow_transfer_insn = insn;
@@ -0,0 +1,13 @@
+/* PR debug/106719 */
+/* { dg-do compile { target sync_char_short } } */
+/* { dg-options "-O2 -fcompare-debug" } */
+
+extern short int esi, easi[2];
+
+void
+foo (void)
+{
+ short int *psi = &easi[1];
+ __atomic_nand_fetch (psi, esi, 0);
+ psi = &easi[1];
+}