| Message ID | CADzB+2niH+fNdufwCs6tzbjo1Ums=JJnCyriqB0df6YBX3DdwA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | C++ PATCH for c++/86094, wrong calling convention for move-only class | expand |
On Mon, Jun 11, 2018 at 2:38 PM, Jason Merrill <jason@redhat.com> wrote: > The fix for 80178 was broken, because I forgot that copy_fn_p is false > for move constructors. As a result, the calling convention for a > class with a trivial move constructor and deleted copy constructor > changed inappropriately. This patch restores the broken behavior to -fabi-version=12 and adds -fabi-version=13 for the fix; people can use -Wabi=12 with GCC 8.2 to check for compatibility issues against 8.1, or -Wabi=11 to check compatibility with GCC 7. Tested x86_64-pc-linux-gnu, applying to trunk and 8. Do we want to accelerate 8.2 because of this issue? commit f118d95dd7802a80387982135686be27b3e0dbb7 Author: Jason Merrill <jason@redhat.com> Date: Wed Jun 13 13:35:29 2018 -0400 PR c++/86094 - wrong code with defaulted move ctor. gcc/c-family/ * c-opts.c (c_common_post_options): Bump the current ABI version to 13. Set warn_abi_version and flag_abi_compat_version to the current version rather than 0. Fix defaulting flag_abi_compat_version from warn_abi_version. gcc/cp/ * class.c (classtype_has_non_deleted_move_ctor): New. * tree.c (maybe_warn_parm_abi, type_has_nontrivial_copy_init): Handle v12 breakage. diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c index ddaaef37e1d..107359ec20d 100644 --- a/gcc/c-family/c-opts.c +++ b/gcc/c-family/c-opts.c @@ -915,31 +915,38 @@ c_common_post_options (const char **pfilename) if (flag_declone_ctor_dtor == -1) flag_declone_ctor_dtor = optimize_size; - if (warn_abi_version == -1) - { - if (flag_abi_compat_version != -1) - warn_abi_version = flag_abi_compat_version; - else - warn_abi_version = 0; - } - if (flag_abi_compat_version == 1) { warning (0, "%<-fabi-compat-version=1%> is not supported, using =2"); flag_abi_compat_version = 2; } - else if (flag_abi_compat_version == -1) + + /* Change flag_abi_version to be the actual current ABI level, for the + benefit of c_cpp_builtins, and to make comparison simpler. */ + const int latest_abi_version = 13; + /* Generate compatibility aliases for ABI v11 (7.1) by default. */ + const int abi_compat_default = 11; + +#define clamp(X) if (X == 0 || X > latest_abi_version) X = latest_abi_version + clamp (flag_abi_version); + clamp (warn_abi_version); + clamp (flag_abi_compat_version); +#undef clamp + + /* Default -Wabi= or -fabi-compat-version= from each other. */ + if (warn_abi_version == -1 && flag_abi_compat_version != -1) + warn_abi_version = flag_abi_compat_version; + else if (flag_abi_compat_version == -1 && warn_abi_version != -1) + flag_abi_compat_version = warn_abi_version; + else if (warn_abi_version == -1 && flag_abi_compat_version == -1) { - /* Generate compatibility aliases for ABI v11 (7.1) by default. */ - flag_abi_compat_version - = (flag_abi_version == 0 ? 11 : 0); + warn_abi_version = latest_abi_version; + if (flag_abi_version == latest_abi_version) + flag_abi_compat_version = abi_compat_default; + else + flag_abi_compat_version = latest_abi_version; } - /* Change flag_abi_version to be the actual current ABI level for the - benefit of c_cpp_builtins. */ - if (flag_abi_version == 0) - flag_abi_version = 12; - /* By default, enable the new inheriting constructor semantics along with ABI 11. New and old should coexist fine, but it is a change in what artificial symbols are generated. */ diff --git a/gcc/common.opt b/gcc/common.opt index 4aebcaf0656..efbeba33819 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -938,7 +938,11 @@ Driver Undocumented ; ; 12: Corrects the calling convention for classes with only deleted copy/move ; constructors and changes passing/returning of empty records. -; Default in G++ 8. +; Default in G++ 8.1. +; +; 13: Fixes the accidental change in 12 to the calling convention for classes +; with deleted copy constructor and trivial move constructor. +; Default in G++ 8.2. ; ; Additional positive integers will be assigned as new versions of ; the ABI become the default version of the ABI. diff --git a/gcc/cp/class.c b/gcc/cp/class.c index b6e78c6377d..9a397e3856c 100644 --- a/gcc/cp/class.c +++ b/gcc/cp/class.c @@ -5175,6 +5175,19 @@ classtype_has_move_assign_or_move_ctor_p (tree t, bool user_p) return false; } +/* True iff T has a move constructor that is not deleted. */ + +bool +classtype_has_non_deleted_move_ctor (tree t) +{ + if (CLASSTYPE_LAZY_MOVE_CTOR (t)) + lazily_declare_fn (sfk_move_constructor, t); + for (ovl_iterator iter (CLASSTYPE_CONSTRUCTORS (t)); iter; ++iter) + if (move_fn_p (*iter) && !DECL_DELETED_FN (*iter)) + return true; + return false; +} + /* If T, a class, has a user-provided copy constructor, copy assignment operator, or destructor, returns that function. Otherwise, null. */ diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 59ad38107a8..9a50d666cad 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -6163,6 +6163,7 @@ extern bool trivial_default_constructor_is_constexpr (tree); extern bool type_has_constexpr_default_constructor (tree); extern bool type_has_virtual_destructor (tree); extern bool classtype_has_move_assign_or_move_ctor_p (tree, bool user_declared); +extern bool classtype_has_non_deleted_move_ctor (tree); extern tree classtype_has_user_copy_or_dtor (tree); extern bool type_build_ctor_call (tree); extern bool type_build_dtor_call (tree); diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index 156d1e469c6..48a0ff37372 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -4071,8 +4071,21 @@ maybe_warn_parm_abi (tree t, location_t loc) || !deleted_copy_types->contains (t)) return; + if ((flag_abi_version == 12 || warn_abi_version == 12) + && classtype_has_non_deleted_move_ctor (t)) + { + if (flag_abi_version > 12) + warning_at (loc, OPT_Wabi, "-fabi-version=13 (GCC 8.2) fixes the " + "calling convention for %qT, which was accidentally " + "changed in 8.1", t); + else + warning_at (loc, OPT_Wabi, "-fabi-version=12 (GCC 8.1) accidentally " + "changes the calling convention for %qT", t); + return; + } + warning_at (loc, OPT_Wabi, "the calling convention for %qT changes in " - "-fabi-version=12 (GCC 8)", t); + "-fabi-version=13 (GCC 8.2)", t); static bool explained = false; if (!explained) { @@ -4114,6 +4127,7 @@ type_has_nontrivial_copy_init (const_tree type) bool saw_copy = false; bool saw_non_deleted = false; + bool saw_non_deleted_move = false; if (CLASSTYPE_LAZY_MOVE_CTOR (t)) saw_copy = saw_non_deleted = true; @@ -4135,7 +4149,7 @@ type_has_nontrivial_copy_init (const_tree type) for (ovl_iterator iter (CLASSTYPE_CONSTRUCTORS (t)); iter; ++iter) { tree fn = *iter; - if (copy_fn_p (fn) || move_fn_p (fn)) + if (copy_fn_p (fn)) { saw_copy = true; if (!DECL_DELETED_FN (fn)) @@ -4145,19 +4159,27 @@ type_has_nontrivial_copy_init (const_tree type) break; } } + else if (move_fn_p (fn)) + if (!DECL_DELETED_FN (fn)) + saw_non_deleted_move = true; } gcc_assert (saw_copy); - if (saw_copy && !saw_non_deleted) - { - if (warn_abi && abi_version_crosses (12)) - remember_deleted_copy (t); - if (abi_version_at_least (12)) - return true; - } + /* ABI v12 buggily ignored move constructors. */ + bool v11nontriv = false; + bool v12nontriv = !saw_non_deleted; + bool v13nontriv = !saw_non_deleted && !saw_non_deleted_move; + bool nontriv = (abi_version_at_least (13) ? v13nontriv + : flag_abi_version == 12 ? v12nontriv + : v11nontriv); + bool warn_nontriv = (warn_abi_version >= 13 ? v13nontriv + : warn_abi_version == 12 ? v12nontriv + : v11nontriv); + if (nontriv != warn_nontriv) + remember_deleted_copy (t); - return false; + return nontriv; } else return 0; diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index c55be3d95fd..99281eb4c85 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -2348,7 +2348,12 @@ implies @option{-fnew-inheriting-ctors}. Version 12, which first appeared in G++ 8, corrects the calling conventions for empty classes on the x86_64 target and for classes -with only deleted copy/move constructors. +with only deleted copy/move constructors. It accidentally changes the +calling convention for classes with a deleted copy constructor and a +trivial move constructor. + +Version 13, which first appeared in G++ 8.2, fixes the accidental +change in version 12. See also @option{-Wabi}. @@ -2359,7 +2364,7 @@ works around mangling changes by creating an alias with the correct mangled name when defining a symbol with an incorrect mangled name. This switch specifies which ABI version to use for the alias. -With @option{-fabi-version=0} (the default), this defaults to 8 (GCC 5 +With @option{-fabi-version=0} (the default), this defaults to 11 (GCC 7 compatibility). If another ABI version is explicitly selected, this defaults to 0. For compatibility with GCC versions 3.2 through 4.9, use @option{-fabi-compat-version=2}. diff --git a/gcc/testsuite/g++.dg/abi/invisiref2a.C b/gcc/testsuite/g++.dg/abi/invisiref2a.C new file mode 100644 index 00000000000..05330553287 --- /dev/null +++ b/gcc/testsuite/g++.dg/abi/invisiref2a.C @@ -0,0 +1,14 @@ +// PR c++/86094 +// { dg-do compile { target c++11 } } +// { dg-additional-options "-fabi-version=12 -Wabi -fdump-tree-gimple" } +// { dg-final { scan-tree-dump "struct S &" "gimple" } } + +struct S { + S(S&&) = default; + int i; +}; + +S foo(S s) // { dg-warning "calling convention" } +{ + return s; +} diff --git a/gcc/testsuite/g++.dg/abi/macro0.C b/gcc/testsuite/g++.dg/abi/macro0.C index 30eb02937c3..01a3541aa7d 100644 --- a/gcc/testsuite/g++.dg/abi/macro0.C +++ b/gcc/testsuite/g++.dg/abi/macro0.C @@ -1,6 +1,6 @@ // This testcase will need to be kept in sync with c_common_post_options. // { dg-options "-fabi-version=0" } -#if __GXX_ABI_VERSION != 1012 +#if __GXX_ABI_VERSION != 1013 #error "Incorrect value of __GXX_ABI_VERSION" #endif
On Wed, Jun 13, 2018 at 4:12 PM, Jason Merrill <jason@redhat.com> wrote: > On Mon, Jun 11, 2018 at 2:38 PM, Jason Merrill <jason@redhat.com> wrote: >> The fix for 80178 was broken, because I forgot that copy_fn_p is false >> for move constructors. As a result, the calling convention for a >> class with a trivial move constructor and deleted copy constructor >> changed inappropriately. > > This patch restores the broken behavior to -fabi-version=12 and adds > -fabi-version=13 for the fix; people can use -Wabi=12 with GCC 8.2 to > check for compatibility issues against 8.1, or -Wabi=11 to check > compatibility with GCC 7. > > Tested x86_64-pc-linux-gnu, applying to trunk and 8. Do we want to > accelerate 8.2 because of this issue? And one more patch, to suggest -Wabi=11 rather than useless plain -Wabi. Tested x86_64-pc-linux-gnu, applying to trunk and 8. commit bf7773e6fa607769b619535e1b22272a36726868 Author: Jason Merrill <jason@redhat.com> Date: Wed Jun 13 16:09:46 2018 -0400 * c-opts.c (c_common_post_options): Warn about useless -Wabi. (c_common_handle_option) [OPT_Wabi_]: Remove flag_abi_compat_version handling. diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c index 107359ec20d..bbcb1bb1a9c 100644 --- a/gcc/c-family/c-opts.c +++ b/gcc/c-family/c-opts.c @@ -417,8 +417,6 @@ c_common_handle_option (size_t scode, const char *arg, int value, value = 2; } warn_abi_version = value; - if (flag_abi_compat_version == -1) - flag_abi_compat_version = value; break; case OPT_fcanonical_system_headers: @@ -942,7 +940,17 @@ c_common_post_options (const char **pfilename) { warn_abi_version = latest_abi_version; if (flag_abi_version == latest_abi_version) - flag_abi_compat_version = abi_compat_default; + { + if (warning (OPT_Wabi, "-Wabi won't warn about anything")) + { + inform (input_location, "-Wabi warns about differences " + "from the most up-to-date ABI, which is also used " + "by default"); + inform (input_location, "use e.g. -Wabi=11 to warn about " + "changes from GCC 7"); + } + flag_abi_compat_version = abi_compat_default; + } else flag_abi_compat_version = latest_abi_version; } diff --git a/gcc/testsuite/g++.dg/abi/pr83489.C b/gcc/testsuite/g++.dg/abi/pr83489.C index aee04eca9d8..b6c82ef16e6 100644 --- a/gcc/testsuite/g++.dg/abi/pr83489.C +++ b/gcc/testsuite/g++.dg/abi/pr83489.C @@ -1,5 +1,5 @@ // PR c++/83489 -// { dg-options "-Wabi" } +// { dg-options "-Wabi=11" } struct A { diff --git a/gcc/testsuite/obj-c++.dg/bitfield-1.mm b/gcc/testsuite/obj-c++.dg/bitfield-1.mm index a63761904b5..084d2cb0916 100644 --- a/gcc/testsuite/obj-c++.dg/bitfield-1.mm +++ b/gcc/testsuite/obj-c++.dg/bitfield-1.mm @@ -5,7 +5,7 @@ superclasses should be removed). */ /* Contributed by Ziemowit Laski <zlaski@apple.com>. */ /* { dg-do run } */ -/* { dg-options "-Wpadded -Wabi" } */ +/* { dg-options "-Wpadded -Wabi=8" } */ /* Leave blank lines here to keep warnings on the same lines. */ diff --git a/gcc/testsuite/obj-c++.dg/layout-1.mm b/gcc/testsuite/obj-c++.dg/layout-1.mm index 33879ad17a4..89921236ec3 100644 --- a/gcc/testsuite/obj-c++.dg/layout-1.mm +++ b/gcc/testsuite/obj-c++.dg/layout-1.mm @@ -1,7 +1,7 @@ /* Ensure that we do not get bizarre warnings referring to __attribute__((packed)) or some such. */ /* { dg-do compile } */ -/* { dg-options "-Wpadded -Wpacked -Wabi" } */ +/* { dg-options "-Wpadded -Wpacked -Wabi=8" } */ #include "../objc-obj-c++-shared/TestsuiteObject.h"
On Wed, Jun 13, 2018 at 04:12:25PM -0400, Jason Merrill wrote: > On Mon, Jun 11, 2018 at 2:38 PM, Jason Merrill <jason@redhat.com> wrote: > > The fix for 80178 was broken, because I forgot that copy_fn_p is false > > for move constructors. As a result, the calling convention for a > > class with a trivial move constructor and deleted copy constructor > > changed inappropriately. > > This patch restores the broken behavior to -fabi-version=12 and adds > -fabi-version=13 for the fix; people can use -Wabi=12 with GCC 8.2 to > check for compatibility issues against 8.1, or -Wabi=11 to check > compatibility with GCC 7. > > Tested x86_64-pc-linux-gnu, applying to trunk and 8. Do we want to > accelerate 8.2 because of this issue? I'd like to see the powerpc64le-linux __ieee128 long double support in 8.2 too, accelerating 8.2 because of this would make that impossible. Jakub
On Wed, Jun 13, 2018 at 5:08 PM, Jakub Jelinek <jakub@redhat.com> wrote: > > On Wed, Jun 13, 2018 at 04:12:25PM -0400, Jason Merrill wrote: > > On Mon, Jun 11, 2018 at 2:38 PM, Jason Merrill <jason@redhat.com> wrote: > > > The fix for 80178 was broken, because I forgot that copy_fn_p is false > > > for move constructors. As a result, the calling convention for a > > > class with a trivial move constructor and deleted copy constructor > > > changed inappropriately. > > > > This patch restores the broken behavior to -fabi-version=12 and adds > > -fabi-version=13 for the fix; people can use -Wabi=12 with GCC 8.2 to > > check for compatibility issues against 8.1, or -Wabi=11 to check > > compatibility with GCC 7. > > > > Tested x86_64-pc-linux-gnu, applying to trunk and 8. Do we want to > > accelerate 8.2 because of this issue? > > I'd like to see the powerpc64le-linux __ieee128 long double support in 8.2 > too, accelerating 8.2 because of this would make that impossible. I'd think we could put that support in an August release regardless of what version number is on that release. Is it important that it be numbered 8.2? Jason
On Wed, Jun 13, 2018 at 5:05 PM, Jason Merrill <jason@redhat.com> wrote: > On Wed, Jun 13, 2018 at 4:12 PM, Jason Merrill <jason@redhat.com> wrote: >> On Mon, Jun 11, 2018 at 2:38 PM, Jason Merrill <jason@redhat.com> wrote: >>> The fix for 80178 was broken, because I forgot that copy_fn_p is false >>> for move constructors. As a result, the calling convention for a >>> class with a trivial move constructor and deleted copy constructor >>> changed inappropriately. >> >> This patch restores the broken behavior to -fabi-version=12 and adds >> -fabi-version=13 for the fix; people can use -Wabi=12 with GCC 8.2 to >> check for compatibility issues against 8.1, or -Wabi=11 to check >> compatibility with GCC 7. >> >> Tested x86_64-pc-linux-gnu, applying to trunk and 8. Do we want to >> accelerate 8.2 because of this issue? > > And one more patch, to suggest -Wabi=11 rather than useless plain -Wabi. ...and another improvement to the warning. Tested x86_64-pc-linux-gnu, applying to trunk and 8. commit 3825732dc47c121f0c2d08dfa1993d0cf008ce89 Author: Jason Merrill <jason@redhat.com> Date: Thu Jun 14 14:36:20 2018 -0400 * tree.c (maybe_warn_parm_abi): Inform the location of the class. diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index 48a0ff37372..a88481db1e0 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -4074,25 +4074,23 @@ maybe_warn_parm_abi (tree t, location_t loc) if ((flag_abi_version == 12 || warn_abi_version == 12) && classtype_has_non_deleted_move_ctor (t)) { + bool w; if (flag_abi_version > 12) - warning_at (loc, OPT_Wabi, "-fabi-version=13 (GCC 8.2) fixes the " - "calling convention for %qT, which was accidentally " - "changed in 8.1", t); + w = warning_at (loc, OPT_Wabi, "-fabi-version=13 (GCC 8.2) fixes the " + "calling convention for %qT, which was accidentally " + "changed in 8.1", t); else - warning_at (loc, OPT_Wabi, "-fabi-version=12 (GCC 8.1) accidentally " - "changes the calling convention for %qT", t); + w = warning_at (loc, OPT_Wabi, "-fabi-version=12 (GCC 8.1) accident" + "ally changes the calling convention for %qT", t); + if (w) + inform (location_of (t), " declared here"); return; } - warning_at (loc, OPT_Wabi, "the calling convention for %qT changes in " - "-fabi-version=13 (GCC 8.2)", t); - static bool explained = false; - if (!explained) - { - inform (loc, " because all of its copy and move constructors " - "are deleted"); - explained = true; - } + if (warning_at (loc, OPT_Wabi, "the calling convention for %qT changes in " + "-fabi-version=13 (GCC 8.2)", t)) + inform (location_of (t), " because all of its copy and move " + "constructors are deleted"); } /* Returns true iff copying an object of type T (including via move diff --git a/gcc/testsuite/g++.dg/abi/invisiref2a.C b/gcc/testsuite/g++.dg/abi/invisiref2a.C index 05330553287..127ee0ae763 100644 --- a/gcc/testsuite/g++.dg/abi/invisiref2a.C +++ b/gcc/testsuite/g++.dg/abi/invisiref2a.C @@ -3,7 +3,7 @@ // { dg-additional-options "-fabi-version=12 -Wabi -fdump-tree-gimple" } // { dg-final { scan-tree-dump "struct S &" "gimple" } } -struct S { +struct S { // { dg-message "" } S(S&&) = default; int i; };
commit d6cc7705f626778cb7e19fccf90117e7ed47d794
Author: Jason Merrill <jason@redhat.com>
Date: Mon Jun 11 12:14:27 2018 -0400
PR c++/86094 - wrong code with defaulted move ctor.
* tree.c (type_has_nontrivial_copy_init): Fix move ctor handling.
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index bbbda7e98b6..156d1e469c6 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -4135,7 +4135,7 @@ type_has_nontrivial_copy_init (const_tree type)
for (ovl_iterator iter (CLASSTYPE_CONSTRUCTORS (t)); iter; ++iter)
{
tree fn = *iter;
- if (copy_fn_p (fn))
+ if (copy_fn_p (fn) || move_fn_p (fn))
{
saw_copy = true;
if (!DECL_DELETED_FN (fn))
diff --git a/gcc/testsuite/g++.dg/abi/invisiref2.C b/gcc/testsuite/g++.dg/abi/invisiref2.C
new file mode 100644
index 00000000000..592d212ab11
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/invisiref2.C
@@ -0,0 +1,14 @@
+// PR c++/86094
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-Wabi=11 -fdump-tree-gimple" }
+// { dg-final { scan-tree-dump-not "struct S &" "gimple" } }
+
+struct S {
+ S(S&&) = default;
+ int i;
+};
+
+S foo(S s)
+{
+ return s;
+}