diff mbox series

RFC: C++ PATCH for c++/69763, making C++ alignof match C _Alignof

Message ID CADzB+2=1oCCX2MNMtdYAXvfAR-nueujX7tKWGx=7_LSyw8t+mg@mail.gmail.com
State New
Headers show
Series RFC: C++ PATCH for c++/69763, making C++ alignof match C _Alignof | expand

Commit Message

Jason Merrill April 10, 2018, 5:12 p.m. UTC
In Joseph's patch for bug 52023, making _Alignof(double) 4 rather than
8 on x86, he deliberately didn't change the C++ behavior, based on
this wording in the C++ standard:

--
The alignment required for a type might be different when it is used
as the type of a complete object and when it is used as the type of a
subobject. [ Example:
  struct B { long double d; };
  struct D : virtual B { char c; };
When D is the type of a complete object, it will have a subobject of
type B, so it must be aligned appropriately for a long double. If D
appears as a subobject of another object that also has B as a virtual
base class, the B subobject might be part of a different subobject,
reducing the alignment requirements on the D subobject. — end example
] The result of the alignof operator reflects the alignment
requirement of the type in the complete-object case.
--

This suggests that alignof shouldn't consider alignment of non-static
data members.  I think that this wording is wrong, that "subobject"
was intended only to refer to base subobjects.

But really this is beside the point: the x86 ABI says that the
alignment of double is 4, so alignof(double) should be 4 regardless of
what GCC wants to do internally.  And I think the same is true of
__alignof__.

Thoughts?
Jason

Comments

Jonathan Wakely April 10, 2018, 7:42 p.m. UTC | #1
On 10/04/18 13:12 -0400, Jason Merrill wrote:
>In Joseph's patch for bug 52023, making _Alignof(double) 4 rather than
>8 on x86, he deliberately didn't change the C++ behavior, based on
>this wording in the C++ standard:
>
>--
>The alignment required for a type might be different when it is used
>as the type of a complete object and when it is used as the type of a
>subobject. [ Example:
>  struct B { long double d; };
>  struct D : virtual B { char c; };
>When D is the type of a complete object, it will have a subobject of
>type B, so it must be aligned appropriately for a long double. If D
>appears as a subobject of another object that also has B as a virtual
>base class, the B subobject might be part of a different subobject,
>reducing the alignment requirements on the D subobject. — end example
>] The result of the alignof operator reflects the alignment
>requirement of the type in the complete-object case.
>--
>
>This suggests that alignof shouldn't consider alignment of non-static
>data members.  I think that this wording is wrong, that "subobject"
>was intended only to refer to base subobjects.
>
>But really this is beside the point: the x86 ABI says that the
>alignment of double is 4, so alignof(double) should be 4 regardless of
>what GCC wants to do internally.  And I think the same is true of
>__alignof__.
>
>Thoughts?

I don't have a view on what's more correct, but better alignment (pun
intended) with C's _Alignof seems good to me.
Jason Merrill April 11, 2018, 6:13 p.m. UTC | #2
On Tue, Apr 10, 2018 at 1:12 PM, Jason Merrill <jason@redhat.com> wrote:
> But really this is beside the point: the x86 ABI says that the
> alignment of double is 4, so alignof(double) should be 4 regardless of
> what GCC wants to do internally.  And I think the same is true of
> __alignof__.

Particularly since the description of __alignof__ has been clarified:

"Some machines never actually require alignment; they allow reference
to any data type even at an odd address.  For these machines,
'__alignof__' reports the smallest alignment that GCC gives the data
type, usually as mandated by the target ABI."

To that end, here's a patch that makes __alignof__ agree with
_Alignof, fixing bug 10360:
commit 3528a49567a23c0fab5916119e8c4f2dcddd8f3c
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Apr 11 11:09:38 2018 -0400

            PR c/10360 - __alignof__(double) on x86.
    
            PR c++/69763 - wrong alignof(double) on x86.
    gcc/c-family/
            * c-common.c (c_sizeof_or_alignof_type): Always return minimum
            alignment.
            * c-common.h: Remove parameter from prototype.
            (c_sizeof, c_alignof): Remove argument.
    gcc/c/
            * c-parser.c (c_parser_alignas_specifier)
            (c_parser_alignof_expression): Drop min_align argument.
    gcc/cp/
            * typeck.c (cxx_sizeof_or_alignof_type): Drop min_align argument.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 7e6905e791e..5da030916cb 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3570,7 +3570,7 @@ c_common_get_alias_set (tree t)
 
 tree
 c_sizeof_or_alignof_type (location_t loc,
-			  tree type, bool is_sizeof, bool min_alignof,
+			  tree type, bool is_sizeof,
 			  int complain)
 {
   const char *op_name;
@@ -3637,10 +3637,8 @@ c_sizeof_or_alignof_type (location_t loc,
 	value = size_binop_loc (loc, CEIL_DIV_EXPR, TYPE_SIZE_UNIT (type),
 				size_int (TYPE_PRECISION (char_type_node)
 					  / BITS_PER_UNIT));
-      else if (min_alignof)
+      else
 	value = size_int (min_align_of_type (type));
-      else
-	value = size_int (TYPE_ALIGN_UNIT (type));
     }
 
   /* VALUE will have the middle-end integer type sizetype.
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 6cf7614f682..db04ce6cf6a 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -827,7 +827,7 @@ extern tree c_fully_fold (tree, bool, bool *, bool = false);
 extern tree c_wrap_maybe_const (tree, bool);
 extern tree c_common_truthvalue_conversion (location_t, tree);
 extern void c_apply_type_quals_to_decl (int, tree);
-extern tree c_sizeof_or_alignof_type (location_t, tree, bool, bool, int);
+extern tree c_sizeof_or_alignof_type (location_t, tree, bool, int);
 extern tree c_alignof_expr (location_t, tree);
 /* Print an error message for invalid operands to arith operation CODE.
    NOP_EXPR is used as a special case (see truthvalue_conversion).  */
@@ -854,8 +854,8 @@ extern tree fold_for_warn (tree);
 extern tree c_common_get_narrower (tree, int *);
 extern bool get_nonnull_operand (tree, unsigned HOST_WIDE_INT *);
 
-#define c_sizeof(LOC, T)  c_sizeof_or_alignof_type (LOC, T, true, false, 1)
-#define c_alignof(LOC, T) c_sizeof_or_alignof_type (LOC, T, false, false, 1)
+#define c_sizeof(LOC, T)  c_sizeof_or_alignof_type (LOC, T, true, 1)
+#define c_alignof(LOC, T) c_sizeof_or_alignof_type (LOC, T, false, 1)
 
 /* Subroutine of build_binary_op, used for certain operations.  */
 extern tree shorten_binary_op (tree result_type, tree op0, tree op1, bool bitwise);
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 47720861d3f..53e120071d9 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -3513,7 +3513,7 @@ c_parser_alignas_specifier (c_parser * parser)
       struct c_type_name *type = c_parser_type_name (parser);
       if (type != NULL)
 	ret = c_sizeof_or_alignof_type (loc, groktypename (type, NULL, NULL),
-					false, true, 1);
+					false, 1);
     }
   else
     ret = c_parser_expr_no_commas (parser, NULL).value;
@@ -7406,7 +7406,7 @@ c_parser_alignof_expression (c_parser *parser)
       in_alignof--;
       ret.value = c_sizeof_or_alignof_type (loc, groktypename (type_name,
 							       NULL, NULL),
-					    false, is_c11_alignof, 1);
+					    false, 1);
       ret.original_code = ERROR_MARK;
       ret.original_type = NULL;
       set_c_expr_source_range (&ret, start_loc, end_loc);
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 1e6996cde09..bf71d7458a8 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -1641,8 +1641,7 @@ cxx_sizeof_or_alignof_type (tree type, enum tree_code op, bool complain)
     }
 
   return c_sizeof_or_alignof_type (input_location, complete_type (type),
-				   op == SIZEOF_EXPR, true,
-				   complain);
+				   op == SIZEOF_EXPR, complain);
 }
 
 /* Return the size of the type, without producing any warnings for
Joseph Myers April 17, 2018, 12:50 p.m. UTC | #3
On Tue, 10 Apr 2018, Jason Merrill wrote:

> But really this is beside the point: the x86 ABI says that the
> alignment of double is 4, so alignof(double) should be 4 regardless of
> what GCC wants to do internally.  And I think the same is true of
> __alignof__.

__alignof__ needs to stay reflecting the preferred, standalone alignment 
of 8 bytes; changing that is ABI-incompatible; code such as that in 
stddef.h uses __attribute__((__aligned__(__alignof__(long long)))) to give 
structure members the same alignment those types would have for standalone 
objects.  (This doesn't affect the alignment of max_align_t *now* on i386, 
but only because that now includes __float128 as well.)
Joseph Myers April 17, 2018, 12:54 p.m. UTC | #4
On Wed, 11 Apr 2018, Jason Merrill wrote:

> On Tue, Apr 10, 2018 at 1:12 PM, Jason Merrill <jason@redhat.com> wrote:
> > But really this is beside the point: the x86 ABI says that the
> > alignment of double is 4, so alignof(double) should be 4 regardless of
> > what GCC wants to do internally.  And I think the same is true of
> > __alignof__.
> 
> Particularly since the description of __alignof__ has been clarified:
> 
> "Some machines never actually require alignment; they allow reference
> to any data type even at an odd address.  For these machines,
> '__alignof__' reports the smallest alignment that GCC gives the data
> type, usually as mandated by the target ABI."

That text seems inaccurate in the x86 double / long long case; it should 
reflect the preferred alignment of the type for a standalone object.
Jason Merrill April 23, 2018, 8:49 p.m. UTC | #5
On Tue, Apr 17, 2018 at 8:50 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Tue, 10 Apr 2018, Jason Merrill wrote:
>
>> But really this is beside the point: the x86 ABI says that the
>> alignment of double is 4, so alignof(double) should be 4 regardless of
>> what GCC wants to do internally.  And I think the same is true of
>> __alignof__.
>
> __alignof__ needs to stay reflecting the preferred, standalone alignment
> of 8 bytes; changing that is ABI-incompatible; code such as that in
> stddef.h uses __attribute__((__aligned__(__alignof__(long long)))) to give
> structure members the same alignment those types would have for standalone
> objects.  (This doesn't affect the alignment of max_align_t *now* on i386,
> but only because that now includes __float128 as well.)

In that case, here's a patch following the 52023 change to the C front
end, to change only "alignof".

Tested x86_64-pc-linux-gnu, applying to trunk.
commit c42f365685b86fa7cecc2666adf93037f7bd0289
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Apr 10 11:48:51 2018 -0400

            PR c++/69560 - wrong alignof(double) on x86.
    
            CWG 1879 - Inadequate definition of alignment requirement.
            * cp-tree.h (ALIGNOF_EXPR_STD_P): New.
            * typeck.c (cxx_sizeof_or_alignof_type): Add std_alignof parm.
            (cxx_sizeof_expr, cxx_sizeof_nowarn, cxx_alignas_expr)
            (cxx_alignof_expr): Pass it.
            * parser.c (cp_parser_unary_expression): Pass it.
            * pt.c (tsubst_copy): Copy it.
            (tsubst_copy_and_build): Pass it.
            * decl.c (fold_sizeof_expr): Pass it.

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 5af4928e2fb..37770770acb 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -372,6 +372,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
       TEMPLATE_TYPE_PARM_FOR_CLASS (TEMPLATE_TYPE_PARM)
       DECL_NAMESPACE_INLINE_P (in NAMESPACE_DECL)
       SWITCH_STMT_ALL_CASES_P (in SWITCH_STMT)
+      ALIGNOF_EXPR_STD_P (in ALIGNOF_EXPR)
    1: IDENTIFIER_KIND_BIT_1 (in IDENTIFIER_NODE)
       TI_PENDING_TEMPLATE_FLAG.
       TEMPLATE_PARMS_FOR_INLINE.
@@ -4954,6 +4955,10 @@ more_aggr_init_expr_args_p (const aggr_init_expr_arg_iterator *iter)
 #define SIZEOF_EXPR_TYPE_P(NODE) \
   TREE_LANG_FLAG_0 (SIZEOF_EXPR_CHECK (NODE))
 
+/* True if the ALIGNOF_EXPR was spelled "alignof".  */
+#define ALIGNOF_EXPR_STD_P(NODE) \
+  TREE_LANG_FLAG_0 (ALIGNOF_EXPR_CHECK (NODE))
+
 /* An enumeration of the kind of tags that C++ accepts.  */
 enum tag_types {
   none_type = 0, /* Not a tag type.  */
@@ -7195,7 +7200,7 @@ extern int comp_cv_qualification		(const_tree, const_tree);
 extern int comp_cv_qualification		(int, int);
 extern int comp_cv_qual_signature		(tree, tree);
 extern tree cxx_sizeof_or_alignof_expr		(tree, enum tree_code, bool);
-extern tree cxx_sizeof_or_alignof_type		(tree, enum tree_code, bool);
+extern tree cxx_sizeof_or_alignof_type		(tree, enum tree_code, bool, bool);
 extern tree cxx_alignas_expr                    (tree);
 extern tree cxx_sizeof_nowarn                   (tree);
 extern tree is_bitfield_expr_with_lowered_type  (const_tree);
@@ -7292,7 +7297,7 @@ extern tree cp_build_binary_op                  (location_t,
 extern tree build_x_vec_perm_expr               (location_t,
 						 tree, tree, tree,
 						 tsubst_flags_t);
-#define cxx_sizeof(T)  cxx_sizeof_or_alignof_type (T, SIZEOF_EXPR, true)
+#define cxx_sizeof(T)  cxx_sizeof_or_alignof_type (T, SIZEOF_EXPR, false, true)
 extern tree build_simple_component_ref		(tree, tree);
 extern tree build_ptrmemfunc_access_expr	(tree, tree);
 extern tree build_address			(tree);
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index d822745b81f..55e234334ac 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -9542,10 +9542,10 @@ fold_sizeof_expr (tree t)
   tree r;
   if (SIZEOF_EXPR_TYPE_P (t))
     r = cxx_sizeof_or_alignof_type (TREE_TYPE (TREE_OPERAND (t, 0)),
-				    SIZEOF_EXPR, false);
+				    SIZEOF_EXPR, false, false);
   else if (TYPE_P (TREE_OPERAND (t, 0)))
     r = cxx_sizeof_or_alignof_type (TREE_OPERAND (t, 0), SIZEOF_EXPR,
-				    false);
+				    false, false);
   else
     r = cxx_sizeof_or_alignof_expr (TREE_OPERAND (t, 0), SIZEOF_EXPR,
 				    false);
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index bf46165f5ae..d8ce28a6d61 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -7993,19 +7993,22 @@ cp_parser_unary_expression (cp_parser *parser, cp_id_kind * pidk,
 	    location_t start_loc = token->location;
 
 	    op = keyword == RID_ALIGNOF ? ALIGNOF_EXPR : SIZEOF_EXPR;
+	    bool std_alignof = id_equal (token->u.value, "alignof");
+
 	    /* Consume the token.  */
 	    cp_lexer_consume_token (parser->lexer);
 	    /* Parse the operand.  */
 	    operand = cp_parser_sizeof_operand (parser, keyword);
 
 	    if (TYPE_P (operand))
-	      ret = cxx_sizeof_or_alignof_type (operand, op, true);
+	      ret = cxx_sizeof_or_alignof_type (operand, op, std_alignof,
+						true);
 	    else
 	      {
 		/* ISO C++ defines alignof only with types, not with
 		   expressions. So pedwarn if alignof is used with a non-
 		   type expression. However, __alignof__ is ok.  */
-		if (id_equal (token->u.value, "alignof"))
+		if (std_alignof)
 		  pedwarn (token->location, OPT_Wpedantic,
 			   "ISO C++ does not allow %<alignof%> "
 			   "with a non-type");
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index e2a12b963ad..9fb819722ec 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -15598,7 +15598,8 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
 		expanded = make_argument_pack (expanded);
 
 	      if (TYPE_P (expanded))
-		return cxx_sizeof_or_alignof_type (expanded, SIZEOF_EXPR, 
+		return cxx_sizeof_or_alignof_type (expanded, SIZEOF_EXPR,
+						   false,
 						   complain & tf_error);
 	      else
 		return cxx_sizeof_or_alignof_expr (expanded, SIZEOF_EXPR,
@@ -15636,7 +15637,10 @@ tsubst_copy (tree t, tree args, tsubst_flags_t complain, tree in_decl)
       {
 	tree type = tsubst (TREE_TYPE (t), args, complain, in_decl);
 	tree op0 = tsubst_copy (TREE_OPERAND (t, 0), args, complain, in_decl);
-	return build1 (code, type, op0);
+	r = build1 (code, type, op0);
+	if (code == ALIGNOF_EXPR)
+	  ALIGNOF_EXPR_STD_P (r) = ALIGNOF_EXPR_STD_P (t);
+	return r;
       }
 
     case COMPONENT_REF:
@@ -18002,6 +18006,8 @@ tsubst_copy_and_build (tree t,
 	op1 = TREE_OPERAND (t, 0);
 	if (TREE_CODE (t) == SIZEOF_EXPR && SIZEOF_EXPR_TYPE_P (t))
 	  op1 = TREE_TYPE (op1);
+	bool std_alignof = (TREE_CODE (t) == ALIGNOF_EXPR
+			    && ALIGNOF_EXPR_STD_P (t));
         if (!args)
 	  {
 	    /* When there are no ARGS, we are trying to evaluate a
@@ -18025,7 +18031,7 @@ tsubst_copy_and_build (tree t,
 	    --c_inhibit_evaluation_warnings;
 	  }
         if (TYPE_P (op1))
-	  r = cxx_sizeof_or_alignof_type (op1, TREE_CODE (t),
+	  r = cxx_sizeof_or_alignof_type (op1, TREE_CODE (t), std_alignof,
 					  complain & tf_error);
 	else
 	  r = cxx_sizeof_or_alignof_expr (op1, TREE_CODE (t),
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index f5081c1661c..01baf6c17a3 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -1597,10 +1597,13 @@ compparms (const_tree parms1, const_tree parms2)
 
 
 /* Process a sizeof or alignof expression where the operand is a
-   type.  */
+   type. STD_ALIGNOF indicates whether an alignof has C++11 (minimum alignment)
+   or GNU (preferred alignment) semantics; it is ignored if op is
+   SIZEOF_EXPR.  */
 
 tree
-cxx_sizeof_or_alignof_type (tree type, enum tree_code op, bool complain)
+cxx_sizeof_or_alignof_type (tree type, enum tree_code op, bool std_alignof,
+			    bool complain)
 {
   tree value;
   bool dependent_p;
@@ -1637,11 +1640,13 @@ cxx_sizeof_or_alignof_type (tree type, enum tree_code op, bool complain)
     {
       value = build_min (op, size_type_node, type);
       TREE_READONLY (value) = 1;
+      if (op == ALIGNOF_EXPR && std_alignof)
+	ALIGNOF_EXPR_STD_P (value) = true;
       return value;
     }
 
   return c_sizeof_or_alignof_type (input_location, complete_type (type),
-				   op == SIZEOF_EXPR, false,
+				   op == SIZEOF_EXPR, std_alignof,
 				   complain);
 }
 
@@ -1659,7 +1664,7 @@ cxx_sizeof_nowarn (tree type)
   else if (!COMPLETE_TYPE_P (type))
     return size_zero_node;
   else
-    return cxx_sizeof_or_alignof_type (type, SIZEOF_EXPR, false);
+    return cxx_sizeof_or_alignof_type (type, SIZEOF_EXPR, false, false);
 }
 
 /* Process a sizeof expression where the operand is an expression.  */
@@ -1725,7 +1730,7 @@ cxx_sizeof_expr (tree e, tsubst_flags_t complain)
   else
     e = TREE_TYPE (e);
 
-  return cxx_sizeof_or_alignof_type (e, SIZEOF_EXPR, complain & tf_error);
+  return cxx_sizeof_or_alignof_type (e, SIZEOF_EXPR, false, complain & tf_error);
 }
 
 /* Implement the __alignof keyword: Return the minimum required
@@ -1786,7 +1791,7 @@ cxx_alignof_expr (tree e, tsubst_flags_t complain)
       t = size_one_node;
     }
   else
-    return cxx_sizeof_or_alignof_type (TREE_TYPE (e), ALIGNOF_EXPR, 
+    return cxx_sizeof_or_alignof_type (TREE_TYPE (e), ALIGNOF_EXPR, false,
                                        complain & tf_error);
 
   return fold_convert (size_type_node, t);
@@ -1825,7 +1830,7 @@ cxx_alignas_expr (tree e)
 	   alignas(type-id ), it shall have the same effect as
 	   alignas(alignof(type-id )).  */
 
-    return cxx_sizeof_or_alignof_type (e, ALIGNOF_EXPR, false);
+    return cxx_sizeof_or_alignof_type (e, ALIGNOF_EXPR, true, false);
   
   /* If we reach this point, it means the alignas expression if of
      the form "alignas(assignment-expression)", so we should follow
diff --git a/gcc/testsuite/g++.dg/abi/align2.C b/gcc/testsuite/g++.dg/abi/align2.C
new file mode 100644
index 00000000000..c9f3a8fae24
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/align2.C
@@ -0,0 +1,5 @@
+// PR c++/69560
+// { dg-do compile { target { ia32 && c++11 } } }
+
+#define SA(X) static_assert ((X), #X)
+SA(alignof(double) == 4);
diff --git a/libcc1/libcp1plugin.cc b/libcc1/libcp1plugin.cc
index 0137cc9e4e1..10341477c56 100644
--- a/libcc1/libcp1plugin.cc
+++ b/libcc1/libcp1plugin.cc
@@ -3046,7 +3046,8 @@ plugin_build_unary_type_expr (cc1_plugin::connection *self,
       break;
 
     default:
-      result = cxx_sizeof_or_alignof_type (type, opcode, true);
+      /* Use the C++11 alignof semantics.  */
+      result = cxx_sizeof_or_alignof_type (type, opcode, true, true);
     }
 
   if (template_dependent_p)
Jakub Jelinek April 24, 2018, 1:19 p.m. UTC | #6
On Mon, Apr 23, 2018 at 04:49:24PM -0400, Jason Merrill wrote:
> On Tue, Apr 17, 2018 at 8:50 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> > On Tue, 10 Apr 2018, Jason Merrill wrote:
> >
> >> But really this is beside the point: the x86 ABI says that the
> >> alignment of double is 4, so alignof(double) should be 4 regardless of
> >> what GCC wants to do internally.  And I think the same is true of
> >> __alignof__.
> >
> > __alignof__ needs to stay reflecting the preferred, standalone alignment
> > of 8 bytes; changing that is ABI-incompatible; code such as that in
> > stddef.h uses __attribute__((__aligned__(__alignof__(long long)))) to give
> > structure members the same alignment those types would have for standalone
> > objects.  (This doesn't affect the alignment of max_align_t *now* on i386,
> > but only because that now includes __float128 as well.)
> 
> In that case, here's a patch following the 52023 change to the C front
> end, to change only "alignof".
> 
> Tested x86_64-pc-linux-gnu, applying to trunk.

This regressed:
+FAIL: c-c++-common/attr-aligned-1.c  -std=c++11 (test for excess errors)
+UNRESOLVED: c-c++-common/attr-aligned-1.c  -std=c++11 compilation failed to produce executable
+FAIL: c-c++-common/attr-aligned-1.c  -std=c++14 (test for excess errors)
+UNRESOLVED: c-c++-common/attr-aligned-1.c  -std=c++14 compilation failed to produce executable
+FAIL: g++.dg/cpp0x/alignas4.C  -std=c++11  scan-assembler align 8
+FAIL: g++.dg/cpp0x/alignas4.C  -std=c++14  scan-assembler align 8
on i686-linux, can be reproduced even with just
make check-gcc check-g++ RUNTESTFLAGS="--target_board=unix\{-m64,-m32\} dg.exp='attr-aligned-1.c alignas4.C'"
on x86_64-linux.

On alignas4.C, this was changed with r249409, perhaps we can limit
the target to && { ! ia32 } or similar, or do we want to expect align 4
for ia32?

attr-aligned-1.c testcase doesn't work if
__alignof__ (double) != alignof (double).  Either we should use __alignof__
in the static_asserts, or guard the static_asserts for targets known not to
have different results between the two.

> commit c42f365685b86fa7cecc2666adf93037f7bd0289
> Author: Jason Merrill <jason@redhat.com>
> Date:   Tue Apr 10 11:48:51 2018 -0400
> 
>             PR c++/69560 - wrong alignof(double) on x86.
>     
>             CWG 1879 - Inadequate definition of alignment requirement.
>             * cp-tree.h (ALIGNOF_EXPR_STD_P): New.
>             * typeck.c (cxx_sizeof_or_alignof_type): Add std_alignof parm.
>             (cxx_sizeof_expr, cxx_sizeof_nowarn, cxx_alignas_expr)
>             (cxx_alignof_expr): Pass it.
>             * parser.c (cp_parser_unary_expression): Pass it.
>             * pt.c (tsubst_copy): Copy it.
>             (tsubst_copy_and_build): Pass it.
>             * decl.c (fold_sizeof_expr): Pass it.

	Jakub
Jason Merrill April 24, 2018, 3:55 p.m. UTC | #7
On Tue, Apr 24, 2018 at 9:19 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Apr 23, 2018 at 04:49:24PM -0400, Jason Merrill wrote:
>> On Tue, Apr 17, 2018 at 8:50 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>> > On Tue, 10 Apr 2018, Jason Merrill wrote:
>> >
>> >> But really this is beside the point: the x86 ABI says that the
>> >> alignment of double is 4, so alignof(double) should be 4 regardless of
>> >> what GCC wants to do internally.  And I think the same is true of
>> >> __alignof__.
>> >
>> > __alignof__ needs to stay reflecting the preferred, standalone alignment
>> > of 8 bytes; changing that is ABI-incompatible; code such as that in
>> > stddef.h uses __attribute__((__aligned__(__alignof__(long long)))) to give
>> > structure members the same alignment those types would have for standalone
>> > objects.  (This doesn't affect the alignment of max_align_t *now* on i386,
>> > but only because that now includes __float128 as well.)
>>
>> In that case, here's a patch following the 52023 change to the C front
>> end, to change only "alignof".
>>
>> Tested x86_64-pc-linux-gnu, applying to trunk.
>
> This regressed:
> +FAIL: c-c++-common/attr-aligned-1.c  -std=c++11 (test for excess errors)
> +UNRESOLVED: c-c++-common/attr-aligned-1.c  -std=c++11 compilation failed to produce executable
> +FAIL: c-c++-common/attr-aligned-1.c  -std=c++14 (test for excess errors)
> +UNRESOLVED: c-c++-common/attr-aligned-1.c  -std=c++14 compilation failed to produce executable
> +FAIL: g++.dg/cpp0x/alignas4.C  -std=c++11  scan-assembler align 8
> +FAIL: g++.dg/cpp0x/alignas4.C  -std=c++14  scan-assembler align 8
> on i686-linux, can be reproduced even with just
> make check-gcc check-g++ RUNTESTFLAGS="--target_board=unix\{-m64,-m32\} dg.exp='attr-aligned-1.c alignas4.C'"
> on x86_64-linux.

Thanks.  How is your build tree configured so that this finds the
appropriate runtime libraries?

> On alignas4.C, this was changed with r249409, perhaps we can limit
> the target to && { ! ia32 } or similar, or do we want to expect align 4
> for ia32?
>
> attr-aligned-1.c testcase doesn't work if
> __alignof__ (double) != alignof (double).  Either we should use __alignof__
> in the static_asserts, or guard the static_asserts for targets known not to
> have different results between the two.

Done, thanks.
commit 006eabc3e6328b219f2eee271439cb90f8890856
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Apr 24 11:50:34 2018 -0400

    Testsuite fixes for C++11 alignof change.
    
    gcc/testsuite:
            * c-c++-common/attr-aligned-1.c: Use __alignof__ in C++11.
            * g++.dg/cpp0x/alignas4.C: Expect 4-byte alignment on x86.

diff --git a/gcc/testsuite/c-c++-common/attr-aligned-1.c b/gcc/testsuite/c-c++-common/attr-aligned-1.c
index 671e86baeb6..26b62393ac0 100644
--- a/gcc/testsuite/c-c++-common/attr-aligned-1.c
+++ b/gcc/testsuite/c-c++-common/attr-aligned-1.c
@@ -17,8 +17,8 @@ main ()
 }
 
 #if defined(__cplusplus) && __cplusplus >= 201103L
-static_assert (alignof (S) == 2 * alignof (double), "alignment of S");
-static_assert (alignof (T) == 2 * alignof (double), "alignment of T");
-static_assert (alignof (const S) == 2 * alignof (double), "alignment of const S");
-static_assert (alignof (const T) == 2 * alignof (double), "alignment of const T");
+static_assert (alignof (S) == 2 * __alignof__ (double), "alignment of S");
+static_assert (alignof (T) == 2 * __alignof__ (double), "alignment of T");
+static_assert (alignof (const S) == 2 * __alignof__ (double), "alignment of const S");
+static_assert (alignof (const T) == 2 * __alignof__ (double), "alignment of const T");
 #endif
diff --git a/gcc/testsuite/g++.dg/cpp0x/alignas4.C b/gcc/testsuite/g++.dg/cpp0x/alignas4.C
index baa56eb8ac5..b66fa651bc2 100644
--- a/gcc/testsuite/g++.dg/cpp0x/alignas4.C
+++ b/gcc/testsuite/g++.dg/cpp0x/alignas4.C
@@ -1,6 +1,7 @@
 // PR c++/59012
 // { dg-do compile { target c++11 } }
-// { dg-final { scan-assembler "align 8" { target { { i?86-*-* x86_64-*-* } && { ! *-*-darwin* } } } } }
+// { dg-final { scan-assembler "align 8" { target { { i?86-*-* x86_64-*-* } && { { ! ia32 } && { ! *-*-darwin* } } } } } }
+// { dg-final { scan-assembler "align 4" { target ia32 } } }
 
 template <class... T>
 struct A
Jakub Jelinek April 24, 2018, 4:24 p.m. UTC | #8
On Tue, Apr 24, 2018 at 11:55:44AM -0400, Jason Merrill wrote:
> >> Tested x86_64-pc-linux-gnu, applying to trunk.
> >
> > This regressed:
> > +FAIL: c-c++-common/attr-aligned-1.c  -std=c++11 (test for excess errors)
> > +UNRESOLVED: c-c++-common/attr-aligned-1.c  -std=c++11 compilation failed to produce executable
> > +FAIL: c-c++-common/attr-aligned-1.c  -std=c++14 (test for excess errors)
> > +UNRESOLVED: c-c++-common/attr-aligned-1.c  -std=c++14 compilation failed to produce executable
> > +FAIL: g++.dg/cpp0x/alignas4.C  -std=c++11  scan-assembler align 8
> > +FAIL: g++.dg/cpp0x/alignas4.C  -std=c++14  scan-assembler align 8
> > on i686-linux, can be reproduced even with just
> > make check-gcc check-g++ RUNTESTFLAGS="--target_board=unix\{-m64,-m32\} dg.exp='attr-aligned-1.c alignas4.C'"
> > on x86_64-linux.
> 
> Thanks.  How is your build tree configured so that this finds the
> appropriate runtime libraries?

For the above make check or for i686-linux bootstraps?

For the former, nothing special is needed, on Fedora/RHEL I guess just
making sure {glibc{,-devel},libgcc}.i686 is installed.

For the latter, I'm using hack scripts:
for i in ~/hbin/*; do echo $i; cat $i; done
/home/jakub/hbin/as
#!/bin/sh
exec /usr/bin/as --32 "$@"
/home/jakub/hbin/g++
#!/bin/sh
exec /usr/bin/g++ -m32 "$@"
/home/jakub/hbin/gcc
#!/bin/sh
exec /usr/bin/gcc -m32 "$@"
/home/jakub/hbin/ld
#!/bin/sh
case "$*" in
  --version) cat <<\EOF
GNU ld version 2.20.52.0.1-10.fc17 20100131
Copyright 2012 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) a later version.
This program has absolutely no warranty.
EOF
  exit 0;;
esac
exec /usr/bin/ld -m elf_i386 -L /usr/lib/ "$@"

The ld ugliness is to avoid lto-plugin.  And then just
PATH=~/hbin:$PATH i386 ../configure --enable-languages=default,obj-c++,lto,go,brig --enable-checking=yes,rtl,extra && PATH=~/hbin:$PATH i386 make -j16 bootstrap > LOG 2>&1 && PATH=~/hbin:$PATH i386 make -j16 -k check > LOGC 2>&1; ../contrib/test_summary > LOGT 2>&1

	Jakub
diff mbox series

Patch

commit 3ca02d94a346c46edc08185897aebfda03d17969
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Apr 10 13:09:37 2018 -0400

            PR c++/69763 - wrong alignof(double) on x86.
    
            * typeck.c (cxx_sizeof_or_alignof_type): Request min_align
            from c_sizeof_or_alignof_type.

diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index e5ad54dbcd1..442cf9df282 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -1641,7 +1641,7 @@  cxx_sizeof_or_alignof_type (tree type, enum tree_code op, bool complain)
     }
 
   return c_sizeof_or_alignof_type (input_location, complete_type (type),
-				   op == SIZEOF_EXPR, false,
+				   op == SIZEOF_EXPR, true,
 				   complain);
 }
 
diff --git a/gcc/testsuite/g++.dg/abi/align2.C b/gcc/testsuite/g++.dg/abi/align2.C
new file mode 100644
index 00000000000..bc993b24a1e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/align2.C
@@ -0,0 +1,5 @@ 
+// PR c++/69763
+// { dg-do compile { target { { i?86-*-* } && c++11 } } }
+
+#define SA(X) static_assert ((X), #X)
+SA(alignof(double) == 4);