Re: [Xen-devel] [PATCH v5 3/5] x86/clang: restore integrated assembler usage with indirect thunks

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Feb 21, 2018 at 04:56:14PM +0000, Andrew Cooper wrote:
> On 20/02/18 14:10, Roger Pau Monne wrote:
> > If the required features are met by the integrated clang assembler
> > (support for .includes and propagation of .macro-s between asm()-s)
> > do not disable it.
> >
> > Only disable the integrated assembler for assembly files, like it was
> > done prior to "x86: Support indirect thunks from assembly code".
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> > Changes since v4:
> >  - Do not use else ifeq on the same line to be compatible with make
> >    3.8.
> >  - Modify as-insn-check to allow adding flags if the test case fails.
> >
> > Changes since v3:
> >  - Do not modify how the thunk is included, clang upstream (and 6) has
> >    been fixed to propagate .macro-s between asm()-s.
> >
> > Changes since v1:
> >  - Use as-insn to check if the assembler supports .include.
> >  - Open code a check for whether the assembler forgets .macro-s
> >    between asm()-s.
> > ---
> >  Config.mk             |  9 +++++----
> >  xen/Rules.mk          |  5 +++--
> >  xen/arch/x86/Rules.mk | 14 ++++++++++++++
> >  3 files changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/Config.mk b/Config.mk
> > index 407472c3fc..8d6d984488 100644
> > --- a/Config.mk
> > +++ b/Config.mk
> > @@ -162,13 +162,14 @@ as-insn = $(if $(shell echo 'void _(void) { asm 
> > volatile ( $(2) ); }' \
> >                         | $(filter-out -M% %.d -include 
> > %/include/xen/config.h,$(1)) \
> >                                -c -x c -o /dev/null - 2>&1),$(4),$(3))
> >  
> > -# as-insn-check: Add an option to compilation flags, but only if insn is
> > -#                supported by assembler.
> > -# Usage: $(call as-insn-check CFLAGS,CC,"nop",-DHAVE_GAS_NOP)
> > -as-insn-check = $(eval $(call as-insn-check-closure,$(1),$(2),$(3),$(4)))
> > +# as-insn-check: Conditionally add an option to compilation flags
> > +# Usage: $(call as-insn-check CFLAGS,CC,"nop",-DHAVE_GAS_NOP,-DNO_GAS_NOP)
> > +as-insn-check = $(eval $(call 
> > as-insn-check-closure,$(1),$(2),$(3),$(4),$(5)))
> 
> It is a shame about the naming.  This is really as-insn-add, even before
> your changes.  I'll prepare a separate patch to rename it, which should
> ideally come ahead of this patch.

Renaming would be fine for me, although -check doesn't seem that bad
TBH.

> >  define as-insn-check-closure
> >      ifeq ($$(call as-insn,$$($(2)) $$($(1)),$(3),y,n),y)
> >          $(1) += $(4)
> > +    else
> > +        $(1) += $(5)
> >      endif
> >  endef
> >  
> > diff --git a/xen/Rules.mk b/xen/Rules.mk
> > index 2918019b92..7adf757fb6 100644
> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -70,8 +70,9 @@ endif
> >  
> >  AFLAGS-y                += -D__ASSEMBLY__
> >  
> > -# Clang's built-in assembler can't handle embedded .include's
> > -CFLAGS-$(clang)         += -no-integrated-as
> > +# Older clang's built-in assembler doesn't understand .skip with labels:
> > +# https://bugs.llvm.org/show_bug.cgi?id=27369
> > +AFLAGS-$(clang)         += -no-integrated-as
> 
> What age is "Older" ?  As soon as these build fixes are in, I will
> refresh my automatic padding calculation series, which will add skip
> with labels to C code as well.

Hm, OK, then we might want to merge patch 5 with this one and move
patch 4 before this one. And use CFLAGS instead of AFLAGS here.

> >  
> >  ALL_OBJS := $(ALL_OBJS-y)
> >  
> > diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
> > index 1dc5c3785a..cee83d392e 100644
> > --- a/xen/arch/x86/Rules.mk
> > +++ b/xen/arch/x86/Rules.mk
> > @@ -44,3 +44,17 @@ endif
> >  
> >  # Set up the assembler include path properly for older toolchains.
> >  CFLAGS += -Wa,-I$(BASEDIR)/include
> > +
> > +ifeq ($(clang),y)
> > +    # Check whether clang asm()-s support .include.
> > +    $(call as-insn-check,CFLAGS,CC,".include 
> > \"asm/indirect_thunk_asm.h\"",, \
> > +                         -no-integrated-as)
> > +    # Check whether clang keeps .macro-s between asm()-s:
> > +    # https://bugs.llvm.org/show_bug.cgi?id=36110
> > +    ifeq ($(if $(shell echo 'void _(void) { asm volatile ( ".macro 
> > FOO\n.endm" ); \
> > +                                            asm volatile ( ".macro 
> > FOO\n.endm" ); }' \
> > +                       | $(CC) $(filter-out -M% %.d -include 
> > %/include/xen/config.h,$(CFLAGS)) \
> > +                               -c -x c -o /dev/null - 2>&1),n,y),y)
> > +        CFLAGS += -no-integrated-as
> 
> This can end up adding -no-integrated-as twice, which would be more
> obvious with as-insn-check renamed to as-insn-add.

-no-integrated-as will only be added once, if it's added in the
as-insn-check above this test will fail (because GNU as will be
used here), and thus the flag won't be added.

Thanks, Roger.

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