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
Related show

Commit Message

Jason Merrill April 10, 2018, 5:12 p.m.
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. | #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. | #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. | #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. | #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.

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);