Re: [Xen-devel] [XEN PATCH v2 2/6] xen: Have Kconfig check $(CC)'s version

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel