[Openvpn-devel,v3] Enable stricter compiler warnings by default

This by default enables the compiler warnings one could previously
enable using the --enable-strict configure option.  I think it is
okay to do so now, because we've taken care of many warnings in the
more standard builds.  (Most of those were totally harmless, but they
prevented us from spotting new more serious mistakes.)

The --enable-strict flag now enables two extra warning flags that I
think can be useful:

-Wsign-compare warns when the compiler promotes a signed type to
unsigned before comparing, which can lead to unexpected behaviour.

-Wuninitialized adds extra warnings about usage of uninitialized variables
or struct elements.

Signed-off-by: Steffan Karger <steffan@karger.me>
---
v2: Just move forward with warnings, don't add --disable-strict.
v3: Nothing is as simple as it seems: put user-supplied CFLAGS behind
    default flags, check that potential esotheric compiler supports
    flags before enabling them.

 configure.ac                | 12 +++++++-
 m4/ax_check_compile_flag.m4 | 74 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100644 m4/ax_check_compile_flag.m4

Hi,

On Thu, Feb 1, 2018 at 10:45 AM, Steffan Karger <steffan@karger.me> wrote:
> This by default enables the compiler warnings one could previously
> enable using the --enable-strict configure option.  I think it is
> okay to do so now, because we've taken care of many warnings in the
> more standard builds.  (Most of those were totally harmless, but they
> prevented us from spotting new more serious mistakes.)
>
> The --enable-strict flag now enables two extra warning flags that I
> think can be useful:
>
> -Wsign-compare warns when the compiler promotes a signed type to
> unsigned before comparing, which can lead to unexpected behaviour.
>
> -Wuninitialized adds extra warnings about usage of uninitialized variables
> or struct elements.
>
> Signed-off-by: Steffan Karger <steffan@karger.me>
> ---
> v2: Just move forward with warnings, don't add --disable-strict.
> v3: Nothing is as simple as it seems:

:)

>     put user-supplied CFLAGS behind
>     default flags, check that potential esotheric compiler supports
>     flags before enabling them.
>
>  configure.ac                | 12 +++++++-
>  m4/ax_check_compile_flag.m4 | 74 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100644 m4/ax_check_compile_flag.m4
>
> diff --git a/configure.ac b/configure.ac
> index 2cbf3358..33b6bc8e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1292,13 +1292,23 @@ if test "${enable_pkcs11}" = "yes"; then
>         )
>  fi
>
> +AX_CHECK_COMPILE_FLAG(
> +    [-Wno-unused-function],
> +    [CFLAGS="-Wno-unused-function ${CFLAGS}"]
> +)

As Steffan himself noted, need tabs instead of spaces for consistency

> +AX_CHECK_COMPILE_FLAG(
> +    [-Wno-unused-parameter],
> +    [CFLAGS="-Wno-unused-parameter ${CFLAGS}"]
> +)

same here

> +AX_CHECK_COMPILE_FLAG([-Wall], [CFLAGS="-Wall ${CFLAGS}"])

The three options could have checked together, but this is fine too.

All said and done, the check is not as useful as I had imagined..
AX_CHECK_COMPLIE_FLAG only catches errors, not warnings.
For instance, IBM's xlc, at least on Linux, only warns about unknown options.
So nothing gained -- in fact 4 warning lines per source file caused by
these options.
Hopefully, its still useful on some other never used build envs..

Anyway, the check protects against probable build failures on some unknown
systems, and I won't waste any more time dwelling on this.

> +
>  if test "${enable_pedantic}" = "yes"; then
>         enable_strict="yes"
>         CFLAGS="${CFLAGS} -pedantic"
>         AC_DEFINE([PEDANTIC], [1], [Enable pedantic mode])
>  fi
>  if test "${enable_strict}" = "yes"; then
> -       CFLAGS="${CFLAGS} -Wall -Wno-unused-parameter -Wno-unused-function"
> +       CFLAGS="${CFLAGS} -Wsign-compare -Wuninitialized"
>  fi
>  if test "${enable_werror}" = "yes"; then
>         CFLAGS="${CFLAGS} -Werror"
> diff --git a/m4/ax_check_compile_flag.m4 b/m4/ax_check_compile_flag.m4
> new file mode 100644
> index 00000000..dcabb92a
> --- /dev/null
> +++ b/m4/ax_check_compile_flag.m4
> @@ -0,0 +1,74 @@
> +# ===========================================================================
> +#  https://www.gnu.org/software/autoconf-archive/ax_check_compile_flag.html
> +# ===========================================================================
> +#
> +# SYNOPSIS
> +#
> +#   AX_CHECK_COMPILE_FLAG(FLAG, [ACTION-SUCCESS], [ACTION-FAILURE], [EXTRA-FLAGS], [INPUT])
> +#
> +# DESCRIPTION
> +#
> +#   Check whether the given FLAG works with the current language's compiler
> +#   or gives an error.  (Warnings, however, are ignored)
> +#
> +#   ACTION-SUCCESS/ACTION-FAILURE are shell commands to execute on
> +#   success/failure.
> +#
> +#   If EXTRA-FLAGS is defined, it is added to the current language's default
> +#   flags (e.g. CFLAGS) when the check is done.  The check is thus made with
> +#   the flags: "CFLAGS EXTRA-FLAGS FLAG".  This can for example be used to
> +#   force the compiler to issue an error when a bad flag is given.
> +#
> +#   INPUT gives an alternative input source to AC_COMPILE_IFELSE.
> +#
> +#   NOTE: Implementation based on AX_CFLAGS_GCC_OPTION. Please keep this
> +#   macro in sync with AX_CHECK_{PREPROC,LINK}_FLAG.
> +#
> +# LICENSE
> +#
> +#   Copyright (c) 2008 Guido U. Draheim <guidod@gmx.de>
> +#   Copyright (c) 2011 Maarten Bosmans <mkbosmans@gmail.com>
> +#
> +#   This program is free software: you can redistribute it and/or modify it
> +#   under the terms of the GNU General Public License as published by the
> +#   Free Software Foundation, either version 3 of the License, or (at your
> +#   option) any later version.
> +#
> +#   This program is distributed in the hope that it will be useful, but
> +#   WITHOUT ANY WARRANTY; without even the implied warranty of
> +#   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General
> +#   Public License for more details.
> +#
> +#   You should have received a copy of the GNU General Public License along
> +#   with this program. If not, see <https://www.gnu.org/licenses/>.
> +#
> +#   As a special exception, the respective Autoconf Macro's copyright owner
> +#   gives unlimited permission to copy, distribute and modify the configure
> +#   scripts that are the output of Autoconf when processing the Macro. You
> +#   need not follow the terms of the GNU General Public License when using
> +#   or distributing such scripts, even though portions of the text of the
> +#   Macro appear in them. The GNU General Public License (GPL) does govern
> +#   all other use of the material that constitutes the Autoconf Macro.
> +#
> +#   This special exception to the GPL applies to versions of the Autoconf
> +#   Macro released by the Autoconf Archive. When you make and distribute a
> +#   modified version of the Autoconf Macro, you may extend this special
> +#   exception to the GPL to apply to your modified version as well.
> +
> +#serial 5
> +
> +AC_DEFUN([AX_CHECK_COMPILE_FLAG],
> +[AC_PREREQ(2.64)dnl for _AC_LANG_PREFIX and AS_VAR_IF
> +AS_VAR_PUSHDEF([CACHEVAR],[ax_cv_check_[]_AC_LANG_ABBREV[]flags_$4_$1])dnl
> +AC_CACHE_CHECK([whether _AC_LANG compiler accepts $1], CACHEVAR, [
> +  ax_check_save_flags=$[]_AC_LANG_PREFIX[]FLAGS
> +  _AC_LANG_PREFIX[]FLAGS="$[]_AC_LANG_PREFIX[]FLAGS $4 $1"
> +  AC_COMPILE_IFELSE([m4_default([$5],[AC_LANG_PROGRAM()])],
> +    [AS_VAR_SET(CACHEVAR,[yes])],
> +    [AS_VAR_SET(CACHEVAR,[no])])
> +  _AC_LANG_PREFIX[]FLAGS=$ax_check_save_flags])
> +AS_VAR_IF(CACHEVAR,yes,
> +  [m4_default([$2], :)],
> +  [m4_default([$3], :)])
> +AS_VAR_POPDEF([CACHEVAR])dnl
> +])dnl AX_CHECK_COMPILE_FLAGS
> --
> 2.14.1

This macro is a part of autoconf archive which, I thought, is
prerequisite for building from the repo. One anyway needs autotools
installed. Can't we not insist on having these common macros be present
as well? After all this is for building from git sources, not from a
dist tarball
which needs no autotools.

Also the notes in the macro file says "Please keep this macro in sync with
AX_CHECK_{PREPROC,LINK}_FLAG." which is easier to do if we use the
system's installed copy.

Anyway, if including such m4 macros in the repo is the policy,  this
is fine.

Having done the self-satisfying nitpicking:

Acked-by: Selva Nair <selva.nair@gmail.com>

(compile tested: --Wall is surprisingly quiet on linux, but exposes
lots of warnings on Windows (mingw) some of which are not benign)

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Hi,

On 02-02-18 05:43, Selva Nair wrote:
> On Thu, Feb 1, 2018 at 10:45 AM, Steffan Karger <steffan@karger.me> wrote:
>> [...]
>>
>> +AX_CHECK_COMPILE_FLAG([-Wall], [CFLAGS="-Wall ${CFLAGS}"])
> 
> The three options could have checked together, but this is fine too.
> 
> All said and done, the check is not as useful as I had imagined..
> AX_CHECK_COMPLIE_FLAG only catches errors, not warnings.
> For instance, IBM's xlc, at least on Linux, only warns about unknown options.
> So nothing gained -- in fact 4 warning lines per source file caused by
> these options.
> Hopefully, its still useful on some other never used build envs..
> 
> Anyway, the check protects against probable build failures on some unknown
> systems, and I won't waste any more time dwelling on this.
> 
>> diff --git a/m4/ax_check_compile_flag.m4 b/m4/ax_check_compile_flag.m4
>> new file mode 100644
>> [...]
> 
> This macro is a part of autoconf archive which, I thought, is
> prerequisite for building from the repo. One anyway needs autotools
> installed. Can't we not insist on having these common macros be present
> as well? After all this is for building from git sources, not from a
> dist tarball
> which needs no autotools.
> 
> Also the notes in the macro file says "Please keep this macro in sync with
> AX_CHECK_{PREPROC,LINK}_FLAG." which is easier to do if we use the
> system's installed copy.
> 
> Anyway, if including such m4 macros in the repo is the policy,  this
> is fine.

We previously did not depend on it (I didn't have the archive installed)
and thought we didn't want to pull in another dependency.  Also, the
error when the archive is missing is quite terrible and will cost a lot
of people time to understand.  Autoconf happily continues, then
configure fails with:

./configure: line 17861: syntax error near unexpected token `newline'
./configure: line 17861: `AX_CHECK_COMPILE_FLAG('

That said, if we prefer to not include the macro, it can simply be
removed from the commit.  I could even rewrite then to directly use
AC_COMPILE_IFELSE, which is bulkier and less clear but doesn't need the
include.

> Having done the self-satisfying nitpicking:
> 
> Acked-by: Selva Nair <selva.nair@gmail.com>

Thanks :)

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

On 02/02/18 07:32, Steffan Karger wrote:
> Hi,
> 
> On 02-02-18 05:43, Selva Nair wrote:
>> On Thu, Feb 1, 2018 at 10:45 AM, Steffan Karger <steffan@karger.me> wrote:
>>> [...]
>>>
>>> +AX_CHECK_COMPILE_FLAG([-Wall], [CFLAGS="-Wall ${CFLAGS}"])
>>
>> The three options could have checked together, but this is fine too.
>>
>> All said and done, the check is not as useful as I had imagined..
>> AX_CHECK_COMPLIE_FLAG only catches errors, not warnings.
>> For instance, IBM's xlc, at least on Linux, only warns about unknown options.
>> So nothing gained -- in fact 4 warning lines per source file caused by
>> these options.
>> Hopefully, its still useful on some other never used build envs..
>>
>> Anyway, the check protects against probable build failures on some unknown
>> systems, and I won't waste any more time dwelling on this.
>>
>>> diff --git a/m4/ax_check_compile_flag.m4 b/m4/ax_check_compile_flag.m4
>>> new file mode 100644
>>> [...]
>>
>> This macro is a part of autoconf archive which, I thought, is
>> prerequisite for building from the repo. One anyway needs autotools
>> installed. Can't we not insist on having these common macros be present
>> as well? After all this is for building from git sources, not from a
>> dist tarball
>> which needs no autotools.
>>
>> Also the notes in the macro file says "Please keep this macro in sync with
>> AX_CHECK_{PREPROC,LINK}_FLAG." which is easier to do if we use the
>> system's installed copy.
>>
>> Anyway, if including such m4 macros in the repo is the policy,  this
>> is fine.
> 
> We previously did not depend on it (I didn't have the archive installed)
> and thought we didn't want to pull in another dependency.  Also, the
> error when the archive is missing is quite terrible and will cost a lot
> of people time to understand.  Autoconf happily continues, then
> configure fails with:
> 
> ./configure: line 17861: syntax error near unexpected token `newline'
> ./configure: line 17861: `AX_CHECK_COMPILE_FLAG('
> 
> That said, if we prefer to not include the macro, it can simply be
> removed from the commit.  I could even rewrite then to directly use
> AC_COMPILE_IFELSE, which is bulkier and less clear but doesn't need the
> include.

I've just done a quick check where/how ax_check_compile_flag.m4 can be found
on Fedora 27, RHEL-7 and RHEL-6.

For Fedora 27 and RHEL-7, it is found in a package named "autoconf-archive".
For RHEL-6, no such package exists and I could also not find any other
packages which provides that .m4 file.

We could check this on other platforms as well (*BSD, Solaris, AIX) to see how
they handle this.  But if there's a similar situation there, I think it makes
perfect sense to ship this macro file.  What I don't like about that, is that
we should have some process to ensure we keep these m4 files up-to-date.
Which we have not really done so far :/

Your patch has been applied to the master and release/2.4 branch.

Tested on FreeBSD 10, Linux and, indeed, AIX 7 :-) - Linux/gcc is nicely
clean, FreeBSD/clang finds a few that are silly and want fixing...

    ../../../openvpn/src/openvpn/push.c:42:13: warning: unused variable
	  'push_reply_cmd' [-Wunused-variable]
    static char push_reply_cmd[] = "PUSH_REPLY";

    ../../../openvpn/src/openvpn/ssl_verify.c:1222:1: warning: unused label
	  'cleanup' [-Wunused-label]
    cleanup:

AIX 7 with gcc has quite a few warnings, but those are "AIX is annoying"
style things like these two:

    options.c:4947:16: warning: missing braces around initializer [-Wmissing-braces]
    options.c:4947:16: warning: (near initialization for 'remote.u6_addr') [-Wmissing-braces]

    tun.c:1791:13: warning: implicit declaration of function 'if_nametoindex' [-Wimplicit-function-declaration]

and interesting enough, one that should have been visible on the other
platforms as well - maybe different OpenSSL version?

    ssl_openssl.c: In function 'backend_tls_ctx_reload_crl':
    ssl_openssl.c:967:13: warning: value computed is not used [-Wunused-value]

.. but overall, AIX is much less noisy than I would have expected
(like, 20 warnings in total, half of them format warnings that Selva's
patch will address).  Good :-)


I have applied the patch to release/2.4 as well, as my worries about
"breaking some platform that previously was compiling nicely" shouldn't
be triggered (this is just turning on *warnings*, after all), and also
because the amount of warnings triggered is so small.


commit adbf68c00bf40089489c5e039138f855fc5e2392 (master)
commit b9d186211c87f52711592f1c29399a088912bf39 (release/2.4)
Author: Steffan Karger
Date:   Thu Feb 1 16:45:21 2018 +0100

     Enable stricter compiler warnings by default

     Signed-off-by: Steffan Karger <steffan@karger.me>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <20180201154521.7642-1-steffan@karger.me>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16426.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Hi,

On Tue, Feb 20, 2018 at 10:41:08AM +0100, Gert Doering wrote:
> Your patch has been applied to the master and release/2.4 branch.
> 
> Tested on FreeBSD 10, Linux and, indeed, AIX 7 :-) - Linux/gcc is nicely
> clean, FreeBSD/clang finds a few that are silly and want fixing...

Yeah.  Right.  And it blows up on centos-6 and opensolaris-10.

Now I can't claim that I understand this...  David double-checked this
macro and m4/ stuff, so "everything should have been good", but it
isn't.

CentOS 6:

checking lz4.h usability... no
checking lz4.h presence... no
checking for lz4.h... no
                usable LZ4 library or header not found, using version in src/com
+pat/compat-lz4.*
checking git checkout... yes
./configure: line 24067: syntax error near unexpected token `newline'
./configure: line 24067: `AX_CHECK_COMPILE_FLAG('
make: *** [config.status] Error 2


OpenSolaris blows up at the same point, but with less detail 
("unexpected (" or something like this)

Now what...?

gert

Hi,

On 20-02-18 11:00, Gert Doering wrote:
> On Tue, Feb 20, 2018 at 10:41:08AM +0100, Gert Doering wrote:
>> Your patch has been applied to the master and release/2.4 branch.
>>
>> Tested on FreeBSD 10, Linux and, indeed, AIX 7 :-) - Linux/gcc is nicely
>> clean, FreeBSD/clang finds a few that are silly and want fixing...
> 
> Yeah.  Right.  And it blows up on centos-6 and opensolaris-10.
> 
> Now I can't claim that I understand this...  David double-checked this
> macro and m4/ stuff, so "everything should have been good", but it
> isn't.
> 
> CentOS 6:
> 
> checking lz4.h usability... no
> checking lz4.h presence... no
> checking for lz4.h... no
>                 usable LZ4 library or header not found, using version in src/com
> +pat/compat-lz4.*
> checking git checkout... yes
> ./configure: line 24067: syntax error near unexpected token `newline'
> ./configure: line 24067: `AX_CHECK_COMPILE_FLAG('
> make: *** [config.status] Error 2

They both already fail in the step before it seems:

> configure.ac:1295: error: Autoconf version 2.64 or higher is required

ContOS 6 ships with autoconf 2.63, I don't know what opensolaris-10 has.

We could revert to an older version of the macro we ship, or to a
simpler (though less clear) AC_CHECK_COMPILE-based check.

I'm tempted to ditch the macro and go for the AC_CHECK_COMPILE.

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Hi,

On Tue, Feb 20, 2018 at 08:40:31PM +0100, Steffan Karger wrote:
> We could revert to an older version of the macro we ship, or to a
> simpler (though less clear) AC_CHECK_COMPILE-based check.
> 
> I'm tempted to ditch the macro and go for the AC_CHECK_COMPILE.

The macro seems to be more painful than expected - so I tend to agree
with you :-) - patches welcome.

Though with the state of the sf.net list, patch handling is quite 
annoying :-/  (read: some mails get through, some mails take ages,
patchwork isn't seeing stuff, the list archive isn't seeing stuff that
I could reference on merging, etc. - gah)

gert