Problem in cxx_fundamental_alignment_p?

Problem in cxx_fundamental_alignment_p?

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

I came across what I think is a bug in cxx_fundamental_alignment_p.

User alignments are specified in units of bytes. This is documented, and 
we can also see the following in handle_aligned_attribute, for the case 
when we have no args:
   align_expr = size_int (ATTRIBUTE_ALIGNED_VALUE / BITS_PER_UNIT);
Then, we compute the log of that alignment in check_user_alignment:
   i = check_user_alignment (align_expr, true)
That's the log of the alignment in bytes, as we can see a little further 
down:
       SET_TYPE_ALIGN (*type, (1U << i) * BITS_PER_UNIT);

Then, we call check_cxx_fundamental_alignment_constraints, which 
recomputes the alignment (in bytes) from that log:
   unsigned requested_alignment = 1U << align_log;
It then calls cxx_fundamental_alignment_p, where we compare it to 
TYPE_ALIGN values, which are specified in bits. So I think we have a 
mismatch there.

Does that sound right? The patch below was bootstrapped and tested on 
x86_64-linux, without issues, but I'm not convinced this code is covered 
by any testcase. Dodji?


Bernd

[-- Attachment #2: fundamental-align.diff --]
[-- Type: text/x-patch, Size: 1842 bytes --]

	* c-common.c (cxx_fundamental_alignment_p): Use TYPE_ALIGN_UNIT,
	not TYPE_ALIGN.

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 237797)
+++ gcc/c-family/c-common.c	(working copy)
@@ -7668,10 +7668,10 @@ fail:
   return NULL_TREE;
 }
 
-/* Check whether ALIGN is a valid user-specified alignment.  If so,
-   return its base-2 log; if not, output an error and return -1.  If
-   ALLOW_ZERO then 0 is valid and should result in a return of -1 with
-   no error.  */
+/* Check whether ALIGN is a valid user-specified alignment, specified
+   in bytes.  If so, return its base-2 log; if not, output an error
+   and return -1.  If ALLOW_ZERO then 0 is valid and should result in
+   a return of -1 with no error.  */
 int
 check_user_alignment (const_tree align, bool allow_zero)
 {
@@ -12700,9 +12700,9 @@ scalar_to_vector (location_t loc, enum t
   return stv_nothing;
 }
 
-/* Return true iff ALIGN is an integral constant that is a fundamental
-   alignment, as defined by [basic.align] in the c++-11
-   specifications.
+/* Return true iff ALIGN, which is specified in bytes, is an integral
+   constant that is a fundamental alignment, as defined by
+   [basic.align] in the c++-11 specifications.
 
    That is:
 
@@ -12712,10 +12712,11 @@ scalar_to_vector (location_t loc, enum t
         alignof(max_align_t)].  */
 
 bool
-cxx_fundamental_alignment_p  (unsigned align)
+cxx_fundamental_alignment_p (unsigned align)
 {
-  return (align <=  MAX (TYPE_ALIGN (long_long_integer_type_node),
-			 TYPE_ALIGN (long_double_type_node)));
+  unsigned limit = MAX (TYPE_ALIGN_UNIT (long_long_integer_type_node),
+			TYPE_ALIGN_UNIT (long_double_type_node));
+  return align <= limit;
 }
 
 /* Return true if T is a pointer to a zero-sized aggregate.  */

Re: Problem in cxx_fundamental_alignment_p?

[Dodji Seketeli]

Hello Bernd,

Bernd Schmidt <bschmidt@redhat.com> writes:

> I came across what I think is a bug in cxx_fundamental_alignment_p.
>
> User alignments are specified in units of bytes. This is documented,
> and we can also see the following in handle_aligned_attribute, for the
> case when we have no args:
>   align_expr = size_int (ATTRIBUTE_ALIGNED_VALUE / BITS_PER_UNIT);
> Then, we compute the log of that alignment in check_user_alignment:
>   i = check_user_alignment (align_expr, true)
> That's the log of the alignment in bytes, as we can see a little
> further down:
>       SET_TYPE_ALIGN (*type, (1U << i) * BITS_PER_UNIT);
>
> Then, we call check_cxx_fundamental_alignment_constraints, which
> recomputes the alignment (in bytes) from that log:
>   unsigned requested_alignment = 1U << align_log;
> It then calls cxx_fundamental_alignment_p, where we compare it to
> TYPE_ALIGN values, which are specified in bits. So I think we have a
> mismatch there.
>
> Does that sound right?

Yes, I think you are right on all account.

> The patch below was bootstrapped and tested on x86_64-linux, without
> issues,

The patch looks good to me, thanks.

> but I'm not convinced this code is covered by any testcase.

Hmhm, looking at the test cases, accompanying the initial patch that
introduced that code, I agree.  I guess we should probably add a test case that
uses [[gnu::aligned (val)]], with 'val' being an alignment that is
greater than MAX (TYPE_ALIGN_UNIT (long_long_integer_type_node),
		  TYPE_ALIGN_UNIT (long_double_type_node))
in the g++.dg/cpp0x/gen-attrs-*.C series of tests?


-- 
		Dodji

Re: Problem in cxx_fundamental_alignment_p?

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

On 07/01/2016 10:34 AM, Dodji Seketeli wrote:

>> The patch below was bootstrapped and tested on x86_64-linux, without
>> issues,
>
> The patch looks good to me, thanks.

Alright. I think we need a C frontend maintainer or maybe Jason to 
approve it. Attaching again.

>> but I'm not convinced this code is covered by any testcase.
>
> Hmhm, looking at the test cases, accompanying the initial patch that
> introduced that code, I agree.  I guess we should probably add a test case that
> uses [[gnu::aligned (val)]], with 'val' being an alignment that is
> greater than MAX (TYPE_ALIGN_UNIT (long_long_integer_type_node),
> 		  TYPE_ALIGN_UNIT (long_double_type_node))
> in the g++.dg/cpp0x/gen-attrs-*.C series of tests?

I really don't know what this code is even doing, so I'll leave that up 
to you...


Bernd


[-- Attachment #2: fundamental-align.diff --]
[-- Type: text/x-patch, Size: 1842 bytes --]

	* c-common.c (cxx_fundamental_alignment_p): Use TYPE_ALIGN_UNIT,
	not TYPE_ALIGN.

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 237797)
+++ gcc/c-family/c-common.c	(working copy)
@@ -7668,10 +7668,10 @@ fail:
   return NULL_TREE;
 }
 
-/* Check whether ALIGN is a valid user-specified alignment.  If so,
-   return its base-2 log; if not, output an error and return -1.  If
-   ALLOW_ZERO then 0 is valid and should result in a return of -1 with
-   no error.  */
+/* Check whether ALIGN is a valid user-specified alignment, specified
+   in bytes.  If so, return its base-2 log; if not, output an error
+   and return -1.  If ALLOW_ZERO then 0 is valid and should result in
+   a return of -1 with no error.  */
 int
 check_user_alignment (const_tree align, bool allow_zero)
 {
@@ -12700,9 +12700,9 @@ scalar_to_vector (location_t loc, enum t
   return stv_nothing;
 }
 
-/* Return true iff ALIGN is an integral constant that is a fundamental
-   alignment, as defined by [basic.align] in the c++-11
-   specifications.
+/* Return true iff ALIGN, which is specified in bytes, is an integral
+   constant that is a fundamental alignment, as defined by
+   [basic.align] in the c++-11 specifications.
 
    That is:
 
@@ -12712,10 +12712,11 @@ scalar_to_vector (location_t loc, enum t
         alignof(max_align_t)].  */
 
 bool
-cxx_fundamental_alignment_p  (unsigned align)
+cxx_fundamental_alignment_p (unsigned align)
 {
-  return (align <=  MAX (TYPE_ALIGN (long_long_integer_type_node),
-			 TYPE_ALIGN (long_double_type_node)));
+  unsigned limit = MAX (TYPE_ALIGN_UNIT (long_long_integer_type_node),
+			TYPE_ALIGN_UNIT (long_double_type_node));
+  return align <= limit;
 }
 
 /* Return true if T is a pointer to a zero-sized aggregate.  */

Re: Problem in cxx_fundamental_alignment_p?

[Jeff Law]

On 07/01/2016 03:56 AM, Bernd Schmidt wrote:
> On 07/01/2016 10:34 AM, Dodji Seketeli wrote:
>
>>> The patch below was bootstrapped and tested on x86_64-linux, without
>>> issues,
>>
>> The patch looks good to me, thanks.
>
> Alright. I think we need a C frontend maintainer or maybe Jason to
> approve it. Attaching again.
>
>>> but I'm not convinced this code is covered by any testcase.
>>
>> Hmhm, looking at the test cases, accompanying the initial patch that
>> introduced that code, I agree.  I guess we should probably add a test
>> case that
>> uses [[gnu::aligned (val)]], with 'val' being an alignment that is
>> greater than MAX (TYPE_ALIGN_UNIT (long_long_integer_type_node),
>>           TYPE_ALIGN_UNIT (long_double_type_node))
>> in the g++.dg/cpp0x/gen-attrs-*.C series of tests?
>
> I really don't know what this code is even doing, so I'll leave that up
> to you...
>
>
> Bernd
>
>
> fundamental-align.diff
>
>
> 	* c-common.c (cxx_fundamental_alignment_p): Use TYPE_ALIGN_UNIT,
> 	not TYPE_ALIGN.
FWIW, this patch is OK.  It'd be even better if we had a testcase.

A patch making cxx_fundamental_alignment_p static, removing its 
protototype from c-common.h is pre-approved as AFAICT 
cxx_fundamental_alignment_p is not used outside c-common.c

jeff