https://gcc.gnu.org/bugzilla/show_bug.cgi?id=127596
Richard Biener <rguenth at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |pinskia at gcc dot gnu.org,
| |rguenth at gcc dot gnu.org
--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Hmm, points to me via r15-99-gbbe83599320288. I will have a look, but only if
nobody beats me to it. Andrea, you were here "latest" ...
✦ Root-Cause Analysis
1. Failure Manifestation
When compiling obj/gcc/t.c with -O3 -fno-thread-jumps -ftracer
-fno-tree-forwprop:
- In main(), t(0, 2146435071) was inlined with constant arguments.
- During early and tree optimization passes, branches inside t were pruned
because the arguments were constants.
- However, because -fno-tree-forwprop disabled forward propagation and
-ftracer duplicated paths, a degenerate single-argument PHI node survived
in main() until RTL expansion in basic block 4:
1 <bb 3>:
2 if (l.5_13 == 0)
3 goto <bb 4>;
4 else
5 goto <bb 3>;
6
7 <bb 4>:
8 # ak_14 = PHI <&x(3)>
9 ag:
10 *ak_14 = &v;
11 pretmp_53 = x;
12 ...
13 v.16_28 = v;
14 ...
15 _59 = *pretmp_53;
- In the expand pass (dumps/t.c.273r.expand), stack partition assignment
collapsed local variables x (int *, size 8) and v (int, size 4) into the
same stack partition:
1 Partition 1: size 8 align 8
2 x v
- Because x and v shared the exact same stack offset (virtual-stack-vars -
8):
1. v = -244; stored -244 into the slot.
2. *ak_14 = &v; (which is *(&x) = &v;) stored the address &v into x—which
was the same slot—thereby corrupting v by overwriting -244 with a stack
pointer.
3. Later reads of v read the pointer value instead of -244, and
dereferencing *x read invalid data, causing main() to compute aj =
885581325 instead of -965382002 and aborting.
2. Root Cause in gcc/cfgexpand.cc
In add_scope_conflicts_1:
1. Variable liveness and stack conflicts are tracked across basic blocks.
2. At the entry to each basic block:
1 /* Addresses coming into the bb via phis are alive at the entry point.
*/
2 for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
3 add_scope_conflicts_2 (cache, gimple_phi_result (gsi_stmt (gsi)),
work, visit_op);
Stack variables referenced in PHI results are added to work using visit_op
(which merely sets the bit in work without recording conflicts).
3. Commit bbe83599320 (middle-end/114579 - speed up add_scope_conflicts)
optimized conflict recording by only running the all-to-all quadratic
conflict recording when EDGE_COUNT (bb->preds) > 1:
1 if (EDGE_COUNT (bb->preds) > 1)
2 EXECUTE_IF_SET_IN_BITMAP (work, 0, i, bi)
3 {
4 class stack_var *a = &stack_vars[i];
5 ...
6 bitmap_ior_into (a->conflicts, work);
7 }
The underlying assumption stated in the comment was:
> "When there's just a single predecessor the set of conflicts is already
up-to-date."
4. Flaw: This assumption does not hold when a single-predecessor basic block
has PHI nodes (degenerate PHIs, such as # ak_14 = PHI <&x(3)>). The PHI
node introduces variable x into work at the block entry, while v was
inherited from the predecessor block. Because EDGE_COUNT (bb->preds) ==
1,
the all-to-all conflict loop was skipped.
5. In subsequent statements in basic block 4, any use of x invoked
visit_conflict (x). However, visit_conflict only adds conflicts if
bitmap_set_bit (work, x) returns true. Since x was already inserted into
work by visit_op in the PHI loop, bitmap_set_bit returned false.
Consequently, a conflict between v and x was never recorded, allowing
them
to be merged into the same stack partition.
---
Fix
In gcc/cfgexpand.cc (add_scope_conflicts_1), run the all-to-all conflict
recording between variables active at block entry if either:
- The block has more than one predecessor (EDGE_COUNT (bb->preds) > 1), or
- The block has PHI nodes (!gimple_seq_empty_p (phi_nodes (bb))).
1 diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
2 index 4bd311b11a3..0ab6809659b 100644
3 --- a/gcc/cfgexpand.cc
4 +++ b/gcc/cfgexpand.cc
5 @@ -901,18 +901,20 @@ add_scope_conflicts_1 (vars_ssa_cache &cache,
basic_block bb, bitmap work, bool
6 if (for_conflict && !had_non_clobbers)
7 {
8 /* When we are inheriting live variables from our predecessors
9 - through a CFG merge we might not see an actual mention of
10 - the variables to record the appropriate conflict as defs/uses
11 - might be through indirect stores/loads. For this reason
12 - we have to make sure each live variable conflicts with
13 - each other. When there's just a single predecessor the
14 - set of conflicts is already up-to-date.
15 + through a CFG merge or through PHI nodes we might not see
16 + an actual mention of the variables to record the appropriate
17 + conflict as defs/uses might be through indirect stores/loads.
18 + For this reason we have to make sure each live variable
19 + conflicts with each other. When there's just a single
20 + predecessor and no PHI nodes the set of conflicts is already
21 + up-to-date.
22 We perform this delayed at the first real instruction to
23 allow clobbers starting this block to remove variables from
24 the set of live variables. */
25 bitmap_iterator bi;
26 unsigned i;
27 - if (EDGE_COUNT (bb->preds) > 1)
28 + if (EDGE_COUNT (bb->preds) > 1
29 + || !gimple_seq_empty_p (phi_nodes (bb)))
30 EXECUTE_IF_SET_IN_BITMAP (work, 0, i, bi)
31 {
32 class stack_var *a = &stack_vars[i];
33 @@ -928,9 +930,11 @@ add_scope_conflicts_1 (vars_ssa_cache &cache,
basic_block bb, bitmap work, bool
34 }
35 }
36
37 - /* When there was no real instruction but there's a CFG merge we need
38 - to add the conflicts now. */
39 - if (for_conflict && !had_non_clobbers && EDGE_COUNT (bb->preds) > 1)
40 + /* When there was no real instruction but there's a CFG merge or PHI
41 + nodes we need to add the conflicts now. */
42 + if (for_conflict && !had_non_clobbers
43 + && (EDGE_COUNT (bb->preds) > 1
44 + || !gimple_seq_empty_p (phi_nodes (bb))))
45 {
46 bitmap_iterator bi;
47 unsigned i;
