[C++] PR c++/35878

Submitted by ville on March 20, 2017, 11:06 p.m.

Details

Message ID CAFk2RUaDq4ga1D-LcdOJzcn+UbF8m-YzeJRYpo3rULKtS=9qRw@mail.gmail.com
State New
Headers show

Commit Message

ville March 20, 2017, 11:06 p.m.
On 20 March 2017 at 04:27, Jason Merrill <jason@redhat.com> wrote:
> On Sun, Mar 19, 2017 at 6:19 PM, Ville Voutilainen
> <ville.voutilainen@gmail.com> wrote:
>> I ran the tests for g++.dg/init thus far. Does this patch make sense?
>
> The condition needs to be a lot more specific: DR 1748 only applies to
> the non-allocating forms in [new.delete.placement], not to other
> placement allocation functions.

Round 2:

The new tests tested on Linux-x64, finishing testing with the full suite on
Linux-PPC64.

2017-03-20  Ville Voutilainen  <ville.voutilainen@gmail.com>

    gcc/

    PR c++/35878
    * cp/init.c (build_new_1): Don't do a null check for
    a namespace-scope non-replaceable placement new
    in C++17 mode unless -fcheck-new is provided.

    testsuite/

    PR c++/35878
    * g++.dg/init/pr35878_1.C: New.
    * g++.dg/init/pr35878_2.C: Likewise.
    * g++.dg/init/pr35878_3.C: Likewise.

Comments

Marc Glisse March 20, 2017, 11:41 p.m.
On Tue, 21 Mar 2017, Ville Voutilainen wrote:

> On 20 March 2017 at 04:27, Jason Merrill <jason@redhat.com> wrote:
>> On Sun, Mar 19, 2017 at 6:19 PM, Ville Voutilainen
>> <ville.voutilainen@gmail.com> wrote:
>>> I ran the tests for g++.dg/init thus far. Does this patch make sense?
>>
>> The condition needs to be a lot more specific: DR 1748 only applies to
>> the non-allocating forms in [new.delete.placement], not to other
>> placement allocation functions.
>
> Round 2:
>
> The new tests tested on Linux-x64, finishing testing with the full suite on
> Linux-PPC64.
>
> 2017-03-20  Ville Voutilainen  <ville.voutilainen@gmail.com>
>
>    gcc/
>
>    PR c++/35878
>    * cp/init.c (build_new_1): Don't do a null check for
>    a namespace-scope non-replaceable placement new
>    in C++17 mode unless -fcheck-new is provided.

It looks strange to me. Why not change the definition of check_new instead 
of changing the condition that uses it? One of the tests for 
flag_check_new is redundant. In C++17 mode, you test for NULL return from 
throwing operator new, why? This is a DR, doesn't it mean that it should 
apply to all modes? Or is the hope that limiting it to an experimental 
mode might let it pass in stage 4?
Jason Merrill March 20, 2017, 11:44 p.m.
On Mon, Mar 20, 2017 at 7:41 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Tue, 21 Mar 2017, Ville Voutilainen wrote:
>
>> On 20 March 2017 at 04:27, Jason Merrill <jason@redhat.com> wrote:
>>> On Sun, Mar 19, 2017 at 6:19 PM, Ville Voutilainen
>>> <ville.voutilainen@gmail.com> wrote:
>>>> I ran the tests for g++.dg/init thus far. Does this patch make sense?
>>>
>>> The condition needs to be a lot more specific: DR 1748 only applies to
>>> the non-allocating forms in [new.delete.placement], not to other
>>> placement allocation functions.
>>
>> Round 2:
>>
>> The new tests tested on Linux-x64, finishing testing with the full suite on
>> Linux-PPC64.
>>
>> 2017-03-20  Ville Voutilainen  <ville.voutilainen@gmail.com>
>>
>>    gcc/
>>    PR c++/35878
>>    * cp/init.c (build_new_1): Don't do a null check for
>>    a namespace-scope non-replaceable placement new
>>    in C++17 mode unless -fcheck-new is provided.
>
> It looks strange to me. Why not change the definition of check_new instead
> of changing the condition that uses it?

Agreed.  Also, let's factor the new tests out into a function, say
non_allocating_fn_p.

> In C++17 mode, you test for NULL return from throwing operator
> new, why? This is a DR, doesn't it mean that it should apply to all modes?
> Or is the hope that limiting it to an experimental mode might let it pass in
> stage 4?

Bingo.

Jason

Patch hide | download patch | download mbox

diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 8bfcbde..cd34141 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -3450,7 +3450,21 @@  build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts,
     rval = TARGET_EXPR_INITIAL (alloc_expr);
   else
     {
-      if (check_new)
+      /* See DR 1748. Skip the null check in C++17 mode for non-replaceable
+	 namespace-scope placement new forms unless the user explicitly
+	 asks for the check.
+	 TODO: Skip the null check in other modes as well.  */
+      if ((check_new && (cxx_dialect <= cxx14))
+	  || flag_check_new
+	  || ((cxx_dialect > cxx14)
+	      && !(DECL_NAMESPACE_SCOPE_P (alloc_fn)
+		   && TREE_CHAIN (TYPE_ARG_TYPES (TREE_TYPE (alloc_fn)))
+		   && (TREE_VALUE (TREE_CHAIN
+				   (TYPE_ARG_TYPES (TREE_TYPE (alloc_fn))))
+		       == ptr_type_node)
+		   && (TREE_CHAIN (TREE_CHAIN
+				   (TYPE_ARG_TYPES (TREE_TYPE (alloc_fn))))
+		       == void_list_node))))
 	{
 	  tree ifexp = cp_build_binary_op (input_location,
 					   NE_EXPR, alloc_node,
diff --git a/gcc/testsuite/g++.dg/init/pr35878_1.C b/gcc/testsuite/g++.dg/init/pr35878_1.C
new file mode 100644
index 0000000..b45c009
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/pr35878_1.C
@@ -0,0 +1,21 @@ 
+// { dg-options "-O2 --std=gnu++11" }
+// { dg-do compile }
+// { dg-final { scan-assembler "test.*%rdi, %rdi" { target i?86-*-* x86_64-*-* } } }
+#include <new>
+#include <utility>
+
+struct s1{
+  int a;
+  int b;
+  int c;
+};
+
+void f1 (s1 * v, s1&& s)
+{
+	new (v) s1(std::move(s));
+}
+
+void f2 (s1 * v, s1&& s)
+{
+	*v = std::move(s);
+}
diff --git a/gcc/testsuite/g++.dg/init/pr35878_2.C b/gcc/testsuite/g++.dg/init/pr35878_2.C
new file mode 100644
index 0000000..0664494
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/pr35878_2.C
@@ -0,0 +1,21 @@ 
+// { dg-options "-O2 --std=gnu++17 -fcheck-new" }
+// { dg-do compile }
+// { dg-final { scan-assembler "test.*%rdi, %rdi" { target i?86-*-* x86_64-*-* } } }
+#include <new>
+#include <utility>
+
+struct s1{
+  int a;
+  int b;
+  int c;
+};
+
+void f1 (s1 * v, s1&& s)
+{
+	new (v) s1(std::move(s));
+}
+
+void f2 (s1 * v, s1&& s)
+{
+	*v = std::move(s);
+}
diff --git a/gcc/testsuite/g++.dg/init/pr35878_3.C b/gcc/testsuite/g++.dg/init/pr35878_3.C
new file mode 100644
index 0000000..8a5614f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/pr35878_3.C
@@ -0,0 +1,21 @@ 
+// { dg-options "-O2 --std=gnu++17" }
+// { dg-do compile }
+// { dg-final { scan-assembler-not "test.*%rdi, %rdi" { target i?86-*-* x86_64-*-* } } }
+#include <new>
+#include <utility>
+
+struct s1{
+  int a;
+  int b;
+  int c;
+};
+
+void f1 (s1 * v, s1&& s)
+{
+	new (v) s1(std::move(s));
+}
+
+void f2 (s1 * v, s1&& s)
+{
+	*v = std::move(s);
+}