Patchwork operator new returns nonzero

login
register
mail settings
Submitter Marc Glisse
Date Sept. 7, 2013, 4:42 p.m.
Message ID <alpine.DEB.2.10.1309071804080.3585@laptop-mg.saclay.inria.fr>
Download mbox | patch
Permalink /patch/273385/
State New
Headers show

Comments

Marc Glisse - Sept. 7, 2013, 4:42 p.m.
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:




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 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.
Gabriel Dos Reis - Sept. 7, 2013, 5:27 p.m.
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
Marc Glisse - Sept. 7, 2013, 5:59 p.m.
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.
Gabriel Dos Reis - Sept. 7, 2013, 7:24 p.m.
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

Patch

--- 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 { }