| Message ID | d0dcef4b-3615-e2b1-4e07-fc255a88e154@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | use MAX_OFILE_ALIGNMENT to validate attribute aligned (PR 87795) | expand |
On 10/30/18 3:40 PM, Martin Sebor wrote: > Bug 87795 - Excessive alignment permitted for functions and labels > points out that the handler for attribute aligned makes it possible > for unsupported alignments to be accepted by the front end only to > be either rejected later on by some targets for variables, or to > cause an ICE for overaligned functions. > > The reason for the problems is that the attribute handler considers > any power of two alignment valid whose log2 is less than > HOST_BITS_PER_INT - LOG2_BITS_PER_UNIT, but later parts of GCC > assume values of at most MAX_OFILE_ALIGNMENT / BITS_PER_UNIT. > The internals manual documents MAX_OFILE_ALIGNMENT as: > > Biggest alignment supported by the object file format of this > machine. Use this macro to limit the alignment which can be > specified using the __attribute__ ((aligned (n))) construct. > > So it seems that the attribute handler should be using this macro > instead. I also took the liberty to add more detail to the error > messages. Attached is a patch that makes this change. Tested on > x86_64-linux, plus using cross-compilers for arm, hppa64, pdp11, > and powerpc64. > > Martin > > gcc-87795.diff > > PR c/87795 - Excessive alignment permitted for functions and labels > > gcc/c-family/ChangeLog: > > PR c/87795 > * c-common.c (check_user_alignment): Use MAX_OFILE_ALIGNMENT. > > gcc/testsuite/ChangeLog: > > PR c/87795 > * gcc.dg/attr-aligned.c: New test. OK jeff
On Tue, 30 Oct 2018, Martin Sebor wrote: > So it seems that the attribute handler should be using this macro > instead. I also took the liberty to add more detail to the error Note that it should only be used for alignments relevant to the object file - *not* for alignments of variables with automatic storage duration (and thus not for alignments of types / struct fields, because such types might only be used on the stack) since GCC supports arbitrary alignments on the stack via dynamically realigning it. So you need testcases that verify that large alignments are still allowed for types / fields / on the stack, even when the object file only supports smaller alignments.
On 10/30/2018 04:34 PM, Joseph Myers wrote: > On Tue, 30 Oct 2018, Martin Sebor wrote: > >> So it seems that the attribute handler should be using this macro >> instead. I also took the liberty to add more detail to the error > > Note that it should only be used for alignments relevant to the object > file - *not* for alignments of variables with automatic storage duration > (and thus not for alignments of types / struct fields, because such types > might only be used on the stack) since GCC supports arbitrary alignments > on the stack via dynamically realigning it. > > So you need testcases that verify that large alignments are still allowed > for types / fields / on the stack, even when the object file only supports > smaller alignments. Good catch, thanks! Attached is an updated patch that relaxes the restriction to allow auto variables to be aligned on a more restrictive boundary than MAX_OFILE_ALIGNMENT would imply. I spent far more time building and testing various cross-toolchains than I did on the GCC change, mainly because I couldn't find a way to programmatically detect the value of MAX_OFILE_ALIGNMENT (or the maximum alignment supported by GCC). In the end I stuck with the hardcoding. If there isn't one, how about adding a couple predefined macros for these? The test is also pretty hacky (and I wouldn't surprised if it failed on some system I didn't exercise). Having GCC expose these parameters in some way would make the test cleaner (and more robust, though one might make the argument that relying on GCC-generated values to verify those same values would actually make it less robust). In this revision I also updated the MAX_OFILE_ALIGNMENT desciption in the internals manual. Retested on x86_64-linux, plus using cross-compilers for hppa64, pdp11, and powerpc-darwin. Martin PR c/87795 - Excessive alignment permitted for functions and labels gcc/ChangeLog: PR c/87795 * doc/tm.texi.in (MAX_OFILE_ALIGNMENT): Clarify. * doc/tm.texi (MAX_OFILE_ALIGNMENT): Ditto. gcc/c/ChangeLog: PR c/87795 * c-decl.c (declspecs_add_alignas): Add argument to a call to check_user_alignment. gcc/c-family/ChangeLog: PR c/87795 * c-attribs (common_handle_aligned_attribute): Add argument to a call to check_user_alignment. * c-common.c (check_user_alignment): Add argument. Use MAX_OFILE_ALIGNMENT. * c-common.h (check_user_alignment): Add argument. gcc/testsuite/ChangeLog: PR c/87795 * gcc.dg/attr-aligned.c: New test. Index: gcc/doc/tm.texi =================================================================== --- gcc/doc/tm.texi (revision 265696) +++ gcc/doc/tm.texi (working copy) @@ -1081,8 +1081,11 @@ If not defined, the default value is @code{STACK_B @defmac MAX_OFILE_ALIGNMENT Biggest alignment supported by the object file format of this machine. Use this macro to limit the alignment which can be specified using the -@code{__attribute__ ((aligned (@var{n})))} construct. If not defined, -the default value is @code{BIGGEST_ALIGNMENT}. +@code{__attribute__ ((aligned (@var{n})))} construct for functions and +objects with static storage duration. The alignment of automatic +objects may exceed the object file format maximum up to the maximum +supported by GCC. If not defined, the default value is +@code{BIGGEST_ALIGNMENT}. On systems that use ELF, the default (in @file{config/elfos.h}) is the largest supported 32-bit ELF section alignment representable on Index: gcc/doc/tm.texi.in =================================================================== --- gcc/doc/tm.texi.in (revision 265696) +++ gcc/doc/tm.texi.in (working copy) @@ -1027,8 +1027,11 @@ If not defined, the default value is @code{STACK_B @defmac MAX_OFILE_ALIGNMENT Biggest alignment supported by the object file format of this machine. Use this macro to limit the alignment which can be specified using the -@code{__attribute__ ((aligned (@var{n})))} construct. If not defined, -the default value is @code{BIGGEST_ALIGNMENT}. +@code{__attribute__ ((aligned (@var{n})))} construct for functions and +objects with static storage duration. The alignment of automatic +objects may exceed the object file format maximum up to the maximum +supported by GCC. If not defined, the default value is +@code{BIGGEST_ALIGNMENT}. On systems that use ELF, the default (in @file{config/elfos.h}) is the largest supported 32-bit ELF section alignment representable on Index: gcc/c/c-decl.c =================================================================== --- gcc/c/c-decl.c (revision 265696) +++ gcc/c/c-decl.c (working copy) @@ -11034,7 +11034,7 @@ declspecs_add_alignas (source_location loc, specs->locations[cdw_alignas] = loc; if (align == error_mark_node) return specs; - align_log = check_user_alignment (align, true); + align_log = check_user_alignment (align, false, true); if (align_log > specs->align_log) specs->align_log = align_log; return specs; Index: gcc/c-family/c-attribs.c =================================================================== --- gcc/c-family/c-attribs.c (revision 265696) +++ gcc/c-family/c-attribs.c (working copy) @@ -1813,8 +1813,11 @@ common_handle_aligned_attribute (tree *node, tree else if (TYPE_P (*node)) type = node, is_type = true; + /* True to consider invalid alignments greater than MAX_OFILE_ALIGNMENT. */ + bool objfile = (TREE_CODE (*node) == FUNCTION_DECL + || (VAR_P (*node) && TREE_STATIC (*node))); /* Log2 of specified alignment. */ - int pow2align = check_user_alignment (align_expr, true); + int pow2align = check_user_alignment (align_expr, objfile, true); if (pow2align == -1 || !check_cxx_fundamental_alignment_constraints (*node, pow2align, flags)) { Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 265696) +++ gcc/c-family/c-common.c (working copy) @@ -5123,17 +5123,21 @@ c_init_attributes (void) #undef DEF_ATTR_TREE_LIST } -/* 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 the byte alignment ALIGN is a valid user-specified + alignment less than the supported maximum. If so, return ALIGN's + base-2 log; if not, output an error and return -1. If OBJFILE + then reject alignments greater than MAX_OFILE_ALIGNMENT when + converted to bits. Otherwise, consider valid only alignments + that are less than HOST_BITS_PER_INT - LOG2_BITS_PER_UNIT. + 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) +check_user_alignment (const_tree align, bool objfile, bool allow_zero) { - int i; - if (error_operand_p (align)) return -1; + if (TREE_CODE (align) != INTEGER_CST || !INTEGRAL_TYPE_P (TREE_TYPE (align))) { @@ -5140,20 +5144,38 @@ int error ("requested alignment is not an integer constant"); return -1; } - else if (allow_zero && integer_zerop (align)) + + if (allow_zero && integer_zerop (align)) return -1; - else if (tree_int_cst_sgn (align) == -1 - || (i = tree_log2 (align)) == -1) + + int log2bitalign; + if (tree_int_cst_sgn (align) == -1 + || (log2bitalign = tree_log2 (align)) == -1) { - error ("requested alignment is not a positive power of 2"); + error ("requested alignment %qE is not a positive power of 2", + align); return -1; } - else if (i >= HOST_BITS_PER_INT - LOG2_BITS_PER_UNIT) + + if (objfile) { - error ("requested alignment is too large"); + unsigned maxalign = MAX_OFILE_ALIGNMENT / BITS_PER_UNIT; + if (tree_to_shwi (align) > maxalign) + { + error ("requested alignment %qE exceeds object file maximum %u", + align, maxalign); + return -1; + } + } + + if (log2bitalign >= HOST_BITS_PER_INT - LOG2_BITS_PER_UNIT) + { + error ("requested alignment %qE exceeds maximum %u", + align, 1U << (HOST_BITS_PER_INT - 1)); return -1; } - return i; + + return log2bitalign; } /* Determine the ELF symbol visibility for DECL, which is either a Index: gcc/c-family/c-common.h =================================================================== --- gcc/c-family/c-common.h (revision 265696) +++ gcc/c-family/c-common.h (working copy) @@ -798,7 +798,7 @@ extern void finish_fname_decls (void); extern const char *fname_as_string (int); extern tree fname_decl (location_t, unsigned, tree); -extern int check_user_alignment (const_tree, bool); +extern int check_user_alignment (const_tree, bool, bool); extern bool check_function_arguments (location_t loc, const_tree, const_tree, int, tree *, vec<location_t> *); extern void check_function_arguments_recurse (void (*) Index: gcc/testsuite/gcc.dg/attr-aligned.c =================================================================== --- gcc/testsuite/gcc.dg/attr-aligned.c (nonexistent) +++ gcc/testsuite/gcc.dg/attr-aligned.c (working copy) @@ -0,0 +1,147 @@ +/* PR c/87795 - Excessive alignment permitted for functions and labels + { dg-do compile } + { dg-options "-Wno-pedantic" } */ + +/* The maximum alignment GCC can handle. */ +#define ALIGN_MAX_HARD 0x10000000 + +/* Hardcode a few known values for testing the tight bounds. */ +#if __hpux__ && __hppa__ && __LP64__ + /* Maximum alignment for functions and objects with static storage + duration that's expected to be accepted. */ +# define ALIGN_MAX_STATIC 0x1000 + /* Excessive alignment for functions and objects with static storage + duration that's expected to trigger an error. */ +#elif __MACH__ +# define ALIGN_MAX_STATIC 0x8000 +#elif pdp11 +# define ALIGN_MAX_STATIC 2 +/* Work around a pdp11 ICE (see PR target/87821). */ +# define ALIGN_MAX_AUTO (ALIGN_MAX_HARD >> 14) +#elif __powerpc64__ || __x86_64__ +/* Is this processor- or operating-system specific? */ +# define ALIGN_MAX_STATIC ALIGN_MAX_HARD +#else + /* Guaranteed to be accepted regardless of the target. */ +# define ALIGN_MAX_STATIC __BIGGEST_ALIGNMENT__ + /* Guaranteed to be rejected regardless of the target. */ +# define ALIGN_TOO_BIG_OFILE (ALIGN_MAX_HARD << 1) +#endif + +/* Maximum alignment for auto objects that's expected to be accepted. */ +#ifndef ALIGN_MAX_AUTO +# define ALIGN_MAX_AUTO ALIGN_MAX_HARD +#endif + +#ifndef ALIGN_TOO_BIG_OFILE +# define ALIGN_TOO_BIG_OFILE (ALIGN_MAX_STATIC << 1) +#endif + + +#define ALIGN(N) __attribute__ ((aligned (N))) + + +/* Verify that types can be defined maximally overaligned using + attribute aligned. */ +typedef ALIGN (ALIGN_MAX_HARD) char CharAlignedMaxHard; +typedef ALIGN (ALIGN_MAX_AUTO) char CharAlignedMaxAuto; +typedef ALIGN (ALIGN_MAX_STATIC) char CharAlignedMaxStatic; + +#if ALIGN_TOO_BIG_OFILE < ALIGN_MAX_HARD +/* Also verify that an alignment greater than MAX_OFILE_ALIGNMENT + is accepted unless the constant is as large as GCC's maximum + supported alignment in any context. */ +typedef ALIGN (ALIGN_TOO_BIG_OFILE) char CharAlignedTooBig; +#endif + +CharAlignedMaxStatic t_max; + +/* Verify that globals can be defined maximally overaligned using + attribute aligned. */ +ALIGN (ALIGN_MAX_STATIC) static const char aligned_sc_max = 0; +ALIGN (ALIGN_MAX_STATIC) const char aligned_c_max = aligned_sc_max; +ALIGN (ALIGN_MAX_STATIC) char aligned_v_max; +ALIGN (ALIGN_MAX_STATIC) void aligned_f_max (void); + +_Static_assert (__alignof__ (aligned_sc_max) == ALIGN_MAX_STATIC); +_Static_assert (__alignof__ (aligned_c_max) == ALIGN_MAX_STATIC); +_Static_assert (__alignof__ (aligned_v_max) == ALIGN_MAX_STATIC); +_Static_assert (__alignof__ (aligned_f_max) == ALIGN_MAX_STATIC); + + +/* Verify that globals can be defined maximally overaligned using + _Alignas. */ +_Alignas (ALIGN_MAX_STATIC) static const char alignas_sc_max = 0; +_Alignas (ALIGN_MAX_STATIC) const char alignas_c_max = alignas_sc_max; +_Alignas (ALIGN_MAX_STATIC) char alignas_v_max; + +_Static_assert (__alignof__ (alignas_sc_max) == ALIGN_MAX_STATIC); +_Static_assert (__alignof__ (alignas_c_max) == ALIGN_MAX_STATIC); +_Static_assert (__alignof__ (alignas_v_max) == ALIGN_MAX_STATIC); + + +/* Verify that auto and static local variables can be defined maximally + overaligned. */ + +int accept_local_attribute_aligned (void) +{ +#if ALIGN_TOO_BIG_OFILE < ALIGN_MAX_HARD + /* Same as above. */ + typedef ALIGN (ALIGN_TOO_BIG_OFILE) char LocalCharAlignedTooBig; + LocalCharAlignedTooBig aligned_lt_too_big = 0; + (void)&aligned_lt_too_big; +#endif + + static CharAlignedMaxStatic aligned_st_max; + _Static_assert (_Alignof (aligned_st_max) == ALIGN_MAX_STATIC); + + CharAlignedMaxAuto aligned_t_max; + _Static_assert (_Alignof (aligned_t_max) == ALIGN_MAX_AUTO); + + ALIGN (ALIGN_MAX_STATIC) char aligned_s_max; + _Static_assert (_Alignof (aligned_s_max) == ALIGN_MAX_STATIC); + + ALIGN (ALIGN_MAX_AUTO) char aligned_l_max; + _Static_assert (_Alignof (aligned_l_max) == ALIGN_MAX_AUTO); + + return aligned_st_max++ + aligned_t_max++ + aligned_s_max++ + aligned_l_max++; +} + + +int accept_local_alignas (void) +{ + _Alignas (ALIGN_MAX_STATIC) char alignas_s_max; + _Static_assert (_Alignof (alignas_s_max) == ALIGN_MAX_STATIC); + + _Alignas (ALIGN_MAX_AUTO) char alignas_l_max; + _Static_assert (_Alignof (alignas_l_max) == ALIGN_MAX_AUTO); + + return alignas_s_max++ + alignas_l_max++; +} + + +/* Verify that auto and static local variables are subject to the object + file alignment limit. The "object file" part may not be mentioned if + the object file maximum is the same as GCC's internal maximum. */ + +int reject_local_align (void) +{ + /* Ironically, the errors below are on different lines for each + of the two declarations if the aligned attribute is on a line + of its own. */ + ALIGN (ALIGN_TOO_BIG_OFILE) static char aligned_sl_max; /* { dg-error "requested alignment .\[0-9\]+. exceeds\( object file\)* maximum \[0-9\]+" } */ + + _Alignas (ALIGN_TOO_BIG_OFILE) static char alignas_sl_max; /* { dg-error "alignment" } */ + + return aligned_sl_max++ + alignas_sl_max++; +} + + +/* Verify that global variables are subject to the object file + alignment limit. */ + +ALIGN (ALIGN_TOO_BIG_OFILE) char a_max_x_2; /* { dg-error "requested alignment .\[0-9\]+. exceeds\( object file\)* maximum \[0-9\]+" } */ + +_Alignas (ALIGN_TOO_BIG_OFILE) char a_max_x_2; /* { dg-error "alignment" } */ + +ALIGN (ALIGN_TOO_BIG_OFILE) void f_max_x_2 (void); /* { dg-error "requested alignment .\[0-9\]+. exceeds\( object file\)* maximum \[0-9\]+" } */
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg02081.html On 10/31/2018 02:52 PM, Martin Sebor wrote: > On 10/30/2018 04:34 PM, Joseph Myers wrote: >> On Tue, 30 Oct 2018, Martin Sebor wrote: >> >>> So it seems that the attribute handler should be using this macro >>> instead. I also took the liberty to add more detail to the error >> >> Note that it should only be used for alignments relevant to the object >> file - *not* for alignments of variables with automatic storage duration >> (and thus not for alignments of types / struct fields, because such types >> might only be used on the stack) since GCC supports arbitrary alignments >> on the stack via dynamically realigning it. >> >> So you need testcases that verify that large alignments are still allowed >> for types / fields / on the stack, even when the object file only >> supports >> smaller alignments. > > Good catch, thanks! Attached is an updated patch that relaxes > the restriction to allow auto variables to be aligned on a more > restrictive boundary than MAX_OFILE_ALIGNMENT would imply. > > I spent far more time building and testing various cross-toolchains > than I did on the GCC change, mainly because I couldn't find a way > to programmatically detect the value of MAX_OFILE_ALIGNMENT (or > the maximum alignment supported by GCC). In the end I stuck with > the hardcoding. If there isn't one, how about adding a couple > predefined macros for these? > > The test is also pretty hacky (and I wouldn't surprised if it > failed on some system I didn't exercise). Having GCC expose > these parameters in some way would make the test cleaner (and > more robust, though one might make the argument that relying > on GCC-generated values to verify those same values would > actually make it less robust). > > In this revision I also updated the MAX_OFILE_ALIGNMENT desciption > in the internals manual. > > Retested on x86_64-linux, plus using cross-compilers for hppa64, > pdp11, and powerpc-darwin. > > Martin >
On 11/6/18 5:06 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg02081.html
I thought I'd already ACK's this one...
OK.
Jeff
On 11/07/2018 03:43 PM, Jeff Law wrote: > On 11/6/18 5:06 PM, Martin Sebor wrote: >> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg02081.html > I thought I'd already ACK's this one... > > > OK. You did but I made changes afterwards in response to Joseph's comment. I've committed the final patch in r265977. Thanks Martin
PR c/87795 - Excessive alignment permitted for functions and labels gcc/c-family/ChangeLog: PR c/87795 * c-common.c (check_user_alignment): Use MAX_OFILE_ALIGNMENT. gcc/testsuite/ChangeLog: PR c/87795 * gcc.dg/attr-aligned.c: New test. Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 265630) +++ gcc/c-family/c-common.c (working copy) @@ -5123,17 +5123,20 @@ c_init_attributes (void) #undef DEF_ATTR_TREE_LIST } -/* 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 the byte alignment ALIGN is a valid user-specified + alignment less than MAX_OFILE_ALIGNMENT when converted to bits. + If so, return ALIGN's 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) { - int i; + int log2bitalign; if (error_operand_p (align)) return -1; + if (TREE_CODE (align) != INTEGER_CST || !INTEGRAL_TYPE_P (TREE_TYPE (align))) { @@ -5140,20 +5143,35 @@ check_user_alignment (const_tree align, bool allow error ("requested alignment is not an integer constant"); return -1; } - else if (allow_zero && integer_zerop (align)) + + if (allow_zero && integer_zerop (align)) return -1; - else if (tree_int_cst_sgn (align) == -1 - || (i = tree_log2 (align)) == -1) + + if (tree_int_cst_sgn (align) == -1 + || (log2bitalign = tree_log2 (align)) == -1) { - error ("requested alignment is not a positive power of 2"); + error ("requested alignment %qE is not a positive power of 2", + align); return -1; } - else if (i >= HOST_BITS_PER_INT - LOG2_BITS_PER_UNIT) + + unsigned maxalign = MAX_OFILE_ALIGNMENT / BITS_PER_UNIT; + if (tree_to_shwi (align) > maxalign) { - error ("requested alignment is too large"); + error ("requested alignment %qE exceeds object file maximum %u", + align, maxalign); return -1; } - return i; + + /* The following is probably redundant given the test above. */ + if (log2bitalign >= HOST_BITS_PER_INT - LOG2_BITS_PER_UNIT) + { + error ("requested alignment %qE exceeds maximum %u", + align, 1U << (HOST_BITS_PER_INT - 1)); + return -1; + } + + return log2bitalign; } /* Determine the ELF symbol visibility for DECL, which is either a Index: gcc/testsuite/gcc.dg/attr-aligned.c =================================================================== --- gcc/testsuite/gcc.dg/attr-aligned.c (nonexistent) +++ gcc/testsuite/gcc.dg/attr-aligned.c (working copy) @@ -0,0 +1,35 @@ +/* PR c/87795 - Excessive alignment permitted for functions and labels + { dg-do compile } */ + +/* Hardcode a few known values for testing the tight bounds. */ +#if __hpux__ && __hppa__ && __LP64__ +# define ALIGN_MAX 4096 +# define ALIGN_TOO_BIG (ALIGN_MAX << 1) +#elif pdp11 +# define ALIGN_MAX 2 +# define ALIGN_TOO_BIG 4 +#elif __powerpc64__ || __x86_64__ +# define ALIGN_MAX 0x10000000 +#else + /* Guaranteed to be accepted regardless of the target. */ +# define ALIGN_MAX __BIGGEST_ALIGNMENT__ +#endif + +#ifndef ALIGN_TOO_BIG + /* Guaranteed to be rejected regardless of the target. */ +# define ALIGN_TOO_BIG (0x10000000 << 1) +#endif + +__attribute__ ((aligned (ALIGN_MAX))) const char c_max = 0; +__attribute__ ((aligned (ALIGN_MAX))) char v_max; +__attribute__ ((aligned (ALIGN_MAX))) void f_max (void); + +_Static_assert (_Alignof (c_max) == ALIGN_MAX); +_Static_assert (_Alignof (v_max) == ALIGN_MAX); + + +__attribute__ ((aligned (ALIGN_TOO_BIG))) char +a_max_x_2; /* { dg-error "requested alignment .\[0-9\]+. exceeds object file maximum \[0-9\]+" } */ + +__attribute__ ((aligned (ALIGN_TOO_BIG))) void +f_max_x_2 (void); /* { dg-error "requested alignment .\[0-9\]+. exceeds object file maximum \[0-9\]+" } */