Re: [Xen-devel] [XEN PATCH v3 15/23] xen/build: have the root Makefile generates the CFLAGS

From: Anthony PERARD <anthony.perard@citrix.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org,
	Daniel De Graaf <dgdegra@tycho.nsa.gov>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [Xen-devel] [XEN PATCH v3 15/23] xen/build: have the root Makefile generates the CFLAGS
Date: Tue, 17 Mar 2020 18:05:24 +0000	[thread overview]
Message-ID: <20200317180524.GA4088@perard.uk.xensource.com> (raw)
In-Reply-To: <20200227110504.GN24458@Air-de-Roger.citrite.net>

On Thu, Feb 27, 2020 at 12:05:04PM +0100, Roger Pau Monné wrote:
> On Wed, Feb 26, 2020 at 11:33:47AM +0000, Anthony PERARD wrote:
> > +ifeq ($(CONFIG_DEBUG),y)
> > +CFLAGS += -O1
> > +else
> > +CFLAGS += -O2
> > +endif
> 
> Long term we might want to make the optimization level selectable in
> Kconfig IMO.

Yep.

> > +ifneq ($(CONFIG_CC_IS_CLANG),y)
> > +# Clang doesn't understand this command line argument, and doesn't appear to
> > +# have an suitable alternative.  The resulting compiled binary does function,
> > +# but has an excessively large symbol table.
> > +CFLAGS += -Wa,--strip-local-absolute
> 
> This is not really related to clang, but to the assembler. If clang is
> used with -no-integrated-as it's quite likely that the GNU assembler
> will be used, and hence this option would be available.
> 
> Can we use cc-option-add here in order to detect whether the build
> toolchain support the option?

That can be done, but I think I'll do that as a follow up of this patch,
to avoid too many changes when moving the cflags around.

> Ideally this should be done after the integrated assembler tests
> performed in x86/Rules.mk.

> > +ifeq ($(CONFIG_UBSAN),y)
> > +CFLAGS_UBSAN := -fsanitize=undefined
> > +else
> > +CFLAGS_UBSAN :=
> 
> Do you need to define this to empty so it can be exported below? Isn't
> it enough to just not set it at all?

That has two functions, setting the correct flavor for the variable and
reset the value if the value happen to already be in the environment.
The second one is important I think.

> > +# define new variables to avoid the ones defines in Config.mk
> > +export XEN_CFLAGS := $(CFLAGS)
> > +export XEN_AFLAGS := $(AFLAGS)
> > +export XEN_LDFLAGS := $(LDFLAGS)
> > +export CFLAGS_UBSAN
> 
> You might want to rename this to XEN_CFLAGS_UBSAN for coherency with
> the rest of the exported variables?

I don't know, I think it's fine like that. They are other exported
variables, like CFLAGS-stack-boundary for flags. Not adding XEN_ prefix
to all of them mean less modifications, and less errors when doing so.

Thanks,

-- 
Anthony PERARD

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