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

Roger Pau Monne posted 1 patch 6 days ago
Failed in applying to current master (apply log)
xen/arch/x86/Rules.mk | 43 ++++++++++++++++++++++++-------------------
1 file changed, 24 insertions(+), 19 deletions(-)

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

Posted by Roger Pau Monne 6 days ago
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')
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reported-by: Doug Goldstein <cardoe@cardoe.com>
signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/Rules.mk | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index a3c5eb9de7..92fdbe9d68 100644
--- 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
-- 
2.24.0


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

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

Posted by Jan Beulich 5 days ago
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

Posted by Roger Pau Monné 5 days ago
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

Posted by Jan Beulich 5 days ago
On 03.12.2019 12:04, Roger Pau Monné wrote:
> On Tue, Dec 03, 2019 at 11:03:31AM +0100, Jan Beulich wrote:
>> 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?

No, I'm changing the mechanism altogether. The various HAVE_AS_*
will be put in a generated header file instead. Its generation
(obviously) happens with CFLAGS already in final shape.

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

For simple checks against insns being known I agree. But already
something like

# Set up the assembler include path properly for older toolchains.
CFLAGS += -Wa,-I$(BASEDIR)/include

could make a difference, if a more complex check involved
including some other file.

Jan

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

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

Posted by Andrew Cooper 6 days ago
On 02/12/2019 11: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')
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reported-by: Doug Goldstein <cardoe@cardoe.com>
> signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Juergen Gross <jgross@suse.com>

Why 4.13?  They are only in 4.14 AFAICT

As for the change, looks plausible.  I'll throw it through CI.

~Andrew

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

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

Posted by Roger Pau Monné 6 days ago
On Mon, Dec 02, 2019 at 11:31:55AM +0000, Andrew Cooper wrote:
> On 02/12/2019 11: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')
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Reported-by: Doug Goldstein <cardoe@cardoe.com>
> > signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Juergen Gross <jgross@suse.com>
> 
> Why 4.13?  They are only in 4.14 AFAICT

Oh sorry, I somehow assumed those ended up in 4.13.

> As for the change, looks plausible.  I'll throw it through CI.

I've got one already, which looks good (still in progress, but some
clang tests passed):

https://gitlab.com/xen-project/people/royger/xen/pipelines/99932608

Thanks, Roger.

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