Bug 19476 - Missed null checking elimination with new
Summary: Missed null checking elimination with new
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.0.0
: P2 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: missed-optimization
Depends on: 20318
Blocks: 58483 24242
  Show dependency treegraph
 
Reported: 2005-01-17 04:54 UTC by Andrew Pinski
Modified: 2013-10-03 23:57 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 3.1, 3.3.3, 3.4.0, 4.0.0
Last reconfirmed: 2007-07-01 00:17:42


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Pinski 2005-01-17 04:54:11 UTC
I don't know if I should place this in C++ or in tree-opt component but the following code still has a 
null check even though it is know that the variable cannot be null as that should have thrown an 
exception (yes this shows up in real code, this is reduced from PR 15855 sources):
struct Node { int i; };
struct SeqR11E0plus {  int i2; };
struct SeqR11E0plus_1 : public SeqR11E0plus, public Node
{
  SeqR11E0plus_1(void) throw();
};
Node* f(void)
{ Node* res = new SeqR11E0plus_1(); return res;}
Comment 1 Steven Bosscher 2005-01-17 08:37:10 UTC
Is this a regression?
Comment 2 Andrew Pinski 2005-01-17 15:30:04 UTC
(In reply to comment #1)
> Is this a regression?
Not that I know of.
Comment 3 Andrew Pinski 2005-01-20 06:29:09 UTC
Diego raised some questions about this around the same time I filed it so confirmed.
Comment 4 Andrew Pinski 2005-11-12 20:05:04 UTC
This is an easy extension on top of PR 20318.  Mine.
Comment 5 Chris Lattner 2005-11-13 01:24:42 UTC
Is this safe?  People can define their own operator new's, some of which may return null...

-Chris
Comment 6 Andrew Pinski 2005-11-13 02:10:46 UTC
(In reply to comment #5)
> Is this safe?  People can define their own operator new's, some of which may
> return null...

Yes because the normal operator new guarante not to return NULL by the C++ standard.  And if it returns a NULL that is undefined behavior, it should be throwing an exception when memory could not be allocated (there is a nonthrow version which can and will return NULL).
Comment 7 Chris Lattner 2005-11-13 02:13:50 UTC
> Yes because the normal operator new guarante not to return NULL by the C++
> standard.

Sure.

> And if it returns a NULL that is undefined behavior, it should be
> throwing an exception when memory could not be allocated (there is a nonthrow
> version which can and will return NULL).

Sure, fine, but you need not be calling the default/normal operator new.  I can define an overload for operator new in a different translation unit, or even by dynamically loading a library with a different one.  This is similar to replacing malloc.  AFAICT, the C++ std does not say that the replacement operator new may not return null.

-Chris 
Comment 8 Andrew Pinski 2005-11-13 02:24:25 UTC
(In reply to comment #7)
From 3.7.3/3:
Any allocation and/or deallocation functions defined in a C++ program shall conform to the sematics specified in 3.7.3.1 and 3.7.3.2.
---

Comment 9 Chris Lattner 2005-11-13 02:51:29 UTC
yup, you're right.  Great!

-Chris
Comment 10 Andrew Pinski 2006-01-09 18:33:57 UTC
No longer working on this, I am too busy working on the gfortran front-end.
Comment 11 Andrew Pinski 2008-04-08 21:40:17 UTC
*** Bug 35878 has been marked as a duplicate of this bug. ***
Comment 12 Ian Lance Taylor 2008-04-10 23:33:03 UTC
Note that bug 35878, which was closed as a duplicate of this one, was a case of placement new.  For placement new the check for a NULL pointer is particularly useless, as the language standard says that placement new is required to return the pointer which was passed in.
Comment 13 Marc Glisse 2013-09-06 16:14:21 UTC
Without adding an attribute, can we identify those operator new that may not return 0? Is DECL_IS_OPERATOR_NEW && !TREE_NOTHROW good enough, or completely wrong? I am basing this on:

"If the request succeeds, the value returned shall be a non-null pointer value (4.10)"

"If an allocation function declared with a non-throwing exception-specification (15.4) fails to allocate storage, it shall return a null pointer. Any other allocation function that fails to allocate storage shall indicate failure only by throwing an exception".

If the test is correct, adding one case to tree_expr_nonzero_warnv_p, gimple_stmt_nonzero_warnv_p (and maybe others as well?) should be easy.
Comment 14 Paolo Carlini 2013-09-06 17:31:52 UTC
In my experience, the only way to actually make progress on these old issues is actually posting a patch even if incomplete, etc, in other terms the straw-man approach: http://en.wikipedia.org/wiki/Straw_man_proposal
Comment 15 Marc Glisse 2013-09-11 13:20:12 UTC
I posted a patch:
http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00676.html

However, note that it only optimizes the testcase from this PR if we add #include <new> at the beginning, otherwise the implicit declaration of operator new doesn't have its operator_new_flag set...
Comment 16 Marc Glisse 2013-09-11 14:14:11 UTC
(In reply to Marc Glisse from comment #15)
> However, note that it only optimizes the testcase from this PR if we add
> #include <new> at the beginning, otherwise the implicit declaration of
> operator new doesn't have its operator_new_flag set...

That can (and probably should, since this flag is also used in alias analysis) be fixed with something like:

--- /data/repos/gcc/newnonzero/gcc/cp/decl.c	(revision 202499)
+++ /data/repos/gcc/newnonzero/gcc/cp/decl.c	(working copy)
@@ -3799,8 +3799,8 @@ cxx_init_decl_processing (void)
     newtype = build_exception_variant (newtype, new_eh_spec);
     deltype = cp_build_type_attribute_variant (void_ftype_ptr, extvisattr);
     deltype = build_exception_variant (deltype, empty_except_spec);
-    push_cp_library_fn (NEW_EXPR, newtype, 0);
-    push_cp_library_fn (VEC_NEW_EXPR, newtype, 0);
+    DECL_IS_OPERATOR_NEW (push_cp_library_fn (NEW_EXPR, newtype, 0)) = 1;
+    DECL_IS_OPERATOR_NEW (push_cp_library_fn (VEC_NEW_EXPR, newtype, 0)) = 1;
     global_delete_fndecl = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
     push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
Comment 17 Marc Glisse 2013-10-03 16:13:56 UTC
Author: glisse
Date: Thu Oct  3 16:13:54 2013
New Revision: 203163

URL: http://gcc.gnu.org/viewcvs?rev=203163&root=gcc&view=rev
Log:
2013-10-03  Marc Glisse  <marc.glisse@inria.fr>

	PR c++/19476
gcc/c-family/
	* c.opt (fcheck-new): Move to common.opt.

gcc/
	* common.opt (fcheck-new): Moved from c.opt. Make it 'Common'.
	* calls.c (alloca_call_p): Use get_callee_fndecl.
	* fold-const.c (tree_expr_nonzero_warnv_p): Handle operator new.
	* tree-vrp.c (gimple_stmt_nonzero_warnv_p, stmt_interesting_for_vrp):
	Likewise.
	(vrp_visit_stmt): Remove duplicated code.

gcc/testsuite/
	* g++.dg/tree-ssa/pr19476-1.C: New file.
	* g++.dg/tree-ssa/pr19476-2.C: Likewise.
	* g++.dg/tree-ssa/pr19476-3.C: Likewise.
	* g++.dg/tree-ssa/pr19476-4.C: Likewise.

Added:
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-1.C   (with props)
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-2.C   (with props)
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-3.C   (with props)
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-4.C   (with props)
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c.opt
    trunk/gcc/calls.c
    trunk/gcc/common.opt
    trunk/gcc/fold-const.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vrp.c

Propchange: trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-1.C
            ('svn:eol-style' added)

Propchange: trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-1.C
            ('svn:keywords' added)

Propchange: trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-2.C
            ('svn:eol-style' added)

Propchange: trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-2.C
            ('svn:keywords' added)

Propchange: trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-3.C
            ('svn:eol-style' added)

Propchange: trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-3.C
            ('svn:keywords' added)

Propchange: trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-4.C
            ('svn:eol-style' added)

Propchange: trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-4.C
            ('svn:keywords' added)
Comment 18 Marc Glisse 2013-10-03 23:48:20 UTC
Author: glisse
Date: Thu Oct  3 23:48:18 2013
New Revision: 203194

URL: http://gcc.gnu.org/viewcvs?rev=203194&root=gcc&view=rev
Log:
2013-10-04  Marc Glisse  <marc.glisse@inria.fr>

	PR c++/19476
gcc/cp/
	* decl.c (cxx_init_decl_processing): Set operator_new_flag.

gcc/testsuite/
	* g++.dg/tree-ssa/pr19476-5.C: New file.
	* g++.dg/tree-ssa/pr19476-1.C: Mention pr19476-5.C.

Added:
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-5.C   (with props)
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/decl.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-1.C

Propchange: trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-5.C
            ('svn:eol-style' added)

Propchange: trunk/gcc/testsuite/g++.dg/tree-ssa/pr19476-5.C
            ('svn:keywords' added)
Comment 19 Marc Glisse 2013-10-03 23:57:30 UTC
Done. Related cases are tracked in PR 35878 and PR 20318.