| Message ID | 1573760407-20662-1-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Fix ACL_CHECK_ADD_COMPILE_FLAGS to work with clang | expand |
Thank you for your efforts. As you are touching this, can you try "dist: bionic" ? It might bring newer compilers On Thu, Nov 14, 2019, 8:41 PM <selva.nair@gmail.com> wrote: > From: Selva Nair <selva.nair@gmail.com> > > Some compilers (e.g., clang) only issue a warning for > unsupported options unless additional flags such > as -Werror are used to convert the warning to an error. > > Add support for extra flags in ACL_CHECK_ADD_COMPILE_FLAGS. > > Note: a similar approach is used in AX_CHECK_COMPILE_FLAG > in autoconf archives. > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > --- > > For successful travis build result see > https://travis-ci.org/selvanair/openvpn/builds/612019849 > > But this + https://patchwork.openvpn.net/patch/908/ > alone does not fix the travis build as clang is not > happy with struct initializers: Like this > > crypto.c:1860:31: error: suggest braces around initialization of subobject > [-Werror,-Wmissing-braces] > struct key server_key = { 0 }; > > I think clang wants {{0}} there. > > Darn compilers and darn -Werror :) > > configure.ac | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 807804e5..e59bd91b 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -1275,18 +1275,21 @@ if test "${enable_pkcs11}" = "yes"; then > ) > fi > > +# Usage: ACL_CHECK_ADD_COMPILE_FLAGS(flag, [extra-flags]) > +# Some compilers require extra flags to trigger an error on > +# unsupported options. E.g., clang requires -Werror. > AC_DEFUN([ACL_CHECK_ADD_COMPILE_FLAGS], [ > old_cflags="$CFLAGS" > - CFLAGS="$1 $CFLAGS" > - AC_MSG_CHECKING([whether the compiler acceppts $1]) > - AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])], > + CFLAGS="$2 $1 $CFLAGS" > + AC_MSG_CHECKING([whether the compiler accepts $1]) > + AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])]; > CFLAGS="$1 $old_cflags", > [AC_MSG_RESULT([no]); CFLAGS="$old_cflags"])] > ) > > -ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-stringop-truncation]) > -ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-function]) > -ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-parameter]) > -ACL_CHECK_ADD_COMPILE_FLAGS([-Wall]) > +ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-stringop-truncation], [-Werror]) > +ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-function], [-Werror]) > +ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-parameter], [-Werror]) > +ACL_CHECK_ADD_COMPILE_FLAGS([-Wall], [-Werror]) > > if test "${enable_pedantic}" = "yes"; then > enable_strict="yes" > -- > 2.20.1 > > > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel > <div dir="auto">Thank you for your efforts.<div dir="auto">As you are touching this, can you try "dist: bionic" ? It might bring newer compilers</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 14, 2019, 8:41 PM <<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank" rel="noreferrer">selva.nair@gmail.com</a>><br> <br> Some compilers (e.g., clang) only issue a warning for<br> unsupported options unless additional flags such<br> as -Werror are used to convert the warning to an error.<br> <br> Add support for extra flags in ACL_CHECK_ADD_COMPILE_FLAGS.<br> <br> Note: a similar approach is used in AX_CHECK_COMPILE_FLAG<br> in autoconf archives.<br> <br> Signed-off-by: Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank" rel="noreferrer">selva.nair@gmail.com</a>><br> ---<br> <br> For successful travis build result see<br> <a href="https://travis-ci.org/selvanair/openvpn/builds/612019849" rel="noreferrer noreferrer" target="_blank">https://travis-ci.org/selvanair/openvpn/builds/612019849</a><br> <br> But this + <a href="https://patchwork.openvpn.net/patch/908/" rel="noreferrer noreferrer" target="_blank">https://patchwork.openvpn.net/patch/908/</a><br> alone does not fix the travis build as clang is not<br> happy with struct initializers: Like this<br> <br> crypto.c:1860:31: error: suggest braces around initialization of subobject<br> [-Werror,-Wmissing-braces]<br> struct key server_key = { 0 };<br> <br> I think clang wants {{0}} there.<br> <br> Darn compilers and darn -Werror :)<br> <br> <a href="http://configure.ac" rel="noreferrer noreferrer" target="_blank">configure.ac</a> | 17 ++++++++++-------<br> 1 file changed, 10 insertions(+), 7 deletions(-)<br> <br> diff --git a/<a href="http://configure.ac" rel="noreferrer noreferrer" target="_blank">configure.ac</a> b/<a href="http://configure.ac" rel="noreferrer noreferrer" target="_blank">configure.ac</a><br> index 807804e5..e59bd91b 100644<br> --- a/<a href="http://configure.ac" rel="noreferrer noreferrer" target="_blank">configure.ac</a><br> +++ b/<a href="http://configure.ac" rel="noreferrer noreferrer" target="_blank">configure.ac</a><br> @@ -1275,18 +1275,21 @@ if test "${enable_pkcs11}" = "yes"; then<br> )<br> fi<br> <br> +# Usage: ACL_CHECK_ADD_COMPILE_FLAGS(flag, [extra-flags])<br> +# Some compilers require extra flags to trigger an error on<br> +# unsupported options. E.g., clang requires -Werror.<br> AC_DEFUN([ACL_CHECK_ADD_COMPILE_FLAGS], [<br> old_cflags="$CFLAGS"<br> - CFLAGS="$1 $CFLAGS"<br> - AC_MSG_CHECKING([whether the compiler acceppts $1])<br> - AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])],<br> + CFLAGS="$2 $1 $CFLAGS"<br> + AC_MSG_CHECKING([whether the compiler accepts $1])<br> + AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])]; CFLAGS="$1 $old_cflags",<br> [AC_MSG_RESULT([no]); CFLAGS="$old_cflags"])]<br> )<br> <br> -ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-stringop-truncation])<br> -ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-function])<br> -ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-parameter])<br> -ACL_CHECK_ADD_COMPILE_FLAGS([-Wall])<br> +ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-stringop-truncation], [-Werror])<br> +ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-function], [-Werror])<br> +ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-parameter], [-Werror])<br> +ACL_CHECK_ADD_COMPILE_FLAGS([-Wall], [-Werror])<br> <br> if test "${enable_pedantic}" = "yes"; then<br> enable_strict="yes"<br> -- <br> 2.20.1<br> <br> <br> <br> _______________________________________________<br> Openvpn-devel mailing list<br> <a href="mailto:Openvpn-devel@lists.sourceforge.net" target="_blank" rel="noreferrer">Openvpn-devel@lists.sourceforge.net</a><br> <a href="https://lists.sourceforge.net/lists/listinfo/openvpn-devel" rel="noreferrer noreferrer" target="_blank">https://lists.sourceforge.net/lists/listinfo/openvpn-devel</a><br> </blockquote></div>
On 14/11/2019 20:40, selva.nair@gmail.com wrote: > From: Selva Nair <selva.nair@gmail.com> > > Some compilers (e.g., clang) only issue a warning for > unsupported options unless additional flags such > as -Werror are used to convert the warning to an error. > > Add support for extra flags in ACL_CHECK_ADD_COMPILE_FLAGS. > > Note: a similar approach is used in AX_CHECK_COMPILE_FLAG > in autoconf archives. I'm glad I didn't spend more time on thinking about this earlier today, as you have almost the same ideas as I had :) [...snip...] > > +# Usage: ACL_CHECK_ADD_COMPILE_FLAGS(flag, [extra-flags]) > +# Some compilers require extra flags to trigger an error on > +# unsupported options. E.g., clang requires -Werror. > AC_DEFUN([ACL_CHECK_ADD_COMPILE_FLAGS], [ > old_cflags="$CFLAGS" > - CFLAGS="$1 $CFLAGS" > - AC_MSG_CHECKING([whether the compiler acceppts $1]) > - AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])], > + CFLAGS="$2 $1 $CFLAGS" > + AC_MSG_CHECKING([whether the compiler accepts $1]) > + AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])]; CFLAGS="$1 $old_cflags", My idea was just to add -Werror right in the line above, and not extend the ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument. But I stopped when I realized that GCC has some quirks as well, as I needed to ponder about this a bit. Clang is fairly consistent: * first with an unknown -Wno-* argument $ clang-6.0 -o test test.c -Wall -Wno-non-existing-flag warning: unknown warning option '-Wno-non-existing-flag'; did you mean '-Wno-non-virtual-dtor'? [-Wunknown-warning-option] $ clang-6.0 -o test test.c -Wall -Wno-non-existing-flag -Werror error: unknown warning option '-Wno-non-existing-flag'; did you mean '-Wno-non-virtual-dtor'? [-Werror,-Wunknown-warning-option] * then with an unknown -W* argument (without the no- prefix) $ clang-6.0 -o test test.c -Wall -Wnon-existing-flag warning: unknown warning option '-Wnon-existing-flag'; did you mean '-Wnon-virtual-dtor'? [-Wunknown-warning-option] $ clang-6.0 -o test test.c -Wall -Wnon-existing-flag -Werror error: unknown warning option '-Wnon-existing-flag'; did you mean '-Wnon-virtual-dtor'? [-Werror,-Wunknown-warning-option] This is consistent and I can live with this behavior, using -Werror to bail out on warnings. So lets running the same tests with GCC: * first with an unknown -Wno-* argument $ gcc -o test test.c -Wall -Wno-non-existing-flag (no error nor warning) $ gcc -o test test.c -Wall -Wno-non-existing-flag -Werror (no error nor warning) * then with an unknown -W* argument (without the no- prefix) $ gcc -o test test.c -Wall -Wnon-existing-flag gcc: error: unrecognized command line option ‘-Wnon-existing-flag’ I think you said it pretty well in your mail: > Darn compilers and darn -Werror So your change does improve Clang ... I dunno what we will do with GCC.
Hi David Thanks for the comments My idea was just to add -Werror right in the line above, and not extend the > ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument. > I'm fine with that approach as well. Let me know if you want a v2. > I think you said it pretty well in your mail: > > > Darn compilers and darn -Werror > > > So your change does improve Clang ... I dunno what we will do with GCC. > It doesn't affect gcc as gcc does error (or ignore -- see below) unsupported options with or without -Werror. So I think we are okay. travis build on my fork confirms that: https://travis-ci.org/selvanair/openvpn/builds/612019849 However, gcc does have quirky behaviour when it comes to options like -Wno-xx-yy. It just accepts them silently[*] with or without -Werror. That works fine for us, but it does generate an error about that unsupported option if any other error is encountered during the build. As other errors are anyway a show-stopper, I think we can live with that. Selva [*] I think their reasoning is that -Wno-xx-yy can be thus used to suppress warnings added to newer versions without breaking builds with older ones. But clang doesn't share that view. <div dir="ltr"><div dir="ltr">Hi David<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">Thanks for the comments</div><div class="gmail_quote"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote">My idea was just to add -Werror right in the line above, and not extend the<br> ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument.</div></blockquote><div><br></div><div>I'm fine with that approach as well. Let me know if you want a v2.<br></div><div> <br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> I think you said it pretty well in your mail:<br> <br> > Darn compilers and darn -Werror<br> <br> <br> So your change does improve Clang ... I dunno what we will do with GCC.<br></blockquote><div><br></div><div>It doesn't affect gcc as gcc does error (or ignore -- see below) unsupported options with or without -Werror. So I think we are okay. travis build on my fork confirms that:</div><div><a href="https://travis-ci.org/selvanair/openvpn/builds/612019849" rel="noreferrer" target="_blank">https://travis-ci.org/selvanair/openvpn/builds/612019849</a></div><div><br></div><div>However, gcc does have quirky behaviour when it comes to options like -Wno-xx-yy. It just accepts them silently[*] with or without -Werror. That works fine for us, but it does generate an error about that unsupported option if any other error is encountered during the build. As other errors are anyway a show-stopper, I think we can live with that.</div><div><br></div><div>Selva</div><div><br></div><div>[*] I think their reasoning is that -Wno-xx-yy can be thus used to suppress warnings added to newer versions without breaking builds with older ones. But clang doesn't share that view.<br></div></div></div>
Hi, On Thu, Nov 14, 2019 at 3:16 PM Илья Шипицин <chipitsine@gmail.com> wrote: > Thank you for your efforts. > As you are touching this, can you try "dist: bionic" ? It might bring > newer compilers > I don't expect newer compilers on bionic break this patch. But fwiw, I've started a travis build with dist: bionic. For results see https://travis-ci.org/selvanair/openvpn/jobs/612099524 Selva <div dir="ltr"><div>Hi,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 14, 2019 at 3:16 PM Илья Шипицин <<a href="mailto:chipitsine@gmail.com" target="_blank">chipitsine@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto">Thank you for your efforts.<div dir="auto">As you are touching this, can you try "dist: bionic" ? It might bring newer compilers</div></div></blockquote><div><br></div><div>I don't expect newer compilers on bionic break this patch. But fwiw, I've started a travis build with dist: bionic. For results see <a href="https://travis-ci.org/selvanair/openvpn/jobs/612099524" target="_blank">https://travis-ci.org/selvanair/openvpn/jobs/612099524<br></a></div><div><br></div><div>Selva<br></div></div></div>
Am 14.11.19 um 22:58 schrieb Selva Nair: > Hi David > > Thanks for the comments > > My idea was just to add -Werror right in the line above, and not > extend the > ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument. > > > I'm fine with that approach as well. Let me know if you want a v2. > > > I think you said it pretty well in your mail: > > > Darn compilers and darn -Werror > > > So your change does improve Clang ... I dunno what we will do with GCC. > > > It doesn't affect gcc as gcc does error (or ignore -- see below) > unsupported options with or without -Werror. So I think we are okay. > travis build on my fork confirms that: > https://travis-ci.org/selvanair/openvpn/builds/612019849 > > However, gcc does have quirky behaviour when it comes to options like > -Wno-xx-yy. It just accepts them silently[*] with or without -Werror. > That works fine for us, but it does generate an error about that > unsupported option if any other error is encountered during the build. > As other errors are anyway a show-stopper, I think we can live with that. > > Selva > > [*] I think their reasoning is that -Wno-xx-yy can be thus used to > suppress warnings added to newer versions without breaking builds with > older ones. But clang doesn't share that view. I agree with Selva here. If we cannot get rid of the warnings themselves this seems to be the only sane way. So I am not totally with this stuff but it is better than the altneratives at the moment. And I really annoyed by all these new warnings of Clang Acked-By: Arne Schwabe <arne@rfc2549.org> Arne
On 14/11/2019 22:58, Selva Nair wrote: > Hi David > > Thanks for the comments > > My idea was just to add -Werror right in the line above, and not extend the > ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument. > > > I'm fine with that approach as well. Let me know if you want a v2. Sorry for the delay responding. I've done some pondering, and I have come to the same conclusion as Arne, so this is a reasonable approach to add -Werror. That said, is there any specific reasons why adding the [-Werror] to essentially all ACL_CHECK_ADD_COMPILE_FLAGS() calls? Our ./configure script is the only one implementing and supporting and using this macro, and it is defined in configure.ac. If there are no foreseeable reason for that now, I think it's just fine to change the AC_COMPILE_IFELSE() call to use CFLAGS="$1 -Werror $old_cflags". Just add a comment in that area why we add the -Werror explicitly. This makes the whole change smaller and clearer. If we need the flexibility later on, we can adjust this when that time arrives.
Hi, On Tue, Nov 19, 2019 at 9:09 AM David Sommerseth < openvpn@sf.lists.topphemmelig.net> wrote: > On 14/11/2019 22:58, Selva Nair wrote: > > Hi David > > > > Thanks for the comments > > > > My idea was just to add -Werror right in the line above, and not > extend the > > ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument. > > > > > > I'm fine with that approach as well. Let me know if you want a v2. > > Sorry for the delay responding. I've done some pondering, and I have come > to > the same conclusion as Arne, so this is a reasonable approach to add > -Werror. > > That said, is there any specific reasons why adding the [-Werror] to > essentially all ACL_CHECK_ADD_COMPILE_FLAGS() calls? Our ./configure > script > is the only one implementing and supporting and using this macro, and it is > defined in configure.ac. > > If there are no foreseeable reason for that now, I think it's just fine to > change the AC_COMPILE_IFELSE() call to use CFLAGS="$1 -Werror $old_cflags". > Just add a comment in that area why we add the -Werror explicitly. This > makes > the whole change smaller and clearer. If we need the flexibility later > on, we > can adjust this when that time arrives. > Agreed. v2 follows. Selva <div dir="ltr"><div>Hi,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 19, 2019 at 9:09 AM David Sommerseth <<a href="mailto:openvpn@sf.lists.topphemmelig.net">openvpn@sf.lists.topphemmelig.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 14/11/2019 22:58, Selva Nair wrote:<br> > Hi David<br> > <br> > Thanks for the comments<br> > <br> > My idea was just to add -Werror right in the line above, and not extend the<br> > ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument.<br> > <br> > <br> > I'm fine with that approach as well. Let me know if you want a v2.<br> <br> Sorry for the delay responding. I've done some pondering, and I have come to<br> the same conclusion as Arne, so this is a reasonable approach to add -Werror.<br> <br> That said, is there any specific reasons why adding the [-Werror] to<br> essentially all ACL_CHECK_ADD_COMPILE_FLAGS() calls? Our ./configure script<br> is the only one implementing and supporting and using this macro, and it is<br> defined in <a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a>.<br> <br> If there are no foreseeable reason for that now, I think it's just fine to<br> change the AC_COMPILE_IFELSE() call to use CFLAGS="$1 -Werror $old_cflags".<br> Just add a comment in that area why we add the -Werror explicitly. This makes<br> the whole change smaller and clearer. If we need the flexibility later on, we<br> can adjust this when that time arrives.<br></blockquote><div><br></div><div>Agreed. v2 follows.</div><div><br></div><div>Selva</div></div></div>
diff --git a/configure.ac b/configure.ac index 807804e5..e59bd91b 100644 --- a/configure.ac +++ b/configure.ac @@ -1275,18 +1275,21 @@ if test "${enable_pkcs11}" = "yes"; then ) fi +# Usage: ACL_CHECK_ADD_COMPILE_FLAGS(flag, [extra-flags]) +# Some compilers require extra flags to trigger an error on +# unsupported options. E.g., clang requires -Werror. AC_DEFUN([ACL_CHECK_ADD_COMPILE_FLAGS], [ old_cflags="$CFLAGS" - CFLAGS="$1 $CFLAGS" - AC_MSG_CHECKING([whether the compiler acceppts $1]) - AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])], + CFLAGS="$2 $1 $CFLAGS" + AC_MSG_CHECKING([whether the compiler accepts $1]) + AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])]; CFLAGS="$1 $old_cflags", [AC_MSG_RESULT([no]); CFLAGS="$old_cflags"])] ) -ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-stringop-truncation]) -ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-function]) -ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-parameter]) -ACL_CHECK_ADD_COMPILE_FLAGS([-Wall]) +ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-stringop-truncation], [-Werror]) +ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-function], [-Werror]) +ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-parameter], [-Werror]) +ACL_CHECK_ADD_COMPILE_FLAGS([-Wall], [-Werror]) if test "${enable_pedantic}" = "yes"; then enable_strict="yes"