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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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.

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. 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.

Jan

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