| Message ID | alpine.DEB.2.02.1309071206340.19326@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On Sat, 2013-09-07 at 12:33 +0200, Marc Glisse wrote: > Hello, > > this patch teaches the compiler that operator new, when it can throw, > isn't allowed to return a null pointer. Note that it doesn't work for > placement new (that one may have to be handled in the front-end or the > inliner), and it will not work on user-defined operator new if they are > inlined. I guess it would be possible to teach the inliner to insert an > assert_expr or something when inlining such a function, but that's not for > now. The code I removed from vrp_visit_stmt was duplicated in > stmt_interesting_for_vrp and thus useless. Interesting patch. However, I feel that we should give advanced users the ability to say that their new operator is never returning null... In particular, if I define my own new operator which never returns nil, I find strange that it would be less optimized than the system's operator new. Perhaps we need an attribute `nonnullresult' which whould means that. (we already the nonnull attribute for function arguments) Cheers.
On Sat, Sep 7, 2013 at 6:44 AM, Basile Starynkevitch <basile@starynkevitch.net> wrote: > On Sat, 2013-09-07 at 12:33 +0200, Marc Glisse wrote: >> Hello, >> >> this patch teaches the compiler that operator new, when it can throw, >> isn't allowed to return a null pointer. Note that it doesn't work for >> placement new (that one may have to be handled in the front-end or the >> inliner), and it will not work on user-defined operator new if they are >> inlined. I guess it would be possible to teach the inliner to insert an >> assert_expr or something when inlining such a function, but that's not for >> now. The code I removed from vrp_visit_stmt was duplicated in >> stmt_interesting_for_vrp and thus useless. > > > Interesting patch. However, I feel that we should give advanced users > the ability to say that their new operator is never returning null... > In particular, if I define my own new operator which never returns nil, > I find strange that it would be less optimized than the system's > operator new. > > Perhaps we need an attribute `nonnullresult' which whould means that. > (we already the nonnull attribute for function arguments) 'operator new' is a replaceable function, so when you define it, you have to abide by the standard semantics. -- Gaby
On Sat, 7 Sep 2013, Basile Starynkevitch wrote: > On Sat, 2013-09-07 at 12:33 +0200, Marc Glisse wrote: >> Hello, >> >> this patch teaches the compiler that operator new, when it can throw, >> isn't allowed to return a null pointer. Note that it doesn't work for >> placement new (that one may have to be handled in the front-end or the >> inliner), and it will not work on user-defined operator new if they are >> inlined. I guess it would be possible to teach the inliner to insert an >> assert_expr or something when inlining such a function, but that's not for >> now. The code I removed from vrp_visit_stmt was duplicated in >> stmt_interesting_for_vrp and thus useless. > > > Interesting patch. However, I feel that we should give advanced users > the ability to say that their new operator is never returning null... Easy, don't specify noexcept on it and that's what the patch already does, as long as the function is not inlined. > In particular, if I define my own new operator which never returns nil, > I find strange that it would be less optimized than the system's > operator new. > > Perhaps we need an attribute `nonnullresult' which whould means that. > (we already the nonnull attribute for function arguments) There is already a PR about that, linked from this PR. But even if we create this attribute, we will still want to be able to guess that new should have it even if it isn't written.
On Sep 7, 2013, at 3:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> this patch teaches the compiler that operator new, when it can throw, isn't allowed to return a null pointer.
You sure:
@item -fcheck-new
@opindex fcheck-new
Check that the pointer returned by @code{operator new} is non-null
before attempting to modify the storage allocated. This check is
normally unnecessary because the C++ standard specifies that
@code{operator new} only returns @code{0} if it is declared
@samp{throw()}, in which case the compiler always checks the
return value even without this option. In all other cases, when
@code{operator new} has a non-empty exception specification, memory
exhaustion is signalled by throwing @code{std::bad_alloc}. See also
@samp{new (nothrow)}.
?
On Sat, 7 Sep 2013, Mike Stump wrote: > On Sep 7, 2013, at 3:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >> this patch teaches the compiler that operator new, when it can throw, isn't allowed to return a null pointer. > > You sure: > > @item -fcheck-new > @opindex fcheck-new > Check that the pointer returned by @code{operator new} is non-null > before attempting to modify the storage allocated. This check is > normally unnecessary because the C++ standard specifies that > @code{operator new} only returns @code{0} if it is declared > @samp{throw()}, in which case the compiler always checks the > return value even without this option. In all other cases, when > @code{operator new} has a non-empty exception specification, memory > exhaustion is signalled by throwing @code{std::bad_alloc}. See also > @samp{new (nothrow)}. > > ? Thanks, I didn't know that option. But it doesn't do the same. -fcheck-new is about the front-end inserting a test !=0 between malloc and the constructor. My patch is about teaching the middle-end that the value is not zero (so even user-coded comparisons with 0 can be simplified). Now flag_check_new should probably disable this optimization... The 3 -fcheck-new testcases in the testsuite probably only pass because they don't have -O2.
On Sat, Sep 7, 2013 at 2:27 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Sat, 7 Sep 2013, Mike Stump wrote: > >> On Sep 7, 2013, at 3:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >>> >>> this patch teaches the compiler that operator new, when it can throw, >>> isn't allowed to return a null pointer. >> >> >> You sure: >> >> @item -fcheck-new >> @opindex fcheck-new >> Check that the pointer returned by @code{operator new} is non-null >> before attempting to modify the storage allocated. This check is >> normally unnecessary because the C++ standard specifies that >> @code{operator new} only returns @code{0} if it is declared >> @samp{throw()}, in which case the compiler always checks the >> return value even without this option. In all other cases, when >> @code{operator new} has a non-empty exception specification, memory >> exhaustion is signalled by throwing @code{std::bad_alloc}. See also >> @samp{new (nothrow)}. >> >> ? > > > Thanks, I didn't know that option. But it doesn't do the same. Indeed. -- Gaby
On Sep 7, 2013, at 12:37 PM, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote: > On Sat, Sep 7, 2013 at 2:27 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >> On Sat, 7 Sep 2013, Mike Stump wrote: >> >>> On Sep 7, 2013, at 3:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >>>> >>>> this patch teaches the compiler that operator new, when it can throw, >>>> isn't allowed to return a null pointer. >>> >>> >>> You sure: >>> >>> @item -fcheck-new >>> @opindex fcheck-new >>> Check that the pointer returned by @code{operator new} is non-null >>> before attempting to modify the storage allocated. This check is >>> normally unnecessary because the C++ standard specifies that >>> @code{operator new} only returns @code{0} if it is declared >>> @samp{throw()}, in which case the compiler always checks the >>> return value even without this option. In all other cases, when >>> @code{operator new} has a non-empty exception specification, memory >>> exhaustion is signalled by throwing @code{std::bad_alloc}. See also >>> @samp{new (nothrow)}. >>> >>> ? >> >> >> Thanks, I didn't know that option. But it doesn't do the same. > > Indeed. Can this throw: void *operator new (long unsigned int s) { return 0; } ? Is this allowed to return 0?
On Sep 7, 2013, at 12:27 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Now flag_check_new should probably disable this optimization…
Yes, this why my point.
On Sat, 7 Sep 2013, Mike Stump wrote: > > On Sep 7, 2013, at 12:37 PM, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote: > >> On Sat, Sep 7, 2013 at 2:27 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >>> On Sat, 7 Sep 2013, Mike Stump wrote: >>> >>>> On Sep 7, 2013, at 3:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >>>>> >>>>> this patch teaches the compiler that operator new, when it can throw, >>>>> isn't allowed to return a null pointer. >>>> >>>> >>>> You sure: >>>> >>>> @item -fcheck-new >>>> @opindex fcheck-new >>>> Check that the pointer returned by @code{operator new} is non-null >>>> before attempting to modify the storage allocated. This check is >>>> normally unnecessary because the C++ standard specifies that >>>> @code{operator new} only returns @code{0} if it is declared >>>> @samp{throw()}, in which case the compiler always checks the >>>> return value even without this option. In all other cases, when >>>> @code{operator new} has a non-empty exception specification, memory >>>> exhaustion is signalled by throwing @code{std::bad_alloc}. See also >>>> @samp{new (nothrow)}. >>>> >>>> ? >>> >>> >>> Thanks, I didn't know that option. But it doesn't do the same. >> >> Indeed. > > Can this throw: > > void *operator new (long unsigned int s) { > return 0; > } > > ? Is this allowed to return 0? I think using this function is illegal. It isn't marked noexcept, so it isn't allowed to return 0.
On Sat, 7 Sep 2013, Marc Glisse wrote: > On Sat, 7 Sep 2013, Mike Stump wrote: > >> Can this throw: >> >> void *operator new (long unsigned int s) { >> return 0; >> } >> >> ? Is this allowed to return 0? > > I think using this function is illegal. It isn't marked noexcept, so it isn't > allowed to return 0. And if I compile your code with gcc, I get nice warnings (though I get them twice and the column number is not so good): m.cc: In function 'void* operator new(long unsigned int)': m.cc:2:12: warning: 'operator new' must not return NULL unless it is declared 'throw()' (or -fcheck-new is in effect) [enabled by default] return 0; ^ m.cc: At global scope: m.cc:1:7: warning: unused parameter 's' [-Wunused-parameter] void *operator new (long unsigned int s) { ^
On Sat, Sep 7, 2013 at 2:46 PM, Mike Stump <mikestump@comcast.net> wrote: > > On Sep 7, 2013, at 12:37 PM, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote: > >> On Sat, Sep 7, 2013 at 2:27 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >>> On Sat, 7 Sep 2013, Mike Stump wrote: >>> >>>> On Sep 7, 2013, at 3:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >>>>> >>>>> this patch teaches the compiler that operator new, when it can throw, >>>>> isn't allowed to return a null pointer. >>>> >>>> >>>> You sure: >>>> >>>> @item -fcheck-new >>>> @opindex fcheck-new >>>> Check that the pointer returned by @code{operator new} is non-null >>>> before attempting to modify the storage allocated. This check is >>>> normally unnecessary because the C++ standard specifies that >>>> @code{operator new} only returns @code{0} if it is declared >>>> @samp{throw()}, in which case the compiler always checks the >>>> return value even without this option. In all other cases, when >>>> @code{operator new} has a non-empty exception specification, memory >>>> exhaustion is signalled by throwing @code{std::bad_alloc}. See also >>>> @samp{new (nothrow)}. >>>> >>>> ? >>> >>> >>> Thanks, I didn't know that option. But it doesn't do the same. >> >> Indeed. > > Can this throw: > > void *operator new (long unsigned int s) { > return 0; > } > > ? Is this allowed to return 0? If that is supposed to be a definition for the global function 'operator new', then it fails the requirement of the C++ standards since 1998. -- Gaby
On Sep 7, 2013, at 5:34 PM, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote: > On Sat, Sep 7, 2013 at 2:46 PM, Mike Stump <mikestump@comcast.net> wrote: >> >> On Sep 7, 2013, at 12:37 PM, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote: >> >>> On Sat, Sep 7, 2013 at 2:27 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >>>> On Sat, 7 Sep 2013, Mike Stump wrote: >>>> >>>>> On Sep 7, 2013, at 3:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >>>>>> >>>>>> this patch teaches the compiler that operator new, when it can throw, >>>>>> isn't allowed to return a null pointer. >>>>> >>>>> >>>>> You sure: >>>>> >>>>> @item -fcheck-new >>>>> @opindex fcheck-new >>>>> Check that the pointer returned by @code{operator new} is non-null >>>>> before attempting to modify the storage allocated. This check is >>>>> normally unnecessary because the C++ standard specifies that >>>>> @code{operator new} only returns @code{0} if it is declared >>>>> @samp{throw()}, in which case the compiler always checks the >>>>> return value even without this option. In all other cases, when >>>>> @code{operator new} has a non-empty exception specification, memory >>>>> exhaustion is signalled by throwing @code{std::bad_alloc}. See also >>>>> @samp{new (nothrow)}. >>>>> >>>>> ? >>>> >>>> >>>> Thanks, I didn't know that option. But it doesn't do the same. >>> >>> Indeed. >> >> Can this throw: >> >> void *operator new (long unsigned int s) { >> return 0; >> } >> >> ? Is this allowed to return 0? > > If that is supposed to be a definition for the global function 'operator new', > then it fails the requirement of the C++ standards since 1998. And that is irrelevant to the point at hand. I'll note that you failed to answer both questions. The answer is, the declaration of the function is not no throw, so by declaration, that operator new can throw (though, this definition will never throw). And yes, it is allowed to return 0 because the language selected by the option informs the compiler of that. Surely since I pointed out the existence of the option, you know what it is for, right? Were you able to read and understand the option or not?
On Sep 7, 2013, at 1:03 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Sat, 7 Sep 2013, Mike Stump wrote: > >> >> On Sep 7, 2013, at 12:37 PM, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote: >> >>> On Sat, Sep 7, 2013 at 2:27 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >>>> On Sat, 7 Sep 2013, Mike Stump wrote: >>>> >>>>> On Sep 7, 2013, at 3:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >>>>>> >>>>>> this patch teaches the compiler that operator new, when it can throw, >>>>>> isn't allowed to return a null pointer. >>>>> >>>>> >>>>> You sure: >>>>> >>>>> @item -fcheck-new >>>>> @opindex fcheck-new >>>>> Check that the pointer returned by @code{operator new} is non-null >>>>> before attempting to modify the storage allocated. This check is >>>>> normally unnecessary because the C++ standard specifies that >>>>> @code{operator new} only returns @code{0} if it is declared >>>>> @samp{throw()}, in which case the compiler always checks the >>>>> return value even without this option. In all other cases, when >>>>> @code{operator new} has a non-empty exception specification, memory >>>>> exhaustion is signalled by throwing @code{std::bad_alloc}. See also >>>>> @samp{new (nothrow)}. >>>>> >>>>> ? >>>> >>>> >>>> Thanks, I didn't know that option. But it doesn't do the same. >>> >>> Indeed. >> >> Can this throw: >> >> void *operator new (long unsigned int s) { >> return 0; >> } >> >> ? Is this allowed to return 0? > > I think using this function is illegal. Yes, according the the standard, and without any compilation flags, it is. However, with -fcheck-new and in the g++ language, it is not.
Index: fold-const.c =================================================================== --- fold-const.c (revision 202351) +++ fold-const.c (working copy) @@ -16171,21 +16171,29 @@ tree_expr_nonzero_warnv_p (tree t, bool case MODIFY_EXPR: case BIND_EXPR: return tree_expr_nonzero_warnv_p (TREE_OPERAND (t, 1), strict_overflow_p); case SAVE_EXPR: return tree_expr_nonzero_warnv_p (TREE_OPERAND (t, 0), strict_overflow_p); case CALL_EXPR: - return alloca_call_p (t); + { + tree fn = CALL_EXPR_FN (t); + if (TREE_CODE (fn) != ADDR_EXPR) return false; + tree fndecl = TREE_OPERAND (fn, 0); + if (TREE_CODE (fndecl) != FUNCTION_DECL) return false; + if (DECL_IS_OPERATOR_NEW (fndecl) && !TREE_NOTHROW (fndecl)) + return true; + return alloca_call_p (t); + } default: break; } return false; } /* Return true when T is an address and is known to be nonzero. Handle warnings about undefined signed overflow. */ Index: testsuite/g++.dg/tree-ssa/pr19476-1.C =================================================================== --- testsuite/g++.dg/tree-ssa/pr19476-1.C (revision 0) +++ testsuite/g++.dg/tree-ssa/pr19476-1.C (revision 0) @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-options "-O -fdump-tree-ccp1" } */ + +#include <new> + +int f(){ + return 33 + (0 == new(std::nothrow) int); +} +int g(){ + return 42 + (0 == new int[50]); +} + +/* { dg-final { scan-tree-dump "return 42" "ccp1" } } */ +/* { dg-final { scan-tree-dump-not "return 33" "ccp1" } } */ +/* { dg-final { cleanup-tree-dump "ccp1" } } */ Property changes on: testsuite/g++.dg/tree-ssa/pr19476-1.C ___________________________________________________________________ Added: svn:keywords + Author Date Id Revision URL Added: svn:eol-style + native Index: testsuite/g++.dg/tree-ssa/pr19476-2.C =================================================================== --- testsuite/g++.dg/tree-ssa/pr19476-2.C (revision 0) +++ testsuite/g++.dg/tree-ssa/pr19476-2.C (revision 0) @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-optimized" } */ + +#include <new> + +int f(){ + int *p = new(std::nothrow) int; + return 33 + (0 == p); +} +int g(){ + int *p = new int[50]; + return 42 + (0 == p); +} + +/* { dg-final { scan-tree-dump "return 42" "optimized" } } */ +/* { dg-final { scan-tree-dump-not "return 33" "optimized" } } */ +/* { dg-final { cleanup-tree-dump "optimized" } } */ Property changes on: testsuite/g++.dg/tree-ssa/pr19476-2.C ___________________________________________________________________ Added: svn:eol-style + native Added: svn:keywords + Author Date Id Revision URL Index: tree-vrp.c =================================================================== --- tree-vrp.c (revision 202351) +++ tree-vrp.c (working copy) @@ -1047,21 +1047,27 @@ gimple_assign_nonzero_warnv_p (gimple st *STRICT_OVERFLOW_P.*/ static bool gimple_stmt_nonzero_warnv_p (gimple stmt, bool *strict_overflow_p) { switch (gimple_code (stmt)) { case GIMPLE_ASSIGN: return gimple_assign_nonzero_warnv_p (stmt, strict_overflow_p); case GIMPLE_CALL: - return gimple_alloca_call_p (stmt); + { + tree fndecl = gimple_call_fndecl (stmt); + if (!fndecl) return false; + if (DECL_IS_OPERATOR_NEW (fndecl) && !TREE_NOTHROW (fndecl)) + return true; + return gimple_alloca_call_p (stmt); + } default: gcc_unreachable (); } } /* Like tree_expr_nonzero_warnv_p, but this function uses value ranges obtained so far. */ static bool vrp_stmt_computes_nonzero (gimple stmt, bool *strict_overflow_p) @@ -6486,21 +6492,22 @@ stmt_interesting_for_vrp (gimple stmt) tree lhs = gimple_get_lhs (stmt); /* In general, assignments with virtual operands are not useful for deriving ranges, with the obvious exception of calls to builtin functions. */ if (lhs && TREE_CODE (lhs) == SSA_NAME && (INTEGRAL_TYPE_P (TREE_TYPE (lhs)) || POINTER_TYPE_P (TREE_TYPE (lhs))) && ((is_gimple_call (stmt) && gimple_call_fndecl (stmt) != NULL_TREE - && DECL_BUILT_IN (gimple_call_fndecl (stmt))) + && (DECL_BUILT_IN (gimple_call_fndecl (stmt)) + || DECL_IS_OPERATOR_NEW (gimple_call_fndecl (stmt)))) || !gimple_vuse (stmt))) return true; } else if (gimple_code (stmt) == GIMPLE_COND || gimple_code (stmt) == GIMPLE_SWITCH) return true; return false; } @@ -7407,30 +7414,21 @@ vrp_visit_stmt (gimple stmt, edge *taken if (dump_file && (dump_flags & TDF_DETAILS)) { fprintf (dump_file, "\nVisiting statement:\n"); print_gimple_stmt (dump_file, stmt, 0, dump_flags); fprintf (dump_file, "\n"); } if (!stmt_interesting_for_vrp (stmt)) gcc_assert (stmt_ends_bb_p (stmt)); else if (is_gimple_assign (stmt) || is_gimple_call (stmt)) - { - /* In general, assignments with virtual operands are not useful - for deriving ranges, with the obvious exception of calls to - builtin functions. */ - if ((is_gimple_call (stmt) - && gimple_call_fndecl (stmt) != NULL_TREE - && DECL_BUILT_IN (gimple_call_fndecl (stmt))) - || !gimple_vuse (stmt)) - return vrp_visit_assignment_or_call (stmt, output_p); - } + return vrp_visit_assignment_or_call (stmt, output_p); else if (gimple_code (stmt) == GIMPLE_COND) return vrp_visit_cond_stmt (stmt, taken_edge_p); else if (gimple_code (stmt) == GIMPLE_SWITCH) return vrp_visit_switch_stmt (stmt, taken_edge_p); /* All other statements produce nothing of interest for VRP, so mark their outputs varying and prevent further simulation. */ FOR_EACH_SSA_TREE_OPERAND (def, stmt, iter, SSA_OP_DEF) set_value_range_to_varying (get_value_range (def));