From: Junio C Hamano <hidden> Date: 2021-08-06 18:11:35
Atharva Raykar [off-list ref] writes:
Carlo Marcelo Arenas Belón [off-list ref] writes:
For those of us using Homebrew and using the LLVM installation from
there, we get:
$ cc --version
Homebrew clang version 12.0.1
...
So maybe we could add another case for "Homebrew clang"?
$ clang --version 2>&1 | sed -ne 's/ version .*//p'
Debian clang
It might be necessary to cope with this "$VENDOR clang version"
convention better with something like the following.
I am afraid that this patch is being a bit too aggressive about
LLVM, as I do not know if "$VENDOR LLVM version" is also a thing, or
it is just oddity only at Apple, though.
From: Jeff King <hidden> Date: 2021-08-06 19:20:25
On Fri, Aug 06, 2021 at 11:11:31AM -0700, Junio C Hamano wrote:
quoted
So maybe we could add another case for "Homebrew clang"?
$ clang --version 2>&1 | sed -ne 's/ version .*//p'
Debian clang
It might be necessary to cope with this "$VENDOR clang version"
convention better with something like the following.
Good catch. Unfortunately your patch below isn't sufficient because
get_family() is broken, too. :(
It insists on there being an extra word after the actual version. There
is for gcc, but not for clang on my system:
$ CC=gcc get_version_line
gcc version 10.2.1 20210110 (Debian 10.2.1-6)
$ CC=clang get_version_line
Debian clang version 11.0.1-2
Doing this on top of your patch makes "./detect-compiler clang" behave
as expected:
@@ -13,11 +13,11 @@ get_version_line() { } get_family() {- get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/'+ get_version_line | sed 's/^\(.*\) version [0-9].*/\1/' } get_version() {- get_version_line | sed 's/^.* version \([0-9][^ ]*\) .*/\1/'+ get_version_line | sed 's/^.* version \([0-9][^ ]*\).*/\1/' } print_flags() {
I am afraid that this patch is being a bit too aggressive about
LLVM, as I do not know if "$VENDOR LLVM version" is also a thing, or
it is just oddity only at Apple, though.
From: Junio C Hamano <hidden> Date: 2021-08-06 20:52:42
So here is a mini-series that summarizes what has been suggested so
far on the topic.
Carlo Marcelo Arenas Belón (1):
build: update detect-compiler for newer Xcode version
Jeff King (1):
build: clang version may not be followed by extra words
Junio C Hamano (1):
build: catch clang that identifies itself as "$VENDOR clang"
detect-compiler | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
--
2.33.0-rc1-159-gdd7297d7fa
From: Junio C Hamano <hidden> Date: 2021-08-06 20:52:45
From: Carlo Marcelo Arenas Belón <redacted>
1da1580e4c (Makefile: detect compiler and enable more warnings in
DEVELOPER=1, 2018-04-14) uses the output of the compiler banner to
detect the compiler family.
Apple had since changed the wording used to refer to its compiler
as clang instead of LLVM as shown by:
$ cc --version
Apple clang version 12.0.5 (clang-1205.0.22.9)
Target: x86_64-apple-darwin20.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
so update the script to match, and allow DEVELOPER=1 to work as
expected again in macOS.
Signed-off-by: Carlo Marcelo Arenas Belón <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
detect-compiler | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Junio C Hamano <hidden> Date: 2021-08-06 20:52:48
From: Jeff King <redacted>
The get_family and get_version helpers of detect-compiler assume
that the line to identify the version from the compilers have a
token "version", followed by the version number, followed by some
other string, e.g.
$ CC=gcc get_version_line
gcc version 10.2.1 20210110 (Debian 10.2.1-6)
But that is not necessarily true, e.g.
$ CC=clang get_version_line
Debian clang version 11.0.1-2
Tweak the script not to require extra string after the version.
Signed-off-by: Junio C Hamano <redacted>
---
detect-compiler | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Junio C Hamano <hidden> Date: 2021-08-06 20:52:50
The case statement in detect-compiler notices 'clang', 'FreeBSD
clang' and 'Apple clang', but there are other platforms that follow
the '$VENDOR clang' pattern (e.g. Debian).
Generalize the pattern to catch them.
Signed-off-by: Junio C Hamano <redacted>
---
detect-compiler | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
So here is a mini-series that summarizes what has been suggested so
far on the topic.
Carlo Marcelo Arenas Belón (1):
build: update detect-compiler for newer Xcode version
Jeff King (1):
build: clang version may not be followed by extra words
Junio C Hamano (1):
build: catch clang that identifies itself as "$VENDOR clang"
detect-compiler | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
Perhaps I've missed some obvious reason not to do this, but why are we
parsing the --version output of two modern compilers, as opposed to just
asking them what type/version they are via their usual macro facilities?
I.e. something like the below:
@@ -5,19 +5,47 @@ CC="$*"-# we get something like (this is at least true for gcc and clang)+#!/bin/sh #-# FreeBSD clang version 3.4.1 (tags/RELEASE...)-get_version_line() {- $CC -v 2>&1 | grep ' version '-}+# Probe the compiler for vintage, version, etc. This is used for setting+# optional make knobs under the DEVELOPER knob.++CC="$*"++v=$($CC -E - <<-EOF 2>&1 | grep -e '=')+GNUC=__GNUC__+GNUC_MINOR=__GNUC_MINOR__+GNUC_PATCHLEVEL=__GNUC_PATCHLEVEL__+clang=__clang__+clang_major=__clang_major__+clang_minor=__clang_minor__+clang_patchlevel=__clang_patchlevel__+EOF+eval "$v" get_family() {- get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/'+ # Clang also sets the GNUC macros, but GCC does not set+ # clang's.+ if test "$clang" != "__clang__"+ then+ echo clang+ elif test "$GNUC" != "__GNUC__"+ then+ echo gcc+ else+ echo unknown+ fi } get_version() {- get_version_line | sed 's/^.* version \([0-9][^ ]*\) .*/\1/'+ case "$(get_family)" in+ clang)+ echo "$clang_major.$clang_minor.$clang_patchlevel"+ ;;+ gcc)+ echo "$GNUC.$GNUC_MINOR.$GNUC_PATCHLEVEL"+ ;;+ esac } print_flags() {
All three patches look fine to me, and functionality-wise are a strict
improvement over the status quo. But I suspect in the long run we'd need
to keep all of the Apple bits in their own case-arm, like:
# this must come first, so we prefer it over "* clang".
"Apple LLVM" | "Apple clang")
print_apple_magic
;;
clang | *" clang")
print_flags clang
;;
and then apple_magic does the version conversion from Wikipedia I linked
to earlier.
I don't think your patch is really making it significantly harder to get
there, though splitting up "Apple LLVM" and "Apple clang" feels a bit
like it's the wrong direction.
I wasn't personally planning to take that next step, as I lack the
platform to test it on. And as noted, unless you have a pretty old
version of Xcode, it doesn't matter either way (so I'm content to leave
it until dev with a mac is bitten by it and cares enough to make it more
accurate).
-Peff
From: Jeff King <hidden> Date: 2021-08-07 02:15:25
On Sat, Aug 07, 2021 at 04:02:45AM +0200, Ævar Arnfjörð Bjarmason wrote:
Perhaps I've missed some obvious reason not to do this, but why are we
parsing the --version output of two modern compilers, as opposed to just
asking them what type/version they are via their usual macro facilities?
I.e. something like the below:
That would probably work OK in practice, but it actually seems more
complex to me (how do other random compilers react to "-E -"? Is it
possible for us to get other output from the preprocessor that would
confuse an eval?).
It could perhaps make the Apple version-string confusion go away,
though, which might make it worth doing.
-Peff
On Sat, Aug 07, 2021 at 04:02:45AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
Perhaps I've missed some obvious reason not to do this, but why are we
parsing the --version output of two modern compilers, as opposed to just
asking them what type/version they are via their usual macro facilities?
I.e. something like the below:
That would probably work OK in practice, but it actually seems more
complex to me (how do other random compilers react to "-E -"?
We only care about gcc and clang in that script, which I think have
supported that form of "-E" on stdin input for any version we're likely
to care about for the purposes of config.mak.dev. It seems unlikely that
we'll care about non-modern compilers in config.mak.dev, so using more
modern features there seems fine (it's all for opting us into even more
modern warning flags and the like...).
Is it possible for us to get other output from the preprocessor that
would confuse an eval?).
Probably, I just meant that as a POC. We could pipe it into some
awk/grep/cut/perl or whatever that would be more strict.
From: Jeff King <hidden> Date: 2021-08-07 14:14:01
On Sat, Aug 07, 2021 at 04:56:04AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
On Sat, Aug 07, 2021 at 04:02:45AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
Perhaps I've missed some obvious reason not to do this, but why are we
parsing the --version output of two modern compilers, as opposed to just
asking them what type/version they are via their usual macro facilities?
I.e. something like the below:
That would probably work OK in practice, but it actually seems more
complex to me (how do other random compilers react to "-E -"?
We only care about gcc and clang in that script, which I think have
supported that form of "-E" on stdin input for any version we're likely
to care about for the purposes of config.mak.dev. It seems unlikely that
we'll care about non-modern compilers in config.mak.dev, so using more
modern features there seems fine (it's all for opting us into even more
modern warning flags and the like...).
Yeah, but we don't find out what we have until we run the script in
question. I guess it is OK as long as we redirect stderr, ignore the
exit code, and only look for a positive outcome in the output (your
patch does the latter two already).
I also wondered how this might interact with CC="ccache gcc" (where
caching might fail to notice version changes). But from some quick
testing, it looks like it doesn't cache in this case (neither stdin, nor
with -E).
quoted
Is it possible for us to get other output from the preprocessor that
would confuse an eval?).
Probably, I just meant that as a POC. We could pipe it into some
awk/grep/cut/perl or whatever that would be more strict.
That would probably be better. I would be curious to hear from somebody
with a mac if this technique gives more sensible version numbers for the
Apple-clang compiler.
-Peff
On Sat, Aug 07, 2021 at 04:56:04AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
quoted
On Sat, Aug 07, 2021 at 04:02:45AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
Perhaps I've missed some obvious reason not to do this, but why are we
parsing the --version output of two modern compilers, as opposed to just
asking them what type/version they are via their usual macro facilities?
I.e. something like the below:
That would probably work OK in practice, but it actually seems more
complex to me (how do other random compilers react to "-E -"?
We only care about gcc and clang in that script, which I think have
supported that form of "-E" on stdin input for any version we're likely
to care about for the purposes of config.mak.dev. It seems unlikely that
we'll care about non-modern compilers in config.mak.dev, so using more
modern features there seems fine (it's all for opting us into even more
modern warning flags and the like...).
Yeah, but we don't find out what we have until we run the script in
question. I guess it is OK as long as we redirect stderr, ignore the
exit code, and only look for a positive outcome in the output (your
patch does the latter two already).
I also wondered how this might interact with CC="ccache gcc" (where
caching might fail to notice version changes). But from some quick
testing, it looks like it doesn't cache in this case (neither stdin, nor
with -E).
quoted
quoted
Is it possible for us to get other output from the preprocessor that
would confuse an eval?).
Probably, I just meant that as a POC. We could pipe it into some
awk/grep/cut/perl or whatever that would be more strict.
That would probably be better. I would be curious to hear from somebody
with a mac if this technique gives more sensible version numbers for the
Apple-clang compiler.
It does, on the gcc304 box on the gccfarm (recent apple M1 Mac Mini):
avar@minimac ~ % uname -a
Darwin minimac.moose.housegordon.com 20.4.0 Darwin Kernel Version 20.4.0: Thu Apr 22 21:46:41 PDT 2021; root:xnu-7195.101.2~1/RELEASE_ARM64_T8101 arm64
avar@minimac ~ % clang --version
Apple clang version 12.0.5 (clang-1205.0.22.9)
Target: arm64-apple-darwin20.4.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
avar@minimac ~ % cat >f
GNUC=__GNUC__
GNUC_MINOR=__GNUC_MINOR__
GNUC_PATCHLEVEL=__GNUC_PATCHLEVEL__
clang=__clang__
clang_major=__clang_major__
clang_minor=__clang_minor__
clang_patchlevel=__clang_patchlevel__
^C
avar@minimac ~ % clang -E - <f
# 1 "<stdin>"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 384 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "<stdin>" 2
GNUC=4
GNUC_MINOR=2
GNUC_PATCHLEVEL=1
clang=1
clang_major=12
clang_minor=0
clang_patchlevel=5
I think nobody who's using clang derivatives is screwing with these
macro variables, they're just changing whatever the "product name" or
whatever is in the --version output.
From: Jeff King <hidden> Date: 2021-08-07 14:40:19
On Sat, Aug 07, 2021 at 04:26:33PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
That would probably be better. I would be curious to hear from somebody
with a mac if this technique gives more sensible version numbers for the
Apple-clang compiler.
It does, on the gcc304 box on the gccfarm (recent apple M1 Mac Mini):
avar@minimac ~ % uname -a
Darwin minimac.moose.housegordon.com 20.4.0 Darwin Kernel Version 20.4.0: Thu Apr 22 21:46:41 PDT 2021; root:xnu-7195.101.2~1/RELEASE_ARM64_T8101 arm64
avar@minimac ~ % clang --version
Apple clang version 12.0.5 (clang-1205.0.22.9)
Target: arm64-apple-darwin20.4.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
avar@minimac ~ % cat >f
GNUC=__GNUC__
GNUC_MINOR=__GNUC_MINOR__
GNUC_PATCHLEVEL=__GNUC_PATCHLEVEL__
clang=__clang__
clang_major=__clang_major__
clang_minor=__clang_minor__
clang_patchlevel=__clang_patchlevel__
^C
avar@minimac ~ % clang -E - <f
# 1 "<stdin>"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 384 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "<stdin>" 2
GNUC=4
GNUC_MINOR=2
GNUC_PATCHLEVEL=1
clang=1
clang_major=12
clang_minor=0
clang_patchlevel=5
Hmm, now I'm really confused, though. Is that really clang 12 (for which
there is no 12.0.5; 12.0.1 is the latest version, shipped in July)? Or
is it XCode 12, shipping with LLVM 11, according to the table in:
https://en.wikipedia.org/wiki/Xcode#Xcode_11.x_-_13.x_(since_SwiftUI_framework)
(sorry, there are actually _two_ tables with that same anchor on the
page; the one you want is the second one, under "Toolchain versions").
The distinction does not matter for our script (where we only care about
"clang4" and up). I guess the most relevant test would be to get XCode
8.x and see what it says. I expect it to claim "clang 8.1.0" or similar,
but actually be clang-3. And therefore not support
-Wtautological-constant-out-of-range-compare.
If we can't get easily get hold of such a platform, then maybe that is a
good indication that this conversation is too academic for now, and we
should wait until somebody wants to add a more recent version-specifier
to config.mak.dev. ;)
-Peff
On Sat, Aug 07, 2021 at 04:26:33PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
quoted
That would probably be better. I would be curious to hear from somebody
with a mac if this technique gives more sensible version numbers for the
Apple-clang compiler.
It does, on the gcc304 box on the gccfarm (recent apple M1 Mac Mini):
avar@minimac ~ % uname -a
Darwin minimac.moose.housegordon.com 20.4.0 Darwin Kernel
Version 20.4.0: Thu Apr 22 21:46:41 PDT 2021;
root:xnu-7195.101.2~1/RELEASE_ARM64_T8101 arm64
avar@minimac ~ % clang --version
Apple clang version 12.0.5 (clang-1205.0.22.9)
Target: arm64-apple-darwin20.4.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
avar@minimac ~ % cat >f
GNUC=__GNUC__
GNUC_MINOR=__GNUC_MINOR__
GNUC_PATCHLEVEL=__GNUC_PATCHLEVEL__
clang=__clang__
clang_major=__clang_major__
clang_minor=__clang_minor__
clang_patchlevel=__clang_patchlevel__
^C
avar@minimac ~ % clang -E - <f
# 1 "<stdin>"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 384 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "<stdin>" 2
GNUC=4
GNUC_MINOR=2
GNUC_PATCHLEVEL=1
clang=1
clang_major=12
clang_minor=0
clang_patchlevel=5
Hmm, now I'm really confused, though. Is that really clang 12 (for which
there is no 12.0.5; 12.0.1 is the latest version, shipped in July)? Or
is it XCode 12, shipping with LLVM 11, according to the table in:
https://en.wikipedia.org/wiki/Xcode#Xcode_11.x_-_13.x_(since_SwiftUI_framework)
(sorry, there are actually _two_ tables with that same anchor on the
page; the one you want is the second one, under "Toolchain versions").
The distinction does not matter for our script (where we only care about
"clang4" and up). I guess the most relevant test would be to get XCode
8.x and see what it says. I expect it to claim "clang 8.1.0" or similar,
but actually be clang-3. And therefore not support
-Wtautological-constant-out-of-range-compare.
If we can't get easily get hold of such a platform, then maybe that is a
good indication that this conversation is too academic for now, and we
should wait until somebody wants to add a more recent version-specifier
to config.mak.dev. ;)
I think it's clang 12.0.5, and Apple just takes upstream versions and
increments them, e.g. I found this:
https://gist.github.com/yamaya/2924292
So you can presumably rely on it for having clang 12 features, and we'd
only ever care about the clang_major...
From: Carlo Arenas <hidden> Date: 2021-08-08 00:30:53
On Sat, Aug 7, 2021 at 7:40 AM Jeff King [off-list ref] wrote:
The distinction does not matter for our script (where we only care about
"clang4" and up). I guess the most relevant test would be to get XCode
8.x and see what it says. I expect it to claim "clang 8.1.0" or similar,
but actually be clang-3. And therefore not support
-Wtautological-constant-out-of-range-compare.
uses Xcode 7.3 (based on clang 3.8) and either does support that flag
or ignores it silently
https://www.travis-ci.com/github/carenas/git/builds/234772346
the same was observed with Xcode 8
both error later and fail to build because of a valid (but harmless)
-Wformat-extra-args warning that doesn't trigger in later versions
Carlo
From: Jeff King <hidden> Date: 2021-08-09 18:09:01
On Sat, Aug 07, 2021 at 05:30:39PM -0700, Carlo Arenas wrote:
On Sat, Aug 7, 2021 at 7:40 AM Jeff King [off-list ref] wrote:
quoted
The distinction does not matter for our script (where we only care about
"clang4" and up). I guess the most relevant test would be to get XCode
8.x and see what it says. I expect it to claim "clang 8.1.0" or similar,
but actually be clang-3. And therefore not support
-Wtautological-constant-out-of-range-compare.
uses Xcode 7.3 (based on clang 3.8) and either does support that flag
or ignores it silently
https://www.travis-ci.com/github/carenas/git/builds/234772346
the same was observed with Xcode 8
both error later and fail to build because of a valid (but harmless)
-Wformat-extra-args warning that doesn't trigger in later versions
Thanks for testing. I think I was wrong that clang4 is the limit for
that option, though. It comes originally from:
https://lore.kernel.org/git/20180317160832.GB15772@sigill.intra.peff.net/
where clang4 just happened to be the oldest thing I had access to at the
time, so we used that as a minimum. So probably all of our "clang4"
could really be "any clang" (but it is probably OK to leave it as-is).
-Peff
From: Jeff King <hidden> Date: 2021-08-09 18:10:22
On Sat, Aug 07, 2021 at 05:36:57PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
Hmm, now I'm really confused, though. Is that really clang 12 (for which
there is no 12.0.5; 12.0.1 is the latest version, shipped in July)? Or
is it XCode 12, shipping with LLVM 11, according to the table in:
https://en.wikipedia.org/wiki/Xcode#Xcode_11.x_-_13.x_(since_SwiftUI_framework)
(sorry, there are actually _two_ tables with that same anchor on the
page; the one you want is the second one, under "Toolchain versions").
The distinction does not matter for our script (where we only care about
"clang4" and up). I guess the most relevant test would be to get XCode
8.x and see what it says. I expect it to claim "clang 8.1.0" or similar,
but actually be clang-3. And therefore not support
-Wtautological-constant-out-of-range-compare.
If we can't get easily get hold of such a platform, then maybe that is a
good indication that this conversation is too academic for now, and we
should wait until somebody wants to add a more recent version-specifier
to config.mak.dev. ;)
I think it's clang 12.0.5, and Apple just takes upstream versions and
increments them, e.g. I found this:
https://gist.github.com/yamaya/2924292
So you can presumably rely on it for having clang 12 features, and we'd
only ever care about the clang_major...
From the reading I've done, I'm unconvinced that this is _actually_
clang 12, and they aren't simply doing funky things with the version
numbers. But since we lack an easy test of how the different versions
behave (even my suggested clang4 test turned out not to be robust!), it
probably doesn't matter much either way.
-Peff