Re: PING [PATCH] use MAX_OFILE_ALIGNMENT to validate attribute aligned (PR 87795)
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
Re: PING [PATCH] use MAX_OFILE_ALIGNMENT to validate attribute aligned (PR 87795)
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
PING [PATCH] use MAX_OFILE_ALIGNMENT to validate attribute aligned (PR 87795)
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
Re: [PATCH] use MAX_OFILE_ALIGNMENT to validate attribute aligned (PR 87795)
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
Re: [PATCH] use MAX_OFILE_ALIGNMENT to validate attribute aligned (PR 87795)
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. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] use MAX_OFILE_ALIGNMENT to validate attribute aligned (PR 87795)
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
[PATCH] use MAX_OFILE_ALIGNMENT to validate attribute aligned (PR 87795)
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
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 0x1000
+#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 (0x1000 << 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
