| Message ID | CAFk2RUbHhy7Qrjn+bPunYTvB_QOzDrrQx2_ZdbqM5H81qEHprA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
OK. Let's leave the BZ open to help remember to remove the cxx_dialect check in stage 1. Jason On Mon, Mar 20, 2017 at 9:21 PM, Ville Voutilainen <ville.voutilainen@gmail.com> wrote: > On 21 March 2017 at 02:46, Ville Voutilainen > <ville.voutilainen@gmail.com> wrote: >> On 21 March 2017 at 02:43, Ville Voutilainen >> <ville.voutilainen@gmail.com> wrote: >>> Hmm. I should either rename that function or flip its logic. Now it's >>> a bit backwards. :) I'll flip its logic. >> >> In other words, see attached. > > Let's call the function what it really is. > > 2017-03-21 Ville Voutilainen <ville.voutilainen@gmail.com> > > gcc/ > > PR c++/35878 > * cp/init.c (std_placement_new_fn_p): New. > (build_new_1): Call it. > > 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.
Hi! On Tue, Mar 21, 2017 at 03:21:11AM +0200, Ville Voutilainen wrote: Formatting etc. nits: > 2017-03-21 Ville Voutilainen <ville.voutilainen@gmail.com> > > gcc/ > > PR c++/35878 This should go into gcc/cp/ ChangeLog > * cp/init.c (std_placement_new_fn_p): New. without cp/ here. > (build_new_1): Call it. > +/* Determine whether an allocation function is a namespace-scope > + non-replaceable placement new function. See DR 1748. > + TODO: Enable in all standard modes. */ > +static bool std_placement_new_fn_p (tree alloc_fn) static bool std_placement_new_fn_p (tree alloc_fn) > @@ -3171,7 +3186,8 @@ build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts, > So check for a null exception spec on the op new we just called. */ > > nothrow = TYPE_NOTHROW_P (TREE_TYPE (alloc_fn)); > - check_new = (flag_check_new || nothrow); > + check_new = flag_check_new > + || (nothrow && !std_placement_new_fn_p (alloc_fn)); This should be either: check_new = flag_check_new || (nothrow && !std_placement_new_fn_p (alloc_fn)); or check_new = (flag_check_new || (nothrow && !std_placement_new_fn_p (alloc_fn))); i.e. || should not be when on next line at different column than flag_check_new (and the ()s to make emacs happy). > --- /dev/null > +++ b/gcc/testsuite/g++.dg/init/pr35878_1.C > @@ -0,0 +1,21 @@ > +// { dg-options "-O2 --std=gnu++11" } -O2 -std=gnu++11 is enough, no need for double dash --std=gnu++11. > +// { dg-do compile } > +// { dg-final { scan-assembler "test.*%rdi, %rdi" { target i?86-*-* x86_64-*-* } } } This will surely fail on 32-bit or with -mx32. So either you need to use it on { target { { i?86-*-* x86_64-*-* } && lp64 } } only, or perhaps instead of scanning assembler add -fdump-tree-optimized to dg-options and scan-tree-dump for the NULL? pointer comparison there. Jakub
On 21 March 2017 at 08:48, Jakub Jelinek <jakub@redhat.com> wrote: > Formatting etc. nits: > >> 2017-03-21 Ville Voutilainen <ville.voutilainen@gmail.com> >> >> gcc/ >> >> PR c++/35878 > > This should go into gcc/cp/ ChangeLog > >> * cp/init.c (std_placement_new_fn_p): New. > > without cp/ here. Yeah, so modified before the commit. >> nothrow = TYPE_NOTHROW_P (TREE_TYPE (alloc_fn)); >> - check_new = (flag_check_new || nothrow); >> + check_new = flag_check_new >> + || (nothrow && !std_placement_new_fn_p (alloc_fn)); > > This should be either: > check_new > = flag_check_new || (nothrow && !std_placement_new_fn_p (alloc_fn)); > or > check_new = (flag_check_new > || (nothrow && !std_placement_new_fn_p (alloc_fn))); > i.e. || should not be when on next line at different column than > flag_check_new (and the ()s to make emacs happy). Bummer - I simply indented that with emacs and thought it does the right thing. > >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/init/pr35878_1.C >> @@ -0,0 +1,21 @@ >> +// { dg-options "-O2 --std=gnu++11" } > > -O2 -std=gnu++11 is enough, no need for double dash --std=gnu++11. > >> +// { dg-do compile } >> +// { dg-final { scan-assembler "test.*%rdi, %rdi" { target i?86-*-* x86_64-*-* } } } > > This will surely fail on 32-bit or with -mx32. So either you need to use it > on { target { { i?86-*-* x86_64-*-* } && lp64 } } only, or perhaps instead > of scanning assembler add -fdump-tree-optimized to dg-options and > scan-tree-dump for the NULL? pointer comparison there. I weakly vote for the former, since that's less intrusive. I managed to commit the patch before I saw these remarks, so we need to patch that fix in as a separate tweak.
diff --git a/gcc/cp/init.c b/gcc/cp/init.c index 8bfcbde..a0cd64c 100644 --- a/gcc/cp/init.c +++ b/gcc/cp/init.c @@ -2693,6 +2693,21 @@ malloc_alignment () return MAX (max_align_t_align(), MALLOC_ABI_ALIGNMENT); } +/* Determine whether an allocation function is a namespace-scope + non-replaceable placement new function. See DR 1748. + TODO: Enable in all standard modes. */ +static bool std_placement_new_fn_p (tree alloc_fn) +{ + if ((cxx_dialect > cxx14) && DECL_NAMESPACE_SCOPE_P (alloc_fn)) + { + tree first_arg = TREE_CHAIN (TYPE_ARG_TYPES (TREE_TYPE (alloc_fn))); + if ((TREE_VALUE (first_arg) == ptr_type_node) + && TREE_CHAIN (first_arg) == void_list_node) + return true; + } + return false; +} + /* Generate code for a new-expression, including calling the "operator new" function, initializing the object, and, if an exception occurs during construction, cleaning up. The arguments are as for @@ -3171,7 +3186,8 @@ build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts, So check for a null exception spec on the op new we just called. */ nothrow = TYPE_NOTHROW_P (TREE_TYPE (alloc_fn)); - check_new = (flag_check_new || nothrow); + check_new = flag_check_new + || (nothrow && !std_placement_new_fn_p (alloc_fn)); if (cookie_size) { 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); +}