Re: [Xen-devel] [PATCH for-4.13] x86: re-order clang no integrated assembler tests

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

On Tue, Dec 03, 2019 at 11:03:31AM +0100, Jan Beulich wrote:
> On 02.12.2019 12:29, Roger Pau Monne wrote:
> > The tests to check whether the integrated assembler is capable of
> > building Xen should be performed before testing any assembler
> > features, or else the feature specific tests would be stale if the
> > integrated assembler is disabled afterwards.
> > 
> > Fixes: ef286f67787a ('x86: move and fix clang .skip check')
> 
> Perhaps this change has made the situation worse (and I'm sorry
> for the breakage), but the issue was definitely there before.
> The change above merely added one check to two already present
> ones in the same place.

I agree this was already broken, that change just made things worse
and caused tests to start failing, so I've used the fixes tag in order
to notice this change did restore things to the previous state.

> > --- a/xen/arch/x86/Rules.mk
> > +++ b/xen/arch/x86/Rules.mk
> > @@ -12,6 +12,30 @@ CFLAGS += '-D__OBJECT_LABEL__=$(subst /,$$,$(subst 
> > -,_,$(subst $(BASEDIR)/,,$(CU
> >  # Prevent floating-point variables from creeping into Xen.
> >  CFLAGS += -msoft-float
> >  
> > +ifeq ($(clang),y)
> > +# Note: Any test which adds -no-integrated-as will cause subsequent tests 
> > to
> > +# succeed, and not trigger further additions.
> > +#
> > +# The tests to select whether the integrated assembler is usable need to 
> > happen
> > +# before testing any assembler features, or else the result of the tests 
> > would
> > +# be stale if the integrated assembler is not used.
> > +
> > +# Older clang's built-in assembler doesn't understand .skip with labels:
> > +# https://bugs.llvm.org/show_bug.cgi?id=27369
> > +$(call as-option-add,CFLAGS,CC,".L0: .L1: .skip (.L1 - .L0)",,\
> > +                     -no-integrated-as)
> > +
> > +# Check whether clang asm()-s support .include.
> > +$(call as-option-add,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
> > +$(call as-option-add,CFLAGS,CC,\
> > +                     ".macro FOO;.endm"$$(close); asm volatile 
> > $$(open)".macro FOO;.endm",\
> > +                     -no-integrated-as)
> > +endif
> > +
> >  $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
> >  $(call cc-option-add,CFLAGS,CC,-Wnested-externs)
> >  $(call as-option-add,CFLAGS,CC,"vmcall",-DHAVE_AS_VMX)
> > @@ -70,22 +94,3 @@ endif
> >  # Set up the assembler include path properly for older toolchains.
> >  CFLAGS += -Wa,-I$(BASEDIR)/include
> >  
> > -ifeq ($(clang),y)
> > -# Note: Any test which adds -no-integrated-as will cause subsequent tests 
> > to
> > -# succeed, and not trigger further additions.
> > -
> > -# Older clang's built-in assembler doesn't understand .skip with labels:
> > -# https://bugs.llvm.org/show_bug.cgi?id=27369
> > -$(call as-option-add,CFLAGS,CC,".L0: .L1: .skip (.L1 - .L0)",,\
> > -                     -no-integrated-as)
> > -
> > -# Check whether clang asm()-s support .include.
> > -$(call as-option-add,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
> > -$(call as-option-add,CFLAGS,CC,\
> > -                     ".macro FOO;.endm"$$(close); asm volatile 
> > $$(open)".macro FOO;.endm",\
> > -                     -no-integrated-as)
> > -endif
> 
> Furthermore I think this moving around of logic (which imo
> would better remain at the bottom of the file, well out of
> sight) is only the second best solution to the issue. The
> reason I didn't notice the breakage was because I had noticed
> what made me create the patch in question only while putting
> together a change moving out the majority of the as-option-add
> invocations, primarily with the goal of not having the
> compiler invoked over and over just to calculate CFLAGS. I
> didn't post this change yet simply because I wanted to give it
> some more (local) testing.

Looks like an improvement, but how do you plan to achieve the same?

Are there some compiler/assembler hints available at build time about
which features are supported?

> Another reason to keep this at the bottom of the file is that
> other CFLAGS additions wouldn't have happened yet at the
> place the checks live now.

Right, but it's unlikely that CFLAGS can influence whether the
internal assembler is capable of building Xen or not, while it's IMO
more likely that using the internal or an external assembler can lead
to a different set of CFLAGS (as CFLAGS also include options that
affect the assembler).

> Since there's one as-option-add
> invocation remaining even after my change (the one
> establishing HAVE_AS_QUOTED_SYM, not fitting the model used
> because of the further option additions), I guess the right
> course of action is going to be to move the block back down
> again after my change (hopefully) went in, moving the one
> remaining as-option-add past it at the same time.

As long as assembler options/features are checked for after whether
the internal assembler is suitable or not has been tested it should be
fine.

Roger.

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