Bug 22939

Summary: [PATCH] Add strict compilation option to util/macros
Product: xorg Reporter: Pauli <suokkos>
Component: Build/ModularAssignee: Jeremy Huddleston Sequoia <jeremyhu>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: medium CC: jeremyhu
Version: git   
Hardware: All   
OS: All   
Whiteboard: 2011BRB_Reviewed
i915 platform: i915 features:
Attachments:
Description Flags
Patch to add --enable-strict-compilation
none
[PATCH 1/4] Add XORG_STRICT_OPTION macro for strict compilation option
none
[PATCH 2/4] Add intel compiler support to XORG_CWARNFLAGS.
none
[PATCH 3/4] Remove -fbad-function-cast and add -fstrict-aliasing=2.
none
[PATCH 4/4] Version bump: 1.3.0
none
Example how to add XORG_STRICT_OPTION to libdrm
none
[PATCH 3/4] Remove -fbad-function-cast and add -wstrict-aliasing=2. none

Description Pauli 2009-07-24 14:24:46 UTC
Created attachment 27987 [details] [review]
Patch to add --enable-strict-compilation

This is idea how to make it easy for all components to add --enable-strict-compilation configure option to help developers find bugs from code early in development.

There is a few additions that I though might be good idea but didn't want to add without comments.

I thought about making the macro automatically add flags to CPPFLAGS but I'm not sure if it is good idea.

Possible addition to macro is detection if we are compiling from git from not tagged version. In that case default could be enabling strict compilation while keeping default disabled for tagged/release versions.

Also flags for sun or intel compilers would be nice (if they are supported build targets).
Comment 1 Alan Coopersmith 2009-07-24 15:11:08 UTC
This is an interesting idea - certainly we'd have to discuss on xorg-devel
before considering making strict the default in development/git builds.

As someone who compiles regularly with the Sun compilers, I can vouch for them
being a supported target.   To enable support of multiple compilers, I'd swap
the order of the tests, making the gcc test the inner test, so you'd instead
have:

AC_ARG_ENABLE(strict-compilation, ....)
if test "x$STRICT_COMPILE" = "xyes"; then
    # Set C standard to C99 because that is closest standard to what code uses
    AC_PROG_CC_C99
    if test "x$GCC" = xyes; then
       STRICT_CFLAGS="$CFLAGS -pedantic -Werror"
    elif test "x$SUNCC" = "xyes"; then
        CWARNFLAGS="-errwarn"
    fi
fi

(You can see how to test for Sun cc and set SUNCC in the current 
 XORG_CWARNFLAGS macro.  -errwarn is the Sun compiler flag to turn
 warnings into errors - if you want to see documentation of the other
 available options, see http://docs.sun.com/app/docs/doc/819-5265 .)

I also wonder if AC_PROG_CC_C99 should be always enabled, not just in
strict compilation mode, since that may cause strange, hard to debug
changes between strict and non-strict modes.

Also, I should probably document that the intended versioning scheme for 
xorg-macros is to bump the minor version when adding new macros, so this
would have Minimum version 1.3.0 (which is why the XORG_MACROS_VERSION
only checks major & minor versions, since micro versions are just bug fixes
and implementation details).
Comment 2 Pauli 2009-07-25 06:43:35 UTC
Created attachment 27988 [details] [review]
[PATCH 1/4] Add XORG_STRICT_OPTION macro for strict compilation option

Updated version of patch (multiple patches this time)
Comment 3 Pauli 2009-07-25 06:44:03 UTC
Created attachment 27989 [details] [review]
[PATCH 2/4] Add intel compiler support to XORG_CWARNFLAGS.
Comment 4 Pauli 2009-07-25 06:44:28 UTC
Created attachment 27990 [details] [review]
[PATCH 3/4] Remove -fbad-function-cast and add -fstrict-aliasing=2.
Comment 5 Pauli 2009-07-25 06:44:59 UTC
Created attachment 27991 [details] [review]
[PATCH 4/4] Version bump: 1.3.0
Comment 6 Pauli 2009-07-25 06:47:15 UTC
Created attachment 27992 [details] [review]
Example how to add XORG_STRICT_OPTION to libdrm
Comment 7 Pauli 2009-07-25 15:49:01 UTC
Created attachment 28003 [details] [review]
[PATCH 3/4] Remove -fbad-function-cast and add -wstrict-aliasing=2.

fixed the error in compiler switch
Comment 8 Peter Hutterer 2009-09-01 22:16:16 UTC
Patch 1/4 (XORG_STRICT_OPTION) pushed as 3b7dd69d0bf6bc19f0e4403bb6611de87497aac3
Comment 9 Alan Coopersmith 2009-09-09 14:13:40 UTC
xorg-macros 1.3.0 has been released with these, so it can be used for the
upcoming X11R7.5 RC's.   The additional patches to tweak the flags for
specific compilers can go in a later 1.3.x release after people who use those
compilers can confirm they're the right ones - since they don't change the
macro definitions, just the implementations, we didn't think it was necessary
to hold 1.3.0 for them.

Thanks for submitting these.
Comment 10 Gaetan Nadon 2009-12-15 07:23:03 UTC
Should this bug report considered fixed?
Comment 11 Jeremy Huddleston Sequoia 2010-10-30 15:01:24 UTC
No, CWARNFLAGS still has fno-strict-aliasing in it.
Comment 12 Jeremy Huddleston Sequoia 2011-10-09 02:50:12 UTC
We've been building without -fno-strict-aliasing (aside from xorg-server) in 
XQuartz for a long time now without issue.  I really think we should remove 
-fno-strict-aliasing from CWARNFLAGS and include it in xorg-server/configure.ac 
... is the only thing holding that up the fear of using the newr util-macros on 
an older server?
Comment 13 Gaetan Nadon 2011-10-16 16:50:26 UTC
(In reply to comment #12)
> We've been building without -fno-strict-aliasing (aside from xorg-server) in 
> XQuartz for a long time now without issue.  I really think we should remove 
> -fno-strict-aliasing from CWARNFLAGS and include it in xorg-server/configure.ac 
> ... is the only thing holding that up the fear of using the newr util-macros on 
> an older server?

Yes, that is the "fear" a.k.a. backward compatibility. For the server and the libs and the drivers...

A simple solution is to leave CWARNFLAGS alone and create a new warning variable without the offending flag. Each module then decide which one to use. That's the usual strategy, but yes, dual maintenance for the macros. Still the lowest cost solution.
Comment 14 Jeremy Huddleston Sequoia 2011-10-16 18:35:46 UTC
Ok, I'll come up with something "soon" then, so we can do this before the next katamari
Comment 15 Gaetan Nadon 2011-10-17 10:20:12 UTC
An interim solution for the server would be to simply append after CWARNFLAGS in configure.ac appropriate counter-flags.

# Quoted so that make will expand $(CWARNFLAGS) in makefiles to allow
# easier overrides at build time.
XSERVER_CFLAGS='$(CWARNFLAGS)' +++ add counter-flags +++

This has the merit of being backward compatible and not requiring any util-macros change. It also limits the discussion to the xserver and reduce complexity for testing.

Once the dust has settled, a solution in util-macros can be devised which will be transparent to the server.
Comment 16 Jeremy Huddleston Sequoia 2011-10-17 11:38:02 UTC
Yeah, I have that patch already, since I'm using it in XQuartz.  I just sent it to the list for review.
Comment 17 Jeremy Huddleston Sequoia 2011-11-01 14:21:27 UTC
I sent a series to xorg-devel to address this.  I need help with the recursion in patch 2.  m4 doesn't like me.

http://lists.x.org/archives/xorg-devel/2011-November/026780.html

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.