[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561 Richard Biener changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #26 from Richard Biener --- Fixed.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561 --- Comment #25 from CVS Commits --- The master branch has been updated by Richard Biener : https://gcc.gnu.org/g:04b0a7b1a6d9e0f3782888f1ebf187c26690038b commit r13-6943-g04b0a7b1a6d9e0f3782888f1ebf187c26690038b Author: Richard Biener Date: Wed Mar 29 13:49:24 2023 +0200 tree-optimization/107561 - reduce -Wstringop-overflow false positives The following tells pointer-query to prefer a zero size when we are querying for the size range for a write into an object we've determined is of zero size. That avoids diagnostics about really varying size arguments that just get a meaningful range for example because they are multiplied by an element size. I've adjusted only one call to get_size_range since that's what I have a testcase for. PR tree-optimization/107561 * gimple-ssa-warn-access.cc (get_size_range): Add flags argument and pass it on. (check_access): When querying for the size range pass SR_ALLOW_ZERO when the known destination size is zero. * g++.dg/pr71488.C: Remove XFAILed bogus diagnostic again. * g++.dg/warn/Warray-bounds-16.C: Likewise.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561 --- Comment #24 from Richard Biener --- Created attachment 54784 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54784=edit another hack
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
Richard Biener changed:
What|Removed |Added
CC||hubicka at gcc dot gnu.org,
||rguenth at gcc dot gnu.org
Priority|P1 |P3
--- Comment #23 from Richard Biener ---
So we can "mitigate" the diagnostic for g++.dg/pr17488.C with a hack, but for
g++.dg/warn/Warray-bounds-16.C we see
[local count: 1073741824]:
a ={v} {CLOBBER};
a.m = 0;
_5 = operator new [] (0);
a.p = _5;
_2 = a.m;
if (_2 > 0)
goto ; [89.00%]
else
goto ; [11.00%]
[local count: 955630225]:
_12 = (sizetype) _2;
_11 = _12 * 4;
__builtin_memset (_5, 0, _11); [tail call]
where we'd have a clear range (_2 > 0) even without the multiplication but
we're only now picking that up. The bug here is quite the same
missed optimization though, we fail to CSE a.m around the 'operator new [] (0)'
call and so obviously dead code remains.
C++ is simply an awful language to work with here. A static analyzer would
maybe simply look past possibly clobbering calls deriving the code is
likely dead and refrain from diagnosing it.
Note while for g++.dg/warn/Warray-bounds-16.C we are again working inside a
CTOR the issue extends to any code with intermediate allocations via
new or delete expressions. Yes, we can add some flag like -fnew-is-not-stupid,
but then we couldn't make it the default. Maybe(?) we can somehow detect
whether we are dealing with overloaded global new/delete with LTO, like
detecting we're resolving it to the copy in libstdc++? The resolution info
just tells us RESOLVED_DYN though, maybe we can add something like
RESOLVED_STDLIB_DYN and handle a set of known libraries specially?
I'm putting this back to P3, we do have a load more (late) diagnostic
regressions in GCC 13.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561 --- Comment #22 from Richard Biener --- (In reply to Jonathan Wakely from comment #20) > (In reply to Jakub Jelinek from comment #16) > > (In reply to Richard Biener from comment #15) > > > where if I understand you correctly, bar () is not allowed to modify *this > > > (unless I pass it an argument to it, of course), even if *this is for > > > example > > > > Why? Because it is a constructor and the object isn't fully constructed yet > > at that point? > > Yes, exactly. The object's lifetime has not started until the constructor > completes, so accessing it is only allowed in very limited ways, described > in [basic.life] p6. However, it looks like for a non-trivial constructor the > results are just unspecified, not undefined, see [class.cdtor] p2. Still, I > don't see how operator new could meaningfully do anything to an object under > construction if the object is in an unspecified state. And frankly, if > anybody did write an operator new like that, they deserve what they get. > > Could we have a flag that says "assume operator new is not stupid"? We certainly could add that and with that revert the effect of not making new/delete "pure" (I'd have to look up the PR we've reverted the change that generally made it so).
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
--- Comment #21 from Richard Biener ---
(In reply to Jakub Jelinek from comment #19)
> And on
> void bar (void);
> struct X {
> X (int);
> int i;
> int j;
> void baz (int);
> };
>
> X::X(int k)
> {
> i = k;
> bar ();
> j = i != k;
> }
>
> void
> X::baz(int k)
> {
> i = k;
> bar ();
> j = i != k;
> }
> while I see
> # PT = { D.2822 } (nonlocal, restrict)
> struct X * const this_5(D) = this;
> later on in the dumps (not really sure what exactly causes it), in baz it is
> not there:
> # PT = nonlocal
> struct X * const this_5(D) = this;
We have
static void
intra_create_variable_infos (struct function *fn)
{
tree t;
bitmap handled_struct_type = NULL;
bool this_parm_in_ctor = DECL_CXX_CONSTRUCTOR_P (fn->decl);
...
varinfo_t p
= create_variable_info_for_1 (t, alias_get_name (t), false, true,
handled_struct_type, this_parm_in_ctor);
and 'this_parm_in_ctor' makes the pointer as if it were restrict qualified.
Not sure why we chose that route as compared to actually making it restrict
(I think we did that at some point but it caused issues?).
As said the real issue is that I didn't implement effects of restrict
qualification on calls. I'll see to give it a try for next stage1.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561 --- Comment #20 from Jonathan Wakely --- (In reply to Jakub Jelinek from comment #16) > (In reply to Richard Biener from comment #15) > > where if I understand you correctly, bar () is not allowed to modify *this > > (unless I pass it an argument to it, of course), even if *this is for > > example > > Why? Because it is a constructor and the object isn't fully constructed yet > at that point? Yes, exactly. The object's lifetime has not started until the constructor completes, so accessing it is only allowed in very limited ways, described in [basic.life] p6. However, it looks like for a non-trivial constructor the results are just unspecified, not undefined, see [class.cdtor] p2. Still, I don't see how operator new could meaningfully do anything to an object under construction if the object is in an unspecified state. And frankly, if anybody did write an operator new like that, they deserve what they get. Could we have a flag that says "assume operator new is not stupid"?
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
--- Comment #19 from Jakub Jelinek ---
And on
void bar (void);
struct X {
X (int);
int i;
int j;
void baz (int);
};
X::X(int k)
{
i = k;
bar ();
j = i != k;
}
void
X::baz(int k)
{
i = k;
bar ();
j = i != k;
}
while I see
# PT = { D.2822 } (nonlocal, restrict)
struct X * const this_5(D) = this;
later on in the dumps (not really sure what exactly causes it), in baz it is
not there:
# PT = nonlocal
struct X * const this_5(D) = this;
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
--- Comment #18 from Jakub Jelinek ---
Does the FE really do that?
I certainly don't see it in the gimple dump:
void X::X (struct X * const this, int k)
{
*this = {CLOBBER};
{
this->i = k;
# USE = anything
# CLB = anything
bar ();
_1 = this->i;
_2 = k != _1;
_3 = (int) _2;
this->j = _3;
}
}
While if I compile the C variant after fixing it up:
struct X { int i, j; };
void bar (void);
void foo (struct X * restrict this, int k)
{
this->i = k;
bar ();
this->j = this->i != k;
}
it is there:
void foo (struct X * restrict this, int k)
{
this->i = k;
bar ();
_1 = this->i;
_2 = k != _1;
_3 = (int) _2;
this->j = _3;
}
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
--- Comment #17 from Richard Biener ---
(In reply to Jakub Jelinek from comment #16)
> (In reply to Richard Biener from comment #15)
> > The compiler doesn't know that the allocation function cannot clobber *this.
> > The C++ frontend tries to communicate this by making 'this' restrict
> > qualified
> > and we make use of that info, but for calls we do not know how to use the
> > info.
> >
> > Maybe we can special-case directly the actual parameter case and compute
> > the restrictness info for the call arguments. The canonical example is
> >
> > void bar (void);
> > struct X {
> > X (int);
> > int i;
> > int j;
> > };
> >
> > X::X(int k)
> > {
> > i = k;
> > bar ();
> > j = i != k;
> > }
> >
> > where if I understand you correctly, bar () is not allowed to modify *this
> > (unless I pass it an argument to it, of course), even if *this is for
> > example
>
> Why? Because it is a constructor and the object isn't fully constructed yet
> at that point? For normal methods I certainly don't see anything that would
> preclude such modifications.
The canonical C example would be
void bar (void);
void foo (struct X * restrict this, int k)
{
this->i = k;
bar ();
this->j = i != k;
}
where as I understand bar () cannot modify what *this points to since it
cannot build a proper derived access from 'this' (unless I pass it to bar).
The C++ frontend annotates 'this' with restrict. For my example I get
void X::X (struct X * const this, int k)
{
# PT = { D.2806 } (nonlocal, restrict)
struct X * const this_5(D) = this;
int k_7(D) = k;
int _1;
bool _2;
int _3;
:
MEM[(struct X *)this_5(D) clique 1 base 1] ={v} {CLOBBER};
MEM[(struct X *)this_5(D) clique 1 base 1].i = k_7(D);
# USE = nonlocal escaped
# CLB = nonlocal escaped
bar ();
_1 = MEM[(struct X *)this_5(D) clique 1 base 1].i;
_2 = _1 != k_7(D);
# RANGE [irange] int [0, 1] NONZERO 0x1
_3 = (int) _2;
MEM[(struct X *)this_5(D) clique 1 base 1].j = _3;
return;
}
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
Jakub Jelinek changed:
What|Removed |Added
CC||jakub at gcc dot gnu.org
--- Comment #16 from Jakub Jelinek ---
(In reply to Richard Biener from comment #15)
> The compiler doesn't know that the allocation function cannot clobber *this.
> The C++ frontend tries to communicate this by making 'this' restrict
> qualified
> and we make use of that info, but for calls we do not know how to use the
> info.
>
> Maybe we can special-case directly the actual parameter case and compute
> the restrictness info for the call arguments. The canonical example is
>
> void bar (void);
> struct X {
> X (int);
> int i;
> int j;
> };
>
> X::X(int k)
> {
> i = k;
> bar ();
> j = i != k;
> }
>
> where if I understand you correctly, bar () is not allowed to modify *this
> (unless I pass it an argument to it, of course), even if *this is for
> example
Why? Because it is a constructor and the object isn't fully constructed yet at
that point? For normal methods I certainly don't see anything that would
preclude such modifications.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
--- Comment #15 from Richard Biener ---
(In reply to Jonathan Wakely from comment #13)
> (In reply to Richard Biener from comment #11)
> > We can again work around this in libstdc++ by CSEing ->_M_size ourselves.
> > The following helps:
> >
> > diff --git a/libstdc++-v3/include/std/valarray
> > b/libstdc++-v3/include/std/valarray
> > index 7a23c27a0ce..7383071f98d 100644
> > --- a/libstdc++-v3/include/std/valarray
> > +++ b/libstdc++-v3/include/std/valarray
> > @@ -647,8 +647,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > inline
> > valarray<_Tp>::valarray(const valarray<_Tp>& __v)
> > : _M_size(__v._M_size),
> > _M_data(__valarray_get_storage<_Tp>(__v._M_size))
> > -{ std::__valarray_copy_construct(__v._M_data, __v._M_data + _M_size,
> > -_M_data); }
> > +{
> > + auto __v_M_size = __v._M_size;
> > + _M_size = __v_M_size;
> > + _M_data = __valarray_get_storage<_Tp>(__v_M_size);
> > + std::__valarray_copy_construct(__v._M_data, __v._M_data + __v_M_size,
> > +_M_data);
> > +}
> >
> > #if __cplusplus >= 201103L
> >template
>
> Ugh, gross.
>
> This makes no sense to me. this->_M_size is already a local copy of
> __v._M_size that cannot have escaped, because its enclosing object hasn't
> been constructed yet. Why do we need another "more local" copy of it?
>
> _M_size is a copy of __v._M_size, which is passed to the get_storage
> function. The compiler thinks that the get_storage call might modify __v,
> but it can't modify this->_M_size. So then _M_size still has the same value
> when passed to the copy_construct call.
>
>
> Since it would be undefined for users to modify this->_M_size or __v._M_size
> from operator new (because they cannot access an object under construction,
> and cannot modify an object while it's in the process of being copied), I
> wish we could say that a specific call to operator new does not modify
> anything reachable from the enclosing function's arguments, including `this`.
>
> Or maybe we just teach the compiler that operator new will not touch
> anything defined in namespace std, on pain of death.
The compiler doesn't know that the allocation function cannot clobber *this.
The C++ frontend tries to communicate this by making 'this' restrict qualified
and we make use of that info, but for calls we do not know how to use the
info.
Maybe we can special-case directly the actual parameter case and compute
the restrictness info for the call arguments. The canonical example is
void bar (void);
struct X {
X (int);
int i;
int j;
};
X::X(int k)
{
i = k;
bar ();
j = i != k;
}
where if I understand you correctly, bar () is not allowed to modify *this
(unless I pass it an argument to it, of course), even if *this is for
example
char *storage;
void bar ()
{
((X *)storage)->i = 0; // the cast is invalid, no object of type X yet there?
}
int main()
{
storage = new char[8];
new (storage) X (1);
}
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
--- Comment #14 from Aldy Hernandez ---
(In reply to Richard Biener from comment #11)
> So I've missed the VR_ANTI_RANGE handling in get_size_range where we run into
>
> wide_int maxsize = wi::to_wide (max_object_size ());
> min = wide_int::from (min, maxsize.get_precision (), UNSIGNED);
> max = wide_int::from (max, maxsize.get_precision (), UNSIGNED);
> if (wi::eq_p (0, min - 1))
> {
> /* EXP is unsigned and not in the range [1, MAX]. That means
> it's either zero or greater than MAX. Even though 0 would
> normally be detected by -Walloc-zero, unless ALLOW_ZERO
> is set, set the range to [MAX, TYPE_MAX] so that when MAX
> is greater than the limit the whole range is diagnosed. */
> wide_int maxsize = wi::to_wide (max_object_size ());
> if (flags & SR_ALLOW_ZERO)
> {
> if (wi::leu_p (maxsize, max + 1)
> || !(flags & SR_USE_LARGEST))
> min = max = wi::zero (expprec);
> else
> {
> min = max + 1;
> max = wi::to_wide (TYPE_MAX_VALUE (exptype));
> }
> }
> else
> {
> min = max + 1;
> max = wi::to_wide (TYPE_MAX_VALUE (exptype));
> }
>
> and from [0,0] [8, +INF] pick [8, +INF] based on the comments reasoning.
Ughh, you're reaching all the problematic cases I ran into while trying to
remove legacy.
>
> This all wouldn't happen if we'd be able to CSE the zero size ...
>
> We can now try to put additional heuristic ontop of the above heuristic,
> namely when the object we write to is of size zero set SR_ALLOW_ZERO.
> Or try to "undo" the multiplication trick which would probably make us
> end up with VARYING.
>
> I'll note that with the earlier proposed change we regress the following,
> that's an observation I make a lot of times - all "weirdness" in the code
> is backed by (artificial) testcases verifying it works exactly as coded ...
+1
>
> FAIL: gcc.dg/Wstringop-overflow-15.c pr82608 (test for warnings, line 37)
> FAIL: gcc.dg/Wstringop-overflow-15.c pr82608 (test for warnings, line 38)
> FAIL: gcc.dg/Wstringop-overflow-15.c pr82608 (test for warnings, line 69)
> FAIL: gcc.dg/Wstringop-overflow-15.c pr82608 (test for warnings, line 70)
> FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 64)
> FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 75)
> FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 86)
> FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 97)
> FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 108)
> FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 148)
> FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 159)
> FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 52)
> FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 53)
> FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 54)
> FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 55)
> FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 56)
> FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 57)
> FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 58)
> FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 64)
> FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 65)
> FAIL: gcc.dg/attr-alloc_size-3.c (test for warnings, line 438)
> FAIL: gcc.dg/attr-alloc_size-4.c (test for warnings, line 138)
> FAIL: gcc.dg/attr-alloc_size-4.c (test for warnings, line 143)
> FAIL: gcc.dg/attr-alloc_size-4.c (test for warnings, line 187)
> FAIL: gcc.dg/pr98721-1.c (test for warnings, line 11)
> FAIL: gcc.dg/pr98721-1.c (test for warnings, line 12)
>
> For example gcc.dg/pr98721-1.c has
>
> int
> foo (int n)
> {
> if (n <= 0)
> {
> char vla[n]; /* { dg-message "source object 'vla'
> of size 0" } */
> return __builtin_strlen (vla);/* { dg-warning "'__builtin_strlen'
> reading 1 or more bytes from a region of size 0" } */
>
> but of course we do not diagnose
>
> int
> foo (int n)
> {
> if (n < 0)
> {
> char vla[n];
>
> or when no condition is present or a n > 32 condition is present.
Yup, ran into that too.
>
> I fear it's not possible to "fix" this testcase without changing the
> expectation on a bunch of other testcases. But the messaging to the
> user is quite unhelpful because it doesn't actually inform him about
> above reasoning.
Agreed.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
--- Comment #13 from Jonathan Wakely ---
(In reply to Richard Biener from comment #11)
> We can again work around this in libstdc++ by CSEing ->_M_size ourselves.
> The following helps:
>
> diff --git a/libstdc++-v3/include/std/valarray
> b/libstdc++-v3/include/std/valarray
> index 7a23c27a0ce..7383071f98d 100644
> --- a/libstdc++-v3/include/std/valarray
> +++ b/libstdc++-v3/include/std/valarray
> @@ -647,8 +647,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> inline
> valarray<_Tp>::valarray(const valarray<_Tp>& __v)
> : _M_size(__v._M_size),
> _M_data(__valarray_get_storage<_Tp>(__v._M_size))
> -{ std::__valarray_copy_construct(__v._M_data, __v._M_data + _M_size,
> -_M_data); }
> +{
> + auto __v_M_size = __v._M_size;
> + _M_size = __v_M_size;
> + _M_data = __valarray_get_storage<_Tp>(__v_M_size);
> + std::__valarray_copy_construct(__v._M_data, __v._M_data + __v_M_size,
> +_M_data);
> +}
>
> #if __cplusplus >= 201103L
>template
Ugh, gross.
This makes no sense to me. this->_M_size is already a local copy of __v._M_size
that cannot have escaped, because its enclosing object hasn't been constructed
yet. Why do we need another "more local" copy of it?
_M_size is a copy of __v._M_size, which is passed to the get_storage function.
The compiler thinks that the get_storage call might modify __v, but it can't
modify this->_M_size. So then _M_size still has the same value when passed to
the copy_construct call.
Since it would be undefined for users to modify this->_M_size or __v._M_size
from operator new (because they cannot access an object under construction, and
cannot modify an object while it's in the process of being copied), I wish we
could say that a specific call to operator new does not modify anything
reachable from the enclosing function's arguments, including `this`.
Or maybe we just teach the compiler that operator new will not touch anything
defined in namespace std, on pain of death.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
--- Comment #12 from Aldy Hernandez ---
(In reply to Richard Biener from comment #10)
> (In reply to Richard Biener from comment #9)
> > Note I think there's still a bug in value_range (irange) here.
> > get_size_range
> > does
> >
> > if (integral)
> > {
> > value_range vr;
> >
> > query->range_of_expr (vr, exp, stmt);
> >
> > if (vr.undefined_p ())
> > vr.set_varying (TREE_TYPE (exp));
> > range_type = vr.kind ();
> > min = wi::to_wide (vr.min ());
> > max = wi::to_wide (vr.max ());
> >
> > and we have vr:
> >
> > (gdb) p vr
> > $13 = { = { = {
> > _vptr.vrange = 0x3693a30 +16>,
> > m_kind = VR_ANTI_RANGE, m_discriminator = VR_IRANGE},
> > m_num_ranges = 1 '\001', m_max_ranges = 1 '\001',
> > m_nonzero_mask = , m_base = 0x7fffc8f0}, m_ranges = {
> > , }}
> > (gdb) p vr.dump (stderr)
> > [irange] unsigned int [0, 0][8, +INF]$17 = void
> >
> > but vr.min () produces 1 and vr.max () produces 7, just as if it doesn't
> > interpret VR_ANTI_RANGE transparently here (if that's the intent?!).
> > At least
> >
> > // Return the highest bound of a range expressed as a tree.
> >
> > inline tree
> > irange::tree_upper_bound () const
> >
> > suggests that. Note that vr.num_pairs () produces 2 (because constant_p ())
> > but vr.m_num_ranges is 1 and tree_upper_bound uses m_num_ranges.
> >
> > I suppose irange::{min,max,tree_lower_bound,tree_upper_bound} miss "support"
> > for legacy_mode_p here.
>
> OTOH gimple-array-bounds.cc does
>
> const value_range *vr = NULL;
> if (TREE_CODE (low_sub_org) == SSA_NAME)
> {
> vr = get_value_range (low_sub_org, stmt);
> if (!vr->undefined_p () && !vr->varying_p ())
> {
> low_sub = vr->kind () == VR_RANGE ? vr->max () : vr->min ();
> up_sub = vr->kind () == VR_RANGE ? vr->min () : vr->max ();
> }
>
> so the bug is a documentation bug on min/max/lower/upper bound?!
>
> I'm policeing other uses of value_range right now.
Haven't looked at this yet, but min/max/lower/upper bound are broken and
inconsistent. They give different results in legacy and the new irange API.
This was not by intent, but it crept in :/. My fault. I noticed this when
removing the legacy code for next stage1.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
Richard Biener changed:
What|Removed |Added
CC||jwakely.gcc at gmail dot com
Keywords|testsuite-fail |missed-optimization
--- Comment #11 from Richard Biener ---
So I've missed the VR_ANTI_RANGE handling in get_size_range where we run into
wide_int maxsize = wi::to_wide (max_object_size ());
min = wide_int::from (min, maxsize.get_precision (), UNSIGNED);
max = wide_int::from (max, maxsize.get_precision (), UNSIGNED);
if (wi::eq_p (0, min - 1))
{
/* EXP is unsigned and not in the range [1, MAX]. That means
it's either zero or greater than MAX. Even though 0 would
normally be detected by -Walloc-zero, unless ALLOW_ZERO
is set, set the range to [MAX, TYPE_MAX] so that when MAX
is greater than the limit the whole range is diagnosed. */
wide_int maxsize = wi::to_wide (max_object_size ());
if (flags & SR_ALLOW_ZERO)
{
if (wi::leu_p (maxsize, max + 1)
|| !(flags & SR_USE_LARGEST))
min = max = wi::zero (expprec);
else
{
min = max + 1;
max = wi::to_wide (TYPE_MAX_VALUE (exptype));
}
}
else
{
min = max + 1;
max = wi::to_wide (TYPE_MAX_VALUE (exptype));
}
and from [0,0] [8, +INF] pick [8, +INF] based on the comments reasoning.
This all wouldn't happen if we'd be able to CSE the zero size ...
We can now try to put additional heuristic ontop of the above heuristic,
namely when the object we write to is of size zero set SR_ALLOW_ZERO.
Or try to "undo" the multiplication trick which would probably make us
end up with VARYING.
I'll note that with the earlier proposed change we regress the following,
that's an observation I make a lot of times - all "weirdness" in the code
is backed by (artificial) testcases verifying it works exactly as coded ...
FAIL: gcc.dg/Wstringop-overflow-15.c pr82608 (test for warnings, line 37)
FAIL: gcc.dg/Wstringop-overflow-15.c pr82608 (test for warnings, line 38)
FAIL: gcc.dg/Wstringop-overflow-15.c pr82608 (test for warnings, line 69)
FAIL: gcc.dg/Wstringop-overflow-15.c pr82608 (test for warnings, line 70)
FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 64)
FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 75)
FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 86)
FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 97)
FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 108)
FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 148)
FAIL: gcc.dg/Wstringop-overflow-56.c (test for warnings, line 159)
FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 52)
FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 53)
FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 54)
FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 55)
FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 56)
FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 57)
FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 58)
FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 64)
FAIL: gcc.dg/attr-alloc_size-11.c (test for warnings, line 65)
FAIL: gcc.dg/attr-alloc_size-3.c (test for warnings, line 438)
FAIL: gcc.dg/attr-alloc_size-4.c (test for warnings, line 138)
FAIL: gcc.dg/attr-alloc_size-4.c (test for warnings, line 143)
FAIL: gcc.dg/attr-alloc_size-4.c (test for warnings, line 187)
FAIL: gcc.dg/pr98721-1.c (test for warnings, line 11)
FAIL: gcc.dg/pr98721-1.c (test for warnings, line 12)
For example gcc.dg/pr98721-1.c has
int
foo (int n)
{
if (n <= 0)
{
char vla[n]; /* { dg-message "source object 'vla' of
size 0" } */
return __builtin_strlen (vla);/* { dg-warning "'__builtin_strlen'
reading 1 or more bytes from a region of size 0" } */
but of course we do not diagnose
int
foo (int n)
{
if (n < 0)
{
char vla[n];
or when no condition is present or a n > 32 condition is present.
I fear it's not possible to "fix" this testcase without changing the
expectation on a bunch of other testcases. But the messaging to the
user is quite unhelpful because it doesn't actually inform him about
above reasoning.
That we are not allowed to optimize the code is not of help either.
We can again work around this in libstdc++ by CSEing ->_M_size ourselves.
The following
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
--- Comment #10 from Richard Biener ---
(In reply to Richard Biener from comment #9)
> Note I think there's still a bug in value_range (irange) here.
> get_size_range
> does
>
> if (integral)
> {
> value_range vr;
>
> query->range_of_expr (vr, exp, stmt);
>
> if (vr.undefined_p ())
> vr.set_varying (TREE_TYPE (exp));
> range_type = vr.kind ();
> min = wi::to_wide (vr.min ());
> max = wi::to_wide (vr.max ());
>
> and we have vr:
>
> (gdb) p vr
> $13 = { = { = {
> _vptr.vrange = 0x3693a30 +16>,
> m_kind = VR_ANTI_RANGE, m_discriminator = VR_IRANGE},
> m_num_ranges = 1 '\001', m_max_ranges = 1 '\001',
> m_nonzero_mask = , m_base = 0x7fffc8f0}, m_ranges = {
> , }}
> (gdb) p vr.dump (stderr)
> [irange] unsigned int [0, 0][8, +INF]$17 = void
>
> but vr.min () produces 1 and vr.max () produces 7, just as if it doesn't
> interpret VR_ANTI_RANGE transparently here (if that's the intent?!).
> At least
>
> // Return the highest bound of a range expressed as a tree.
>
> inline tree
> irange::tree_upper_bound () const
>
> suggests that. Note that vr.num_pairs () produces 2 (because constant_p ())
> but vr.m_num_ranges is 1 and tree_upper_bound uses m_num_ranges.
>
> I suppose irange::{min,max,tree_lower_bound,tree_upper_bound} miss "support"
> for legacy_mode_p here.
OTOH gimple-array-bounds.cc does
const value_range *vr = NULL;
if (TREE_CODE (low_sub_org) == SSA_NAME)
{
vr = get_value_range (low_sub_org, stmt);
if (!vr->undefined_p () && !vr->varying_p ())
{
low_sub = vr->kind () == VR_RANGE ? vr->max () : vr->min ();
up_sub = vr->kind () == VR_RANGE ? vr->min () : vr->max ();
}
so the bug is a documentation bug on min/max/lower/upper bound?!
I'm policeing other uses of value_range right now.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561
Richard Biener changed:
What|Removed |Added
Assignee|unassigned at gcc dot gnu.org |rguenth at gcc dot
gnu.org
Status|NEW |ASSIGNED
--- Comment #9 from Richard Biener ---
For the testcase in comment#2 we end up with
[local count: 1073741824]:
MEM[(struct __as_base &)] ={v} {CLOBBER};
a.m = 0;
_5 = operator new [] (0);
a.p = _5;
_2 = a.m;
if (_2 > 0)
goto ; [89.00%]
else
goto ; [11.00%]
and the code diagnosed is in the unreachable branch we cannot resolve as not
taken because 'operator new [] (0)' is thought to clobber 'a.m'.
We've been circling around improving alias analysis around new and delete
but since users can override them we threw the towel.
For the original testcase in g++.dg/pr71488.C we have
[local count: 214748368]:
*_51._M_size = 0;
_147 = operator new (0);
[local count: 214748368]:
*_51._M_data = _147;
_101 = *_51._M_size;
_99 = _101 * 8;
__builtin_memcpy (_147, _72, _99);
so again we fail to CSE '*_51._M_size' because of 'operator new (0)' but
also the diagnostic code simply assumes that size is at least 1 and
doesn't consider zero here for "reasons".
Note I think there's still a bug in value_range (irange) here. get_size_range
does
if (integral)
{
value_range vr;
query->range_of_expr (vr, exp, stmt);
if (vr.undefined_p ())
vr.set_varying (TREE_TYPE (exp));
range_type = vr.kind ();
min = wi::to_wide (vr.min ());
max = wi::to_wide (vr.max ());
and we have vr:
(gdb) p vr
$13 = { = { = {
_vptr.vrange = 0x3693a30 +16>,
m_kind = VR_ANTI_RANGE, m_discriminator = VR_IRANGE},
m_num_ranges = 1 '\001', m_max_ranges = 1 '\001',
m_nonzero_mask = , m_base = 0x7fffc8f0}, m_ranges = {
, }}
(gdb) p vr.dump (stderr)
[irange] unsigned int [0, 0][8, +INF]$17 = void
but vr.min () produces 1 and vr.max () produces 7, just as if it doesn't
interpret VR_ANTI_RANGE transparently here (if that's the intent?!).
At least
// Return the highest bound of a range expressed as a tree.
inline tree
irange::tree_upper_bound () const
suggests that. Note that vr.num_pairs () produces 2 (because constant_p ())
but vr.m_num_ranges is 1 and tree_upper_bound uses m_num_ranges.
I suppose irange::{min,max,tree_lower_bound,tree_upper_bound} miss "support"
for legacy_mode_p here.
Either using int_range<2> or vr.lower_bound/upper_bound works to avoid this
issue. Still it's fragile. I suppose since 'value_range' is legacy
itself using int_range<2> is better here. So I'm testing the following
to get rid of that legacy, plus get rid of ::min/max which are legacy as well.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561 --- Comment #8 from CVS Commits --- The master branch has been updated by Hans-Peter Nilsson : https://gcc.gnu.org/g:41015797ad14bc9030a87d102e4ab1ad891345f6 commit r13-5766-g41015797ad14bc9030a87d102e4ab1ad891345f6 Author: Hans-Peter Nilsson Date: Thu Feb 2 20:35:52 2023 +0100 testsuite: XFAIL g++.dg/pr71488.C and warn/Warray-bounds-16.C, PR107561 These appear as regressions from a baseline before r13-3761-ga239a63f868e29. See the PR trail. Note that the warning for g++.dg/pr71488.C is for a *header* file, thus we can't match the line number (sanely). gcc/testsuite: PR tree-optimization/107561 * g++.dg/warn/Warray-bounds-16.C: XFAIL bogus "overflows destination" warning. * g++.dg/pr71488.C: Ditto, but just for ilp32 targets.
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561 --- Comment #7 from Hans-Peter Nilsson --- (In reply to Hans-Peter Nilsson from comment #6) > IMHO these tests and AFAICT the underlying issue has seen no attention for > months and should be xfailed. On it... https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611206.html
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561 Hans-Peter Nilsson changed: What|Removed |Added CC||hp at gcc dot gnu.org --- Comment #6 from Hans-Peter Nilsson --- IMHO these tests and AFAICT the underlying issue has seen no attention for months and should be xfailed. On it...
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561 Richard Biener changed: What|Removed |Added Priority|P3 |P1
[Bug tree-optimization/107561] [13 Regression] g++.dg/pr71488.C and [g++.dg/warn/Warray-bounds-16.C -m32] regression due to -Wstringop-overflow problem
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107561 John David Anglin changed: What|Removed |Added Status|UNCONFIRMED |NEW Ever confirmed|0 |1 Last reconfirmed||2022-12-14 CC||danglin at gcc dot gnu.org
