Bug 97357 - Use AX_COMPILER_FLAGS and fix many gcc warnings
Summary: Use AX_COMPILER_FLAGS and fix many gcc warnings
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on: 88922
Blocks: 98191 98192 98195
  Show dependency treegraph
 
Reported: 2016-08-15 19:14 UTC by Thomas Zimmermann
Modified: 2016-10-13 17:02 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
[01] Replace configure option '--enable-developer' by '--enable-debug' (4.90 KB, patch)
2016-08-16 19:01 UTC, Thomas Zimmermann
Details | Splinter Review
[02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (7.67 KB, patch)
2016-08-16 19:02 UTC, Thomas Zimmermann
Details | Splinter Review
[03] Test additonal compiler flags with AX_COMPILER_FLAGS_CFLAGS (4.83 KB, patch)
2016-08-16 19:02 UTC, Thomas Zimmermann
Details | Splinter Review
[10] Remove configure option '--disable-compiler-optimisations' (3.65 KB, patch)
2016-08-16 19:03 UTC, Thomas Zimmermann
Details | Splinter Review
[01] Add configure option '--enable-debug' to control debugging and profiling (v2) (2.83 KB, patch)
2016-08-18 19:58 UTC, Thomas Zimmermann
Details | Splinter Review
[02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v2) (8.00 KB, patch)
2016-08-18 20:01 UTC, Thomas Zimmermann
Details | Splinter Review
[03] Test additonal compiler flags with AX_COMPILER_FLAGS_CFLAGS (v2) (4.72 KB, patch)
2016-08-18 20:02 UTC, Thomas Zimmermann
Details | Splinter Review
[05] Minor fixes for Windows builds (4.46 KB, patch)
2016-08-19 10:04 UTC, Thomas Zimmermann
Details | Splinter Review
[01] Add configure option '--enable-debug' to control debugging and profiling (v3) (2.92 KB, patch)
2016-08-20 19:16 UTC, Thomas Zimmermann
Details | Splinter Review
[02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v3) (11.08 KB, patch)
2016-08-20 19:23 UTC, Thomas Zimmermann
Details | Splinter Review
[05] Remove unused functions from Windows builds (v2) (3.79 KB, patch)
2016-08-20 19:26 UTC, Thomas Zimmermann
Details | Splinter Review
[06] Fix function declarations (1.81 KB, patch)
2016-08-20 19:27 UTC, Thomas Zimmermann
Details | Splinter Review
[07] Cast -1 to DWORD for comparing to variable (998 bytes, patch)
2016-08-20 19:27 UTC, Thomas Zimmermann
Details | Splinter Review
[08] Protect debug-only variables behind DBUS_ENABLE_VERBOSE_MODE (815 bytes, patch)
2016-08-20 19:28 UTC, Thomas Zimmermann
Details | Splinter Review
[09] Include string.h for strcmp() (636 bytes, patch)
2016-08-20 19:28 UTC, Thomas Zimmermann
Details | Splinter Review
[05] Remove unused functions from Windows builds (v3) (3.80 KB, patch)
2016-08-20 19:39 UTC, Thomas Zimmermann
Details | Splinter Review
[01] Add configure option '--enable-debug' to control debugging and profiling (3.08 KB, patch)
2016-10-01 13:51 UTC, Simon McVittie
Details | Splinter Review
[02] Fix function declarations (1.94 KB, patch)
2016-10-01 13:51 UTC, Simon McVittie
Details | Splinter Review
[03] Cast -1 to DWORD for comparing to variable (1.14 KB, patch)
2016-10-01 13:52 UTC, Simon McVittie
Details | Splinter Review
[04] Protect debug-only variables behind DBUS_ENABLE_VERBOSE_MODE (930 bytes, patch)
2016-10-01 13:53 UTC, Simon McVittie
Details | Splinter Review
[05] Include string.h for strcmp() (750 bytes, patch)
2016-10-01 13:53 UTC, Simon McVittie
Details | Splinter Review
[06] Remove unused functions from Windows builds (3.91 KB, patch)
2016-10-01 13:54 UTC, Simon McVittie
Details | Splinter Review
[07] Be more const-correct (15.62 KB, patch)
2016-10-01 13:54 UTC, Simon McVittie
Details | Splinter Review
[08] Linux: use readdir(), not deprecated readdir_r() (1.35 KB, patch)
2016-10-01 13:55 UTC, Simon McVittie
Details | Splinter Review
[09] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (11.86 KB, patch)
2016-10-01 13:56 UTC, Simon McVittie
Details | Splinter Review
[08½] Revert "Keep cmake gcc builds in sync with autotools warnings." (4.60 KB, patch)
2016-10-01 14:12 UTC, Simon McVittie
Details | Splinter Review
[11] Rename distro-style CI build from "release" to "production" (1.93 KB, patch)
2016-10-01 14:22 UTC, Simon McVittie
Details | Splinter Review
[09] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v6) (11.85 KB, patch)
2016-10-06 12:35 UTC, Thomas Zimmermann
Details | Splinter Review
[12] Fix warnings from compiler option '-Wdiscarded-qualifiers' (3.24 KB, patch)
2016-10-06 12:40 UTC, Thomas Zimmermann
Details | Splinter Review
[13] Remove unnecessary compiler option '-Wno-address' (931 bytes, patch)
2016-10-06 12:41 UTC, Thomas Zimmermann
Details | Splinter Review
[14] Fix warnings from compiler option '-Wformat-nonliteral' (7.85 KB, patch)
2016-10-06 12:41 UTC, Thomas Zimmermann
Details | Splinter Review
[15] Fix warnings from compiler option '-Wredundant-decls' (4.95 KB, patch)
2016-10-06 12:42 UTC, Thomas Zimmermann
Details | Splinter Review
[16] Fix warnings from compiler option '-Wsuggest-attribute=noreturn' (4.27 KB, patch)
2016-10-06 12:43 UTC, Thomas Zimmermann
Details | Splinter Review
[17] Fix warnings from compiler option '-Wshadow' (10.35 KB, patch)
2016-10-06 12:43 UTC, Thomas Zimmermann
Details | Splinter Review
[18] Fix warnings from compiler option '-Wswitch-default' (14.30 KB, patch)
2016-10-06 12:44 UTC, Thomas Zimmermann
Details | Splinter Review
[19] Fix warnings from compiler option '-Wswitch-enum' (7.28 KB, patch)
2016-10-06 12:44 UTC, Thomas Zimmermann
Details | Splinter Review
[20] Remove unnecessary compiler option '-Wno-type-limits' (792 bytes, patch)
2016-10-06 12:45 UTC, Thomas Zimmermann
Details | Splinter Review
[21] Fix warnings from compiler option '-Wundef' (2.91 KB, patch)
2016-10-06 12:46 UTC, Thomas Zimmermann
Details | Splinter Review
[17] Fix warnings from compiler option '-Wshadow' (v2) (10.45 KB, patch)
2016-10-06 13:46 UTC, Thomas Zimmermann
Details | Splinter Review
[14a] Partially fix warnings from compiler option '-Wformat-nonliteral' (5.98 KB, patch)
2016-10-07 17:30 UTC, Simon McVittie
Details | Splinter Review
[14b] Reimplement _dbus_warn_return_if_fail without -Wformat-nonliteral (5.07 KB, patch)
2016-10-07 17:31 UTC, Simon McVittie
Details | Splinter Review
[14c] Stop disabling -Wformat-nonliteral (864 bytes, patch)
2016-10-07 17:31 UTC, Simon McVittie
Details | Splinter Review
[15a] Clean up how we arrange for environ to be declared (3.50 KB, patch)
2016-10-07 17:32 UTC, Simon McVittie
Details | Splinter Review
[15b] Fix remaining warnings from compiler option '-Wredundant-decls' (3.78 KB, patch)
2016-10-07 17:33 UTC, Simon McVittie
Details | Splinter Review
[21a] Remove leftover declarations for assuming int manipulation is atomic (3.26 KB, patch)
2016-10-07 17:38 UTC, Simon McVittie
Details | Splinter Review
[07] Be more const-correct (17.07 KB, patch)
2016-10-07 17:43 UTC, Simon McVittie
Details | Splinter Review
[21b] Fix remaining -Wundef warnings, and enable it (1.91 KB, patch)
2016-10-07 18:48 UTC, Simon McVittie
Details | Splinter Review
[01] Be more const-correct (17.07 KB, patch)
2016-10-10 14:39 UTC, Simon McVittie
Details | Splinter Review
[02] Linux: use readdir(), not deprecated readdir_r() (1.35 KB, patch)
2016-10-10 14:40 UTC, Simon McVittie
Details | Splinter Review
[03] Reimplement _dbus_warn_return_if_fail without -Wformat-nonliteral (5.07 KB, patch)
2016-10-10 14:40 UTC, Simon McVittie
Details | Splinter Review
[04] Clean up how we arrange for environ to be declared (3.50 KB, patch)
2016-10-10 14:41 UTC, Simon McVittie
Details | Splinter Review
[05] Remove leftover declarations for assuming int manipulation is atomic (3.26 KB, patch)
2016-10-10 14:41 UTC, Simon McVittie
Details | Splinter Review
[06] Fix remaining -Wundef warnings (2.32 KB, patch)
2016-10-10 14:42 UTC, Simon McVittie
Details | Splinter Review
[07] Revert "Keep cmake gcc builds in sync with autotools warnings." (4.61 KB, patch)
2016-10-10 14:42 UTC, Simon McVittie
Details | Splinter Review
[08] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (11.70 KB, patch)
2016-10-10 14:43 UTC, Simon McVittie
Details | Splinter Review
[06] Fix remaining -Wundef warnings (2.76 KB, patch)
2016-10-10 15:07 UTC, Simon McVittie
Details | Splinter Review
[08] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (11.48 KB, patch)
2016-10-10 22:12 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Zimmermann 2016-08-15 19:14:03 UTC
This bug report is a follow-up to bug 88922.

The Autoconf archive contains macros for debugging (AX_CHECK_ENABLE_DEBUG), for testing compiler flags (AX_COMPILER_FLAGS et al), and other. A clean-up of the dbus configuration should replace the current scripts with these macros.
Comment 1 Simon McVittie 2016-08-15 20:16:53 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 :-(
Comment 2 Thomas Zimmermann 2016-08-16 07:04:32 UTC
(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.
Comment 3 Thomas Zimmermann 2016-08-16 19:01:26 UTC
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
Comment 4 Thomas Zimmermann 2016-08-16 19:02:01 UTC
Created attachment 125823 [details] [review]
[02] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros
Comment 5 Thomas Zimmermann 2016-08-16 19:02:41 UTC
Created attachment 125824 [details] [review]
[03] Test additonal compiler flags with AX_COMPILER_FLAGS_CFLAGS
Comment 6 Thomas Zimmermann 2016-08-16 19:03:43 UTC
Created attachment 125825 [details] [review]
[10] Remove configure option '--disable-compiler-optimisations'

As discussed in bug 88922 comment 5.
Comment 7 Simon McVittie 2016-08-16 19:21:21 UTC
(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 8 Simon McVittie 2016-08-16 19:24:14 UTC
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 9 Simon McVittie 2016-08-16 19:30:43 UTC
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 10 Simon McVittie 2016-08-16 19:31:48 UTC
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 11 Simon McVittie 2016-08-16 19:32:16 UTC
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.
Comment 12 Thomas Zimmermann 2016-08-18 07:07:52 UTC
> > [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.
Comment 13 Simon McVittie 2016-08-18 11:00:48 UTC
(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.
Comment 14 Thomas Zimmermann 2016-08-18 12:14:00 UTC
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
Comment 15 Simon McVittie 2016-08-18 12:35:25 UTC
(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.
Comment 16 Thomas Zimmermann 2016-08-18 19:58:20 UTC
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.
Comment 17 Thomas Zimmermann 2016-08-18 20:01:16 UTC
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
Comment 18 Thomas Zimmermann 2016-08-18 20:02:51 UTC
Created attachment 125889 [details] [review]
[03] Test additonal compiler flags with AX_COMPILER_FLAGS_CFLAGS (v2)

Changes since v1

  - removed explicit '-fno-script-aliasing'
Comment 19 Thomas Zimmermann 2016-08-18 20:03:20 UTC
(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
Comment 20 Thomas Zimmermann 2016-08-19 10:04:57 UTC
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 21 Simon McVittie 2016-08-19 10:45:03 UTC
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 22 Simon McVittie 2016-08-19 10:49:25 UTC
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 23 Simon McVittie 2016-08-19 10:50:11 UTC
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 24 Simon McVittie 2016-08-19 11:04:52 UTC
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 25 Simon McVittie 2016-08-19 11:06:36 UTC
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.
Comment 26 Thomas Zimmermann 2016-08-19 11:57:13 UTC
(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
Comment 27 Simon McVittie 2016-08-19 13:02:52 UTC
(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 :-)
Comment 28 Simon McVittie 2016-08-19 13:07:32 UTC
(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.
Comment 29 Thomas Zimmermann 2016-08-20 19:12:39 UTC
(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.
Comment 30 Thomas Zimmermann 2016-08-20 19:16:30 UTC
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'
Comment 31 Thomas Zimmermann 2016-08-20 19:23:48 UTC
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'
Comment 32 Thomas Zimmermann 2016-08-20 19:26:41 UTC
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
Comment 33 Thomas Zimmermann 2016-08-20 19:27:13 UTC
Created attachment 125926 [details] [review]
[06] Fix function declarations
Comment 34 Thomas Zimmermann 2016-08-20 19:27:44 UTC
Created attachment 125927 [details] [review]
[07] Cast -1 to DWORD for comparing to variable
Comment 35 Thomas Zimmermann 2016-08-20 19:28:14 UTC
Created attachment 125928 [details] [review]
[08] Protect debug-only variables behind DBUS_ENABLE_VERBOSE_MODE
Comment 36 Thomas Zimmermann 2016-08-20 19:28:48 UTC
Created attachment 125929 [details] [review]
[09] Include string.h for strcmp()
Comment 37 Thomas Zimmermann 2016-08-20 19:39:28 UTC
Created attachment 125930 [details] [review]
[05] Remove unused functions from Windows builds (v3)

Changes since v2:

  - removed a now unused variable
Comment 38 Thomas Zimmermann 2016-09-03 14:52:09 UTC
Simon, please take a look at the updated patch set.
Comment 39 Simon McVittie 2016-09-05 10:15:40 UTC
(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 40 Simon McVittie 2016-10-01 11:01:33 UTC
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 41 Philip Withnall 2016-10-01 12:04:32 UTC
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 42 Philip Withnall 2016-10-01 12:18:02 UTC
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.
Comment 43 Simon McVittie 2016-10-01 13:25:46 UTC
(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.
Comment 44 Simon McVittie 2016-10-01 13:51:06 UTC
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>
Comment 45 Simon McVittie 2016-10-01 13:51:37 UTC
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>
Comment 46 Simon McVittie 2016-10-01 13:52:11 UTC
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
Comment 47 Simon McVittie 2016-10-01 13:53:24 UTC
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
Comment 48 Simon McVittie 2016-10-01 13:53:57 UTC
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
Comment 49 Simon McVittie 2016-10-01 13:54:21 UTC
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
Comment 50 Simon McVittie 2016-10-01 13:54:55 UTC
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
Comment 51 Simon McVittie 2016-10-01 13:55:20 UTC
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
Comment 52 Simon McVittie 2016-10-01 13:56:05 UTC
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 53 Simon McVittie 2016-10-01 14:00:05 UTC
Comment on attachment 125825 [details] [review]
[10] Remove configure option '--disable-compiler-optimisations'

Reviewed-by: me, looks fine.
Comment 54 Simon McVittie 2016-10-01 14:00:51 UTC
Testing this current patch series on travis-ci.
Comment 55 Simon McVittie 2016-10-01 14:05:00 UTC
(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.
Comment 56 Simon McVittie 2016-10-01 14:12:12 UTC
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.
Comment 57 Simon McVittie 2016-10-01 14:22:52 UTC
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).
Comment 58 Thomas Zimmermann 2016-10-02 17:35:48 UTC
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.
Comment 59 Simon McVittie 2016-10-03 09:34:11 UTC
(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.
Comment 60 Simon McVittie 2016-10-03 10:32:18 UTC
(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).
Comment 61 Thomas Zimmermann 2016-10-06 12:35:41 UTC
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
Comment 62 Thomas Zimmermann 2016-10-06 12:40:32 UTC
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.
Comment 63 Thomas Zimmermann 2016-10-06 12:41:13 UTC
Created attachment 127053 [details] [review]
[13] Remove unnecessary compiler option '-Wno-address'
Comment 64 Thomas Zimmermann 2016-10-06 12:41:55 UTC
Created attachment 127054 [details] [review]
[14] Fix warnings from compiler option '-Wformat-nonliteral'
Comment 65 Thomas Zimmermann 2016-10-06 12:42:26 UTC
Created attachment 127055 [details] [review]
[15] Fix warnings from compiler option '-Wredundant-decls'
Comment 66 Thomas Zimmermann 2016-10-06 12:43:06 UTC
Created attachment 127056 [details] [review]
[16] Fix warnings from compiler option '-Wsuggest-attribute=noreturn'
Comment 67 Thomas Zimmermann 2016-10-06 12:43:41 UTC
Created attachment 127057 [details] [review]
[17] Fix warnings from compiler option '-Wshadow'
Comment 68 Thomas Zimmermann 2016-10-06 12:44:12 UTC
Created attachment 127058 [details] [review]
[18] Fix warnings from compiler option '-Wswitch-default'
Comment 69 Thomas Zimmermann 2016-10-06 12:44:43 UTC
Created attachment 127059 [details] [review]
[19] Fix warnings from compiler option '-Wswitch-enum'
Comment 70 Thomas Zimmermann 2016-10-06 12:45:13 UTC
Created attachment 127060 [details] [review]
[20] Remove unnecessary compiler option '-Wno-type-limits'
Comment 71 Thomas Zimmermann 2016-10-06 12:46:25 UTC
Created attachment 127061 [details] [review]
[21] Fix warnings from compiler option '-Wundef'
Comment 72 Thomas Zimmermann 2016-10-06 12:51:36 UTC
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 73 Simon McVittie 2016-10-06 13:20:42 UTC
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 74 Thomas Zimmermann 2016-10-06 13:23:14 UTC
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 75 Simon McVittie 2016-10-06 13:24:01 UTC
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 76 Simon McVittie 2016-10-06 13:25:10 UTC
Comment on attachment 127056 [details] [review]
[16] Fix warnings from compiler option '-Wsuggest-attribute=noreturn'

Review of attachment 127056 [details] [review]:
-----------------------------------------------------------------

Looks good
Comment 77 Simon McVittie 2016-10-06 13:29:28 UTC
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
Comment 78 Simon McVittie 2016-10-06 13:32:53 UTC
(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.)
Comment 79 Thomas Zimmermann 2016-10-06 13:46:35 UTC
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
Comment 80 Thomas Zimmermann 2016-10-07 08:21:53 UTC
(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.
Comment 81 Thomas Zimmermann 2016-10-07 08:26:51 UTC
(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].
Comment 82 Simon McVittie 2016-10-07 16:31:26 UTC
(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 83 Simon McVittie 2016-10-07 16:38:40 UTC
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 84 Simon McVittie 2016-10-07 16:49:38 UTC
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.
Comment 85 Simon McVittie 2016-10-07 16:55:53 UTC
(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 86 Simon McVittie 2016-10-07 16:58:30 UTC
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 87 Simon McVittie 2016-10-07 17:05:41 UTC
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.
Comment 88 Simon McVittie 2016-10-07 17:08:26 UTC
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 89 Simon McVittie 2016-10-07 17:28:21 UTC
Comment on attachment 127051 [details] [review]
[09] Test compiler and linker flags with AX_COMPILER_FLAGS_* macros (v6)

I liked my version better :-P
Comment 90 Simon McVittie 2016-10-07 17:30:36 UTC
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
Comment 91 Simon McVittie 2016-10-07 17:31:32 UTC
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.
Comment 92 Simon McVittie 2016-10-07 17:31:59 UTC
Created attachment 127116 [details] [review]
[14c] Stop disabling -Wformat-nonliteral
Comment 93 Simon McVittie 2016-10-07 17:32:45 UTC
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.
Comment 94 Simon McVittie 2016-10-07 17:33:30 UTC
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
Comment 95 Simon McVittie 2016-10-07 17:38:20 UTC
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.
Comment 96 Simon McVittie 2016-10-07 17:43:50 UTC
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.
Comment 97 Simon McVittie 2016-10-07 17:47:40 UTC
(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?
Comment 98 Simon McVittie 2016-10-07 18:48:01 UTC
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.)
Comment 99 Simon McVittie 2016-10-07 18:58:52 UTC
(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().
Comment 100 Thomas Zimmermann 2016-10-07 21:49:41 UTC
(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.
Comment 101 Thomas Zimmermann 2016-10-07 21:52:36 UTC
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 102 Ralf Habacker 2016-10-07 22:21:28 UTC
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
Comment 103 Ralf Habacker 2016-10-07 22:31:16 UTC
(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.
Comment 104 Thomas Zimmermann 2016-10-08 09:59:36 UTC
(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>
Comment 105 Simon McVittie 2016-10-09 22:38:58 UTC
(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 106 Simon McVittie 2016-10-10 14:23:38 UTC
Comment on attachment 126934 [details] [review]
[01] Add configure option '--enable-debug' to control  debugging and profiling

Applied
Comment 107 Simon McVittie 2016-10-10 14:23:49 UTC
Comment on attachment 126935 [details] [review]
[02] Fix function declarations

Applied
Comment 108 Simon McVittie 2016-10-10 14:24:24 UTC
Comment on attachment 126936 [details] [review]
[03] Cast -1 to DWORD for comparing to variable

Applied
Comment 109 Simon McVittie 2016-10-10 14:24:39 UTC
Comment on attachment 126937 [details] [review]
[04] Protect debug-only variables behind  DBUS_ENABLE_VERBOSE_MODE

Applied
Comment 110 Simon McVittie 2016-10-10 14:24:55 UTC
Comment on attachment 126938 [details] [review]
[05] Include string.h for strcmp()

Applied
Comment 111 Simon McVittie 2016-10-10 14:25:10 UTC
Comment on attachment 126939 [details] [review]
[06] Remove unused functions from Windows builds

Applied
Comment 112 Simon McVittie 2016-10-10 14:27:40 UTC
Comment on attachment 127114 [details] [review]
[14a] Partially fix warnings from compiler option  '-Wformat-nonliteral'

Applied
Comment 113 Simon McVittie 2016-10-10 14:28:39 UTC
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 114 Simon McVittie 2016-10-10 14:29:15 UTC
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 115 Simon McVittie 2016-10-10 14:30:03 UTC
Comment on attachment 127056 [details] [review]
[16] Fix warnings from compiler option '-Wsuggest-attribute=noreturn'

Applied
Comment 116 Simon McVittie 2016-10-10 14:39:35 UTC
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.
Comment 117 Simon McVittie 2016-10-10 14:40:07 UTC
Created attachment 127173 [details] [review]
[02] Linux: use readdir(), not deprecated readdir_r()
Comment 118 Simon McVittie 2016-10-10 14:40:30 UTC
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.
Comment 119 Simon McVittie 2016-10-10 14:41:05 UTC
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.
Comment 120 Simon McVittie 2016-10-10 14:41:47 UTC
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).
Comment 121 Simon McVittie 2016-10-10 14:42:19 UTC
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.
Comment 122 Simon McVittie 2016-10-10 14:42:48 UTC
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.
Comment 123 Simon McVittie 2016-10-10 14:43:46 UTC
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 124 Simon McVittie 2016-10-10 14:44:44 UTC
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 125 Simon McVittie 2016-10-10 14:44:55 UTC
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 126 Simon McVittie 2016-10-10 14:45:21 UTC
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.
Comment 127 Simon McVittie 2016-10-10 14:49:24 UTC
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.
Comment 128 Simon McVittie 2016-10-10 15:07:54 UTC
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.
Comment 129 Simon McVittie 2016-10-10 15:08:18 UTC
Assigning this to myself since all the remaining patches that need review are from me.
Comment 130 Simon McVittie 2016-10-10 19:51:39 UTC
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 131 Simon McVittie 2016-10-10 19:52:35 UTC
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.
Comment 132 Simon McVittie 2016-10-10 19:56:57 UTC
(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.)
Comment 133 Simon McVittie 2016-10-10 19:57:29 UTC
(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)
Comment 134 Simon McVittie 2016-10-10 22:12:35 UTC
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
Comment 135 Thomas Zimmermann 2016-10-11 11:39:16 UTC
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 136 Ralf Habacker 2016-10-12 21:17:54 UTC
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.
Comment 137 Simon McVittie 2016-10-13 09:03:26 UTC
(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 138 Ralf Habacker 2016-10-13 09:26:04 UTC
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.
Comment 139 Ralf Habacker 2016-10-13 09:40:52 UTC
(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 ?
Comment 140 Simon McVittie 2016-10-13 11:47:46 UTC
(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.
Comment 141 Simon McVittie 2016-10-13 17:02:48 UTC
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.