[Mesa-dev] [PATCH 2/2] configure: Leverage gcc warn options to enable safe use of C99 features where possible.

Jose Fonseca jfonseca at vmware.com
Mon Mar 2 08:10:54 PST 2015 

On 27/02/15 15:59, Ian Romanick wrote:
> I like the idea as it should prevent future thrash.  There are a couple
> comments below.
>
> On 02/26/2015 08:51 AM, Jose Fonseca wrote:
>> The main objective of this change is to enable Linux developers to use
>> more of C99 throughout Mesa, with confidence that the portions that need
>> to be built with MSVC -- and only those portions --, stay portable.
>>
>> This is achieved by using the appropriate -Werror= options only on the
>> places they need to be used.
>>
>> Unfortunately we still need MSVC 2008 on a few portions of the code
>> (namely llvmpipe and its dependencies).  I hope to eventually eliminate
>> this so that we can use C99 everywhere, but there are technical/logistic
>> challenges (specifically, newer Windows SDKs no longer bundle MSVC,
>> instead require a full installation of Visual Studio, and that has
>> hindered adoption of newer MSVC versions on our build processes.)
>> Thankfully we have more directy control over our OpenGL driver, which is
>> why we're now able to migrate to MSVC 2013 for most of the tree.
>> ---
>>   configure.ac                               | 17 +++++++++++++++++
>>   src/egl/main/Makefile.am                   |  1 +
>>   src/gallium/auxiliary/Makefile.am          |  7 +++++--
>>   src/gallium/drivers/llvmpipe/Makefile.am   |  6 ++++--
>>   src/gallium/state_trackers/egl/Makefile.am |  3 ++-
>>   src/gallium/targets/egl-static/Makefile.am |  3 ++-
>>   src/glsl/Makefile.am                       |  8 ++++++--
>>   src/loader/Makefile.am                     |  1 +
>>   src/mapi/Makefile.am                       |  4 +++-
>>   src/mesa/Makefile.am                       | 10 ++++++++--
>>   src/util/Makefile.am                       |  3 ++-
>>   11 files changed, 51 insertions(+), 12 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 5fbb7bc..22dc023 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -263,6 +263,18 @@ if test "x$GCC" = xyes; then
>>       # gcc's builtin memcmp is slower than glibc's
>>       # https://urldefense.proofpoint.com/v2/url?u=http-3A__gcc.gnu.org_bugzilla_show-5Fbug.cgi-3Fid-3D43052&d=AwICaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=gzXuw5K38HQj__hbWmSmucwxyJyOitvZ880mS5zyZ-w&s=KMhA_Gf3BUCyos0GcxsRFjkuWQogkFDUojBIet4t1o4&e=
>>       CFLAGS="$CFLAGS -fno-builtin-memcmp"
>> +
>> +    # Flags to help ensure that certain portions of the code -- and only those
>> +    # portions -- can be built with MSVC:
>> +    # - src/util, src/gallium/auxiliary, and src/gallium/drivers/llvmpipe needs
>> +    #   to build with Windows SDK 7.0.7600, which bundles MSVC 2008
>> +    # - non-Linux/Posix OpenGL portions needs to build on MSVC 2013 (which
>> +    #   supports most of C99)
>> +    # - the rest has no compiler compiler restrictions
>> +    MSVC2013_COMPAT_CFLAGS="-Werror=vla -Werror=pointer-arith"
>> +    MSVC2013_COMPAT_CXXFLAGS="-Werror=vla -Werror=pointer-arith"
>> +    MSVC2008_COMPAT_CFLAGS="$MSVC2013_COMPAT_CFLAGS -Werror=declaration-after-statement"
>> +    MSVC2008_COMPAT_CXXFLAGS="$MSVC2013_COMPAT_CXXFLAGS"
>>   fi
>>   if test "x$GXX" = xyes; then
>>       CXXFLAGS="$CXXFLAGS -Wall"
>> @@ -288,6 +300,11 @@ if test "x$GXX" = xyes; then
>>       CXXFLAGS="$CXXFLAGS -fno-builtin-memcmp"
>>   fi
>>
>> +AC_SUBST([MSVC2013_COMPAT_CFLAGS])
>> +AC_SUBST([MSVC2013_COMPAT_CXXFLAGS])
>> +AC_SUBST([MSVC2008_COMPAT_CFLAGS])
>> +AC_SUBST([MSVC2008_COMPAT_CXXFLAGS])
>> +
>>   dnl even if the compiler appears to support it, using visibility attributes isn't
>>   dnl going to do anything useful currently on cygwin apart from emit lots of warnings
>>   case "$host_os" in
>> diff --git a/src/egl/main/Makefile.am b/src/egl/main/Makefile.am
>> index d21d8a9..a4db210 100644
>> --- a/src/egl/main/Makefile.am
>> +++ b/src/egl/main/Makefile.am
>> @@ -26,6 +26,7 @@ AM_CFLAGS = \
>>   	-I$(top_srcdir)/src/gbm/main \
>>   	$(DEFINES) \
>>   	$(VISIBILITY_CFLAGS) \
>> +	$(MSVC2013_COMPAT_CFLAGS) \
>>   	$(EGL_CFLAGS) \
>>   	-D_EGL_NATIVE_PLATFORM=$(EGL_NATIVE_PLATFORM) \
>>   	-D_EGL_DRIVER_SEARCH_DIR=\"$(libdir)/egl\" \
>> diff --git a/src/gallium/auxiliary/Makefile.am b/src/gallium/auxiliary/Makefile.am
>> index 4b62057..27a8b3f 100644
>> --- a/src/gallium/auxiliary/Makefile.am
>> +++ b/src/gallium/auxiliary/Makefile.am
>> @@ -12,9 +12,12 @@ noinst_LTLIBRARIES = libgallium.la
>>   AM_CFLAGS = \
>>   	-I$(top_srcdir)/src/gallium/auxiliary/util \
>>   	$(GALLIUM_CFLAGS) \
>> -	$(VISIBILITY_CFLAGS)
>> +	$(VISIBILITY_CFLAGS) \
>> +	$(MSVC2008_COMPAT_CXXFLAGS)
>>
>> -AM_CXXFLAGS = $(VISIBILITY_CXXFLAGS)
>> +AM_CXXFLAGS = \
>> +	$(VISIBILITY_CXXFLAGS) \
>> +	$(MSVC2008_COMPAT_CXXFLAGS)
>>
>>   libgallium_la_SOURCES = \
>>   	$(C_SOURCES) \
>> diff --git a/src/gallium/drivers/llvmpipe/Makefile.am b/src/gallium/drivers/llvmpipe/Makefile.am
>> index 0bd4282..1d3853e 100644
>> --- a/src/gallium/drivers/llvmpipe/Makefile.am
>> +++ b/src/gallium/drivers/llvmpipe/Makefile.am
>> @@ -25,10 +25,12 @@ include $(top_srcdir)/src/gallium/Automake.inc
>>
>>   AM_CFLAGS = \
>>   	$(GALLIUM_DRIVER_CFLAGS) \
>> -	$(LLVM_CFLAGS)
>> +	$(LLVM_CFLAGS) \
>> +	$(MSVC2008_COMPAT_CFLAGS)
>>   AM_CXXFLAGS= \
>>   	$(GALLIUM_DRIVER_CXXFLAGS) \
>> -	$(LLVM_CXXFLAGS)
>> +	$(LLVM_CXXFLAGS) \
>> +	$(MSVC2008_COMPAT_CXXFLAGS)
>>
>>   noinst_LTLIBRARIES = libllvmpipe.la
>>
>> diff --git a/src/gallium/state_trackers/egl/Makefile.am b/src/gallium/state_trackers/egl/Makefile.am
>> index 31546a7..f13fcb2 100644
>> --- a/src/gallium/state_trackers/egl/Makefile.am
>> +++ b/src/gallium/state_trackers/egl/Makefile.am
>> @@ -27,7 +27,8 @@ include $(top_srcdir)/src/gallium/Automake.inc
>>
>>   AM_CFLAGS = \
>>   	$(GALLIUM_CFLAGS) \
>> -	$(VISIBILITY_CFLAGS)
>> +	$(VISIBILITY_CFLAGS) \
>> +	$(MSVC2013_COMPAT_CFLAGS)
>>
>>   AM_CPPFLAGS = \
>>   	-I$(top_srcdir)/src/egl/main \
>> diff --git a/src/gallium/targets/egl-static/Makefile.am b/src/gallium/targets/egl-static/Makefile.am
>> index 85f2ac1..51c3f05 100644
>> --- a/src/gallium/targets/egl-static/Makefile.am
>> +++ b/src/gallium/targets/egl-static/Makefile.am
>> @@ -31,7 +31,8 @@
>>   include $(top_srcdir)/src/gallium/Automake.inc
>>
>>   AM_CFLAGS = \
>> -	$(GALLIUM_TARGET_CFLAGS)
>> +	$(GALLIUM_TARGET_CFLAGS) \
>> +	$(MSVC2013_COMPAT_CFLAGS)
>>
>>   AM_CPPFLAGS = \
>>   	-I$(top_srcdir)/src/gallium/state_trackers/egl \
>> diff --git a/src/glsl/Makefile.am b/src/glsl/Makefile.am
>> index 5a0a643..b466a3b 100644
>> --- a/src/glsl/Makefile.am
>> +++ b/src/glsl/Makefile.am
>> @@ -33,8 +33,12 @@ AM_CPPFLAGS = \
>>   	-I$(top_srcdir)/src/gtest/include \
>>   	-I$(top_builddir)/src/glsl/nir \
>>   	$(DEFINES)
>> -AM_CFLAGS = $(VISIBILITY_CFLAGS)
>> -AM_CXXFLAGS = $(VISIBILITY_CXXFLAGS)
>> +AM_CFLAGS = \
>> +	$(VISIBILITY_CFLAGS) \
>> +	$(MSVC2013_COMPAT_CFLAGS)
>> +AM_CXXFLAGS = \
>> +	$(VISIBILITY_CXXFLAGS) \
>> +	$(MSVC2013_COMPAT_CXXFLAGS)
>>
>>   EXTRA_DIST = tests glcpp/tests README TODO glcpp/README	\
>>   	glsl_lexer.ll					\
>> diff --git a/src/loader/Makefile.am b/src/loader/Makefile.am
>> index 36ddba8..3d32279 100644
>> --- a/src/loader/Makefile.am
>> +++ b/src/loader/Makefile.am
>> @@ -30,6 +30,7 @@ libloader_la_CPPFLAGS = \
>>   	-I$(top_srcdir)/include \
>>   	-I$(top_srcdir)/src \
>>   	$(VISIBILITY_CFLAGS) \
>> +	$(MSVC2013_COMPAT_CFLAGS) \
>>   	$(LIBUDEV_CFLAGS)
>
> I don't think this is necessary for src/loader.  Isn't that only built
> for DRI?

No, it's being built with MSVC too, and unconditionally.

But I think that after we remove src/gallium/state_trackers/egl  and 
src/gallium/targets/egl-static  then we could stop building it too.

>
>>   libloader_la_SOURCES = $(LOADER_C_FILES)
>> diff --git a/src/mapi/Makefile.am b/src/mapi/Makefile.am
>> index 6794682..b0a6c8c 100644
>> --- a/src/mapi/Makefile.am
>> +++ b/src/mapi/Makefile.am
>> @@ -39,7 +39,9 @@ EXTRA_DIST = \
>>   	glapi/SConscript \
>>   	shared-glapi/SConscript
>>
>> -AM_CFLAGS = $(PTHREAD_CFLAGS)
>> +AM_CFLAGS = \
>> +	$(PTHREAD_CFLAGS) \
>> +	$(MSVC2013_COMPAT_CFLAGS)
>>   AM_CPPFLAGS =							\
>>   	$(DEFINES)						\
>>   	$(SELINUX_CFLAGS)					\
>> diff --git a/src/mesa/Makefile.am b/src/mesa/Makefile.am
>> index b6cb8f1..5e9a820 100644
>> --- a/src/mesa/Makefile.am
>> +++ b/src/mesa/Makefile.am
>> @@ -136,8 +136,14 @@ noinst_LTLIBRARIES += libmesagallium.la
>>   endif
>>
>>   AM_CPPFLAGS = $(DEFINES) $(INCLUDE_DIRS)
>> -AM_CFLAGS = $(LLVM_CFLAGS) $(VISIBILITY_CFLAGS)
>> -AM_CXXFLAGS = $(LLVM_CFLAGS) $(VISIBILITY_CXXFLAGS)
>> +AM_CFLAGS = \
>> +	$(LLVM_CFLAGS) \
>> +	$(VISIBILITY_CFLAGS) \
>> +	$(MSVC2013_COMPAT_CFLAGS)
>> +AM_CXXFLAGS = \
>> +	$(LLVM_CFLAGS) \
>> +	$(VISIBILITY_CXXFLAGS) \
>> +	$(MSVC2013_COMPAT_CXXFLAGS)
>
> Forgive my ignorance about the build system... does applying this here
> affect building files down in src/mesa/drivers/dri?

I _think_ it applies to the current dir.

For e.g., src/mesa/drivers/dri/swrast/Makefile.am defines AM_CFLAGS from 
scratch.

But I confess I'm not that familiar with autotools neither, so I'm not 
sure the exact semantics, i.e., whether AM_*FLAGS _need_ to be defined 
on every directory, or merely that we happen to overwrite AM_*FLAGS on 
most directories...



>
>>   ARCH_LIBS =
>>
>> diff --git a/src/util/Makefile.am b/src/util/Makefile.am
>> index 9af2330..29b66e7 100644
>> --- a/src/util/Makefile.am
>> +++ b/src/util/Makefile.am
>> @@ -34,7 +34,8 @@ libmesautil_la_CPPFLAGS = \
>>   	-I$(top_srcdir)/src/gallium/include \
>>   	-I$(top_srcdir)/src/gallium/auxiliary \
>>   	$(SHA1_CFLAGS) \
>> -	$(VISIBILITY_CFLAGS)
>> +	$(VISIBILITY_CFLAGS) \
>> +	$(MSVC2008_COMPAT_CFLAGS)
>>
>>   libmesautil_la_SOURCES = \
>>   	$(MESA_UTIL_FILES) \
>

Jose

More information about the mesa-dev mailing list