[Mesa-dev] [PATCH mesa] build: make passing an incorrect pointer type a hard error

[Eric Engestrom]

On Thursday, 2018-11-22 15:04:54 +0000, Emil Velikov wrote:
> On 2018/11/22, Eric Engestrom wrote:
> > On Thursday, 2018-11-22 13:15:05 +0000, Eric Engestrom wrote:
> > > More or less any of this issue pointed out by the compiler is
> > > a coding error. Make sure we flag it and bail loudly.
> > > 
> > > v2: - apply the change to autotools and scons as well (Emil)
> > >     - C++ doesn't need this, it's already an error and the flag
> > >       doesn't exist (Gert)
> > > 
> > > Signed-off-by: Eric Engestrom <eric.engestrom at intel.com>
> > > Reviewed-by: Dylan Baker <dylan at pnwbakers.com> # v1
> > > Reviewed-by: Emil Velikov <emil.velikov at collabora.com> # v1
> > > [Emil: apply the same change to autotools and scons]
> > > Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> > > ---
> > > Emil, I dropped the Android bit as this `-Wno-error` makes it look to me
> > > like they don't want compiler warnings to stop the build.
> 
> By default Android builds with -Werror which threats all warnings as
> errors. In an extremely well controlled environment that is fine.

Yeah, -Werror is a recipe for headaches if not disasters, but if they
chose it...

> 
> The individual Werror=foo is separate thing. The following example`
> illustrates that. Note adding -Wno-error after -Werror=foo does not
> change anything, rightfully so.
> 
> $ echo -e "#include <stdio.h>
> int main(void)
> { int foo; return fprintf(\"test\"); }" >> eric.c
> $ gcc -Wall -Werror  -Wno-error -Werror=incompatible-pointer-types test.c
> 
> Please add the hunk back?

Right, I thought the -Wno-error after the -Werror=foo would disable all
-Werror=*, but after testing it, you're right, I had misunderstood.
Adding the android hunk back.

That said, android doesn't have any of the other -Werror=foo that meson
and autotools have; should it? Did it just get neglected when those were
added, or is there a reason not to have any -Werror=foo on android?

> 
> > > ---
> > >  configure.ac     | 1 +
> > >  meson.build      | 1 +
> > >  scons/gallium.py | 1 +
> > >  3 files changed, 3 insertions(+)
> > > 
> > > diff --git a/configure.ac b/configure.ac
> > > index 93a52475d50e6ee65432..422c8e3c87c897a88b51 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -307,6 +307,7 @@ dnl
> > >  AX_CHECK_COMPILE_FLAG([-Wall],                                 [CFLAGS="$CFLAGS -Wall"])
> > >  AX_CHECK_COMPILE_FLAG([-Werror=implicit-function-declaration], [CFLAGS="$CFLAGS -Werror=implicit-function-declaration"])
> > >  AX_CHECK_COMPILE_FLAG([-Werror=missing-prototypes],            [CFLAGS="$CFLAGS -Werror=missing-prototypes"])
> > > +AX_CHECK_COMPILE_FLAG([-Werror=incompatible-pointer-types],    [CFLAGS="$CFLAGS -Werror=incompatible-pointer-types"])
> > >  AX_CHECK_COMPILE_FLAG([-Wmissing-prototypes],                  [CFLAGS="$CFLAGS -Wmissing-prototypes"])
> > >  dnl Dylan Baker: gcc and clang always accepr -Wno-*, hence check for the original warning, then set the no-* flag
> > >  AX_CHECK_COMPILE_FLAG([-Wmissing-field-initializers],          [CFLAGS="$CFLAGS -Wno-missing-field-initializers"])
> > > diff --git a/meson.build b/meson.build
> > > index 4ad41fde13a33f546d48..2fde2b23e24fd2e4321e 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -788,6 +788,7 @@ endif
> > >  c_args = []
> > >  foreach a : ['-Werror=implicit-function-declaration',
> > >               '-Werror=missing-prototypes', '-Werror=return-type',
> > > +             '-Werror=incompatible-pointer-types',
> > >               '-fno-math-errno',
> > >               '-fno-trapping-math', '-Qunused-arguments']
> > >    if cc.has_argument(a)
> > > diff --git a/scons/gallium.py b/scons/gallium.py
> > > index 963834a5fbc12a378ad4..04175014925cf088e5ea 100755
> > > --- a/scons/gallium.py
> > > +++ b/scons/gallium.py
> > > @@ -451,6 +451,7 @@ def generate(env):
> > >          # - http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
> > >          ccflags += [
> > >              '-Wall',
> > > +            '-Werror=incompatible-pointer-types',
> > 
> > Hmm, just got the results from travis, and I hadn't realised that these
> > flags aren't checked before being given to the compiler, so this just
> > breaks the scons build. Until someone adds a 'does the compiler
> > understand this flag' feature to scons, we just can't add this flag.
> > 
> > I'll just revert the scons bit as well, so it's gonna be just meson and
> > autotools.
> > 
> Right, I forgot the toggle is present since GCC 5.0 while we use older
> one there.
> 
> Technically, one can reinstate the ccversion hunk from commit
> c068610a7df and bar on v5.0 or later. I don't feel strongly about it
> though.

I don't really care about scons, certainly not enough to do that right
now :P
And compiler versions tables are fragile and I dislike them. Ideally
scons would be able to actually test the compiler to see what it
understands or not.

> 
> With the Android piece, regardless of scons the patch is
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
> 
> Thanks
> Emil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev