[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 --- Comment #19 from CVS Commits --- The master branch has been updated by Richard Biener : https://gcc.gnu.org/g:09a0affdb0598a54835ac4bb0dd6b54122c12916 commit r12-4319-g09a0affdb0598a54835ac4bb0dd6b54122c12916 Author: Richard Biener Date: Mon Oct 11 16:06:03 2021 +0200 middle-end/101480 - overloaded global new/delete The following fixes the issue of ignoring side-effects on memory from overloaded global new/delete operators by not marking them as effectively 'const' apart from other explicitely specified side-effects. This will cause FAIL: g++.dg/warn/Warray-bounds-16.C -std=gnu++1? (test for excess errors) because we now no longer statically see the initialization loop never executes because the call to operator new can now clobber 'a.m'. This seems to be an issue with the warning code and/or ranger so I'm leaving this FAIL to be addressed as followup. 2021-10-11 Richard Biener PR middle-end/101480 * gimple.c (gimple_call_fnspec): Do not mark operator new/delete as const. * g++.dg/torture/pr10148.C: New testcase.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480
Richard Biener changed:
What|Removed |Added
CC||amacleod at redhat dot com
--- Comment #18 from Richard Biener ---
(In reply to Martin Sebor from comment #16)
> (In reply to Richard Biener from comment #14)
> ...
> > the testcase does
> >
> > m = i;
> > p = (int*) new unsigned char [sizeof (int) * m];
> >
> > for (int i = 0; i < m; i++)
> > new (p + i) int ();
> >
> > and we likely have to assume that 'new' changes 'm'.
>
> Why? Because of the flow-insensitivity of the alias analysis?
>
> m is a member of the class whose ctor has the loop above. Neither the
> enclosing object nor the member actually escapes (the operator new to which
> p is passed in the loop is the nonreplaceable placement new), so there is no
> way it can be changed.
What we see is the global variable construction function which accesses
just 'a', and yes, the call to 'new' is considered clobbering global
variables (including 'a'):
[local count: 1073741824]:
MEM[(struct __as_base &)] ={v} {CLOBBER};
a.m = 0;
_5 = operator new [] (0);
a.p = _5;
goto ; [100.00%]
[local count: 8687547547]:
_7 = (long unsigned int) i_6;
_8 = _7 * 4;
_9 = _5 + _8;
MEM[(int *)_9] = 0;
i_10 = i_6 + 1;
[local count: 9761289362]:
# i_6 = PHI <0(2), i_10(3)>
_11 = a.m;
if (i_6 < _11)
goto ; [89.00%]
else
goto ; [11.00%]
[local count: 1073741824]:
return;
I suppose implementing the global operator new as accessing a.m would
be valid as IIRC lifetime of a starts when the CTOR is invoked, not
when it finished (otherwise the CTOR could not access the variable itself).
We somehow conclude that
_9: void * [1B, +INF] EQUIVALENCES: { } (0 elements)
possibly because it cannot be NULL (?):
extract_range_from_stmt visiting:
_5 = operator new [] (0);
Found new range for _5: void * [1B, +INF]
marking stmt to be not simulated again
(huh?)
and then the -Warray-bounds warning concludes the access is always outside
of the allocated area.
I suspect when we'd arrive at VARYING we'd not issue the warning even
when the access would always extend beyond a zero-sized allocation.
I'm going to ignore this diagnostic issue, it concerns the 'offrange'
code I'm not familiar with at all (and maybe the value-range code for
which I now have to say sth similar).
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 Richard Biener changed: What|Removed |Added Assignee|unassigned at gcc dot gnu.org |rguenth at gcc dot gnu.org Status|NEW |ASSIGNED --- Comment #17 from Richard Biener --- I'm re-testing the patch and will investigate the failure more if it still happens.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 Martin Sebor changed: What|Removed |Added CC||msebor at gcc dot gnu.org --- Comment #16 from Martin Sebor --- (In reply to Richard Biener from comment #14) ... > the testcase does > > m = i; > p = (int*) new unsigned char [sizeof (int) * m]; > > for (int i = 0; i < m; i++) > new (p + i) int (); > > and we likely have to assume that 'new' changes 'm'. Why? Because of the flow-insensitivity of the alias analysis? m is a member of the class whose ctor has the loop above. Neither the enclosing object nor the member actually escapes (the operator new to which p is passed in the loop is the nonreplaceable placement new), so there is no way it can be changed.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 Richard Biener changed: What|Removed |Added Target Milestone|11.2|11.3 --- Comment #15 from Richard Biener --- GCC 11.2 is being released, retargeting bugs to GCC 11.3
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 --- Comment #14 from Richard Biener --- diff --git a/gcc/gimple.c b/gcc/gimple.c index 863bc0d17f1..e085d9de49a 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -1516,12 +1516,12 @@ gimple_call_fnspec (const gcall *stmt) && DECL_IS_OPERATOR_DELETE_P (fndecl) && DECL_IS_REPLACEABLE_OPERATOR (fndecl) && gimple_call_from_new_or_delete (stmt)) -return ".co "; +return ". o "; /* Similarly operator new can be treated as malloc. */ if (fndecl && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl) && gimple_call_from_new_or_delete (stmt)) -return "mC"; +return "m "; return ""; } regresses FAIL: g++.dg/warn/Warray-bounds-16.C -std=gnu++14 scan-tree-dump-not optimized "goto" FAIL: g++.dg/warn/Warray-bounds-16.C -std=gnu++14 (test for excess errors) where we now diagnose /home/rguenther/src/trunk/gcc/testsuite/g++.dg/warn/Warray-bounds-16.C:22:7: warning: array subscript 0 is outside array bounds of 'void [0]' [-Warray-bounds] /home/rguenther/src/trunk/gcc/testsuite/g++.dg/warn/Warray-bounds-16.C:22:7: warning: 'void* __builtin_memset(void*, int, long unsigned int)' offset [0, 3] is out of the bounds [0, 0] [-Warray-bounds] the testcase does m = i; p = (int*) new unsigned char [sizeof (int) * m]; for (int i = 0; i < m; i++) new (p + i) int (); and we likely have to assume that 'new' changes 'm'.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 --- Comment #13 from Richard Biener --- In PR98130 I suggested "..o " for delete and it would be "m " for new (does a C++ new expression really set/clobber errno?).
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 --- Comment #12 from Richard Biener --- That was because of PR98130 it seems.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 --- Comment #11 from rguenther at suse dot de --- On Mon, 19 Jul 2021, redi at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 > > Jonathan Wakely changed: > >What|Removed |Added > >See Also||https://gcc.gnu.org/bugzill >||a/show_bug.cgi?id=59894 > > --- Comment #9 from Jonathan Wakely --- > I don't think this optimization should be on by default. A switch that says > "this program does not replace operator new and delete" would make it OK. That > is the subject of PR 59894. The odd thing is we do /* If the call is to a replaceable operator delete and results from a delete expression as opposed to a direct call to such operator, then we can treat it as free. */ if (fndecl && DECL_IS_OPERATOR_DELETE_P (fndecl) && DECL_IS_REPLACEABLE_OPERATOR (fndecl) && gimple_call_from_new_or_delete (stmt)) return ".co "; /* Similarly operator new can be treated as malloc. */ if (fndecl && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl) && gimple_call_from_new_or_delete (stmt)) return "mC"; so explicitely asking for the replaceable variants only (well, not sure if ones that don't have DECL_IS_REPLACEABLE_OPERATOR set are any "better" or worse here). Then we have valid_new_delete_pair_p used by new/delete pair removal which explicitely checks mangled symbol names.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 --- Comment #10 from Jens Maurer --- I agree with Jonathan here: The difference is that "malloc" comes with the compiler/library and cannot be replaced (within the scope of the C or C++ standards), but "operator new" is expressly specified to be replaceable by the C++ standard. There is text in the standard that restricts what "operator new" can and cannot do, but otherwise it is specified as-if a regular function call. As-is, gcc 11 is non-conforming with this optimization turned on by default. I'm all for providing an extra command-line switch so that the user can assert that he's not replacing "operator new" anywhere in the program, but that switch should be off by default.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 Jonathan Wakely changed: What|Removed |Added See Also||https://gcc.gnu.org/bugzill ||a/show_bug.cgi?id=59894 --- Comment #9 from Jonathan Wakely --- I don't think this optimization should be on by default. A switch that says "this program does not replace operator new and delete" would make it OK. That is the subject of PR 59894.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 --- Comment #8 from Jonathan Wakely --- But operator new is not defined in the runtime here, it's a replaceable global allocation function. The assumption seems unsafe for replaceable functions that users can define in their own code.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 --- Comment #7 from Richard Biener --- (In reply to Richard Biener from comment #6) > As Jakub said the behavior is the same for malloc() since years. > ... > So it might work to disable the new/delete/malloc/free optimization when > we see a definition of those functions. But it's surely not enough to disable transforms like x = 0; ... = new ...; if (x != 0) .. to elide the load from x after the 'new'. Like with malloc we assume that state of the runtime (where malloc / new is defined) is only accessible via API calls and not visible to the compiler at the same time its users are.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480
--- Comment #6 from Richard Biener ---
As Jakub said the behavior is the same for malloc() since years.
When you split the testcase like
> cat t.C
#include
#include
bool flag = false;
class C
{
bool prev;
public:
C() : prev(flag)
{
flag = true; // #1
}
~C() {
flag = prev;
}
};
void g(int* p)
{
delete p;
}
void f()
{
int* p;
{
C c;
p = new int;
}
g(p);
}
int main(int, char**)
{
f();
}
> cat t2.C
#include
#include
extern bool flag;
void* operator new(unsigned long size)
{
assert(flag); // #2
return malloc(size);
}
void operator delete(void *p)
{
free(p);
}
it works as 'flag' is then global and no longer local to the TU.
So it might work to disable the new/delete/malloc/free optimization when
we see a definition of those functions.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 Volker Schmidt changed: What|Removed |Added CC||volker.schmidt at factset dot com --- Comment #5 from Volker Schmidt --- I would simply add, that we have breaking change to gcc here, that does do something different, without any warning or documentation, simply based on the idea (which I find no source to point too, that new should not use globals). Without digging to deep, I find a number of areas (inter process communication, inter processor communication, numa optimizations, architectures with special function memory areas), where calling new is depended on a global status. So I find this assumption, not very intuitive.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 Jakub Jelinek changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #4 from Jakub Jelinek --- It is extremely important optimization and is done only if new/delete is used, not when one calls these operators. While when one calls the operator by hand, the C++ standard doesn't give the possibility of optimizing away e.g. a pair of operator delete (operator new (23)), when delete new int; is used, it can be optimized away. With reading and modification of the global state, sure, under the hood the function will need to read and store some global state, but the point is that typically that state doesn't include anything that the callers care about. It is similar to malloc, if one defines in C or C++ malloc and uses variables the callers of malloc set or read in the malloc implementation, it will not work properly. We have a -fno-builtin-malloc or -fno-builtin-free for that case though, so people that want to do weird things can at the expense of massive code penalization get what they want (better is to use some compiler barriers for such state). For the operator new/delete I think we don't have a switch, maybe it should be added. But it certainly should default to the current state.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 --- Comment #3 from Jens Maurer --- "We treat the global operator new as not reading from global memory" If I implement my own global "operator new" afresh, certainly it'll need to access global memory, e.g. to read a global pointer to the heap or so. I have carefully reviewed [expr.new] before posting this bug report. The compiler can omit a call to an allocation function entirely (and e.g. provide the memory on the stack, if the lifetime fits), but I haven't found any wording that would allow making assumptions about the global memory behavior of the allocation function when it is, in fact, called. "we cannot do the optimization that was added" I understand this might be an important optimization to offer (similar to -ffast-float), but it would be good to have a separate command-line flag to enable or disable it.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 Richard Biener changed: What|Removed |Added CC||jason at gcc dot gnu.org, ||rguenth at gcc dot gnu.org Priority|P3 |P2 --- Comment #2 from Richard Biener --- We treat the global operator new as not reading from global memory but here it does. I'm not sure if this overload is "valid". There's also the distinction made between replaceable and non-replaceable(?) operators. If a replaced global operator new can have arbitrary side-effects we cannot do the optimization that was added.
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 Jonathan Wakely changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2021-07-18 Ever confirmed|0 |1
[Bug c++/101480] [11/12 Regression] Miscompiled code involving operator new
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101480 Jonathan Wakely changed: What|Removed |Added Known to work||10.3.0 Keywords||wrong-code Known to fail||11.1.0, 12.0 Summary|Miscompiled code involving |[11/12 Regression] |operator new|Miscompiled code involving ||operator new Target Milestone|--- |11.2 CC||hubicka at gcc dot gnu.org --- Comment #1 from Jonathan Wakely --- Started with r11-4745 Add fnspecs for C++ new and delete operators gcc/ChangeLog: * gimple.c (gimple_call_fnspec): Handle C++ new and delete. * gimple.h (gimple_call_from_new_or_delete): Constify parameter. gcc/testsuite/ChangeLog: * g++.dg/ipa/devirt-24.C: Update template.
