Re: [Xen-devel] [PATCH for-4.13] x86: re-order clang no integrated assembler tests
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
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 >
Re: [Xen-devel] [PATCH for-4.13] x86: re-order clang no integrated assembler tests
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
On Mon, Dec 02, 2019 at 11:31:55AM +, 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 > > Reported-by: Doug Goldstein > > signed-off-by: Roger Pau Monné > > --- > > Cc: Juergen Gross > > 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
Re: [Xen-devel] [PATCH for-4.13] x86: re-order clang no integrated assembler tests
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 > Reported-by: Doug Goldstein > signed-off-by: Roger Pau Monné > --- > Cc: Juergen Gross 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