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

[Emil Velikov]

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.
> 
True. Not to mention that having -Werror=pointer-arith and
-Werror=declaration-after-statement does not sounds too bad either.

Although I would suspect that either one might cause a bit of chaos atm
- i.e. we do (ab)use them rather often.

> 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
>>      # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43052
>>      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"
Jose,

I'm guessing that we might want to move the CXXFLAGS within the GXX
check below. Or maybe compact the two conditionals, as they are almost
identical - if you're feeling bored, that is :)

...
>> 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?
> 
There are quite a few users actually

src/egl/drivers/dri2/Makefile.am
src/gallium/auxiliary/pipe-loader/Makefile.am
src/gallium/targets/*/Makefile.am
src/gbm/Makefile.am
src/glx/Makefile.am

...
>> 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?
> 
It affects only the objects (_PROGRAMS, _LTLIBRARIES) built within this
Makefile. Namely:

libmesa_sse41.la
libmesa.la
libmesagallium.la
gen_matypes

Those are used in many places, but at that stage the above CFLAGS are
irrelevant.

-Emil