diff mbox

Change warnings for unsupported alignment to errors

Message ID Pine.LNX.4.64.1311190144180.8831@digraph.polyomino.org.uk
State New
Headers show

Commit Message

Joseph Myers Nov. 19, 2013, 1:45 a.m. UTC
When implementing C11 _Alignas in
<http://gcc.gnu.org/ml/gcc-patches/2011-11/msg00841.html>, I noted
that I has omitted checks that alignment was supported (which, as
constraints in C11, should be errors or pedwarns rather than just
plain warnings).

The issues with the C11 definition of alignment remain unresolved,
although there's now N1731 for them (assigned DR numbers 444 and 445).
However, it seems clearly right to me, and in accord with the intent
of C11, that declaring an object with an unsupported alignment should
be an error; in that case, any code generated must be presumed to be
wrong code as it can't respect the alignment required by the user's
source code.

Contrary to what I said when implementing _Alignas, arbitrary stack
alignments are in fact supported on all architectures (PR 33721, fixed
a year before that patch).  Alignments too big for host unsigned int,
when measured in bits, are rejected in
c-common.c:check_user_alignment.  (That's less than ideal - ELF can
represent alignments of up to 2^31 or 2^63 bytes, not bits, depending
on whether it's 32-bit or 64-bit ELF.  But no doubt converting
alignments to be represented in bytes would be a lot of work.)

The alignment checks in varasm.c, however, give warnings rather than
errors.  This patch changes them to errors.

There's no testcase, as at least for the align_variable case it can't
actually generate an error on ELF systems, where MAX_OFILE_ALIGNMENT
is the largest alignment GCC can represent.  (I'm not sure under what
circumstances assemble_noswitch_variable might generate its
diagnostic.)

(I do not make an assertion about whether the existing checks for
supported alignment cover all cases where a requested alignment may
not be supported; any failure to generate the requested alignment,
with no diagnostic given, is an ordinary wrong-code bug.)

Bootstrapped with no regressions on x86_64-unknown-linux-gnu.  OK to
commit?

2013-11-19  Joseph Myers  <joseph@codesourcery.com>

	* varasm.c (align_variable): Give error instead of warning for
	unsupported alignment.
	(assemble_noswitch_variable): Likewise.

Comments

Richard Biener Nov. 19, 2013, 12:23 p.m. UTC | #1
On Tue, Nov 19, 2013 at 2:45 AM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> When implementing C11 _Alignas in
> <http://gcc.gnu.org/ml/gcc-patches/2011-11/msg00841.html>, I noted
> that I has omitted checks that alignment was supported (which, as
> constraints in C11, should be errors or pedwarns rather than just
> plain warnings).
>
> The issues with the C11 definition of alignment remain unresolved,
> although there's now N1731 for them (assigned DR numbers 444 and 445).
> However, it seems clearly right to me, and in accord with the intent
> of C11, that declaring an object with an unsupported alignment should
> be an error; in that case, any code generated must be presumed to be
> wrong code as it can't respect the alignment required by the user's
> source code.
>
> Contrary to what I said when implementing _Alignas, arbitrary stack
> alignments are in fact supported on all architectures (PR 33721, fixed
> a year before that patch).  Alignments too big for host unsigned int,
> when measured in bits, are rejected in
> c-common.c:check_user_alignment.  (That's less than ideal - ELF can
> represent alignments of up to 2^31 or 2^63 bytes, not bits, depending
> on whether it's 32-bit or 64-bit ELF.  But no doubt converting
> alignments to be represented in bytes would be a lot of work.)
>
> The alignment checks in varasm.c, however, give warnings rather than
> errors.  This patch changes them to errors.
>
> There's no testcase, as at least for the align_variable case it can't
> actually generate an error on ELF systems, where MAX_OFILE_ALIGNMENT
> is the largest alignment GCC can represent.  (I'm not sure under what
> circumstances assemble_noswitch_variable might generate its
> diagnostic.)
>
> (I do not make an assertion about whether the existing checks for
> supported alignment cover all cases where a requested alignment may
> not be supported; any failure to generate the requested alignment,
> with no diagnostic given, is an ordinary wrong-code bug.)
>
> Bootstrapped with no regressions on x86_64-unknown-linux-gnu.  OK to
> commit?
>
> 2013-11-19  Joseph Myers  <joseph@codesourcery.com>
>
>         * varasm.c (align_variable): Give error instead of warning for
>         unsupported alignment.
>         (assemble_noswitch_variable): Likewise.
>
> Index: gcc/varasm.c
> ===================================================================
> --- gcc/varasm.c        (revision 204948)
> +++ gcc/varasm.c        (working copy)
> @@ -960,9 +960,9 @@ align_variable (tree decl, bool dont_output_data)
>       In particular, a.out format supports a maximum alignment of 4.  */
>    if (align > MAX_OFILE_ALIGNMENT)
>      {
> -      warning (0, "alignment of %q+D is greater than maximum object "
> -               "file alignment.  Using %d", decl,
> -              MAX_OFILE_ALIGNMENT/BITS_PER_UNIT);
> +      error ("alignment of %q+D is greater than maximum object "
> +            "file alignment.  Using %d", decl,
> +            MAX_OFILE_ALIGNMENT/BITS_PER_UNIT);

The "Using %d" part of the diagnostic is now pointless, no?

Ok with removing it or changing it to tell the user the maximum supported
alignment.

Thanks,
Richard.

>        align = MAX_OFILE_ALIGNMENT;
>      }
>
> @@ -1908,8 +1908,8 @@ assemble_noswitch_variable (tree decl, const char
>
>    if (!sect->noswitch.callback (decl, name, size, rounded)
>        && (unsigned HOST_WIDE_INT) (align / BITS_PER_UNIT) > rounded)
> -    warning (0, "requested alignment for %q+D is greater than "
> -            "implemented alignment of %wu", decl, rounded);
> +    error ("requested alignment for %q+D is greater than "
> +          "implemented alignment of %wu", decl, rounded);
>  }
>
>  /* A subroutine of assemble_variable.  Output the label and contents of
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers Nov. 19, 2013, 4:32 p.m. UTC | #2
On Tue, 19 Nov 2013, Richard Biener wrote:

> > -      warning (0, "alignment of %q+D is greater than maximum object "
> > -               "file alignment.  Using %d", decl,
> > -              MAX_OFILE_ALIGNMENT/BITS_PER_UNIT);
> > +      error ("alignment of %q+D is greater than maximum object "
> > +            "file alignment.  Using %d", decl,
> > +            MAX_OFILE_ALIGNMENT/BITS_PER_UNIT);
> 
> The "Using %d" part of the diagnostic is now pointless, no?
> 
> Ok with removing it or changing it to tell the user the maximum supported
> alignment.

I've applied this patch with "... is greater than maximum object file 
alignment %d" (no ".  Using").
diff mbox

Patch

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 204948)
+++ gcc/varasm.c	(working copy)
@@ -960,9 +960,9 @@  align_variable (tree decl, bool dont_output_data)
      In particular, a.out format supports a maximum alignment of 4.  */
   if (align > MAX_OFILE_ALIGNMENT)
     {
-      warning (0, "alignment of %q+D is greater than maximum object "
-               "file alignment.  Using %d", decl,
-	       MAX_OFILE_ALIGNMENT/BITS_PER_UNIT);
+      error ("alignment of %q+D is greater than maximum object "
+	     "file alignment.  Using %d", decl,
+	     MAX_OFILE_ALIGNMENT/BITS_PER_UNIT);
       align = MAX_OFILE_ALIGNMENT;
     }
 
@@ -1908,8 +1908,8 @@  assemble_noswitch_variable (tree decl, const char
 
   if (!sect->noswitch.callback (decl, name, size, rounded)
       && (unsigned HOST_WIDE_INT) (align / BITS_PER_UNIT) > rounded)
-    warning (0, "requested alignment for %q+D is greater than "
-	     "implemented alignment of %wu", decl, rounded);
+    error ("requested alignment for %q+D is greater than "
+	   "implemented alignment of %wu", decl, rounded);
 }
 
 /* A subroutine of assemble_variable.  Output the label and contents of