[libvirt] [PATCH] Use gnulib's manywarnings & warnings modules
Eric Blake
eblake at redhat.com
Fri Apr 1 16:02:18 UTC 2011
On 04/01/2011 09:39 AM, Daniel P. Berrange wrote:
> Remove custom code for checking compiler warnings, using
> gl_WARN_ADD instead. Don't list all flags ourselves, use
> gnulib's gl_MANYWARN_ALL_GCC to get all possible GCC flags,
> then turn off the ones we don't want yet.
>
> This patch alone doesn't buy us much. What we want is some
> more followup patches, which remove entries from the
> 'dontwarn' list, fixing up the corresponding code problems.
Should we therefore defer this patch until post-0.9.0 when we can do
more about the warnings cleanup?
>
> We also need to fix our stack usage in many places so we can
> lower the ridiculous size of -Wframe-larger-than=20480
5 pages, ouch! Windows will (silently!) abort a process that has a page
fault that jumps larger than the 1 page stack guard; a good goal is
-Wframe-larger-than=4096.
>
> * acinclude.m4: Rewrite to use gl_WARN_ADD and gl_MANYWARN_ALL_GCC
> * bootstrap.conf: Add warnings & manywarnings
> * configure.ac: Switch to gl_WARN_ADD
> * m4/compiler-flags.m4: Obsoleted by gl_WARN_ADD
> ---
> acinclude.m4 | 153 ++++++++++++++++++++++++++------------------------
I really don't like how we have crammed so much into acinclude.m4; it
would be nicer if we could separate things into individual m4/foo.m4
files...
> bootstrap.conf | 2 +
> configure.ac | 15 +++--
> m4/compiler-flags.m4 | 48 ----------------
...much like this used to be. But that's orthogonal to this patch. (It
always takes me too long to figure out where the libvirt list of -W
flags is to modify that list).
> @@ -13,90 +8,102 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[
> dnl ******************************
>
> AC_ARG_ENABLE(compile-warnings,
> - [AC_HELP_STRING([--enable-compile-warnings=@<:@no/minimum/yes/maximum/error@:>@],
> + [AC_HELP_STRING([--enable-compile-warnings=@<:@no/yes/error@:>@],
> [Turn on compiler warnings])],,
So we're getting rid of minimum and maximum (truthfully, I never used
them). Sounds okay, especially since they weren't documented in HACKING
(just in ./configure --help).
> + yes|minimum|maximum|error)
Hmm, so you still silently accept them, but they now are synonyms for yes.
> +
> + # List of warnings that are not relevant / wanted
> + dontwarn="$dontwarn -Wc++-compat" # Don't care about C++ compiler compat
> + dontwarn="$dontwarn -Wtraditional" # Don't care about ancient C standard compat
> + dontwarn="$dontwarn -Wtraditional-conversion" # Don't care about ancient C standard compat
> + dontwarn="$dontwarn -Wsystem-headers" # Ignore warnings in /usr/include
> + dontwarn="$dontwarn -Wpadded" # Happy for compiler to add struct padding
> + dontwarn="$dontwarn -Wunreachable-code" # GCC very confused with -O2
> + dontwarn="$dontwarn -Wconversion" # Too many to deal with
> + dontwarn="$dontwarn -Wsign-conversion" # Too many to deal with
> + dontwarn="$dontwarn -Wvla" # GNULIB gettext.h violates
> + dontwarn="$dontwarn -Wundef" # Many GNULIB violations
We really ought to follow coreutils' lead of generating two separate
lists of -W flags - a larger set for libvirt and a smaller set for
gnulib. Then exemptions such as -Wvla could be used just for gnulib.
But that can be a followup patch.
> + dontwarn="$dontwarn -Wcast-qual" # Need to allow bad cast for execve()
> + dontwarn="$dontwarn -Wlong-long" # We need to use long long in many places
> +
> + # We might fundamentally need some of these disabled forever, but ideally
> + # we'd turn many of them on
> + dontwarn="$dontwarn -Wformat-nonliteral"
> + dontwarn="$dontwarn -Wswitch-default"
> + dontwarn="$dontwarn -Wswitch-enum"
> + dontwarn="$dontwarn -Wstrict-overflow"
> + dontwarn="$dontwarn -Wfloat-equal"
> + dontwarn="$dontwarn -Wdeclaration-after-statement"
> + dontwarn="$dontwarn -Wunsafe-loop-optimizations"
> + dontwarn="$dontwarn -Wcast-qual"
> + dontwarn="$dontwarn -Wconversion"
> + dontwarn="$dontwarn -Wsign-conversion"
> + dontwarn="$dontwarn -Wold-style-definition"
> + dontwarn="$dontwarn -Wmissing-noreturn"
> + dontwarn="$dontwarn -Wpacked"
> + dontwarn="$dontwarn -Wunused-macros"
> + dontwarn="$dontwarn -W"
-W is the same as -Wextra - that turns on a lot of useful warnings. Can
we fine-tune it to just disable the needed warnings?
> + dontwarn="$dontwarn -Woverlength-strings"
> + dontwarn="$dontwarn -Wmissing-format-attribute"
> + dontwarn="$dontwarn -Wstack-protector"
> +
> + # Get all possible GCC warnings
> + gl_MANYWARN_ALL_GCC([maybewarn])
> +
> + # Remove the ones we don't want, blacklisted earlier
> + gl_MANYWARN_COMPLEMENT([wantwarn], [$maybewarn], [$dontwarn])
> +
> + # Check for $CC support of each warning
> + for w in $wantwarn; do
> + gl_WARN_ADD([$w])
> + done
Yep, gnulib's macros are nice for this!
> +
> + # This should be < 256 really, but with PATH_MAX everywhere
> + # we have doom, even with 4096. In fact we have some functions
> + # with several PATH_MAX sized variables :-( We should kill off
> + # all PATH_MAX usage and then lower this limit
> + gl_WARN_ADD([-Wframe-larger-than=20480])
> + dnl gl_WARN_ADD([-Wframe-larger-than=4096])
> + dnl gl_WARN_ADD([-Wframe-larger-than=256])
256 is a bit tight; I don't know of any platform where 4096 is an
unreasonable bound for still reliably detecting stack overflow (then
again, 4096 compared to 256 is 16 times more likely to stack overflow on
a deeply nested call graph where every function in the graph uses a lot
of stack).
> +
> + # Extra special flags
> + gl_WARN_ADD([-Wp,-D_FORTIFY_SOURCE=2])
> + dnl Fedora only uses -fstack-protector, but doesn't seem to
> + dnl be great overhead in adding -fstack-protector-all instead
> + dnl gl_WARN_ADD([-fstack-protector])
> + gl_WARN_ADD([-fstack-protector-all])
> + gl_WARN_ADD([--param=ssp-buffer-size=4])
> + gl_WARN_ADD([-fexceptions])
> + gl_WARN_ADD([-fasynchronous-unwind-tables])
> + gl_WARN_ADD([-fdiagnostics-show-option])
> +
> + if test "$enable_compile_warnings" = "error"
> + then
> + gl_WARN_ADD([-Werror])
> + fi
> ;;
> *)
> AC_MSG_ERROR(Unknown argument '$enable_compile_warnings' to --enable-compile-warnings)
> ;;
> esac
>
>
> AC_ARG_ENABLE([test-oom],
> @@ -2538,6 +2540,7 @@ AC_MSG_NOTICE([Miscellaneous])
> AC_MSG_NOTICE([])
> AC_MSG_NOTICE([ Debug: $enable_debug])
> AC_MSG_NOTICE([ Warnings: $enable_compile_warnings])
> +AC_MSG_NOTICE([Warning Flags: $WARN_CFLAGS])
Nice configure summary addition.
Overall, I like the direction of this patch.
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110401/f28c75ce/attachment-0001.sig>
More information about the libvir-list
mailing list