diff mbox series

[XEN,v2,2/6] xen: Have Kconfig check $(CC)'s version

Message ID 20191217105901.68158-3-anthony.perard@citrix.com (mailing list archive)
State Superseded
Headers show
Series xen: Kconfig update with few extra | expand

Commit Message

Anthony PERARD Dec. 17, 2019, 10:58 a.m. UTC 
This import several files from Linux v5.3
 - scripts/Kconfig.include
 - scripts/clang-version.sh
 - scripts/gcc-version.sh
 and several config values from from Linux's init/Kconfig file.

Files are copied into scripts/ directory because that's were the files
are found in Linux tree, and also because we are going to import more
of Kbuild from Linux which is located in scripts/.

CONFIG_GCC_VERSION and CONFIG_CC_IS_CLANG are going to be use in
follow-up patches.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---

Notes:
    v2:
    - move the export CC* earlier in xen/Makefile, and add CXX to the list.

 xen/Kconfig                  | 17 ++++++++++++++++
 xen/Makefile                 |  2 ++
 xen/scripts/Kconfig.include  | 39 ++++++++++++++++++++++++++++++++++++
 xen/scripts/clang-version.sh | 19 ++++++++++++++++++
 xen/scripts/gcc-version.sh   | 20 ++++++++++++++++++
 5 files changed, 97 insertions(+)
 create mode 100644 xen/scripts/Kconfig.include
 create mode 100755 xen/scripts/clang-version.sh
 create mode 100755 xen/scripts/gcc-version.sh

Comments

Jan Beulich Jan. 3, 2020, 4:42 p.m. UTC | #1 
On 17.12.2019 11:58, Anthony PERARD wrote:
> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -4,9 +4,26 @@
>  #
>  mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
>  
> +source "scripts/Kconfig.include"
> +
>  config BROKEN
>  	bool
>  
> +config CC_IS_GCC
> +	def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
> +
> +config GCC_VERSION
> +	int
> +	default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC)) if CC_IS_GCC
> +	default 0

Why "if" and a 2nd "default" line here but ...

> +config CC_IS_CLANG
> +	def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
> +
> +config CLANG_VERSION
> +	int
> +	default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))

... just a single, unconditional one here? Wouldn't both better
have a "depends on CC_IS_*" line instead? This would then also
result (afaict) in no CONFIG_CLANG_VERSION in .config if building
with gcc (and vice versa), instead of a bogus CONFIG_CLANG_VERSION=0.

Jan
Anthony PERARD Jan. 6, 2020, 2:01 p.m. UTC | #2 
On Fri, Jan 03, 2020 at 05:42:18PM +0100, Jan Beulich wrote:
> On 17.12.2019 11:58, Anthony PERARD wrote:
> > --- a/xen/Kconfig
> > +++ b/xen/Kconfig
> > @@ -4,9 +4,26 @@
> >  #
> >  mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
> >  
> > +source "scripts/Kconfig.include"
> > +
> >  config BROKEN
> >  	bool
> >  
> > +config CC_IS_GCC
> > +	def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
> > +
> > +config GCC_VERSION
> > +	int
> > +	default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC)) if CC_IS_GCC
> > +	default 0
> 
> Why "if" and a 2nd "default" line here but ...
> 
> > +config CC_IS_CLANG
> > +	def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
> > +
> > +config CLANG_VERSION
> > +	int
> > +	default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))
> 
> ... just a single, unconditional one here?

clang-version.sh returns 0 when CC isn't clang, but gcc-version.sh
doesn't check if CC is gcc, and returns a bogus values instead.

e.g.:

$ ./clang-version.sh clang
90000
$ ./gcc-version.sh clang
40201

> Wouldn't both better
> have a "depends on CC_IS_*" line instead? This would then also
> result (afaict) in no CONFIG_CLANG_VERSION in .config if building
> with gcc (and vice versa), instead of a bogus CONFIG_CLANG_VERSION=0.

It sounds attracting to remove variables from .config, but it is equally
attracting to always have a variable set. It can be used
unconditionally when always set (without risking invalid syntax for
example).

Thanks,
Jan Beulich Jan. 6, 2020, 2:34 p.m. UTC | #3 
On 06.01.2020 15:01, Anthony PERARD wrote:
> On Fri, Jan 03, 2020 at 05:42:18PM +0100, Jan Beulich wrote:
>> On 17.12.2019 11:58, Anthony PERARD wrote:
>>> --- a/xen/Kconfig
>>> +++ b/xen/Kconfig
>>> @@ -4,9 +4,26 @@
>>>  #
>>>  mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
>>>  
>>> +source "scripts/Kconfig.include"
>>> +
>>>  config BROKEN
>>>  	bool
>>>  
>>> +config CC_IS_GCC
>>> +	def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
>>> +
>>> +config GCC_VERSION
>>> +	int
>>> +	default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC)) if CC_IS_GCC
>>> +	default 0
>>
>> Why "if" and a 2nd "default" line here but ...
>>
>>> +config CC_IS_CLANG
>>> +	def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
>>> +
>>> +config CLANG_VERSION
>>> +	int
>>> +	default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))
>>
>> ... just a single, unconditional one here?
> 
> clang-version.sh returns 0 when CC isn't clang, but gcc-version.sh
> doesn't check if CC is gcc, and returns a bogus values instead.
> 
> e.g.:
> 
> $ ./clang-version.sh clang
> 90000
> $ ./gcc-version.sh clang
> 40201

Imo both scripts should behave identically in this regard (and in fact
in all usage related ones). If adjusting the scripts is entirely
unacceptable for some reason, then the description should highlight
the need for the differences.

>> Wouldn't both better
>> have a "depends on CC_IS_*" line instead? This would then also
>> result (afaict) in no CONFIG_CLANG_VERSION in .config if building
>> with gcc (and vice versa), instead of a bogus CONFIG_CLANG_VERSION=0.
> 
> It sounds attracting to remove variables from .config, but it is equally
> attracting to always have a variable set. It can be used
> unconditionally when always set (without risking invalid syntax for
> example).

Hmm, yes, as long as we don't have (by mechanical conversion) or gain
constructs like

#if CONFIG_GCC_VERSION < 50000 /* must be gcc 4.x */

Plus - what's CONFIG_CC_IS_{GCC,CLANG} good for then? The same can
then be achieved by comparing CONFIG_{GCC,CLANG}_VERSION against zero.

Jan
Anthony PERARD Jan. 8, 2020, 2:47 p.m. UTC | #4 
On Mon, Jan 06, 2020 at 03:34:43PM +0100, Jan Beulich wrote:
> On 06.01.2020 15:01, Anthony PERARD wrote:
> > On Fri, Jan 03, 2020 at 05:42:18PM +0100, Jan Beulich wrote:
> >> On 17.12.2019 11:58, Anthony PERARD wrote:
> >>> --- a/xen/Kconfig
> >>> +++ b/xen/Kconfig
> >>> @@ -4,9 +4,26 @@
> >>>  #
> >>>  mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
> >>>  
> >>> +source "scripts/Kconfig.include"
> >>> +
> >>>  config BROKEN
> >>>  	bool
> >>>  
> >>> +config CC_IS_GCC
> >>> +	def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
> >>> +
> >>> +config GCC_VERSION
> >>> +	int
> >>> +	default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC)) if CC_IS_GCC
> >>> +	default 0
> >>
> >> Why "if" and a 2nd "default" line here but ...
> >>
> >>> +config CC_IS_CLANG
> >>> +	def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
> >>> +
> >>> +config CLANG_VERSION
> >>> +	int
> >>> +	default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))
> >>
> >> ... just a single, unconditional one here?
> > 
> > clang-version.sh returns 0 when CC isn't clang, but gcc-version.sh
> > doesn't check if CC is gcc, and returns a bogus values instead.
> > 
> > e.g.:
> > 
> > $ ./clang-version.sh clang
> > 90000
> > $ ./gcc-version.sh clang
> > 40201
> 
> Imo both scripts should behave identically in this regard (and in fact
> in all usage related ones). If adjusting the scripts is entirely
> unacceptable for some reason, then the description should highlight
> the need for the differences.

I think in gcc-version.sh case it would be fine to have a different
version than the one in Linux. I didn't find the script to be used with
a compiler other that GCC.  I'll adjust the script to return 0 when CC
isn't a gcc, like clang-version does.

> >> Wouldn't both better
> >> have a "depends on CC_IS_*" line instead? This would then also
> >> result (afaict) in no CONFIG_CLANG_VERSION in .config if building
> >> with gcc (and vice versa), instead of a bogus CONFIG_CLANG_VERSION=0.
> > 
> > It sounds attracting to remove variables from .config, but it is equally
> > attracting to always have a variable set. It can be used
> > unconditionally when always set (without risking invalid syntax for
> > example).
> 
> Hmm, yes, as long as we don't have (by mechanical conversion) or gain
> constructs like
> 
> #if CONFIG_GCC_VERSION < 50000 /* must be gcc 4.x */
> 
> Plus - what's CONFIG_CC_IS_{GCC,CLANG} good for then? The same can
> then be achieved by comparing CONFIG_{GCC,CLANG}_VERSION against zero.

Sure, but it is much easier to understand what "ifdef CONFIG_CC_IS_GCC"
is actually checking than it is to understand what
"[ $CONFIG_GCC_VERSION -ne 0 ]" is for. In the second form, it isn't
immediatly obvious for humans that we are simply checking which compiler
is in use.

Thanks,
Jan Beulich Jan. 8, 2020, 3:28 p.m. UTC | #5 
On 08.01.2020 15:47, Anthony PERARD wrote:
> On Mon, Jan 06, 2020 at 03:34:43PM +0100, Jan Beulich wrote:
>> On 06.01.2020 15:01, Anthony PERARD wrote:
>>> On Fri, Jan 03, 2020 at 05:42:18PM +0100, Jan Beulich wrote:
>>>> Wouldn't both better
>>>> have a "depends on CC_IS_*" line instead? This would then also
>>>> result (afaict) in no CONFIG_CLANG_VERSION in .config if building
>>>> with gcc (and vice versa), instead of a bogus CONFIG_CLANG_VERSION=0.
>>>
>>> It sounds attracting to remove variables from .config, but it is equally
>>> attracting to always have a variable set. It can be used
>>> unconditionally when always set (without risking invalid syntax for
>>> example).
>>
>> Hmm, yes, as long as we don't have (by mechanical conversion) or gain
>> constructs like
>>
>> #if CONFIG_GCC_VERSION < 50000 /* must be gcc 4.x */
>>
>> Plus - what's CONFIG_CC_IS_{GCC,CLANG} good for then? The same can
>> then be achieved by comparing CONFIG_{GCC,CLANG}_VERSION against zero.
> 
> Sure, but it is much easier to understand what "ifdef CONFIG_CC_IS_GCC"
> is actually checking than it is to understand what
> "[ $CONFIG_GCC_VERSION -ne 0 ]" is for. In the second form, it isn't
> immediatly obvious for humans that we are simply checking which compiler
> is in use.

And I wasn't really suggesting to drop the CC_IS_* ones. What I
dislike is the duplication resulting from the *_VERSION ones not
having a "depends on CC_IS_*".

Jan
diff mbox series

Patch

diff --git a/xen/Kconfig b/xen/Kconfig
index 01067326b4e7..9f6512d65b08 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -4,9 +4,26 @@ 
 #
 mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
 
+source "scripts/Kconfig.include"
+
 config BROKEN
 	bool
 
+config CC_IS_GCC
+	def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
+
+config GCC_VERSION
+	int
+	default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC)) if CC_IS_GCC
+	default 0
+
+config CC_IS_CLANG
+	def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
+
+config CLANG_VERSION
+	int
+	default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))
+
 source "arch/$(SRCARCH)/Kconfig"
 
 config DEFCONFIG_LIST
diff --git a/xen/Makefile b/xen/Makefile
index efbe9605e52b..c326fee5880e 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -18,6 +18,8 @@  export XEN_CONFIG_EXPERT ?= n
 PYTHON_INTERPRETER	:= $(word 1,$(shell which python3 python python2 2>/dev/null) python)
 export PYTHON		?= $(PYTHON_INTERPRETER)
 
+export CC CXX LD
+
 export BASEDIR := $(CURDIR)
 export XEN_ROOT := $(BASEDIR)/..
 
diff --git a/xen/scripts/Kconfig.include b/xen/scripts/Kconfig.include
new file mode 100644
index 000000000000..8221095ca34b
--- /dev/null
+++ b/xen/scripts/Kconfig.include
@@ -0,0 +1,39 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+# Kconfig helper macros
+
+# Convenient variables
+comma       := ,
+quote       := "
+squote      := '
+empty       :=
+space       := $(empty) $(empty)
+dollar      := $
+right_paren := )
+left_paren  := (
+
+# $(if-success,<command>,<then>,<else>)
+# Return <then> if <command> exits with 0, <else> otherwise.
+if-success = $(shell,{ $(1); } >/dev/null 2>&1 && echo "$(2)" || echo "$(3)")
+
+# $(success,<command>)
+# Return y if <command> exits with 0, n otherwise
+success = $(if-success,$(1),y,n)
+
+# $(failure,<command>)
+# Return n if <command> exits with 0, y otherwise
+failure = $(if-success,$(1),n,y)
+
+# $(cc-option,<flag>)
+# Return y if the compiler supports <flag>, n otherwise
+cc-option = $(success,$(CC) -Werror $(CLANG_FLAGS) $(1) -E -x c /dev/null -o /dev/null)
+
+# $(ld-option,<flag>)
+# Return y if the linker supports <flag>, n otherwise
+ld-option = $(success,$(LD) -v $(1))
+
+# check if $(CC) and $(LD) exist
+$(error-if,$(failure,command -v $(CC)),compiler '$(CC)' not found)
+$(error-if,$(failure,command -v $(LD)),linker '$(LD)' not found)
+
+# gcc version including patch level
+gcc-version := $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC))
diff --git a/xen/scripts/clang-version.sh b/xen/scripts/clang-version.sh
new file mode 100755
index 000000000000..6fabf0695761
--- /dev/null
+++ b/xen/scripts/clang-version.sh
@@ -0,0 +1,19 @@ 
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# clang-version clang-command
+#
+# Print the compiler version of `clang-command' in a 5 or 6-digit form
+# such as `50001' for clang-5.0.1 etc.
+
+compiler="$*"
+
+if ! ( $compiler --version | grep -q clang) ; then
+	echo 0
+	exit 1
+fi
+
+MAJOR=$(echo __clang_major__ | $compiler -E -x c - | tail -n 1)
+MINOR=$(echo __clang_minor__ | $compiler -E -x c - | tail -n 1)
+PATCHLEVEL=$(echo __clang_patchlevel__ | $compiler -E -x c - | tail -n 1)
+printf "%d%02d%02d\\n" $MAJOR $MINOR $PATCHLEVEL
diff --git a/xen/scripts/gcc-version.sh b/xen/scripts/gcc-version.sh
new file mode 100755
index 000000000000..ae353432539b
--- /dev/null
+++ b/xen/scripts/gcc-version.sh
@@ -0,0 +1,20 @@ 
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# gcc-version gcc-command
+#
+# Print the gcc version of `gcc-command' in a 5 or 6-digit form
+# such as `29503' for gcc-2.95.3, `30301' for gcc-3.3.1, etc.
+
+compiler="$*"
+
+if [ ${#compiler} -eq 0 ]; then
+	echo "Error: No compiler specified." >&2
+	printf "Usage:\n\t$0 <gcc-command>\n" >&2
+	exit 1
+fi
+
+MAJOR=$(echo __GNUC__ | $compiler -E -x c - | tail -n 1)
+MINOR=$(echo __GNUC_MINOR__ | $compiler -E -x c - | tail -n 1)
+PATCHLEVEL=$(echo __GNUC_PATCHLEVEL__ | $compiler -E -x c - | tail -n 1)
+printf "%d%02d%02d\\n" $MAJOR $MINOR $PATCHLEVEL