Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 04:55:25PM +0100, Duy Nguyen wrote: > On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy > wrote: > > +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter > > clang4,$(COMPILER_FEATURES))),) > > +CFLAGS += -Wextra > > Another thing we can add here is -Og instead of standard -O2 (or -O0 > in my build), which is supported since gcc 4.8. clang seems to support > it too (mapped to -O1 at least for clang5) but I don't know what > version added that flag. I don't know, that feels kind of weird to me. I thought DEVELOPER was supposed to mean "ratchet up the linting, I want to know if I've broken something". But tweaking -O is about "I am in an edit-compile-debug cycle". Sometimes you are and sometimes you aren't, but you'd presumably want to have extra warnings regardless (especially because some warnings only trigger under particular optimization settings). Personally, I default to -O0, but then crank up to -O2 for performance testing or for my daily-driver builds. But I _always_ have as many warnings enabled as possible[1]. -Peff [1] I do have some pretty horrific magic to turn on -Werror when I'm visiting a "historical" commit, such as during a bisect.
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 7:56 PM, Ramsay Jones
wrote:
>
>
> On 18/03/18 15:55, Duy Nguyen wrote:
>> On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy
>> wrote:
>>> +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter
>>> clang4,$(COMPILER_FEATURES))),)
>>> +CFLAGS += -Wextra
>>
>> Another thing we can add here is -Og instead of standard -O2 (or -O0
>> in my build), which is supported since gcc 4.8. clang seems to support
>> it too (mapped to -O1 at least for clang5) but I don't know what
>> version added that flag.
>
> I've been doing a lot of compiling recently, using 10 'different'
> versions of clang and gcc ('different' if you count 64-bit and 32-bit
> compilers with the same version number as different!)
>
> However, I can tell you that clang version 3.4 and 3.8.0 don't
> support -Og, but clang version 5.0.1 does.
Yeah. I checked clang git mirror and this -Og is in 4.x release branch
(couldn't nail down exact release) so 5.x should be a safe bet.
--
Duy
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On 18/03/18 15:55, Duy Nguyen wrote:
> On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy
> wrote:
>> +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter
>> clang4,$(COMPILER_FEATURES))),)
>> +CFLAGS += -Wextra
>
> Another thing we can add here is -Og instead of standard -O2 (or -O0
> in my build), which is supported since gcc 4.8. clang seems to support
> it too (mapped to -O1 at least for clang5) but I don't know what
> version added that flag.
I've been doing a lot of compiling recently, using 10 'different'
versions of clang and gcc ('different' if you count 64-bit and 32-bit
compilers with the same version number as different!)
However, I can tell you that clang version 3.4 and 3.8.0 don't
support -Og, but clang version 5.0.1 does.
ATB,
Ramsay Jones
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy wrote: > +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter > clang4,$(COMPILER_FEATURES))),) > +CFLAGS += -Wextra Another thing we can add here is -Og instead of standard -O2 (or -O0 in my build), which is supported since gcc 4.8. clang seems to support it too (mapped to -O1 at least for clang5) but I don't know what version added that flag. -- Duy
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 10:26 AM, Jeff King wrote: > On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote: > >> The set of extra warnings we enable when DEVELOPER has to be >> conservative because we can't assume any compiler version the >> developer may use. Detect the compiler version so we know when it's >> safe to enable -Wextra and maybe more. > > Hrm, I was planning to expand my patch into a series. I started this so I think people expect me to move it forward. But I'll gladly hand it to you. It looks like you have more compilers to play with anyway (I'm on gentoo and installing a new compiler, especially llvm-based, takes ages). -- Duy
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 5:28 AM, Jeff King wrote:
> On Sun, Mar 18, 2018 at 05:06:07AM -0400, Eric Sunshine wrote:
>> On MacOS, "cc -v" output is:
>> --- >8 ---
>> Apple LLVM version 9.0.0 (clang-900.0.39.2)
>> Target: x86_64-apple-darwin16.7.0
>> Thread model: posix
>> InstalledDir: ...
>> --- >8 ---
>
> Is that really way ahead of the clang releases (which are at 7.0,
> AFAIK), or do they use a totally different number scheme?
I have no idea, but from past experience with Apple, I'd guess that
the version number is an Apple invention.
{...goes and researches a bit...}
Indeed, there doesn't seem to be any correlation between what Apple
reports and the actual clang version number[1].
[1]:
https://stackoverflow.com/questions/33603027/get-apple-clang-version-and-corresponding-upstream-llvm-version
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 05:06:07AM -0400, Eric Sunshine wrote: > On MacOS, "cc -v" output is: > > --- >8 --- > Apple LLVM version 9.0.0 (clang-900.0.39.2) > Target: x86_64-apple-darwin16.7.0 > Thread model: posix > InstalledDir: ... > --- >8 --- Is that really way ahead of the clang releases (which are at 7.0, AFAIK), or do they use a totally different number scheme? If the latter, I guess we'd have to treat it separately from clang and have all of the conditionals treat it independently? Yuck. -Peff
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote: > The set of extra warnings we enable when DEVELOPER has to be > conservative because we can't assume any compiler version the > developer may use. Detect the compiler version so we know when it's > safe to enable -Wextra and maybe more. Hrm, I was planning to expand my patch into a series. In particular, in my experiments some of those warnings do not need to be disabled under -Wextra (I tested going back to gcc 5 and clang 4.0). I think it's worth splitting up the boilerplate changes from the decisions on individual warnings, because I suspect we'll need to revise the latter going forward. I also noticed that all of those options seem to go back at least to gcc-5. It makes me wonder how much we even need this version-detecting framework. There are one or two "clang vs gcc" things I've found, but I haven't found a case where old versions of gcc don't support a particular option. IOW, could we get away with just adding these to the DEVELOPER knob and assuming that people who use it have a reasonably modern gcc or clang setup? That's more or less what the existing knobs do. But I didn't follow whether there was any earlier discussion on specific problems. > -dumpversion does not work correctly for clang. So I use "-v" instead > which seems to produce the same output for both gcc and clang (with a > minor difference in freebsd where it says "FreeBSD clang" instead of > just "clang"). Not sure if it helps your "cc on debian" case though Heh. At first I was confused, as it seems to work for me: $ clang-4.0 -dumpversion 4.2.1 But then I tried this: $ clang-5.0 -dumpversion 4.2.1 Whoops. It returns the same value up through clang 7. -Peff
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 5:17 AM, Duy Nguyen wrote: > On Sun, Mar 18, 2018 at 10:06 AM, Eric Sunshine > wrote: >> On MacOS, "cc -v" output is: >> --- >8 --- >> Apple LLVM version 9.0.0 (clang-900.0.39.2) >> Target: x86_64-apple-darwin16.7.0 >> Thread model: posix >> InstalledDir: ... >> --- >8 --- > > Does it still build ok for you with your changes squashed in? I think > the check for clang4/gcc6 and setting -Wextra in config.mak.dev may > backfire because clang9 probably has a lot more warnings enabled and > some of them we don't want and cause compile error... The project does still compile cleanly for me. (And I used "make V=1" to verify that the new flags are used.)
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 10:17:41AM +0100, Duy Nguyen wrote:
> On Sun, Mar 18, 2018 at 10:06 AM, Eric Sunshine
> wrote:
> > On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote:
> >> The set of extra warnings we enable when DEVELOPER has to be
> >> conservative because we can't assume any compiler version the
> >> developer may use. Detect the compiler version so we know when it's
> >> safe to enable -Wextra and maybe more.
> >>
> >> Helped-by: Jeff King
> >> Signed-off-by: Nguyễn Thái Ngọc Duy
> >> ---
> >> diff --git a/detect-compiler b/detect-compiler
> >> --- /dev/null
> >> +++ b/detect-compiler
> >> @@ -0,0 +1,50 @@
> >> +get_version_line() {
> >> + "$CC" -v 2>&1 | grep ' version '
> >> +}
> >
> > On MacOS, "cc -v" output is:
> >
> > --- >8 ---
> > Apple LLVM version 9.0.0 (clang-900.0.39.2)
> > Target: x86_64-apple-darwin16.7.0
> > Thread model: posix
> > InstalledDir: ...
> > --- >8 ---
>
> Does it still build ok for you with your changes squashed in? I think
> the check for clang4/gcc6 and setting -Wextra in config.mak.dev may
> backfire because clang9 probably has a lot more warnings enabled and
> some of them we don't want and cause compile error...
I don't think we can predict that going forward, though. We'd have to
wait for people to use the new compiler and then send a patch disabling
whatever causes a problem (or better yet, fixing the code if
appropriate).
This framework can only help us write those patches; I don't think it
can save us from having to do them in the first place.
-Peff
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 10:06 AM, Eric Sunshine wrote:
> On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote:
>> The set of extra warnings we enable when DEVELOPER has to be
>> conservative because we can't assume any compiler version the
>> developer may use. Detect the compiler version so we know when it's
>> safe to enable -Wextra and maybe more.
>>
>> Helped-by: Jeff King
>> Signed-off-by: Nguyễn Thái Ngọc Duy
>> ---
>> diff --git a/detect-compiler b/detect-compiler
>> --- /dev/null
>> +++ b/detect-compiler
>> @@ -0,0 +1,50 @@
>> +get_version_line() {
>> + "$CC" -v 2>&1 | grep ' version '
>> +}
>
> On MacOS, "cc -v" output is:
>
> --- >8 ---
> Apple LLVM version 9.0.0 (clang-900.0.39.2)
> Target: x86_64-apple-darwin16.7.0
> Thread model: posix
> InstalledDir: ...
> --- >8 ---
Does it still build ok for you with your changes squashed in? I think
the check for clang4/gcc6 and setting -Wextra in config.mak.dev may
backfire because clang9 probably has a lot more warnings enabled and
some of them we don't want and cause compile error...
--
Duy
Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote:
> The set of extra warnings we enable when DEVELOPER has to be
> conservative because we can't assume any compiler version the
> developer may use. Detect the compiler version so we know when it's
> safe to enable -Wextra and maybe more.
>
> Helped-by: Jeff King
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> diff --git a/detect-compiler b/detect-compiler
> --- /dev/null
> +++ b/detect-compiler
> @@ -0,0 +1,50 @@
> +get_version_line() {
> + "$CC" -v 2>&1 | grep ' version '
> +}
On MacOS, "cc -v" output is:
--- >8 ---
Apple LLVM version 9.0.0 (clang-900.0.39.2)
Target: x86_64-apple-darwin16.7.0
Thread model: posix
InstalledDir: ...
--- >8 ---
> +get_family() {
> + get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/'
> +}
So, this returns "Apple LLVM".
> +case "$(get_family)" in
> +gcc)
> + print_flags gcc
> + ;;
> +*clang)
> + print_flags clang
> + ;;
> +"FreeBSD clang")
> + print_flags clang
> + ;;
> +*)
> + : unknown compiler family
> + ;;
> +esac
Which means you probably want to squash in:
--- >8 ---
diff --git a/detect-compiler b/detect-compiler
index bc2ea39ef5..3140416b40 100755
--- a/detect-compiler
+++ b/detect-compiler
@@ -41,6 +41,9 @@ gcc)
*clang)
print_flags clang
;;
+"Apple LLVM")
+ print_flags clang
+ ;;
"FreeBSD clang")
print_flags clang
;;
--- >8 ---
[PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1
The set of extra warnings we enable when DEVELOPER has to be
conservative because we can't assume any compiler version the
developer may use. Detect the compiler version so we know when it's
safe to enable -Wextra and maybe more.
Helped-by: Jeff King
Signed-off-by: Nguyễn Thái Ngọc Duy
---
-dumpversion does not work correctly for clang. So I use "-v" instead
which seems to produce the same output for both gcc and clang (with a
minor difference in freebsd where it says "FreeBSD clang" instead of
just "clang"). Not sure if it helps your "cc on debian" case though
Tested with clang-5.0.1 and gcc-6.4.0 (too lazy to test on freebsd
clang 3.4.1 but I don't expect surprises there)
I will still need to pull more modern gcc/clang on travis, but that
can wait until this patch settles.
Makefile| 11 +--
config.mak.dev | 28 +++
detect-compiler | 50 +
3 files changed, 79 insertions(+), 10 deletions(-)
create mode 100644 config.mak.dev
create mode 100755 detect-compiler
diff --git a/Makefile b/Makefile
index a1d8775adb..9dfd152a1e 100644
--- a/Makefile
+++ b/Makefile
@@ -442,15 +442,6 @@ GIT-VERSION-FILE: FORCE
# CFLAGS and LDFLAGS are for the users to override from the command line.
CFLAGS = -g -O2 -Wall
-DEVELOPER_CFLAGS = -Werror \
- -Wdeclaration-after-statement \
- -Wno-format-zero-length \
- -Wold-style-definition \
- -Woverflow \
- -Wpointer-arith \
- -Wstrict-prototypes \
- -Wunused \
- -Wvla
LDFLAGS =
ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
ALL_LDFLAGS = $(LDFLAGS)
@@ -1051,7 +1042,7 @@ include config.mak.uname
-include config.mak
ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
endif
comma := ,
diff --git a/config.mak.dev b/config.mak.dev
new file mode 100644
index 00..59aef342c4
--- /dev/null
+++ b/config.mak.dev
@@ -0,0 +1,28 @@
+CFLAGS += -Werror
+CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wno-format-zero-length
+CFLAGS += -Wold-style-definition
+CFLAGS += -Woverflow
+CFLAGS += -Wpointer-arith
+CFLAGS += -Wstrict-prototypes
+CFLAGS += -Wunused
+CFLAGS += -Wvla
+
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+
+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
+CFLAGS += -Wtautological-constant-out-of-range-compare
+endif
+
+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter
clang4,$(COMPILER_FEATURES))),)
+CFLAGS += -Wextra
+CFLAGS += -Wmissing-prototypes
+CFLAGS += -Wno-empty-body
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-maybe-uninitialized
+endif
+endif
diff --git a/detect-compiler b/detect-compiler
new file mode 100755
index 00..bc2ea39ef5
--- /dev/null
+++ b/detect-compiler
@@ -0,0 +1,50 @@
+#!/bin/sh
+#
+# Probe the compiler for vintage, version, etc. This is used for setting
+# optional make knobs under the DEVELOPER knob.
+
+CC="$*"
+
+# we get something like (this is at least true for gcc and clang)
+#
+# FreeBSD clang version 3.4.1 (tags/RELEASE...)
+get_version_line() {
+ "$CC" -v 2>&1 | grep ' version '
+}
+
+get_family() {
+ get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/'
+}
+
+get_version() {
+ get_version_line | sed 's/^.* version \([0-9][^ ]*\) .*/\1/'
+}
+
+print_flags() {
+ family=$1
+ version=$(get_version | cut -f 1 -d .)
+
+ # Print a feature flag not only for the current version, but also
+ # for any prior versions we encompass. This avoids needing to do
+ # numeric comparisons in make, which are awkward.
+ while test "$version" -gt 0
+ do
+ echo $family$version
+ version=$((version - 1))
+ done
+}
+
+case "$(get_family)" in
+gcc)
+ print_flags gcc
+ ;;
+*clang)
+ print_flags clang
+ ;;
+"FreeBSD clang")
+ print_flags clang
+ ;;
+*)
+ : unknown compiler family
+ ;;
+esac
--
2.17.0.rc0.347.gf9cf61673a
