| Message ID | alpine.DEB.2.10.1309071804080.3585@laptop-mg.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On Sat, Sep 7, 2013 at 11:42 AM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Sat, 7 Sep 2013, Marc Glisse wrote: > >> 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), > > > Placement new is a completely different thing. For one, it is nothrow (so > the test doesn't apply). But mostly, it is a condition on the argument more > than the result. Using the nonnull attribute would make sense, but the > compiler doesn't seem clever enough to use that information. The easiest > seems to be in the library: > > --- o/new 2013-09-07 11:11:17.388756320 +0200 > +++ i/new 2013-09-07 18:00:32.204797291 +0200 > @@ -144,9 +144,9 @@ > > // Default placement versions of operator new. > inline void* operator new(std::size_t, void* __p) _GLIBCXX_USE_NOEXCEPT > -{ return __p; } > +{ if (!__p) __builtin_unreachable(); return __p; } > inline void* operator new[](std::size_t, void* __p) _GLIBCXX_USE_NOEXCEPT > -{ return __p; } > +{ if (!__p) __builtin_unreachable(); return __p; } > > // Default placement versions of operator delete. > inline void operator delete (void*, void*) _GLIBCXX_USE_NOEXCEPT { } > > > > Though I am not sure what the policy is for this kind of thing. And that's > assuming it is indeed undefined to pass a null pointer to it, I don't have a > good reference for that. > > > >> and it will not work on user-defined operator new if they are inlined. > > > But then if that operator new is inlined, it may already contain a line like > if(p==0)throw(); which lets the compiler know all it needs. I am not sure I like this version of the patch. I think the appropriate patch should be in the compiler, not here. C++ has several places where certain parameters cannot have non-null values. For example, the implicit parameter 'this' in non-static member functions. This is another instance. Furthermore, I do think that the compiler should have special nodes for both standard placement new and the global operator new functions, as I explained in previous messages. -- Gaby > > >> 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. >> >> I ran the c,c++ testsuite on a non-bootstrap compiler. I'll do more proper >> testing when trunk bootstraps again. >> >> 2013-09-07 Marc Glisse <marc.glisse@inria.fr> >> >> PR c++/19476 >> gcc/ >> * 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. > > > -- > Marc Glisse
On Sat, 7 Sep 2013, Gabriel Dos Reis wrote: > On Sat, Sep 7, 2013 at 11:42 AM, Marc Glisse <marc.glisse@inria.fr> wrote: >> On Sat, 7 Sep 2013, Marc Glisse wrote: >> >>> 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), >> >> >> Placement new is a completely different thing. For one, it is nothrow (so >> the test doesn't apply). But mostly, it is a condition on the argument more >> than the result. Using the nonnull attribute would make sense, but the >> compiler doesn't seem clever enough to use that information. The easiest >> seems to be in the library: >> >> --- o/new 2013-09-07 11:11:17.388756320 +0200 >> +++ i/new 2013-09-07 18:00:32.204797291 +0200 >> @@ -144,9 +144,9 @@ >> >> // Default placement versions of operator new. >> inline void* operator new(std::size_t, void* __p) _GLIBCXX_USE_NOEXCEPT >> -{ return __p; } >> +{ if (!__p) __builtin_unreachable(); return __p; } >> inline void* operator new[](std::size_t, void* __p) _GLIBCXX_USE_NOEXCEPT >> -{ return __p; } >> +{ if (!__p) __builtin_unreachable(); return __p; } >> >> // Default placement versions of operator delete. >> inline void operator delete (void*, void*) _GLIBCXX_USE_NOEXCEPT { } >> >> >> >> Though I am not sure what the policy is for this kind of thing. And that's >> assuming it is indeed undefined to pass a null pointer to it, I don't have a >> good reference for that. >> >> >> >>> and it will not work on user-defined operator new if they are inlined. >> >> >> But then if that operator new is inlined, it may already contain a line like >> if(p==0)throw(); which lets the compiler know all it needs. > > I am not sure I like this version of the patch. The 2 patches are really independent, one (in the compiler) for regular operator new, and one (in the library) for the placement version. I mostly like this second patch because it is so easy ;-) But I will be happy if someone posts a better solution. > I think the appropriate patch should be in the compiler, not > here. C++ has several places where certain parameters cannot > have non-null values. For example, the implicit parameter 'this' > in non-static member functions. This is another instance. Indeed, I didn't check how gcc handles "this" being non-0. > Furthermore, I do think that the compiler should have special nodes > for both standard placement new and the global operator new functions, That's one way to do it. Since this is the first time I look at those, I don't really see the advantage compared to the current status, but I trust you. What would you do with this special-node placement new? Keep it as is until after vrp so we can use the !=0 property and then expand it to its first argument? Or expand it early to the equivalent of the library code I wrote above? > as I explained in previous messages. I couldn't find them, sorry if they contained information that makes this post irrelevant.
On Sat, Sep 7, 2013 at 12:59 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >> Furthermore, I do think that the compiler should have special nodes >> for both standard placement new and the global operator new functions, > > > That's one way to do it. Since this is the first time I look at those, I > don't really see the advantage compared to the current status, but I trust > you. What would you do with this special-node placement new? placement new really is about "calling" a contractor, and marking the beginning of the lifetime of a new object, hence aiding lifetime-based alias analysis. > Keep it as is > until after vrp so we can use the !=0 property and then expand it to its > first argument? Or expand it early to the equivalent of the library code I > wrote above? > > >> as I explained in previous messages. > > > I couldn't find them, sorry if they contained information that makes this > post irrelevant. http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01276.html http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01291.html -- Gaby
--- o/new 2013-09-07 11:11:17.388756320 +0200 +++ i/new 2013-09-07 18:00:32.204797291 +0200 @@ -144,9 +144,9 @@ // Default placement versions of operator new. inline void* operator new(std::size_t, void* __p) _GLIBCXX_USE_NOEXCEPT -{ return __p; } +{ if (!__p) __builtin_unreachable(); return __p; } inline void* operator new[](std::size_t, void* __p) _GLIBCXX_USE_NOEXCEPT -{ return __p; } +{ if (!__p) __builtin_unreachable(); return __p; } // Default placement versions of operator delete. inline void operator delete (void*, void*) _GLIBCXX_USE_NOEXCEPT { }