| Message ID | 98900257-b106-63b6-6579-be0873a61587@redhat.com |
|---|---|
| State | New |
| Headers | show |
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?
* 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. */
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