Disable -Werror when error attribute generates warnings

Disable -Werror when error attribute generates warnings

[Grégoire Sutre]

Hi,

With an older version of gcc that does not understand the error 
attribute, gcc generates warnings when compiling files that include 
include/grub/list.h.  Since TARGET_CFLAGS contains -Werror by default, 
the build of modules fails.

The following patch checks whether the C compiler supports the error 
attribute without warning, and disables -Werror if that is not the case 
(as otherwise the build will fail).

http://pkgsrc-wip.cvs.sourceforge.net/viewvc/*checkout*/pkgsrc-wip/wip/grub2-current/patches/patch-gcc-warning-on-error-attribute

This is merely a suggestion.  I'm no autoconf expert, and I'm not even 
sure that this kind of check is a good idea, as configure has the option 
--disable-werror that can be used anyway.

Grégoire

p.s. I did not see this problem when I tested the patch discussed in the 
thread [1] as I focused on building utilities at that time, sorry.

[1] http://lists.gnu.org/archive/html/grub-devel/2009-12/msg00324.html

Re: Disable -Werror when error attribute generates warnings

[Colin Watson]

On Sat, Jan 02, 2010 at 01:58:20PM +0100, Grégoire Sutre wrote:
> With an older version of gcc that does not understand the error  
> attribute, gcc generates warnings when compiling files that include  
> include/grub/list.h.  Since TARGET_CFLAGS contains -Werror by default,  
> the build of modules fails.
>
> The following patch checks whether the C compiler supports the error  
> attribute without warning, and disables -Werror if that is not the case  
> (as otherwise the build will fail).

Instead of this, why not only use the attribute if it's available? I
couldn't find an entry about it in GCC's human-readable change
summaries, but support was committed on 2007-09-23 so I think it's
available from GCC 4.3.

I use this GNUC_PREREQ approach in other projects and rather like it. It
could be extended to cover our other uses of attributes quite easily.

2010-01-02  Colin Watson  <cjwatson@ubuntu.com>

	* include/grub/misc.h (GNUC_PREREQ): New macro.
        (ATTRIBUTE_ERROR): New macro.
        * include/grub/list.h (grub_bad_type_cast_real): Use
        ATTRIBUTE_ERROR.

=== modified file 'include/grub/list.h'
--- include/grub/list.h	2009-12-31 14:03:09 +0000
+++ include/grub/list.h	2010-01-02 15:31:44 +0000
@@ -42,7 +42,7 @@ void EXPORT_FUNC(grub_list_insert) (grub
 
 static inline void *
 grub_bad_type_cast_real (int line, const char *file)
-     __attribute__ ((error ("bad type cast between incompatible grub types")));
+     ATTRIBUTE_ERROR ("bad type cast between incompatible grub types");
 
 static inline void *
 grub_bad_type_cast_real (int line, const char *file)

=== modified file 'include/grub/misc.h'
--- include/grub/misc.h	2009-12-18 02:57:32 +0000
+++ include/grub/misc.h	2010-01-02 15:31:31 +0000
@@ -25,6 +25,22 @@
 #include <grub/symbol.h>
 #include <grub/err.h>
 
+/* GCC version checking borrowed from glibc. */
+#if defined(__GNUC__) && defined(__GNUC_MINOR__)
+#  define GNUC_PREREQ(maj,min) \
+	((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min))
+#else
+#  define GNUC_PREREQ(maj,min) 0
+#endif
+
+/* Does this compiler support compile-time error attributes? */
+#if GNUC_PREREQ(4,3)
+#  define ATTRIBUTE_ERROR(msg) \
+	__attribute__ ((__error__ (msg)))
+#else
+#  define ATTRIBUTE_ERROR(msg)
+#endif
+
 #define ALIGN_UP(addr, align) \
 	((addr + (typeof (addr)) align - 1) & ~((typeof (addr)) align - 1))
 #define ARRAY_SIZE(array) (sizeof (array) / sizeof (array[0]))

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Disable -Werror when error attribute generates warnings

[Grégoire Sutre]

Colin Watson wrote:

> Instead of this, why not only use the attribute if it's available? I
> couldn't find an entry about it in GCC's human-readable change
> summaries, but support was committed on 2007-09-23 so I think it's
> available from GCC 4.3.
> 
> I use this GNUC_PREREQ approach in other projects and rather like it. It
> could be extended to cover our other uses of attributes quite easily.

Nice trick :-)

I confirm that this patch works with gcc 4.1.3 on NetBSD.  And it's 
definitely better than completely disabling -Werror...

Thanks,

Grégoire

Re: Disable -Werror when error attribute generates warnings

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

>  =20
>> With an older version of gcc that does not understand the error =20
>> attribute, gcc generates warnings when compiling files that include =20
>> include/grub/list.h.  Since TARGET_CFLAGS contains -Werror by default,=
 =20
>> the build of modules fails.
>>
>> The following patch checks whether the C compiler supports the error  =

>> attribute without warning, and disables -Werror if that is not the cas=
e =20
>> (as otherwise the build will fail).
>>    =20
>
> Instead of this, why not only use the attribute if it's available? I
> couldn't find an entry about it in GCC's human-readable change
> summaries, but support was committed on 2007-09-23 so I think it's
> available from GCC 4.3.
>
>  =20
Why not have configure.ac check specifically if this attribute is
available and use it only if it is?
> I use this GNUC_PREREQ approach in other projects and rather like it. I=
t
> could be extended to cover our other uses of attributes quite easily.
>
> 2010-01-02  Colin Watson  <cjwatson@ubuntu.com>
>
> 	* include/grub/misc.h (GNUC_PREREQ): New macro.
>         (ATTRIBUTE_ERROR): New macro.
>         * include/grub/list.h (grub_bad_type_cast_real): Use
>         ATTRIBUTE_ERROR.
>
> =3D=3D=3D modified file 'include/grub/list.h'
> --- include/grub/list.h	2009-12-31 14:03:09 +0000
> +++ include/grub/list.h	2010-01-02 15:31:44 +0000
> @@ -42,7 +42,7 @@ void EXPORT_FUNC(grub_list_insert) (grub
> =20
>  static inline void *
>  grub_bad_type_cast_real (int line, const char *file)
> -     __attribute__ ((error ("bad type cast between incompatible grub t=
ypes")));
> +     ATTRIBUTE_ERROR ("bad type cast between incompatible grub types")=
;
> =20
>  static inline void *
>  grub_bad_type_cast_real (int line, const char *file)
>
> =3D=3D=3D modified file 'include/grub/misc.h'
> --- include/grub/misc.h	2009-12-18 02:57:32 +0000
> +++ include/grub/misc.h	2010-01-02 15:31:31 +0000
> @@ -25,6 +25,22 @@
>  #include <grub/symbol.h>
>  #include <grub/err.h>
> =20
> +/* GCC version checking borrowed from glibc. */
> +#if defined(__GNUC__) && defined(__GNUC_MINOR__)
> +#  define GNUC_PREREQ(maj,min) \
> +	((__GNUC__ << 16) + __GNUC_MINOR__ >=3D ((maj) << 16) + (min))
> +#else
> +#  define GNUC_PREREQ(maj,min) 0
> +#endif
> +
> +/* Does this compiler support compile-time error attributes? */
> +#if GNUC_PREREQ(4,3)
> +#  define ATTRIBUTE_ERROR(msg) \
> +	__attribute__ ((__error__ (msg)))
> +#else
> +#  define ATTRIBUTE_ERROR(msg)
> +#endif
> +
>  #define ALIGN_UP(addr, align) \
>  	((addr + (typeof (addr)) align - 1) & ~((typeof (addr)) align - 1))
>  #define ARRAY_SIZE(array) (sizeof (array) / sizeof (array[0]))
>
>  =20


--=20
Regards
Vladimir '=CF=86-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]

Re: Disable -Werror when error attribute generates warnings

[Robert Millan]

On Sat, Jan 02, 2010 at 07:21:21PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > Instead of this, why not only use the attribute if it's available? I
> > couldn't find an entry about it in GCC's human-readable change
> > summaries, but support was committed on 2007-09-23 so I think it's
> > available from GCC 4.3.
> >
> >  =20
> Why not have configure.ac check specifically if this attribute is
> available and use it only if it is?

Well, yes it'd be better.  Although in this case, it's not terribly needed,
since we only support GCC and nowadays nobody would use a snapshot from
in-between 4.2 and 4.3.

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi

Re: Disable -Werror when error attribute generates warnings

[Colin Watson]

On Sat, Jan 02, 2010 at 07:21:21PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Colin Watson wrote:
> > Instead of this, why not only use the attribute if it's available? I
> > couldn't find an entry about it in GCC's human-readable change
> > summaries, but support was committed on 2007-09-23 so I think it's
> > available from GCC 4.3.
> 
> Why not have configure.ac check specifically if this attribute is
> available and use it only if it is?

Perhaps it's a style thing; for some reason I've always preferred this
approach, even though normally I would advocate doing feature tests at
configure time rather than version tests at compile time. My preference
may be because __attribute__ is GCC-specific anyway, and there's no
guarantee that another compiler would implement the same attribute names
in the same way if they happened to use them, so it's better to be quite
explicit that this is specific to appropriate versions of GCC.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Disable -Werror when error attribute generates warnings

[Robert Millan]

On Sat, Jan 02, 2010 at 03:37:38PM +0000, Colin Watson wrote:
> 
> 2010-01-02  Colin Watson  <cjwatson@ubuntu.com>
> 
> 	* include/grub/misc.h (GNUC_PREREQ): New macro.
>         (ATTRIBUTE_ERROR): New macro.
>         * include/grub/list.h (grub_bad_type_cast_real): Use
>         ATTRIBUTE_ERROR.

Looks fine.  Please remember to update copyright year in misc.h.

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi

Re: Disable -Werror when error attribute generates warnings

[Colin Watson]

On Sun, Jan 03, 2010 at 05:46:14PM +0100, Robert Millan wrote:
> On Sat, Jan 02, 2010 at 03:37:38PM +0000, Colin Watson wrote:
> > 2010-01-02  Colin Watson  <cjwatson@ubuntu.com>
> > 
> > 	* include/grub/misc.h (GNUC_PREREQ): New macro.
> >         (ATTRIBUTE_ERROR): New macro.
> >         * include/grub/list.h (grub_bad_type_cast_real): Use
> >         ATTRIBUTE_ERROR.
> 
> Looks fine.  Please remember to update copyright year in misc.h.

OK, thanks; committed with that change.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]