[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519 Andrew Pinski changed: What|Removed |Added CC||fchelnokov at gmail dot com --- Comment #12 from Andrew Pinski --- *** Bug 112346 has been marked as a duplicate of this bug. ***
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519 Jakub Jelinek changed: What|Removed |Added Resolution|--- |FIXED Status|NEW |RESOLVED --- Comment #11 from Jakub Jelinek --- Should be fixed now.
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519 --- Comment #10 from CVS Commits --- The releases/gcc-13 branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:16a4df27436c8f594a784028591dd3e47cabe5c0 commit r13-7944-g16a4df27436c8f594a784028591dd3e47cabe5c0 Author: Jakub Jelinek Date: Wed Oct 11 08:58:29 2023 +0200 tree-ssa-strlen: optimization skips clobbering store [PR111519] The following testcase is miscompiled, because count_nonzero_bytes incorrectly uses get_strinfo information on a pointer from which an earlier instruction loads SSA_NAME stored at the current instruction. get_strinfo shows a state right before the current store though, so if there are some stores in between the current store and the load, the string length information might have changed. The patch passes around gimple_vuse from the store and punts instead of using strinfo on loads from MEM_REF which have different gimple_vuse from that. 2023-10-11 Richard Biener Jakub Jelinek PR tree-optimization/111519 * tree-ssa-strlen.cc (strlen_pass::count_nonzero_bytes): Add vuse argument and pass it through to recursive calls and count_nonzero_bytes_addr calls. Don't shadow the stmt argument, but change stmt for gimple_assign_single_p statements for which we don't immediately punt. (strlen_pass::count_nonzero_bytes_addr): Add vuse argument and pass it through to recursive calls and count_nonzero_bytes calls. Don't use get_strinfo if gimple_vuse (stmt) is different from vuse. Don't shadow the stmt argument. * gcc.dg/torture/pr111519.c: New testcase. (cherry picked from commit e75bf1985fdc9a5d3a307882a9251d8fd6e93def)
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519 --- Comment #9 from CVS Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:e75bf1985fdc9a5d3a307882a9251d8fd6e93def commit r14-4552-ge75bf1985fdc9a5d3a307882a9251d8fd6e93def Author: Jakub Jelinek Date: Wed Oct 11 08:58:29 2023 +0200 tree-ssa-strlen: optimization skips clobbering store [PR111519] The following testcase is miscompiled, because count_nonzero_bytes incorrectly uses get_strinfo information on a pointer from which an earlier instruction loads SSA_NAME stored at the current instruction. get_strinfo shows a state right before the current store though, so if there are some stores in between the current store and the load, the string length information might have changed. The patch passes around gimple_vuse from the store and punts instead of using strinfo on loads from MEM_REF which have different gimple_vuse from that. 2023-10-11 Richard Biener Jakub Jelinek PR tree-optimization/111519 * tree-ssa-strlen.cc (strlen_pass::count_nonzero_bytes): Add vuse argument and pass it through to recursive calls and count_nonzero_bytes_addr calls. Don't shadow the stmt argument, but change stmt for gimple_assign_single_p statements for which we don't immediately punt. (strlen_pass::count_nonzero_bytes_addr): Add vuse argument and pass it through to recursive calls and count_nonzero_bytes calls. Don't use get_strinfo if gimple_vuse (stmt) is different from vuse. Don't shadow the stmt argument. * gcc.dg/torture/pr111519.c: New testcase.
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519
--- Comment #8 from Richard Biener ---
(In reply to Richard Biener from comment #6)
> Created attachment 56089 [details]
> patch
>
> This patch (against the 13 branch) fixes the bug but it might be a bit
> conservative since some entries (the _addr one for example) may not have
> a live virtual use. Maybe getting the relevant one from up the call chain
> is better.
>
> I'm going to throw this at testing but expect some testsuite fallout because
> of this conservativeness.
It causes
FAIL: gcc.dg/Wstringop-overflow-69.c (test for warnings, line 60)
FAIL: gcc.dg/Wstringop-overflow-69.c (test for warnings, line 62)
the testcase has
*(VC4*)a2 = c4; // { dg-warning "writing 4 bytes into a region of size
2" }
*(VC4*)a3 = c4; // { dg-warning "writing 4 bytes into a region of size
3" }
*(VC8*)a4 = c8; // { dg-warning "writing 8 bytes into a region of size
4" }
*(VC8*)a7 = c8; // { dg-warning "writing 8 bytes into a region of size
7" }
and the later of the two assignments from c4 and c8 fail to be diagnosed
as their load is CSEd to a definition before the earlier of the two
assignments. Which means the fix is working, not sure if we want to beef
it up with an alias walk, that definitely would require careful looking
as to what random trees we feed into this kitchen-sink routine.
I'm inclined to XFAIL the sub-test, at least on branches, and only maybe
fix it on trunk (well, not myself actually).
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519 --- Comment #7 from Richard Biener --- I'll note the problem appears to be latent for longer.
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519 --- Comment #6 from Richard Biener --- Created attachment 56089 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56089=edit patch This patch (against the 13 branch) fixes the bug but it might be a bit conservative since some entries (the _addr one for example) may not have a live virtual use. Maybe getting the relevant one from up the call chain is better. I'm going to throw this at testing but expect some testsuite fallout because of this conservativeness.
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519
--- Comment #5 from Richard Biener ---
Reverting
@@ -5089,8 +5123,9 @@ strlen_pass::handle_store (bool *zero_write)
return false;
}
- if (storing_all_zeros_p
- || storing_nonzero_p
+ if (storing_nonzero_p
+ || storing_all_zeros_p
+ || (full_string_p && lenrange[1] == 0)
|| (offset != 0 && store_before_nul[1] > 0))
{
/* When STORING_NONZERO_P, we know that the string will start
fixes the issue. We somehow compute a lenrange of { 0, 0, 1 } for
d = _8 but also set storing_all_zeros_p to false.
It seems that strlen_pass::count_nonzero_bytes happily skips intermediate
stores when it handles SSA names here:
gimple *stmt = SSA_NAME_DEF_STMT (exp);
if (gimple_assign_single_p (stmt))
{
exp = gimple_assign_rhs1 (stmt);
if (!DECL_P (exp)
&& TREE_CODE (exp) != CONSTRUCTOR
&& TREE_CODE (exp) != MEM_REF)
return false;
/* Handle DECLs, CONSTRUCTOR and MEM_REF below. */
we have
_8 = *_7;
...
*_7 = prephitmp_89;
d = _8;
when we recursively process a memory reference we need to verify there
are no intermediate aliases, the simplest thing would be to pass down
a virtual use. Trying some prototype.
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519
--- Comment #4 from Jakub Jelinek ---
Slightly cleaned up testcase:
int a, o;
char b, f, i;
long c;
static signed char d;
static char g;
unsigned *h;
signed char *e =
static signed char **j =
static long k[2];
unsigned **l =
short m;
volatile int z;
__attribute__((noipa)) void
foo (char *p)
{
(void) p;
}
int
main ()
{
int p = z;
signed char *n =
*n = 0;
while (c)
for (; i; i--)
;
for (g = 0; g <= 1; g++)
{
*n = **j;
k[g] = 0 !=
*e = l && k[0];
}
if (p)
foo ();
for (; o < 4; o++)
{
a = d;
if (p)
foo ();
}
if (a != 1)
__builtin_abort ();
}
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519
Richard Biener changed:
What|Removed |Added
CC||jakub at gcc dot gnu.org
--- Comment #3 from Richard Biener ---
g = 0;
for (; g <= 1; g++) {
*n = **j;
k[g] = 0 !=
*e = l && k[0];
}
this is
g = 0;
d = f; // 0
k[0] = 1;
f = 1;
g = 1;
d = f; // 1
k[1] = 1;
f = 1;
g = 2;
that looks still equivalent to what we have after unrolling this loop in
cunroll
and also after DOM3 which really points to strlen wrongly considering 'd' zero.
Before strlen we have
[local count: 1605787]:
_2 = e;
_3 = *_2;
k[0] = 1;
l.8_91 = l;
if (l.8_91 != 0B)
goto ; [70.00%]
else
goto ; [30.00%]
[local count: 1124051]:
[local count: 1605787]:
# prephitmp_82 = PHI <0(7), 1(8)>
*_2 = prephitmp_82;
_7 = e;
_8 = *_7;
k[1] = 1;
l.8_10 = l;
if (l.8_10 != 0B)
goto ; [70.00%]
else
goto ; [30.00%]
[local count: 1124051]:
[local count: 14598063]:
# prephitmp_89 = PHI <1(10), 0(9)>
*_7 = prephitmp_89;
d = _8;
g = 2;
if (q_32(D) == 0)
goto ; [33.00%]
else
goto ; [67.00%]
that's the OK part, now into the tail loop - q_32(D) is 1:
[local count: 9780702]:
o.16_80 = o;
if (o.16_80 <= 3)
goto ; [89.00%]
else
goto ; [11.00%]
[local count: 8704825]:
d_lsm0.28_35 = d;
[local count: 79134774]:
# prephitmp_6 = PHI
_96 = (int) d_lsm0.28_35;
_85 = prephitmp_6 + 1;
if (_85 != 4)
goto ; [89.00%]
else
goto ; [11.00%]
[local count: 8704825]:
a = _96;
o = 4;
[local count: 14598063]:
a.17_23 = a;
printf ("%d\n", a.17_23);
return 0;
and the strlen pass replaces d_lsm0.28_35 = d; with d_lsm0.28_35 = 0; which
is wrong.
Your assessment
"Here the assignment to *_73 overwrites the value of f (at *e) which then
invalidates the use of _72 resulting in the wrong value for d."
seems odd, it's exactly writing the correct value (in fact both stores
write the value one, only the very original value is zero).
I don't know the strlen pass at all so I can't tell where it goes wrong.
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519 --- Comment #2 from Roger Sayle --- Complicated. Things have gone wrong before the strlen pass which is given: _73 = e; _72 = *_73; ... *_73 = prephitmp_23; d = _72; Here the assignment to *_73 overwrites the value of f (at *e) which then invalidates the use of _72 resulting in the wrong value for d. But figuring out which pass is at fault (perhaps complete loop unrolling?) is tricky.
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519 Richard Biener changed: What|Removed |Added CC||rguenth at gcc dot gnu.org Priority|P3 |P2
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519 Andrew Pinski changed: What|Removed |Added Status|UNCONFIRMED |NEW Ever confirmed|0 |1 Last reconfirmed||2023-09-21 --- Comment #1 from Andrew Pinski --- Confirmed.
[Bug tree-optimization/111519] [13/14 Regression] Wrong code at -O3 on x86_64-linux-gnu since r13-455-g1fe04c497d
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111519 Andrew Pinski changed: What|Removed |Added Target Milestone|--- |13.3 Keywords||wrong-code
