|
Description
Thomas Zimmermann
2016-08-15 19:14:03 UTC
> AX_COMPILER_FLAGS
I would recommend not jumping into this one. I started on it, but there's a huge number of compiler warnings we need to fix first :-(
(In reply to Simon McVittie from comment #1) > > AX_COMPILER_FLAGS > > I would recommend not jumping into this one. I started on it, but there's a > huge number of compiler warnings we need to fix first :-( I have patches that work quite well and don't generate additional warnings. They just need a bit more clean-up. Specifically, I used AX_COMPILER_FLAGS_{CFLAGS,CXXFLAGS,LDFLAGS}. The 'super-macro' AX_COMPILER_FLAGS didn't seem helpful. Created attachment 125822 [details] [review] [01] Replace configure option '--enable-developer' by '--enable-debug' I also submitted a patch to the Autoconf archive to make AX_IS_RELEASE work correctly with out-of-tree builds. [1] But I don't think this is a problem for dbus, as debugging is always enabled in autogen.sh. [1] https://github.com/peti/autoconf-archive/pull/88 Created attachment 125823 [details] [review] [02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros Created attachment 125824 [details] [review] [03] Test additonal compiler flags with AX_COMPILER_FLAGS_CFLAGS Created attachment 125825 [details] [review] [10] Remove configure option '--disable-compiler-optimisations' As discussed in bug 88922 comment 5. (In reply to Thomas Zimmermann from comment #3) > [01] Replace configure option '--enable-developer' by '--enable-debug' I appreciate the thought, but I'm not sure we want to do this. We consider assertions, verbose mode and (in particular) "embedded tests" to be things that should really not be in used in production dbus binaries. If we called those "debug", then distributions would likely enable them, get insecure binaries, and blame us. I *really* don't want to have to do the embargo/CVE ID/pre-scheduled releases routine every time there is an attacker-triggerable assertion failure, and the "embedded tests" embed a lot of unaudited code into the normal binaries, with extra environment variables looked at and so on. Comment on attachment 125822 [details] [review] [01] Replace configure option '--enable-developer' by '--enable-debug' Review of attachment 125822 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +7,5 @@ > m4_define([dbus_version], > [dbus_major_version.dbus_minor_version.dbus_micro_version]) > AC_INIT([dbus],[dbus_version],[https://bugs.freedesktop.org/enter_bug.cgi?product=dbus],[dbus]) > + > +AX_IS_RELEASE([git-directory]) Our AX_IS_RELEASE policy is exactly micro-version, which was added because I asked a colleague for it. I don't think git-directory is a good policy in general, tbh: if a downstream builds our git tags, they should get the same build as a different downstream that used tarballs. (But we don't currently use this for anything, and other maintainers have expressed opposition to doing so.) Comment on attachment 125823 [details] [review] [02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros Review of attachment 125823 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +303,4 @@ > AC_DEFINE(DBUS_ENABLE_VERBOSE_MODE,1,[Support a verbose mode]) > fi > > +DISABLE_UNUSED_WARNINGS="$DISABLE_UNUSED_WARNINGS The name is misleading now. Please rename to DISABLE_WARNINGS. If I'm reading correctly, this is appended to here, but unconditionally overwritten further down (at the TODO)? That seems wrong. The comment that says *why* we ignore these three specific warnings should be kept (with the bits that are no longer true deleted). @@ +334,4 @@ > if test x$enable_checks = xno; then > AC_DEFINE(DBUS_DISABLE_CHECKS,1,[Disable public API sanity checking]) > AC_DEFINE(G_DISABLE_CHECKS,1,[Disable GLib public API sanity checking]) > + DISABLE_UNUSED_WARNINGS="-Wno-unused-label" This overwrites where it should append. @@ +1330,2 @@ > test "x$rc" = xyes > } I hope cc_supports_flag can go away (it might even be dead code already). @@ +1344,5 @@ > + -Wno-undef" > + > +AX_COMPILER_FLAGS > + > +AX_COMPILER_FLAGS_CFLAGS([EXTRA_CFLAGS],,, Please use "[], []," for empty arguments @@ +1362,5 @@ > +AX_COMPILER_FLAGS_LDFLAGS([EXTRA_LDFLAGS]) > + > +CFLAGS="$CFLAGS $EXTRA_CFLAGS" > +CXXFLAGS="$CXXFLAGS $EXTRA_CXXFLAGS" > +LDFLAGS="$LDFLAGS $EXTRA_LDFLAGS" We should never append to these variables, only prepend, so that the user can override whatever we do. Ideally we'd set $(WARN_CFLAGS), $(WARN_LDFLAGS) etc. instead, and use them wherever we compile C/C++. Comment on attachment 125824 [details] [review] [03] Test additonal compiler flags with AX_COMPILER_FLAGS_CFLAGS Review of attachment 125824 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +1322,5 @@ > +# strict, because on some systems the linker is *always* this strict > +TEST_CFLAGS="$TEST_CFLAGS -fno-common" > + > +# http://bugs.freedesktop.org/show_bug.cgi?id=10599 > +TEST_CFLAGS="$TEST_CFLAGS -fno-strict-aliasing" AX_COMPILER_FLAGS does this by default, in fact. Comment on attachment 125825 [details] [review] [10] Remove configure option '--disable-compiler-optimisations' Review of attachment 125825 [details] [review]: ----------------------------------------------------------------- This seems a perfectly reasonable approach. > > [01] Replace configure option '--enable-developer' by '--enable-debug' > > I appreciate the thought, but I'm not sure we want to do this. We consider > assertions, verbose mode and (in particular) "embedded tests" to be things > that should really not be in used in production dbus binaries. If we called > those "debug", then distributions would likely enable them, get insecure > binaries, and blame us. The nice thing about AX_CHECK_ENABLE_DEBUG is that is cleans up compiler flags and sets '-g -O0' if required. > > +AX_IS_RELEASE([git-directory]) > > Our AX_IS_RELEASE policy is exactly micro-version, which was added because I > asked a colleague for it. I don't think git-directory is a good policy in > general, tbh: if a downstream builds our git tags, they should get the same > build as a different downstream that used tarballs. AX_IS_RELEASE controls build flags in other macros. With minor-release policy, a distribution shipping dbus 1.11.5 would build with debug flags, assertions, etc. That's probably not what they want. What do you think about the following idea: (1) continue to support --enable-developer (2) add AX_IS_RELEASE with its policy depending on --enable-developer (3) add AX_CHECK_ENABLE_DEBUG which is controlled by AX_IS_RELEASE Developer settings would still be controlled by -enable-developer. The value is fed into AX_IS_RELEASE as 'always' or 'never', to set debug or release builds. Several AX_* macros (including AX_CHECK_ENABLE_DEBUG) would then automatically respect this setting. Developers who have to debug a release build can override the debugging flags with --enable-debug. Assertions, verbose mode, etc would still be disabled. (In reply to Thomas Zimmermann from comment #12) > (2) add AX_IS_RELEASE with its policy depending on --enable-developer No, that would be misleading. --enable-developer does not determine whether a version is a release or not. 1.11.4 is a release, whether you build it in "production" or "developer" mode - in Debian I build both, with production mode installed by default, and the developer build available in the dbus-1-dbg package in directories that can be added to $PATH and $LD_LIBRARY_PATH if required. Conversely, the current state of git master (which identifies itself as 1.11.5) does not become a release just because someone built it in production mode; it only becomes a release when a maintainer bumps the version to 1.11.6 and tags it. > Several AX_* macros (including AX_CHECK_ENABLE_DEBUG) would then > automatically respect this setting. I know that the AX_ macros all defaulting to following AX_IS_RELEASE is convenient, but in this case we do not want the "is it a release?" status to determine whether these things are enabled/disabled. I would personally be in favour of having -Wno-error vs. -Werror determined by the micro version, but other D-Bus maintainers objected to that idea. The options that potentially introduce security flaws or denial-of-service opportunities (embedded tests, assertions, and to a lesser extent verbose mode) should certainly always be explicit. Hi! (In reply to Simon McVittie from comment #13) > (In reply to Thomas Zimmermann from comment #12) > > (2) add AX_IS_RELEASE with its policy depending on --enable-developer > > No, that would be misleading. --enable-developer does not determine whether > a version is a release or not. 1.11.4 is a release, whether you build it in > "production" or "developer" mode - in Debian I build both, with production > mode installed by default, and the developer build available in the > dbus-1-dbg package in directories that can be added to $PATH and > $LD_LIBRARY_PATH if required. > > Conversely, the current state of git master (which identifies itself as > 1.11.5) does not become a release just because someone built it in > production mode; it only becomes a release when a maintainer bumps the > version to 1.11.6 and tags it. OK, understood. But maybe let's first clarify the term 'release'. AX_IS_RELEASE is described as: Determine whether the code is being configured as a release [1] So to me AX_IS_RELEASE means 'this is a release build'; while to you it apparently means 'this is a release version'. (Right?) It seems to me we actually agree, but we're talking about different meanings of 'release'. > > Several AX_* macros (including AX_CHECK_ENABLE_DEBUG) would then > > automatically respect this setting. > > I know that the AX_ macros all defaulting to following AX_IS_RELEASE is > convenient, but in this case we do not want the "is it a release?" status to > determine whether these things are enabled/disabled. If you read AX_IS_RELEASE as 'is a release build', it makes a lot more sense to have other macros depend on it. > I would personally be in favour of having -Wno-error vs. -Werror determined > by the micro version, but other D-Bus maintainers objected to that idea. > > The options that potentially introduce security flaws or denial-of-service > opportunities (embedded tests, assertions, and to a lesser extent verbose > mode) should certainly always be explicit. I agree. With my proposal from comment 12, these options would be switched off in a 'release build'. Whether that build is created from a 'release version' wouldn't matter. [1] https://www.gnu.org/software/autoconf-archive/ax_is_release.html (In reply to Thomas Zimmermann from comment #14) > So to me AX_IS_RELEASE means 'this is a release build'; while to you it > apparently means 'this is a release version'. (Right?) It seems to me we > actually agree, but we're talking about different meanings of 'release'. Yes, I think we have those differing interpretations. I had always interpreted AX_IS_RELEASE as "this is a release version", for two reasons: 1. The choice between a "Release" or "Debug" build is a fact about a particular *build*, not a particular *source tree*. The policies that AX_IS_RELEASE implements (minor-version, micro-version, git-directory) are all based on facts about the source tree, which makes me think it was intended to mean "is release version". 2. A binary choice between (often incompatible!) "Release" and "Debug" builds is seen in (for example) CPython, Windows development (where it chooses between different MSVC runtime library flavours), and many CMake build systems. However, it's far from universal. In Autotools, and open source in general, it's quite common to want to make debug builds be a drop-in replacement for release builds, so that you can overwrite a release build with a debug build while investigating a bug. It's also somewhat common to have several orthogonal developer features - for example dbus' assertions, verbose mode, Stats interface and embedded tests - which can be turned on and off separately. In dbus, the Stats interface and the installed-tests are primarily debug features too, but are enabled in at least Debian's production builds - Stats adds code size to the dbus-daemon but is considered sufficiently safe and "low-cost" that it is on by default, while the installed-tests have no impact at all on the performance, functionality or contents of the important components (dbus-daemon, libdbus etc.), so their only cost is that there's more code to compile. Created attachment 125887 [details] [review] [01] Add configure option '--enable-debug' to control debugging and profiling (v2) Changes since v1: - reintroduced --enable-developer - make --enable-debug an additional option, not _the_ option - set ax_is_release by hand That's maybe still not what you want, but I'm trying to make progress with this patch. I more or less restored the old configuration. I added --enable-debug as extra option for debugging and profiling flags. It's default value depends on --enable-developer, like with other config options (assert, verbose). I added a lengthy comment to ax_is_release and all the debugging options. Hopefully that should avoid any confusion about the meaning of the options. Created attachment 125888 [details] [review] [02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v2) Changes since v1: - renamed DISABLE_UNUSED_WARNINGS to DISABLE_WARNINGS - fixed cases where DISABLE_WARNINGS was accidentally cleared - added [] for empty macro options - prepend flags to CFLAGS et al, don't append Created attachment 125889 [details] [review] [03] Test additonal compiler flags with AX_COMPILER_FLAGS_CFLAGS (v2) Changes since v1 - removed explicit '-fno-script-aliasing' (In reply to Thomas Zimmermann from comment #17) > Created attachment 125888 [details] [review] [review] > [02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v2) > > Changes since v1: > > - renamed DISABLE_UNUSED_WARNINGS to DISABLE_WARNINGS > - fixed cases where DISABLE_WARNINGS was accidentally cleared > - added [] for empty macro options > - prepend flags to CFLAGS et al, don't append - added the 'why' comment back Created attachment 125907 [details] [review] [05] Minor fixes for Windows builds With the new compiler flags, Windows compilers (mingw32) show a few more (trivial) warnings. This patch fixes these issues. Comment on attachment 125907 [details] [review] [05] Minor fixes for Windows builds Review of attachment 125907 [details] [review]: ----------------------------------------------------------------- I'd prefer each category of changes (fixing old-style declarations; #ifdef'ing out code not referenced on Windows; #ifdef'ing out code not referenced in verbose mode; not testing DWORD < 0 which can never be true) to be its own commit. ::: bus/config-parser.c @@ +3527,3 @@ > static const char *test_system_service_dir_matches[] = > { > #ifdef DBUS_UNIX There's no point in having #ifdef DBUS_UNIX inside #ifndef DBUS_WIN, since those are the only two possibilities we support. Please remove the redundant DBUS_UNIX check. ::: dbus/dbus-sysdeps-win.c @@ +99,4 @@ > #endif > } > > +static BOOL is_winxp_sp3_or_lower(void); Minor: our coding style puts a space before the opening parenthesis (I know the existing code isn't 100% consistent, particularly in Windows-specifics, but I'd like new code to be consistently right) ::: dbus/dbus-test-main.c @@ +27,4 @@ > #include "dbus-test.h" > #include <stdio.h> > #include <stdlib.h> > +#include <string.h> What's this for? Comment on attachment 125887 [details] [review] [01] Add configure option '--enable-debug' to control debugging and profiling (v2) Review of attachment 125887 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +76,5 @@ > + [set defaults to be appropriate for a D-Bus developer instead of a distribution/end-user])], > + [enable_developer=$enableval], > + [enable_developer=no]) > + > +dnl ax_is_release means 'release build' not 'release version.' So its Please don't use ax_is_release for this. In projects that use AX_IS_RELEASE, it means "release version". Using the same name that other projects use, but with a different meaning, is just confusing. Use enable_developer explicitly instead. Comment on attachment 125888 [details] [review] [02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v2) Review of attachment 125888 [details] [review]: ----------------------------------------------------------------- > to make DBus build without errors The protocol is D-Bus; the reference implementation (what you're patching) is dbus. There is no DBus. Comment on attachment 125888 [details] [review] [02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v2) Review of attachment 125888 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +329,5 @@ > +dnl - missing field initializers being 0 is a C feature, not a bug > +dnl - unused-parameter is to make writing callbacks less annoying > +dnl > +dnl To be fixed one day: > +dnl - pointer-sign is a workaround for fd.o #15522 We have in fact fixed #15522, so this one can be removed. @@ +1360,2 @@ > test "x$rc" = xyes > } If cc_supports_flag is not used any more (actually I think it's already unused), please remove. I think it's left over from before we used TP_COMPILER_WARNINGS. @@ +1361,5 @@ > } > > +dnl TODO: The compiler flags below disable warnings that the > +dnl compiler emits while building DBus. Fix the source > +dnl code and remove these flags. If you move -Wno-type-limits from the "Intentional" block to here, that would make more sense. @@ -1345,5 @@ > } > > -TP_COMPILER_WARNINGS([WARNING_CFLAGS], > - dnl Use -Werror by default if: > - dnl - we're not on Windows (too many warnings), and I'm glad you've managed to get rid of the special case for Windows here. @@ +1375,5 @@ > + -Wno-undef" > + > +AX_COMPILER_FLAGS > + > +AX_COMPILER_FLAGS_CFLAGS([EXTRA_CFLAGS],[],[], Use $enable_developer rather than the implicit $ax_is_release (you might have to add a new $not_developer or $is_production_build which is set to be the opposite of $enable_developer) @@ +1378,5 @@ > + > +AX_COMPILER_FLAGS_CFLAGS([EXTRA_CFLAGS],[],[], > + [-Wchar-subscripts \ > + -Wfloat-equal \ > + -Wno-address \ -Wno-address should move to the "TODO" block, too. (Pre-existing bug in how we used TP_COMPILER_WARNINGS) @@ +1385,5 @@ > + > +AX_COMPILER_FLAGS_CXXFLAGS([EXTRA_CXXFLAGS],[],[], > + [-Wchar-subscripts \ > + -Wfloat-equal \ > + -Wno-address \ Same here for -Wno-address. @@ +1393,5 @@ > +AX_COMPILER_FLAGS_LDFLAGS([EXTRA_LDFLAGS]) > + > +CFLAGS="$EXTRA_CFLAGS $CFLAGS" > +CXXFLAGS="$EXTRA_CXXFLAGS $CXXFLAGS" > +LDFLAGS="$EXTRA_LDFLAGS $LDFLAGS" You don't seem to have removed this, which was used by TP_COMPILER_WARNINGS: dnl In principle we should put WARNING_CFLAGS in each Makefile.am like dnl telepathy-glib does, since CFLAGS is meant to be reserved for the user... dnl but prepending to CFLAGS (so the user can override it with later CFLAGS) dnl is the next best thing CFLAGS="$WARNING_CFLAGS $CFLAGS" Please move and adapt the comments so they apply to this use of EXTRA_CFLAGS etc., and (assuming there is no other use of WARNING_CFLAGS) remove the line that prepends WARNING_CFLAGS to CFLAGS. (I would also be in favour of removing these, and instead explicitly putting $(EXTRA_FOO) in AM_FOO/target_FOO in every Makefile.am that compiles C or C++ - but that's probably better as a follow-up change.) @@ +1398,3 @@ > > if test "x$GCC" = "xyes"; then > # We're treating -fno-common like a warning: it makes the linker more -fno-common, -fno-strict-aliasing and the ANSI stuff should probably move from WARNING_CFLAGS to EXTRA_CFLAGS too, so that we only have one set to deal with. That would mean the lines that prepend EXTRA_FOO to FOO would have to move down below here. Comment on attachment 125889 [details] [review] [03] Test additonal compiler flags with AX_COMPILER_FLAGS_CFLAGS (v2) Review of attachment 125889 [details] [review]: ----------------------------------------------------------------- Looks good. I think this could be squashed into the change that adds AX_COMPILER_FLAGS_CFLAGS in the first place. (In reply to Simon McVittie from comment #21) > ::: dbus/dbus-test-main.c > @@ +27,4 @@ > > #include "dbus-test.h" > > #include <stdio.h> > > #include <stdlib.h> > > +#include <string.h> > > What's this for? I got a warning about strcmp being undefined below in that file. (In reply to Simon McVittie from comment #23) > > to make DBus build without errors > > The protocol is D-Bus; the reference implementation (what you're patching) > is dbus. There is no DBus. LOL OK :D (In reply to Thomas Zimmermann from comment #26) > (In reply to Simon McVittie from comment #21) > > > +#include <string.h> > > > > What's this for? > > I got a warning about strcmp being undefined below in that file. Then please do a commit whose commit message says so :-) (In reply to Thomas Zimmermann from comment #14) > So to me AX_IS_RELEASE means 'this is a release build'; while to you it > apparently means 'this is a release version'. (Right?) It seems to me we > actually agree, but we're talking about different meanings of 'release'. Can we perhaps refer to the duality you're interested in as a production vs. development or production vs. debug build, and refer to the difference between 1.11.4 and 1.11.5 as a release vs. snapshot version? I think that might make it clearer. Confusingly, .travis.yml and tools/ci-build.sh talk about a "release" build variant, but I'd be happy to swap those to "production" to be consistent. (In reply to Simon McVittie from comment #28) > (In reply to Thomas Zimmermann from comment #14) > > So to me AX_IS_RELEASE means 'this is a release build'; while to you it > > apparently means 'this is a release version'. (Right?) It seems to me we > > actually agree, but we're talking about different meanings of 'release'. > > Can we perhaps refer to the duality you're interested in as a production vs. > development or production vs. debug build, and refer to the difference > between 1.11.4 and 1.11.5 as a release vs. snapshot version? I think that > might make it clearer. Sure. With the next iteration of the patch set, the comments have been cleaned up, so this is less of an issue. Created attachment 125923 [details] [review] [01] Add configure option '--enable-debug' to control debugging and profiling (v3) Changes since v2: - renamed 'ax_is_release' to 'disable_developer'; clarifies dependency on 'enable_debug' Created attachment 125924 [details] [review] [02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v3) Changes since v2: - merged with patch [03] - removed ref to bug 15522 from comments - moved -Wno-type-limits, -Wno-address below 'TODO' comment - pass 'disable_developer' to AX_ macros instead of relying on implicit 'ax_is_release' Created attachment 125925 [details] [review] [05] Remove unused functions from Windows builds (v2) Changes since v1: - moved unrelated fixes into separate patches - removed unused DBUS_UNIX blocks within newly added DBUS_WIN blocks Created attachment 125926 [details] [review] [06] Fix function declarations Created attachment 125927 [details] [review] [07] Cast -1 to DWORD for comparing to variable Created attachment 125928 [details] [review] [08] Protect debug-only variables behind DBUS_ENABLE_VERBOSE_MODE Created attachment 125929 [details] [review] [09] Include string.h for strcmp() Created attachment 125930 [details] [review] [05] Remove unused functions from Windows builds (v3) Changes since v2: - removed a now unused variable Simon, please take a look at the updated patch set. (In reply to Thomas Zimmermann from comment #38) > Simon, please take a look at the updated patch set. Sorry, I have various high priorities right now, so build-system cleanups might take a bit of time to get to. Comment on attachment 125924 [details] [review] [02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v3) Review of attachment 125924 [details] [review]: ----------------------------------------------------------------- I'd like to apply this, but it makes the build fail on travis-ci (Ubuntu 14.04) because some of the compiler warnings on that platform aren't actually disabled by the "-Wno-whatever" options that you added; I think on older gcc versions there might be some warnings that *can't* be disabled individually. The one I noticed in the build log was some missing const-correctness (discarding the const qualifier). In newer gcc, you've disabled that one with -Wno-discarded-qualifiers, but the gcc on travis-ci doesn't have that option. I think we might need to do this the way I'd initially intended to: fix more of the warnings *first*, and only apply this patch afterwards. ::: configure.ac @@ +1356,5 @@ > > +dnl TODO: The compiler flags below disable warnings that the > +dnl compiler emits while building DBus. Fix the source > +dnl code and remove these flags. > +DISABLE_WARNINGS="DISABLE_WARNINGS Missing a $ here. Comment on attachment 125923 [details] [review] [01] Add configure option '--enable-debug' to control debugging and profiling (v3) Review of attachment 125923 [details] [review]: ----------------------------------------------------------------- (As a non-D-Bus-developer:) Seems reasonable to me. Comment on attachment 125924 [details] [review] [02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v3) Review of attachment 125924 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +1355,4 @@ > ]) > > +dnl TODO: The compiler flags below disable warnings that the > +dnl compiler emits while building DBus. Fix the source Nitpick: ‘D-Bus’, not ‘DBus’. @@ +1369,5 @@ > + -Wno-switch-enum > + -Wno-type-limits > + -Wno-undef" > + > +AX_COMPILER_FLAGS This will check for lots of options and store them in WARN_CFLAGS, WARN_LDFLAGS, etc. You don’t seem to use $(WARN_*) anywhere, so this call seems redundant? It also isn’t chained to $disable_developer like the other invocations below. (In reply to Philip Withnall from comment #42) > > +AX_COMPILER_FLAGS > > This will check for lots of options and store them in WARN_CFLAGS, > WARN_LDFLAGS, etc. You don’t seem to use $(WARN_*) anywhere, so this call > seems redundant? I think this is being called solely to get AC_ARG_ENABLE([compile-warnings]). How did you intend for AX_COMPILER_FLAGS and AX_COMPILER_FLAGS_fooFLAGS to be used together? There should probably be an internal _AX_COMPILER_FLAGS_OPTION macro which is depended on by all the AX_COMPILER_FLAGS_* family and by AX_COMPILER_FLAGS, and does the common code (AC_ARG_ENABLE etc.) but none of the calls to AX_COMPILER_FLAGS_FOO, so that you can use AX_COMPILER_FLAGS_FOO without first using AX_COMPILER_FLAGS? But we can't rely on that for dbus until it's in an autoconf-archive release (and preferably packaged in Debian too, since that's what I use to make dbus releases). If I do what appears to be the obvious thing AX_COMPILER_FLAGS([WARN_CFLAGS], [WARN_LDFLAGS], [$disable_developer]) AX_COMPILER_FLAGS_CFLAGS([WARN_CFLAGS], [$disable_developer], ...) then aclocal fails: CDPATH="${ZSH_VERSION+.}:" && cd /home/smcv/src/dbus && /bin/bash /home/smcv/src/dbus/build-aux/missing aclocal-1.15 -I m4 /usr/bin/m4:configure.ac:1376: recursion limit of 1024 exceeded, use -L<N> to change it I think this is a bug in AX_COMPILER_FLAGS_CFLAGS (and the others): it can't be called twice with the same variable name, because it calls m4_define() with the first argument unquoted The first time through, m4_define(ax_warn_cflags_variable, [m4_normalize(ifelse([$1],,[WARN_CFLAGS],[$1]))]) sets ax_warn_cflags_variable to WARN_CFLAGS. The second time through, because ax_warn_cflags_variable is already defined, m4_define(ax_warn_cflags_variable, [m4_normalize(ifelse([$1],,[WARN_CFLAGS],[$1]))]) expands to the equivalent of m4_define([WARN_CFLAGS], [WARN_CFLAGS]) which results in infinite recursion every time WARN_CFLAGS is mentioned from then on. Created attachment 126934 [details] [review] [01] Add configure option '--enable-debug' to control debugging and profiling From: Thomas Zimmermann <tdz@users.sourceforge.net> See above for full commit message [smcv: remove trailing whitespace from new lines] Reviewed-by: Simon McVittie <smcv@debian.org> Created attachment 126935 [details] [review] [02] Fix function declarations This patch adds 'void' to function declarations without parameters. Signed-off-by: Thomas Zimmermann <tdz@users.sourceforge.net> [smcv: fix coding style while we're touching these lines anyway] Reviewed-by: Simon McVittie <smcv@debian.org> Created attachment 126936 [details] [review] [03] Cast -1 to DWORD for comparing to variable From: Thomas Zimmermann <tdz@users.sourceforge.net> Signed-off-by: Thomas Zimmermann <tdz@users.sourceforge.net> [smcv: add space after cast, that is our coding style] Reviewed-by: Simon McVittie <smcv@debian.org> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=97357 Created attachment 126937 [details] [review] [04] Protect debug-only variables behind DBUS_ENABLE_VERBOSE_MODE From: Thomas Zimmermann <tdz@users.sourceforge.net> Signed-off-by: Thomas Zimmermann <tdz@users.sourceforge.net> Reviewed-by: Simon McVittie <smcv@debian.org> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=97357 --- No changes, just re-attaching to put it into sequence Created attachment 126938 [details] [review] [05] Include string.h for strcmp() From: Thomas Zimmermann <tdz@users.sourceforge.net> Signed-off-by: Thomas Zimmermann <tdz@users.sourceforge.net> Reviewed-by: Simon McVittie <smcv@debian.org> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=97357 Created attachment 126939 [details] [review] [06] Remove unused functions from Windows builds From: Thomas Zimmermann <tdz@users.sourceforge.net> Several internal functions are not used on Windows. This patch hides them behind DBUS_WIN. Signed-off-by: Thomas Zimmermann <tdz@users.sourceforge.net> Reviewed-by: Simon McVittie <smcv@debian.org> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=97357 Created attachment 126940 [details] [review] [07] Be more const-correct As a general design principle, strings that we aren't going to modify should usually be const. When compiling with -Wwrite-strings, quoted string constants are of type "const char *", causing compiler warnings when they are assigned to char * variables. Unfortunately, we need to add casts in a few places: * _dbus_list_append(), _dbus_test_oom_handling() and similar generic "user-data" APIs take a void *, not a const void *, so we have to cast * For historical reasons the execve() family of functions take a (char * const *), i.e. a constant pointer to an array of mutable strings, so again we have to cast * _dbus_spawn_async_with_babysitter similarly takes a char **, although we can make it a little more const-correct by making it take (char * const *) like execve() does Signed-off-by: Simon McVittie <smcv@debian.org> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=97357 Created attachment 126941 [details] [review] [08] Linux: use readdir(), not deprecated readdir_r() glibc >= 2.24 marks readdir_r() as deprecated. It is meant to be a thread-safe version of readdir(), but modern implementations of readdir() are thread-safe anyway (when called with a distinct DIR * argument), and readdir_r() has some design issues involving PATH_MAX. This code path is in Linux-specific code, so we can safely assume a high-quality implementation of readdir(). Signed-off-by: Simon McVittie <smcv@debian.org> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=97357 Created attachment 126942 [details] [review] [09] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros From: Thomas Zimmermann <tdz@users.sourceforge.net> The autoconf macros AX_COMPILER_FLAGS_{CFLAGS|CXXFLAGS|LDFLAGS} test for compiler and linker support of various flags, and add the flags to the generated output. If the command-line option '--enable-compile-warnings' is specified to 'configure', a number of additional warning options is also added to the output. This is the default. The AX_COMPILER_FLAGS_* macros add stricter warnings then before. The patch disables some of them to make dbus build without errors. A later patch set should fix the warnings and remove the compiler flags. This patch integrates all tests for compiler flags into the call to AX_COMPILER_FLAGS_CFLAGS. All tests for compiler flags are now done in a single place. The old macros have been removed. Signed-off-by: Thomas Zimmermann <tdz@users.sourceforge.net> [smcv: add missing $ to DISABLE_WARNINGS] [smcv: drop -Wno-discarded-qualifiers] [smcv: drop non-C++ option -Wpointer-sign in C++ mode] [smcv: work around an AX_COMPILER_FLAGS_CFLAGS bug] [smcv: this source tree is called dbus, not DBus] Signed-off-by: Simon McVittie <smcv@debian.org> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=97357 --- This is the best I can do without depending on a patched AX_COMPILER_FLAGS_CFLAGS. Comment on attachment 125825 [details] [review] [10] Remove configure option '--disable-compiler-optimisations' Reviewed-by: me, looks fine. Testing this current patch series on travis-ci. (In reply to Simon McVittie from comment #52) > [09] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros Sigh. This also breaks the CMake build, because we have some rather elaborate machinery to parse configure.ac looking for compiler warning flags and apply the same compiler warning flags to the CMake build. I'm very tempted to just revert that, because having the same compiler warnings under CMake and Autotools seems less important than having reasonable compiler warnings under Autotools. Created attachment 126944 [details] [review] [08½] Revert "Keep cmake gcc builds in sync with autotools warnings." When reviewing this commit, I said Looks OK, although this is going to become impossible if we start using the externally-curated list of warnings from <https://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html>, which I've been quite tempted to do. That time has now come. I think it's more valuable to have comprehensive warnings under our primary build system, Autotools, than to have some fairly elaborate CMake scripting to pick up the same compiler warnings in both build systems; the CMake build system is primarily there to give us the ability to compile with MSVC, which has orthogonal compiler warning options anyway. This reverts commit 41427560af2c9923a48e50ddbf72e53aad5b2983. Created attachment 126945 [details] [review] [11] Rename distro-style CI build from "release" to "production" This avoids confusion with the meaning of "release" used by AX_IS_RELEASE. AX_IS_RELEASE is about facts about the source tree, namely the distinction between releases (tags) and random snapshots. The build variants in .travis.yml are about facts about the build being done, namely the distinction between production and debug/developer builds. Production builds are sometimes referred to as "release builds", for example in typical CMake and MSVC build environments, but a different term seems better here. --- I'm not going to block on review for this one (for the continuous integration stuff I usually haven't been, since it doesn't affect the contents of builds that have real users). Hi (In reply to Simon McVittie from comment #40) > Comment on attachment 125924 [details] [review] [review] > [02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v3) > > Review of attachment 125924 [details] [review] [review]: > ----------------------------------------------------------------- > > I'd like to apply this, but it makes the build fail on travis-ci (Ubuntu > 14.04) because some of the compiler warnings on that platform aren't > actually disabled by the "-Wno-whatever" options that you added; Do you have a link to the build logs? > I think we might need to do this the way I'd initially intended to: fix more > of the warnings *first*, and only apply this patch afterwards. I have a patch set for fixing most of the '-Wno-*'-related warnings in the source code and removing the '-Wno-*' from configure.ac. Would it make sense land them in a different bug before cleaning up the build system? In that case, I'd rebase them on master and post them before continuing with this bug report. (In reply to Thomas Zimmermann from comment #58) > > I'd like to apply this, but it makes the build fail on travis-ci (Ubuntu > > 14.04) because some of the compiler warnings on that platform aren't > > actually disabled by the "-Wno-whatever" options that you added; > > Do you have a link to the build logs? https://travis-ci.org/smcv/dbus/branches (look at "warnings") https://github.com/smcv/dbus/tree/warnings (the relevant branch) If you fork https://github.com/d-bus/dbus and enable Travis-CI on your github account, you can have your own travis-ci builds. (Please note that we don't use GitHub for issue tracking or canonical VCS services, only as a mirror and to enable CI; in particular we prefer patches here, not in GitHub pull requests.) My "warnings" branch includes the patches that I've attached to this bug, which is enough to make the build work on both travis-ci (Ubuntu 14.04, possibly with a custom gcc build) and my laptop (Debian unstable, gcc 6.2, glibc 2.24). Reviews welcome for the patches that I added; also, please let me know if I've inadvertently broken anything in modifying your patches. > I have a patch set for fixing most of the '-Wno-*'-related warnings in the > source code and removing the '-Wno-*' from configure.ac. Would it make sense > land them in a different bug before cleaning up the build system? Please rebase those onto master (or probably better to be onto my "warnings" branch, which is equivalent to applying all the patches from this bug to master) and put them up for review, either here or on a new bug. It might be better to keep them all here, and retitle this bug to "Use AX_COMPILER_FLAGS and fix many gcc warnings" or something, since this bug has the infrastructure for enabling those warnings nicely. (In reply to Simon McVittie from comment #59) > (In reply to Thomas Zimmermann from comment #58) > > Do you have a link to the build logs? > > https://travis-ci.org/smcv/dbus/branches (look at "warnings") ... and https://travis-ci.org/smcv/dbus/builds/164109645 for the version before I'd made significant changes (in particular fixing const-correctness). Created attachment 127051 [details] [review] [09] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v6) Changes since v5: - 'DBus' -> 'D-Bus' in a comment Created attachment 127052 [details] [review] [12] Fix warnings from compiler option '-Wdiscarded-qualifiers' My original patch incorporated the changes of [07] as well. I'm uploading this patch file separately for review, but I'd like to see it merged with [07]. I kept the 'static const' qualifiers over what is done in patch [07], as it should be cleaner and maybe reduce the number of symbol relocations at load time. Created attachment 127053 [details] [review] [13] Remove unnecessary compiler option '-Wno-address' Created attachment 127054 [details] [review] [14] Fix warnings from compiler option '-Wformat-nonliteral' Created attachment 127055 [details] [review] [15] Fix warnings from compiler option '-Wredundant-decls' Created attachment 127056 [details] [review] [16] Fix warnings from compiler option '-Wsuggest-attribute=noreturn' Created attachment 127057 [details] [review] [17] Fix warnings from compiler option '-Wshadow' Created attachment 127058 [details] [review] [18] Fix warnings from compiler option '-Wswitch-default' Created attachment 127059 [details] [review] [19] Fix warnings from compiler option '-Wswitch-enum' Created attachment 127060 [details] [review] [20] Remove unnecessary compiler option '-Wno-type-limits' Created attachment 127061 [details] [review] [21] Fix warnings from compiler option '-Wundef' These are the clean-up patches I have; rebased onto your warnings branch. I admit that some of them are fairly pointless, but given that they allow for stricter error checking, they might be worth applying. Comment on attachment 127052 [details] [review] [12] Fix warnings from compiler option '-Wdiscarded-qualifiers' Review of attachment 127052 [details] [review]: ----------------------------------------------------------------- Does this actually fix any warnings beyond [07] at this point, or is it now just allocating strings in memory in a way that might be more efficient? Comment on attachment 127057 [details] [review] [17] Fix warnings from compiler option '-Wshadow' I just noticed that this patch breaks one of the test cases: FAIL: run-test-systemserver.sh 1 test-expected-echo-fail --print-reply --dest=org.freedesktop.DBus.TestSuiteEchoService /org/freedesktop/TestSuite org.freedesktop.TestSuite.Echo string:hi (Did not see "DBus.Error" in output) Investigating... Comment on attachment 127055 [details] [review] [15] Fix warnings from compiler option '-Wredundant-decls' Review of attachment 127055 [details] [review]: ----------------------------------------------------------------- Sorry, I thought I already attached a version of this. I'll do that soon. Comment on attachment 127056 [details] [review] [16] Fix warnings from compiler option '-Wsuggest-attribute=noreturn' Review of attachment 127056 [details] [review]: ----------------------------------------------------------------- Looks good Comment on attachment 127058 [details] [review] [18] Fix warnings from compiler option '-Wswitch-default' Review of attachment 127058 [details] [review]: ----------------------------------------------------------------- Some of these "default: break;" should probably be a _dbus_assert_not_reached(). I'll need to review locally with more context than Splinter provides. ::: dbus/dbus-credentials.c @@ +274,1 @@ > return FALSE; The return can probably be hoisted into the default case too, unless there is a case that uses break instead of return? (It cannot be reached unless assertions are disabled) ::: dbus/dbus-memory.c @@ +340,1 @@ > return "invalid!"; Again, this can probably be hoisted into the switch too (In reply to Simon McVittie from comment #73) > Does this actually fix any warnings beyond [07] at this point, or is it now > just allocating strings in memory in a way that might be more efficient? (What I'm getting at here is: if your commit message is now inaccurate - which I think it is - then please amend the commit message to justify why the change is still good - which I think it might well be, but for a different reason than the one you gave.) Created attachment 127066 [details] [review] [17] Fix warnings from compiler option '-Wshadow' (v2) Changes since v1: - renamed missing instance of variable 'type' to 'type2' in tools/dbus-send.c (In reply to Simon McVittie from comment #73) > Comment on attachment 127052 [details] [review] [review] > [12] Fix warnings from compiler option '-Wdiscarded-qualifiers' > > Review of attachment 127052 [details] [review] [review]: > ----------------------------------------------------------------- > > Does this actually fix any warnings beyond [07] at this point, or is it now > just allocating strings in memory in a way that might be more efficient? The changes to dbus/dbus-sysdeps-unix.c fix a warning about the strings being const when they shouldn't be. The other changes are only more efficiency. (In reply to Simon McVittie from comment #78) > (In reply to Simon McVittie from comment #73) > > Does this actually fix any warnings beyond [07] at this point, or is it now > > just allocating strings in memory in a way that might be more efficient? > > (What I'm getting at here is: if your commit message is now inaccurate - > which I think it is - then please amend the commit message to justify why > the change is still good - which I think it might well be, but for a > different reason than the one you gave.) Instead of changing the commit message, I would rather suggest to merge this patch into [07]. (In reply to Simon McVittie from comment #75) > [15] Fix warnings from compiler option '-Wredundant-decls' > > Sorry, I thought I already attached a version of this. I'll do that soon. The bits with `environ` are a bit more involved than this, so I've kept those parts of my commit, then applied the rest of this one afterwards. Testing the result now. Comment on attachment 127052 [details] [review] [12] Fix warnings from compiler option '-Wdiscarded-qualifiers' Review of attachment 127052 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-spawn-test.c @@ +56,4 @@ > static dbus_bool_t > check_spawn_nonexistent (void *data) > { > + static char arg_does_not_exist[] = "/this/does/not/exist/32542sdgafgafdg"; I'm not delighted about putting this string constant in non-const storage just to work around it being inconvenient for _dbus_spawn_async_with_babysitter() to be const-correct. If we're going far enough with micro-optimizations to be distinguishing between const char * and static const char[], then I'd prefer to keep the cast. I'll make that change when I squash it into my earlier commit. ::: dbus/dbus-sysdeps-unix.c @@ +3760,5 @@ > #ifdef DBUS_ENABLE_X11_AUTOLAUNCH > + static char arg_dbus_launch[] = "dbus-launch"; > + static char arg_autolaunch[] = "--autolaunch"; > + static char arg_binary_syntax[] = "--binary-syntax"; > + static char arg_close_stderr[] = "--close-stderr"; I think these can now be static const char[], because I gave _read_subprocess_line_argv() a const argument. Again, I'll do that when I squash it. Comment on attachment 127054 [details] [review] [14] Fix warnings from compiler option '-Wformat-nonliteral' Review of attachment 127054 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-internals.h @@ +168,5 @@ > +/** String used in _dbus_return_if_fail macro */ > +static const char _dbus_return_if_fail_warning_format[] = > + "arguments to %s() were incorrect, assertion \"%s\" failed in " > + "file %s line %d.\nThis is normally a bug in some application " > + "using the D-Bus library.\n"; I don't like this part: as far as I can see, it's going to duplicate this rather long string in every translation unit that does this check, which is most of them. Checks are normally enabled in production builds, so every distro's dbus build will pay the price. I have a work-in-progress patch on my other computer (from my last attempt to get through all these warnings) to redo this by adding an extern _dbus_warn_return_if_fail() function that is called if the check fails, and having *that* do the printf. I think that's maybe a cleaner approach; I'll clean it up a bit and attach it. ::: dbus/dbus-marshal-recursive-util.c @@ +1701,4 @@ > } > > static void > +start_next_test (const char *description, I like the solutions to the issue in this file, and they'd be fine as their own commit. (In reply to Thomas Zimmermann from comment #61) > [09] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v6) > > Changes since v5: > > - 'DBus' -> 'D-Bus' in a comment I changed "DBus" to "dbus", and you changed it to D-Bus here. dbus is slightly more correct than this version (the dbus software project is the reference implementation of the D-Bus protocol, so what we're building is dbus) so I'm keeping mine :-) Comment on attachment 127053 [details] [review] [13] Remove unnecessary compiler option '-Wno-address' Review of attachment 127053 [details] [review]: ----------------------------------------------------------------- I thought I'd encountered one in a non-default code path... but all the build configurations on travis-ci seem to pass, so it's probably fine. I'm running it through a batch of compiles locally which take even longer, but cover slightly more options. Comment on attachment 127061 [details] [review] [21] Fix warnings from compiler option '-Wundef' Review of attachment 127061 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-marshal-recursive.h @@ +30,5 @@ > +/** turn this on to get deluged in TypeReader verbose spam */ > +#define RECURSIVE_MARSHAL_READ_TRACE 0 > + > +/** turn this on to get deluged in TypeWriter verbose spam */ > +#define RECURSIVE_MARSHAL_WRITE_TRACE 0 I think I'd prefer to duplicate these in dbus/dbus-marshal-recursive-util.c rather than having them leak into other modules... or tbh just delete the one instance from -util. ::: dbus/dbus-sysdeps.h @@ +290,4 @@ > * convert that to a conventional defined/undef switch. (We can't get > * the conventional defined/undef because of multiarch builds only running > * ./configure once, on Darwin.) */ > +#if defined(DBUS_HAVE_ATIOMIC_INT_COND) && DBUS_HAVE_ATOMIC_INT_COND This isn't how you're meant to spell it. I have a commit locally from my previous attempt at addressing these warnings which just gets rid of this whole block, because it is in fact dead code (since we fixed Bug #38005). I'll attach that soon. To be honest I think the -Wswitch-* commits here are just too big to be usefully reviewed. It isn't trivial to decide what the right resolution is for a particular warning, and if we take whatever change shuts the compiler up instead of thinking about it, we lose these flags' ability to detect real bugs. So I'd prefer to fast-track the rest of this, and come back to the switch stuff, with a commit per switch or related group of switches that addresses both warnings. You can test with "make CFLAGS='-Wswitch-default -Wswitch-enum -Wno-error=switch-default -Wno-error=switch-enum'", for example. Comment on attachment 127051 [details] [review] [09] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v6) I liked my version better :-P Created attachment 127114 [details] [review] [14a] Partially fix warnings from compiler option '-Wformat-nonliteral' From: Thomas Zimmermann <tdz@users.sourceforge.net> Signed-off-by: Thomas Zimmermann <tdz@users.sourceforge.net> [smcv: split out from a larger commit] Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=97357 Created attachment 127115 [details] [review] [14b] Reimplement _dbus_warn_return_if_fail without -Wformat-nonliteral We can avoid duplicating the format string between translation units, without the compiler warning us that it can't check non-literal format strings for format-string security vulnerabilities based on %p, by breaking out the "assertion failed" case into a slow-path. Created attachment 127116 [details] [review] [14c] Stop disabling -Wformat-nonliteral Created attachment 127117 [details] [review] [15a] Clean up how we arrange for environ to be declared Annoyingly, the POSIX way to declare environ (as "extern char **environ") is a redundant declaration in glibc with _GNU_SOURCE; work around that. We also have a workaround for _NSGetEnviron() needing to be used instead of direct access to environ in at least some circumstances on Mac OS. Attempt to sync that up between all the files that use environ, consistently sorting the most special special-cases first (Windows for files that are compiled there, then Mac, then GNU, with lowest-common-denominator POSIX last). The affected files are already OS-specific, so I'm not bothering to introduce a nicer or higher-level API for this. Based on the best bits of an earlier patch from me, and an earlier patch from Thomas Zimmermann. Created attachment 127118 [details] [review] [15b] Fix remaining warnings from compiler option '-Wredundant-decls' From: Thomas Zimmermann <tdz@users.sourceforge.net> Signed-off-by: Thomas Zimmermann <tdz@users.sourceforge.net> [smcv: omit the part involving environ, which was more involved] Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=97357 Created attachment 127119 [details] [review] [21a] Remove leftover declarations for assuming int manipulation is atomic We never assume this since <https://bugs.freedesktop.org/show_bug.cgi?id=38005> was fixed, because it isn't true in modern compilers. --- The other parts of your commit for -Wundef are not obsolete, but are not fixed yet. I haven't included -Wswitch-* and -Wshadow in my warnings branch yet, because they're harder to review. Next, I'm intending to work through -Wshadow (your patch is *probably* right, but it's hard to review on the web) and -Wundef. I'm tempted to kick out the -Wswitch-* ones into a separate bug - it's a lot more involved to determine whether a fix to those is correct. Created attachment 127120 [details] [review] [07] Be more const-correct As a general design principle, strings that we aren't going to modify should usually be const. When compiling with -Wwrite-strings, quoted string constants are of type "const char *", causing compiler warnings when they are assigned to char * variables. Unfortunately, we need to add casts in a few places: * _dbus_list_append(), _dbus_test_oom_handling() and similar generic "user-data" APIs take a void *, not a const void *, so we have to cast * For historical reasons the execve() family of functions take a (char * const *), i.e. a constant pointer to an array of mutable strings, so again we have to cast * _dbus_spawn_async_with_babysitter similarly takes a char **, although we can make it a little more const-correct by making it take (char * const *) like execve() does This also incorporates a subsequent patch by Thomas Zimmermann to put various string constants in static storage, which is a little more efficient. --- I applied the additional constness that I suggested, at the cost of one more cast. (In reply to Thomas Zimmermann from comment #66) > Created attachment 127056 [details] [review] > [16] Fix warnings from compiler option '-Wsuggest-attribute=noreturn' > This patch fixes warnings from '-Wsuggest-attribute=noreturn'. We cannot > enable it unconditionally as it would break libtool. In what way does it break libtool? It seems fine for me... If it breaks libtool for you, can we avoid that failure mode by setting up these compiler warnings *after* detecting libtool stuff, for example? Created attachment 127122 [details] [review] [21b] Fix remaining -Wundef warnings, and enable it Vaguely based on a patch from Thomas Zimmermann, but with a different solution to RECURSIVE_MARSHAL_WRITE_TRACE, and additionally fixing a build failure that only occurs when you build without libsystemd. (Yes, that's a configuration I test.) (In reply to Simon McVittie from comment #98) > [21b] Fix remaining -Wundef warnings, and enable it travis-ci says this is incomplete: ../../dbus/dbus-sysdeps-win.c: In function ‘dump_backtrace_for_thread’: ../../dbus/dbus-sysdeps-win.c:2467:7: error: "_M_X64" is not defined [-Werror=undef] #elif _M_X64 ^ ../../dbus/dbus-sysdeps-win.c:2475:7: error: "_M_IA64" is not defined [-Werror=undef] #elif _M_IA64 ^ MSDN says we should be testing these with defined(). (In reply to Simon McVittie from comment #97) > (In reply to Thomas Zimmermann from comment #66) > > Created attachment 127056 [details] [review] [review] > > [16] Fix warnings from compiler option '-Wsuggest-attribute=noreturn' > > > This patch fixes warnings from '-Wsuggest-attribute=noreturn'. We cannot > > enable it unconditionally as it would break libtool. > > In what way does it break libtool? It seems fine for me... > > If it breaks libtool for you, can we avoid that failure mode by setting up > these compiler warnings *after* detecting libtool stuff, for example? Without the -Wno-suggest options I see on mingw32 builds warnings like these: CC test-segfault.o ../../test/test-segfault.c: In function 'exception_handler': ../../test/test-segfault.c:25:1: warning: function might be candidate for attribute 'noreturn' [-Wsuggest-attribute=noreturn] exception_handler(LPEXCEPTION_POINTERS p) ^~~~~~~~~~~~~~~~~ ../../dbus/dbus-string.c: In function '_dbus_string_append_printf_valist': ../../dbus/dbus-string.c:1096:13: warning: function might be possible candidate for 'ms_printf' format attribute [-Wsuggest-attribute=format] format, args_copy); ^~~~~~ Now that you ask, I cannot reproduce the problem with libtool (of course!). I only remember that gcc 6.1 complained about libtool interfaces without 'noreturn' and 'format' attributes. Maybe an update already changed this. My libtool is now at 2.4.6, gcc is 6.2.1. If the switch fixes are too hard to review, just put them in a new bug report. This patch set seems way tool long already. FYI I remember that I added all missing enum cases to the switches. Therefore all the defaults should probably have a 'not reached' assert. Comment on attachment 127053 [details] [review] [13] Remove unnecessary compiler option '-Wno-address' Review of attachment 127053 [details] [review]: ----------------------------------------------------------------- I tried to apply this patch and got: Apply: Remove unnecessary compiler option '-Wno-address' fatal: sha1 information is lacking or useless (configure.ac). error: could not build fake ancestor Looks broken (In reply to Ralf Habacker from comment #102) > Apply: Remove unnecessary compiler option '-Wno-address' > fatal: sha1 information is lacking or useless (configure.ac). > error: could not build fake ancestor > > Looks broken Same with - Fix warnings from compiler option '-Wshadow' - Fix warnings from compiler option '-Wswitch-default' - Remove unnecessary compiler option '-Wno-type-limits' I did run git-bz apply 97357 in interactive mode I reordered related patch according to their number to avoid diff conflict. (In reply to Ralf Habacker from comment #102) > Comment on attachment 127053 [details] [review] [review] > [13] Remove unnecessary compiler option '-Wno-address' > > Review of attachment 127053 [details] [review] [review]: > ----------------------------------------------------------------- > > I tried to apply this patch and got: > Apply: Remove unnecessary compiler option '-Wno-address' > fatal: sha1 information is lacking or useless (configure.ac). > error: could not build fake ancestor > > Looks broken Try git am <patchfile> (In reply to Thomas Zimmermann from comment #101) > If the switch fixes are too hard to review, just put them in a new bug > report. I'll do that soon. I'll do the same with -Wshadow if my conclusion from reviewing it with more context isn't just "yes, ship it". Ralf: please ignore two -Wswitch-* commits and the -Wshadow one for now, and review the ones I wrote? I can review Thomas' commits, but I can't meaningfully review my own. (In reply to Ralf Habacker from comment #102) > I tried to apply this patch and got: > Apply: Remove unnecessary compiler option '-Wno-address' > fatal: sha1 information is lacking or useless (configure.ac). > error: could not build fake ancestor Please try <https://github.com/smcv/dbus/tree/warnings> instead. (The travis-ci failure there is known; see Comment #99. It's a problem with the last commit on the branch, I just haven't got as far as fixing it and retrying.) Comment on attachment 126934 [details] [review] [01] Add configure option '--enable-debug' to control debugging and profiling Applied Comment on attachment 126935 [details] [review] [02] Fix function declarations Applied Comment on attachment 126936 [details] [review] [03] Cast -1 to DWORD for comparing to variable Applied Comment on attachment 126937 [details] [review] [04] Protect debug-only variables behind DBUS_ENABLE_VERBOSE_MODE Applied Comment on attachment 126938 [details] [review] [05] Include string.h for strcmp() Applied Comment on attachment 126939 [details] [review] [06] Remove unused functions from Windows builds Applied Comment on attachment 127114 [details] [review] [14a] Partially fix warnings from compiler option '-Wformat-nonliteral' Applied Comment on attachment 127118 [details] [review] [15b] Fix remaining warnings from compiler option '-Wredundant-decls' Retitled to "Partially fix warnings from..." and applied. I attached my own patch for the part involving environ, which is necessary to silence these warnings completely. Comment on attachment 126945 [details] [review] [11] Rename distro-style CI build from "release" to "production" Applied unreviewed, since this is just the CI. Comment on attachment 127056 [details] [review] [16] Fix warnings from compiler option '-Wsuggest-attribute=noreturn' Applied Created attachment 127172 [details] [review] [01] Be more const-correct As a general design principle, strings that we aren't going to modify should usually be const. When compiling with -Wwrite-strings, quoted string constants are of type "const char *", causing compiler warnings when they are assigned to char * variables. Unfortunately, we need to add casts in a few places: * _dbus_list_append(), _dbus_test_oom_handling() and similar generic "user-data" APIs take a void *, not a const void *, so we have to cast * For historical reasons the execve() family of functions take a (char * const *), i.e. a constant pointer to an array of mutable strings, so again we have to cast * _dbus_spawn_async_with_babysitter similarly takes a char **, although we can make it a little more const-correct by making it take (char * const *) like execve() does This also incorporates a subsequent patch by Thomas Zimmermann to put various string constants in static storage, which is a little more efficient. Created attachment 127173 [details] [review] [02] Linux: use readdir(), not deprecated readdir_r() Created attachment 127174 [details] [review] [03] Reimplement _dbus_warn_return_if_fail without -Wformat-nonliteral We can avoid duplicating the format string between translation units, without the compiler warning us that it can't check non-literal format strings for format-string security vulnerabilities based on %p, by breaking out the "assertion failed" case into a slow-path. Created attachment 127175 [details] [review] [04] Clean up how we arrange for environ to be declared Annoyingly, the POSIX way to declare environ (as "extern char **environ") is a redundant declaration in glibc with _GNU_SOURCE; work around that. We also have a workaround for _NSGetEnviron() needing to be used instead of direct access to environ in at least some circumstances on Mac OS. Attempt to sync that up between all the files that use environ, consistently sorting the most special special-cases first (Windows for files that are compiled there, then Mac, then GNU, with lowest-common-denominator POSIX last). The affected files are already OS-specific, so I'm not bothering to introduce a nicer or higher-level API for this. Based on the best bits of an earlier patch from me, and an earlier patch from Thomas Zimmermann. Created attachment 127176 [details] [review] [05] Remove leftover declarations for assuming int manipulation is atomic We never assume this since <https://bugs.freedesktop.org/show_bug.cgi?id=38005> was fixed, because it isn't true in modern compilers. --- In particular, nothing is conditioned on those macros any more (you can verify that with git grep). Created attachment 127177 [details] [review] [06] Fix remaining -Wundef warnings Vaguely based on a patch from Thomas Zimmermann, but with a different solution to RECURSIVE_MARSHAL_WRITE_TRACE, and additionally fixing a build failure that only occurs when targeting Unix without libsystemd, and another that occurs when targeting Windows. Created attachment 127178 [details] [review] [07] Revert "Keep cmake gcc builds in sync with autotools warnings." When reviewing this commit, I said Looks OK, although this is going to become impossible if we start using the externally-curated list of warnings from <https://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html>, which I've been quite tempted to do. That time has now come. I think it's more valuable to have comprehensive warnings under our primary build system, Autotools, than to have some fairly elaborate CMake scripting to pick up the same compiler warnings in both build systems; the CMake build system is primarily there to give us the ability to compile with MSVC, which has orthogonal compiler warning options anyway. This reverts commit 41427560af2c9923a48e50ddbf72e53aad5b2983. Created attachment 127179 [details] [review] [08] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros From: Thomas Zimmermann <tdz@users.sourceforge.net> The autoconf macros AX_COMPILER_FLAGS_{CFLAGS|CXXFLAGS|LDFLAGS} test for compiler and linker support of various flags, and add the flags to the generated output. If the command-line option '--enable-compile-warnings' is specified to 'configure', a number of additional warning options is also added to the output. This is the default. The AX_COMPILER_FLAGS_* macros add stricter warnings then before. The patch disables some of them to make dbus build without errors. A later patch set should fix the warnings and remove the compiler flags. This patch integrates all tests for compiler flags into the call to AX_COMPILER_FLAGS_CFLAGS. All tests for compiler flags are now done in a single place. The old macros have been removed. Signed-off-by: Thomas Zimmermann <tdz@users.sourceforge.net> [smcv: add missing $ to DISABLE_WARNINGS] [smcv: drop -Wno-discarded-qualifiers] [smcv: drop non-C++ option -Wpointer-sign in C++ mode] [smcv: work around an AX_COMPILER_FLAGS_CFLAGS bug] [smcv: this source tree is called dbus, not DBus] Signed-off-by: Simon McVittie <smcv@debian.org> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=97357 Comment on attachment 127058 [details] [review] [18] Fix warnings from compiler option '-Wswitch-default' Marking this as "obsolete" just to take it off the to-do list for this particular bug. Please see Bug #98191, and attach any revised patches there. Comment on attachment 127059 [details] [review] [19] Fix warnings from compiler option '-Wswitch-enum' Marking this as "obsolete" just to take it off the to-do list for this particular bug. Please see Bug #98191, and attach any revised patches there. Comment on attachment 127066 [details] [review] [17] Fix warnings from compiler option '-Wshadow' (v2) Marking this as "obsolete" just to take it off the to-do list for this particular bug - but it isn't really, and I still need to review it. Please see Bug #98192, and attach any revised patches there. OK, now we're down to a manageable sequence of patches. 01 to 06 need review by someone who isn't me. They can probably be applied in any order. (Thomas, any opinions? Since I'm a D-Bus maintainer and I wrote those patches, I'd be happy enough to apply them based on positive review from a non-maintainer + no veto from another maintainer.) 07 is a revert, so I think I'd be happy with applying it unreviewed. I know it's a bit of a regression for the CMake build, but the improved diagnostics in our recommended build system make it worth it, IMO. 08 doesn't need review, but we shouldn't apply it until 01 to 07 have landed. Created attachment 127183 [details] [review] [06] Fix remaining -Wundef warnings Vaguely based on a patch from Thomas Zimmermann, but with a different solution to RECURSIVE_MARSHAL_WRITE_TRACE, and additionally fixing a build failure that only occurs when targeting Unix without libsystemd, and another that occurs when targeting Windows. --- One more: if we don't have setrlimit() (for example on Windows), there's another -Wundef. Assigning this to myself since all the remaining patches that need review are from me. I've enabled -Wshadow on master (Bug #98192), together with -Wimplicit, -Wold-style-declaration, -Wold-style-definition, -Wstrict-prototypes, -Wtype-limits. I pushed those unreviewed since the change is trivial; the intention is to make sure Bug #98192, and the patches I already pushed from here, don't regress. I've also enabled the same compiler warnings under Autotools when targeting Windows as when targeting Unix. The comment said we're not on Windows (too many warnings) but Thomas has fixed the remaining warnings, so I think we're good there; enabling the warnings means his good work won't get reversed by future changes. Comment on attachment 127179 [details] [review] [08] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros Review of attachment 127179 [details] [review]: ----------------------------------------------------------------- This is going to need a (reasonably straightforward) rebase onto master, to adapt to the extra warnings added on master while dropping -Wno-shadow. (In reply to Simon McVittie from comment #130) > I've enabled -Wshadow on master (Bug #98192), together with -Wimplicit, > -Wold-style-declaration, -Wold-style-definition, -Wstrict-prototypes, > -Wtype-limits (There's no need to enable those explicitly with AX_COMPILER_FLAGS: they're all opted-in by AX_COMPILER_FLAGS anyway, either directly or as part of -Wall or -Wextra.) (In reply to Simon McVittie from comment #131) > This is going to need a (reasonably straightforward) rebase onto master, to > adapt to the extra warnings added on master while dropping -Wno-shadow. (Testing it on Travis-CI) Created attachment 127202 [details] [review] [08] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros --- Rebased onto current master. Also available as the warnings branch on my github clone: https://github.com/smcv/dbus/tree/warnings Hi
> 01 to 06 need review by someone who isn't me. They can probably be applied
> in any order. (Thomas, any opinions? Since I'm a D-Bus maintainer and I
> wrote those patches, I'd be happy enough to apply them based on positive
> review from a non-maintainer + no veto from another maintainer.)
These patches look good to me.
Comment on attachment 127178 [details] [review] [07] Revert "Keep cmake gcc builds in sync with autotools warnings." Review of attachment 127178 [details] [review]: ----------------------------------------------------------------- Please let the macro stay there. There are additional autotools package like gnucash out there for which this macro may be useful. As far as I know are all dbus developments performed on windows are using cmake because of the reduced build dependencies and better IDE integration (for example QtCreator) compared to autotools. Autotools is currently used to cross compile dbus packages to be integrated into windows installer containing KDE applications. Developing with cmake with a different set of warning flags may result into not detected issue which may later raises up on compiling autotools and requires additional iterations in bug fixing especial when using -Werror. For that I'm going to find a similar ax_compiler_flag implementation for cmake. (In reply to Ralf Habacker from comment #136) > Please let the macro stay there. There are additional autotools package like > gnucash out there for which this macro may be useful. They're welcome to get it from the dbus-1.10 branch, from which it isn't going away (we used to have similar situations in Telepathy where an older stable-branch was the de facto upstream for scripts or macros that git master no longer used). I really don't want to leave code in the tree that the relevant branch is no longer using, though - that's misleading at best. > Developing with cmake with a different set of warning flags may result into > not detected issue which may later raises up on compiling autotools and > requires additional iterations in bug fixing especial when using -Werror. We shouldn't be merging anything that makes the Autotools build fail. If you want to use the CMake build for development, that's fine, but please do at least one successful Autotools build (either locally, or by putting the branch in a Github clone that has travis-ci enabled) before landing it. Similarly, we preferably shouldn't be merging anything that makes the CMake build fail; if I didn't test on CMake, I wouldn't have added this patch. > For that I'm going to find a similar ax_compiler_flag implementation for > cmake. I'd be happy to review such a thing, but I really don't think it should block merging this stuff, because so far our work on fixing these warnings has found one security vulnerability (Bug #98157). Comment on attachment 127178 [details] [review] [07] Revert "Keep cmake gcc builds in sync with autotools warnings." Review of attachment 127178 [details] [review]: ----------------------------------------------------------------- As it does not break cmake building looks good. (In reply to Simon McVittie from comment #137) > please do at least one successful Autotools build (either locally, or by > putting the branch in a Github clone that has travis-ci enabled) before > landing it. Which configure command line parameters are required ? (In reply to Ralf Habacker from comment #139) > Which configure command line parameters are required ? Choose your favourites, but if you're doing a single Autotools build, --enable-developer --enable-tests (as used for the "debug" configuration in tools/ci-build.sh) is the most useful. The configurations in tools/ci-build.sh are intended to be reasonably complete, and are based on the semi-automated testing I do before releases. All merged. Thanks! Bug #98195 (ready for review) and Bug #98191 (patches need some breaking up) are the follow-up bugs. clang also produces some interesting warnings that gcc doesn't; I haven't looked into those. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.