[v4] c++: Fix up synthetization of defaulted comparison operators on classes with bitfields [PR102490]

Message ID 20211001150717.GQ304296@tucnak
State Committed
Headers
Series [v4] c++: Fix up synthetization of defaulted comparison operators on classes with bitfields [PR102490] |

Commit Message

Jakub Jelinek Oct. 1, 2021, 3:07 p.m. UTC
  On Thu, Sep 30, 2021 at 03:01:49PM -0400, Jason Merrill wrote:
> > After fixing the incomplete std::strong_ordering spaceship-synth8.C is now
> > accepted, but I'm afraid I'm getting lost in this - clang++ rejects that
> > testcase instead complaining that D has <=> operator, but has it pure virtual.
> 
> Ah, I think we need to add LOOKUP_NO_VIRTUAL to the flags variable, as we do
> in do_build_copy_assign.  I suppose it wouldn't hurt to add LOOKUP_DEFAULTED
> as well.

I've tried that (see patch below), but neither in build_comparison_op, nor
in genericize_spaceship those changes made any difference for
spaceship-synth8.C, it is still accepted instead of rejected.

> > +      if (special_function_p (fn) == sfk_comparison)
> > +	{
> > +	  tree lhs = DECL_ARGUMENTS (fn);
> > +	  if (is_this_parameter (lhs))
> > +	    lhs = cp_build_fold_indirect_ref (lhs);
> > +	  else
> > +	    lhs = convert_from_reference (lhs);
> > +	  tree ctype = TYPE_MAIN_VARIANT (TREE_TYPE (lhs));
> > +	  /* If the comparison type is still incomplete, don't synthesize the
> > +	     method, just see if it is not implicitly deleted.  */
> > +	  if (!COMPLETE_TYPE_P (ctype))
> > +	    {
> > +	      push_deferring_access_checks (dk_no_deferred);
> > +	      build_comparison_op (fn, false, tf_none);
> > +	      pop_deferring_access_checks ();
> > +	      return !DECL_MAYBE_DELETED (fn);
> > +	    }
> > +	}
> > +
> >         ++function_depth;
> >         synthesize_method (fn);
> >         --function_depth;
> 
> Let's factor this (from the added code to here) into a
> maybe_synthesize_method in method.c.  That way build_comparison_op can stay
> static.

Ok, done.
In addition, I've added the testcases from PR98712 to the patch.
I'm still worried about maybe_synthesize_method, if done e.g. from
maybe_instantiate_noexcept before the class is COMPLETE_TYPE_P, could
end up deducing a wrong return type, one that e.g. doesn't take into account
base classes.  Tried that with spaceship-synth13.C testcase, but there
maybe_instantiate_noexcept isn't called early, and I'm out of ideas how to do
that.  Also, do maybe_instantiate_noexcept callers care just about the
return type of the method or also about whether something in the body could
throw?

2021-10-01  Jakub Jelinek  <jakub@redhat.com>

	PR c++/98712
	PR c++/102490
	* cp-tree.h (maybe_synthesize_method): Declare.
	* method.c (genericize_spaceship): Use
	LOOKUP_NORMAL | LOOKUP_NONVIRTUAL | LOOKUP_DEFAULTED instead of
	LOOKUP_NORMAL for flags.
	(comp_info): Remove defining member.
	(comp_info::comp_info): Remove complain argument, don't initialize
	defining.
	(build_comparison_op): Add defining argument. Adjust comp_info
	construction.  Use defining instead of info.defining.  Assert that if
	defining, ctype is a complete type.  Use
	LOOKUP_NORMAL | LOOKUP_NONVIRTUAL | LOOKUP_DEFAULTED instead of
	LOOKUP_NORMAL for flags.
	(synthesize_method, maybe_explain_implicit_delete,
	explain_implicit_non_constexpr): Adjust build_comparison_op callers.
	(maybe_synthesize_method): New function.
	* class.c (check_bases_and_members): Don't call defaulted_late_check
	for sfk_comparison.
	(finish_struct_1): Call it here instead after class has been completed.
	* pt.c (maybe_instantiate_noexcept): Call maybe_synthesize_method
	instead of synthesize_method.

	* g++.dg/cpp2a/spaceship-synth8.C (std::strong_ordering): Provide more
	complete definition.
	(std::strong_ordering::less, std::strong_ordering::equal,
	std::strong_ordering::greater): Define.
	* g++.dg/cpp2a/spaceship-synth12.C: New test.
	* g++.dg/cpp2a/spaceship-synth13.C: New test.
	* g++.dg/cpp2a/spaceship-eq11.C: New test.
	* g++.dg/cpp2a/spaceship-eq12.C: New test.
	* g++.dg/cpp2a/spaceship-eq13.C: New test.



	Jakub
  

Patch

--- gcc/cp/cp-tree.h.jj	2021-10-01 10:24:37.500266902 +0200
+++ gcc/cp/cp-tree.h	2021-10-01 16:40:08.880795343 +0200
@@ -7013,6 +7013,7 @@  extern void explain_implicit_non_constex
 extern bool deduce_inheriting_ctor		(tree);
 extern bool decl_remember_implicit_trigger_p	(tree);
 extern void synthesize_method			(tree);
+extern void maybe_synthesize_method		(tree);
 extern tree lazily_declare_fn			(special_function_kind,
 						 tree);
 extern tree skip_artificial_parms_for		(const_tree, tree);
--- gcc/cp/method.c.jj	2021-10-01 10:24:58.312971997 +0200
+++ gcc/cp/method.c	2021-10-01 16:46:18.018588835 +0200
@@ -1098,7 +1098,7 @@  genericize_spaceship (location_t loc, tr
 
   tree gt = lookup_comparison_result (tag, type, 1);
 
-  int flags = LOOKUP_NORMAL;
+  int flags = LOOKUP_NORMAL | LOOKUP_NONVIRTUAL | LOOKUP_DEFAULTED;
   tsubst_flags_t complain = tf_none;
   tree comp;
 
@@ -1288,21 +1288,16 @@  struct comp_info
 {
   tree fndecl;
   location_t loc;
-  bool defining;
   bool first_time;
   bool constexp;
   bool was_constexp;
   bool noex;
 
-  comp_info (tree fndecl, tsubst_flags_t &complain)
+  comp_info (tree fndecl)
     : fndecl (fndecl)
   {
     loc = DECL_SOURCE_LOCATION (fndecl);
 
-    /* We only have tf_error set when we're called from
-       explain_invalid_constexpr_fn or maybe_explain_implicit_delete.  */
-    defining = !(complain & tf_error);
-
     first_time = DECL_MAYBE_DELETED (fndecl);
     DECL_MAYBE_DELETED (fndecl) = false;
 
@@ -1364,15 +1359,15 @@  struct comp_info
    to use synthesize_method at the earliest opportunity and bail out if the
    function ends up being deleted.  */
 
-static void
-build_comparison_op (tree fndecl, tsubst_flags_t complain)
+void
+build_comparison_op (tree fndecl, bool defining, tsubst_flags_t complain)
 {
-  comp_info info (fndecl, complain);
+  comp_info info (fndecl);
 
-  if (!info.defining && !(complain & tf_error) && !DECL_MAYBE_DELETED (fndecl))
+  if (!defining && !(complain & tf_error) && !DECL_MAYBE_DELETED (fndecl))
     return;
 
-  int flags = LOOKUP_NORMAL;
+  int flags = LOOKUP_NORMAL | LOOKUP_NONVIRTUAL | LOOKUP_DEFAULTED;
   const ovl_op_info_t *op = IDENTIFIER_OVL_OP_INFO (DECL_NAME (fndecl));
   tree_code code = op->tree_code;
 
@@ -1384,6 +1379,7 @@  build_comparison_op (tree fndecl, tsubst
     lhs = convert_from_reference (lhs);
   rhs = convert_from_reference (rhs);
   tree ctype = TYPE_MAIN_VARIANT (TREE_TYPE (lhs));
+  gcc_assert (!defining || COMPLETE_TYPE_P (ctype));
 
   iloc_sentinel ils (info.loc);
 
@@ -1399,7 +1395,7 @@  build_comparison_op (tree fndecl, tsubst
     }
 
   tree compound_stmt = NULL_TREE;
-  if (info.defining)
+  if (defining)
     compound_stmt = begin_compound_stmt (0);
   else
     ++cp_unevaluated_operand;
@@ -1478,8 +1474,8 @@  build_comparison_op (tree fndecl, tsubst
 		break;
 	      tree idx;
 	      /* [1] array, no loop needed, just add [0] ARRAY_REF.
-		 Similarly if !info.defining.  */
-	      if (integer_zerop (maxval) || !info.defining)
+		 Similarly if !defining.  */
+	      if (integer_zerop (maxval) || !defining)
 		idx = size_zero_node;
 	      /* Some other array, will need runtime loop.  */
 	      else
@@ -1588,7 +1584,7 @@  build_comparison_op (tree fndecl, tsubst
 	  tree comp = comps[i];
 	  tree eq, retval = NULL_TREE, if_ = NULL_TREE;
 	  tree loop_indexes = NULL_TREE;
-	  if (info.defining)
+	  if (defining)
 	    {
 	      if (TREE_CODE (comp) == TREE_LIST)
 		{
@@ -1636,7 +1632,7 @@  build_comparison_op (tree fndecl, tsubst
 		comp = build_static_cast (input_location, rettype, comp,
 					  complain);
 	      info.check (comp);
-	      if (info.defining)
+	      if (defining)
 		{
 		  tree var = create_temporary_var (rettype);
 		  pushdecl (var);
@@ -1649,7 +1645,7 @@  build_comparison_op (tree fndecl, tsubst
 	    }
 	  tree ceq = contextual_conv_bool (eq, complain);
 	  info.check (ceq);
-	  if (info.defining)
+	  if (defining)
 	    {
 	      finish_if_stmt_cond (ceq, if_);
 	      finish_then_clause (if_);
@@ -1662,7 +1658,7 @@  build_comparison_op (tree fndecl, tsubst
 		finish_for_stmt (TREE_VALUE (loop_index));
 	    }
 	}
-      if (info.defining)
+      if (defining)
 	{
 	  tree val;
 	  if (code == EQ_EXPR)
@@ -1683,7 +1679,7 @@  build_comparison_op (tree fndecl, tsubst
 				NULL_TREE, NULL, complain);
       comp = contextual_conv_bool (comp, complain);
       info.check (comp);
-      if (info.defining)
+      if (defining)
 	{
 	  tree neg = build1 (TRUTH_NOT_EXPR, boolean_type_node, comp);
 	  finish_return_stmt (neg);
@@ -1696,12 +1692,12 @@  build_comparison_op (tree fndecl, tsubst
       tree comp2 = build_new_op (info.loc, code, flags, comp, integer_zero_node,
 				 NULL_TREE, NULL, complain);
       info.check (comp2);
-      if (info.defining)
+      if (defining)
 	finish_return_stmt (comp2);
     }
 
  out:
-  if (info.defining)
+  if (defining)
     finish_compound_stmt (compound_stmt);
   else
     --cp_unevaluated_operand;
@@ -1780,7 +1776,7 @@  synthesize_method (tree fndecl)
   else if (sfk == sfk_comparison)
     {
       /* Pass tf_none so the function is just deleted if there's a problem.  */
-      build_comparison_op (fndecl, tf_none);
+      build_comparison_op (fndecl, true, tf_none);
       need_body = false;
     }
 
@@ -1814,6 +1810,32 @@  synthesize_method (tree fndecl)
 	      fndecl);
 }
 
+/* Like synthesize_method, but don't actually synthesize defaulted comparison
+   methods if their class is still incomplete.  Just deduce the return
+   type in that case.  */
+
+void
+maybe_synthesize_method (tree fndecl)
+{
+  if (special_function_p (fndecl) == sfk_comparison)
+    {
+      tree lhs = DECL_ARGUMENTS (fndecl);
+      if (is_this_parameter (lhs))
+	lhs = cp_build_fold_indirect_ref (lhs);
+      else
+	lhs = convert_from_reference (lhs);
+      tree ctype = TYPE_MAIN_VARIANT (TREE_TYPE (lhs));
+      if (!COMPLETE_TYPE_P (ctype))
+	{
+	  push_deferring_access_checks (dk_no_deferred);
+	  build_comparison_op (fndecl, false, tf_none);
+	  pop_deferring_access_checks ();
+	  return;
+	}
+    }
+  return synthesize_method (fndecl);
+}
+
 /* Build a reference to type TYPE with cv-quals QUALS, which is an
    rvalue if RVALUE is true.  */
 
@@ -2753,7 +2775,7 @@  maybe_explain_implicit_delete (tree decl
 	  inform (DECL_SOURCE_LOCATION (decl),
 		  "%q#D is implicitly deleted because the default "
 		  "definition would be ill-formed:", decl);
-	  build_comparison_op (decl, tf_warning_or_error);
+	  build_comparison_op (decl, false, tf_warning_or_error);
 	}
       else if (!informed)
 	{
@@ -2814,7 +2836,7 @@  explain_implicit_non_constexpr (tree dec
   if (sfk == sfk_comparison)
     {
       DECL_DECLARED_CONSTEXPR_P (decl) = true;
-      build_comparison_op (decl, tf_warning_or_error);
+      build_comparison_op (decl, false, tf_warning_or_error);
       DECL_DECLARED_CONSTEXPR_P (decl) = false;
     }
   else
--- gcc/cp/class.c.jj	2021-10-01 10:24:37.440267752 +0200
+++ gcc/cp/class.c	2021-10-01 16:30:39.900821821 +0200
@@ -6133,7 +6133,8 @@  check_bases_and_members (tree t)
 		 no longer ill-formed, it is defined as deleted instead.  */
 	      DECL_DELETED_FN (fn) = true;
 	  }
-	defaulted_late_check (fn);
+	if (special_function_p (fn) != sfk_comparison)
+	  defaulted_late_check (fn);
       }
 
   if (LAMBDA_TYPE_P (t))
@@ -7467,7 +7468,14 @@  finish_struct_1 (tree t)
      for any static member objects of the type we're working on.  */
   for (x = TYPE_FIELDS (t); x; x = DECL_CHAIN (x))
     if (DECL_DECLARES_FUNCTION_P (x))
-      DECL_IN_AGGR_P (x) = false;
+      {
+	/* Synthesize constexpr defaulted comparisons.  */
+	if (!DECL_ARTIFICIAL (x)
+	    && DECL_DEFAULTED_IN_CLASS_P (x)
+	    && special_function_p (x) == sfk_comparison)
+	  defaulted_late_check (x);
+	DECL_IN_AGGR_P (x) = false;
+      }
     else if (VAR_P (x) && TREE_STATIC (x)
 	     && TREE_TYPE (x) != error_mark_node
 	     && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (x)), t))
--- gcc/cp/pt.c.jj	2021-10-01 10:24:37.639264932 +0200
+++ gcc/cp/pt.c	2021-10-01 16:41:01.157058011 +0200
@@ -25766,7 +25766,7 @@  maybe_instantiate_noexcept (tree fn, tsu
 	return true;
 
       ++function_depth;
-      synthesize_method (fn);
+      maybe_synthesize_method (fn);
       --function_depth;
       return !DECL_MAYBE_DELETED (fn);
     }
--- gcc/testsuite/g++.dg/cpp2a/spaceship-synth8.C.jj	2021-10-01 10:24:37.879261532 +0200
+++ gcc/testsuite/g++.dg/cpp2a/spaceship-synth8.C	2021-10-01 16:30:39.905821751 +0200
@@ -1,7 +1,18 @@ 
 // PR c++/94907
 // { dg-do compile { target c++20 } }
 
-namespace std { struct strong_ordering { }; }
+namespace std { struct strong_ordering {
+  int _v;
+  constexpr strong_ordering (int v) :_v(v) {}
+  constexpr operator int (void) const { return _v; }
+  static const strong_ordering less;
+  static const strong_ordering equal;
+  static const strong_ordering greater;
+};
+constexpr strong_ordering strong_ordering::less = -1;
+constexpr strong_ordering strong_ordering::equal = 0;
+constexpr strong_ordering strong_ordering::greater = 1;
+}
 
 struct E;
 struct D {
--- gcc/testsuite/g++.dg/cpp2a/spaceship-synth12.C.jj	2021-10-01 16:35:19.989870012 +0200
+++ gcc/testsuite/g++.dg/cpp2a/spaceship-synth12.C	2021-10-01 16:36:54.691534289 +0200
@@ -0,0 +1,24 @@ 
+// PR c++/98712
+// { dg-do run { target c++20 } }
+
+#include <compare>
+
+struct S
+{
+  int s = 0;
+  S(int s) : s(s) {}
+  auto operator<=>(const S&) const = default;
+};
+
+struct T : S
+{
+  T(int s) : S(s) {}
+  constexpr auto operator<=>(const T&) const = default;
+};
+
+int
+main()
+{
+  if (T(0) >= T(1))
+    __builtin_abort ();
+}
--- gcc/testsuite/g++.dg/cpp2a/spaceship-synth13.C.jj	2021-10-01 16:36:01.211288600 +0200
+++ gcc/testsuite/g++.dg/cpp2a/spaceship-synth13.C	2021-10-01 16:37:14.471255306 +0200
@@ -0,0 +1,29 @@ 
+// { dg-do compile { target c++20 } }
+
+#include <compare>
+#include <type_traits>
+
+struct E;
+struct D {
+  auto operator<=>(const D&) const = default;
+  float f;
+};
+struct E : D {
+  auto operator<=>(const E&) const = default;
+  int i;
+};
+extern E e1, e2;
+auto a = e1 <=> e2;
+static_assert (std::is_same_v <decltype (a), std::partial_ordering>);
+struct G;
+struct H {
+  constexpr auto operator<=>(const H&) const = default;
+  float f;
+};
+struct G : H {
+  constexpr auto operator<=>(const G&) const = default;
+  int i;
+};
+extern G g1, g2;
+auto c = g1 <=> g2;
+static_assert (std::is_same_v <decltype (c), std::partial_ordering>);
--- gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C.jj	2021-10-01 16:30:39.905821751 +0200
+++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq11.C	2021-10-01 16:30:39.905821751 +0200
@@ -0,0 +1,43 @@ 
+// PR c++/102490
+// { dg-do run { target c++20 } }
+
+struct A
+{
+  unsigned char a : 1;
+  unsigned char b : 1;
+  constexpr bool operator== (const A &) const = default;
+};
+
+struct B
+{
+  unsigned char a : 8;
+  int : 0;
+  unsigned char b : 7;
+  constexpr bool operator== (const B &) const = default;
+};
+
+struct C
+{
+  unsigned char a : 3;
+  unsigned char b : 1;
+  constexpr bool operator== (const C &) const = default;
+};
+
+void
+foo (C &x, int y)
+{
+  x.b = y;
+}
+
+int
+main ()
+{
+  A a{}, b{};
+  B c{}, d{};
+  C e{}, f{};
+  a.b = 1;
+  d.b = 1;
+  foo (e, 0);
+  foo (f, 1);
+  return a == b || c == d || e == f;
+}
--- gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C.jj	2021-10-01 16:30:39.905821751 +0200
+++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq12.C	2021-10-01 16:30:39.905821751 +0200
@@ -0,0 +1,5 @@ 
+// PR c++/102490
+// { dg-do run { target c++20 } }
+// { dg-options "-O2" }
+
+#include "spaceship-eq11.C"
--- gcc/testsuite/g++.dg/cpp2a/spaceship-eq13.C.jj	2021-10-01 16:33:54.176080628 +0200
+++ gcc/testsuite/g++.dg/cpp2a/spaceship-eq13.C	2021-10-01 16:38:40.436042812 +0200
@@ -0,0 +1,22 @@ 
+// PR c++/98712
+// { dg-do run { target c++20 } }
+
+struct S
+{
+  int s = 0;
+  S(int s) : s(s) {}
+  bool operator==(const S&) const = default;
+};
+
+struct T : S
+{
+  T(int s) : S(s) {}
+  constexpr bool operator==(const T&) const = default;
+};
+
+int
+main()
+{
+  if (T(0) == T(1))
+    __builtin_abort ();
+}