[PATCH v4 0/4] clang fixes

[PATCH v4 0/4] clang fixes

[Roger Pau Monne]

Hello,

The following series re-enable the integrated clang assembler when the
features required in order to build Xen are available.

First 2 patches remove the unconditional addition of -no-integrated-as
to CFLAGS (only adding it to AFLAGS like it was done before). Finally
patches 3 and 4 remove the usage of -no-integrated-as from AFLAGS when
possible.

This series has been tested with clang 6 (without -no-integrated-as),
clang 3.5 and gcc 6.

Thanks, Roger.

Roger Pau Monne (4):
  build: filter out command line assembler arguments
  x86/clang: restore integrated assembler usage with indirect thunks
  x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK
  build/clang: add a check whether the assembler supports .skip with
    labels

 Config.mk                                |  6 +++---
 xen/Rules.mk                             |  9 +++++++--
 xen/arch/x86/Makefile                    |  6 +++---
 xen/arch/x86/Rules.mk                    | 17 ++++++++++++++---
 xen/include/Makefile                     |  2 +-
 xen/include/asm-x86/indirect_thunk_asm.h |  4 ++++
 6 files changed, 32 insertions(+), 12 deletions(-)

-- 
2.16.1


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

[PATCH v4 1/4] build: filter out command line assembler arguments

[Roger Pau Monne]

If the assembler is not used. This happens when using cc -E or cc -S
for example. GCC will just ignore the -Wa,... when the assembler is
not called, but clang will complain loudly and fail.

Also enable passing -Wa,-I$(BASEDIR)/include to clang now that it's
safe to do so.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v3:
 - Filter using '-Wa,%' instead of '-Wa%' so that -Wall is not
   matched.
 - Pass -Wa,-I$(BASEDIR)/include to clang also.
---
 xen/arch/x86/Makefile | 6 +++---
 xen/arch/x86/Rules.mk | 5 +----
 xen/include/Makefile  | 2 +-
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index d903b7abb9..389096139c 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -215,15 +215,15 @@ efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/ef
 efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ;
 
 asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
-	$(CC) $(filter-out -flto,$(CFLAGS)) -S -o $@ $<
+	$(CC) $(filter-out -Wa$(comma)% -flto,$(CFLAGS)) -S -o $@ $<
 
 xen.lds: xen.lds.S
-	$(CC) -P -E -Ui386 $(AFLAGS) -o $@ $<
+	$(CC) -P -E -Ui386 $(filter-out -Wa$(comma)%,$(AFLAGS)) -o $@ $<
 	sed -e 's/xen\.lds\.o:/xen\.lds:/g' <.xen.lds.d >.xen.lds.d.new
 	mv -f .xen.lds.d.new .xen.lds.d
 
 efi.lds: xen.lds.S
-	$(CC) -P -E -Ui386 -DEFI $(AFLAGS) -o $@ $<
+	$(CC) -P -E -Ui386 -DEFI $(filter-out -Wa$(comma)%,$(AFLAGS)) -o $@ $<
 	sed -e 's/efi\.lds\.o:/efi\.lds:/g' <.$(@F).d >.$(@F).d.new
 	mv -f .$(@F).d.new .$(@F).d
 
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 56b2ea8356..1dc5c3785a 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -42,8 +42,5 @@ CFLAGS += -DCONFIG_INDIRECT_THUNK
 export CONFIG_INDIRECT_THUNK=y
 endif
 
-# Set up the assembler include path properly for older GCC toolchains.  Clang
-# objects to the agument being passed however.
-ifneq ($(clang),y)
+# Set up the assembler include path properly for older toolchains.
 CFLAGS += -Wa,-I$(BASEDIR)/include
-endif
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 19066a33a0..69052ade24 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -65,7 +65,7 @@ compat/%.h: compat/%.i Makefile $(BASEDIR)/tools/compat-build-header.py
 	mv -f $@.new $@
 
 compat/%.i: compat/%.c Makefile
-	$(CPP) $(filter-out -M% %.d -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
+	$(CPP) $(filter-out -Wa$(comma)% -M% %.d -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
 
 compat/%.c: public/%.h xlat.lst Makefile $(BASEDIR)/tools/compat-build-source.py
 	mkdir -p $(@D)
-- 
2.16.1


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

Re: [PATCH v4 1/4] build: filter out command line assembler arguments

[Jan Beulich]

>>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> ---
>  xen/arch/x86/Makefile | 6 +++---
>  xen/arch/x86/Rules.mk | 5 +----
>  xen/include/Makefile  | 2 +-
>  3 files changed, 5 insertions(+), 8 deletions(-)

What about the "%.i: %.c", "%.s: %.c", and "%.s: %.S" rules
near the end of xen/Rules.mk?

Jan


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

Re: [PATCH v4 1/4] build: filter out command line assembler arguments

[Roger Pau Monné]

On Mon, Feb 19, 2018 at 08:43:51AM -0700, Jan Beulich wrote:
> >>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> > ---
> >  xen/arch/x86/Makefile | 6 +++---
> >  xen/arch/x86/Rules.mk | 5 +----
> >  xen/include/Makefile  | 2 +-
> >  3 files changed, 5 insertions(+), 8 deletions(-)
> 
> What about the "%.i: %.c", "%.s: %.c", and "%.s: %.S" rules
> near the end of xen/Rules.mk?

Hm, those rules don't seem to be used at all in x86, are they not
needed anymore because there are more specific rules instead?

I can add the filtering there, but maybe it would be better to simply
remove those?

Thanks, Roger.

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

Re: [PATCH v4 1/4] build: filter out command line assembler arguments

[Jan Beulich]

>>> On 19.02.18 at 17:05, <roger.pau@citrix.com> wrote:
> On Mon, Feb 19, 2018 at 08:43:51AM -0700, Jan Beulich wrote:
>> >>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
>> > ---
>> >  xen/arch/x86/Makefile | 6 +++---
>> >  xen/arch/x86/Rules.mk | 5 +----
>> >  xen/include/Makefile  | 2 +-
>> >  3 files changed, 5 insertions(+), 8 deletions(-)
>> 
>> What about the "%.i: %.c", "%.s: %.c", and "%.s: %.S" rules
>> near the end of xen/Rules.mk?
> 
> Hm, those rules don't seem to be used at all in x86, are they not
> needed anymore because there are more specific rules instead?
> 
> I can add the filtering there, but maybe it would be better to simply
> remove those?

No, don't, ever. They're not needed for a _normal_ build, but in case
of build problems they may be needed for analyzing the issue, i.e.
when you need to look at one of the intermediate files. I happen to
use this every once in a while, especially when macro invocations
nest so deeply that the compiler doesn't produce useful location
information anymore.

Jan


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

Re: [PATCH v4 1/4] build: filter out command line assembler arguments

[Roger Pau Monné]

On Mon, Feb 19, 2018 at 09:14:30AM -0700, Jan Beulich wrote:
> >>> On 19.02.18 at 17:05, <roger.pau@citrix.com> wrote:
> > On Mon, Feb 19, 2018 at 08:43:51AM -0700, Jan Beulich wrote:
> >> >>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> >> > ---
> >> >  xen/arch/x86/Makefile | 6 +++---
> >> >  xen/arch/x86/Rules.mk | 5 +----
> >> >  xen/include/Makefile  | 2 +-
> >> >  3 files changed, 5 insertions(+), 8 deletions(-)
> >> 
> >> What about the "%.i: %.c", "%.s: %.c", and "%.s: %.S" rules
> >> near the end of xen/Rules.mk?
> > 
> > Hm, those rules don't seem to be used at all in x86, are they not
> > needed anymore because there are more specific rules instead?
> > 
> > I can add the filtering there, but maybe it would be better to simply
> > remove those?
> 
> No, don't, ever. They're not needed for a _normal_ build, but in case
> of build problems they may be needed for analyzing the issue, i.e.
> when you need to look at one of the intermediate files. I happen to
> use this every once in a while, especially when macro invocations
> nest so deeply that the compiler doesn't produce useful location
> information anymore.

Oh, OK, thanks for the clarification. I'm adding the filtering there
then.

Roger.

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

[PATCH v4 2/4] x86/clang: restore integrated assembler usage with indirect thunks

[Roger Pau Monne]

If the required features are meet by the integrated clang assembler
(support for .includes and propagation of .macro-s between asm()-s)
do not disable it.

Only switch off the non integrated assembler for assembly file, like
it was done prior to "x86: Support indirect thunks from assembly
code".

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
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             |  6 +++---
 xen/Rules.mk          |  5 +++--
 xen/arch/x86/Rules.mk | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/Config.mk b/Config.mk
index 51adc27d83..87472195be 100644
--- a/Config.mk
+++ b/Config.mk
@@ -157,9 +157,9 @@ ifndef XEN_HAS_CHECKPOLICY
 endif
 
 # as-insn: Check whether assembler supports an instruction.
-# Usage: cflags-y += $(call as-insn "insn",option-yes,option-no)
+# Usage: cflags-y += $(call as-insn cc FLAGS,"insn",option-yes,option-no)
 as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \
-                       | $(1) $(filter-out -M% %.d -include %/include/xen/config.h,$(AFLAGS)) \
+                       | $(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
@@ -167,7 +167,7 @@ as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \
 # Usage: $(call as-insn-check CFLAGS,CC,"nop",-DHAVE_GAS_NOP)
 as-insn-check = $(eval $(call as-insn-check-closure,$(1),$(2),$(3),$(4)))
 define as-insn-check-closure
-    ifeq ($$(call as-insn,$$($(2)),$(3),y,n),y)
+    ifeq ($$(call as-insn,$$($(2)) $$(AFLAGS),$(3),y,n),y)
         $(1) += $(4)
     endif
 endef
diff --git a/xen/Rules.mk b/xen/Rules.mk
index da3c35ba36..322e11dba2 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
 
 ALL_OBJS := $(ALL_OBJS-y)
 
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 1dc5c3785a..ceb026dc79 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.
+    ifeq ($(call as-insn,$(CC) $(CFLAGS),".include \"asm/indirect_thunk_asm.h\"",y,n),n)
+        CFLAGS += -no-integrated-as
+    # Check whether clang keeps .macro-s between asm()-s:
+    # https://bugs.llvm.org/show_bug.cgi?id=36110
+    else 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
+    endif
+endif
-- 
2.16.1


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

Re: [PATCH v4 2/4] x86/clang: restore integrated assembler usage with indirect thunks

[Jan Beulich]

>>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> --- a/Config.mk
> +++ b/Config.mk
> @@ -157,9 +157,9 @@ ifndef XEN_HAS_CHECKPOLICY
>  endif
>  
>  # as-insn: Check whether assembler supports an instruction.
> -# Usage: cflags-y += $(call as-insn "insn",option-yes,option-no)
> +# Usage: cflags-y += $(call as-insn cc FLAGS,"insn",option-yes,option-no)

Why lowercase cc and uppercase FLAGS? Along the lines of the
usage comment ahead of as-insn-check both should be uppercase.

> --- 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.
> +    ifeq ($(call as-insn,$(CC) $(CFLAGS),".include \"asm/indirect_thunk_asm.h\"",y,n),n)

Is there anything keeping you from using the slightly less ugly to use
as-insn-check here? Oh, it's apparently that you want to use CFLAGS,
not AFLAGS. I wonder whether the other as-insn-check uses wouldn't
better have CFLAGS passed too. Otherwise please clarify why the
other construct can't be used by extending the comment.

> +        CFLAGS += -no-integrated-as
> +    # Check whether clang keeps .macro-s between asm()-s:
> +    # https://bugs.llvm.org/show_bug.cgi?id=36110 
> +    else ifeq ($(if $(shell echo 'void _(void) { asm volatile ( ".macro FOO\n.endm" ); \

Careful with this; ./README still says "GNU make 3.80 or later" and
iirc 3.80 doesn't support "else if..." on a single line.

Jan


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

Re: [PATCH v4 2/4] x86/clang: restore integrated assembler usage with indirect thunks

[Roger Pau Monné]

On Mon, Feb 19, 2018 at 08:57:15AM -0700, Jan Beulich wrote:
> >>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> > --- 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.
> > +    ifeq ($(call as-insn,$(CC) $(CFLAGS),".include \"asm/indirect_thunk_asm.h\"",y,n),n)
> 
> Is there anything keeping you from using the slightly less ugly to use
> as-insn-check here? Oh, it's apparently that you want to use CFLAGS,
> not AFLAGS. I wonder whether the other as-insn-check uses wouldn't
> better have CFLAGS passed too. Otherwise please clarify why the
> other construct can't be used by extending the comment.

Right, it's because the .include directive is used with asm()-s in C
files so the parameters for building those use CFLAGS, not AFLAGS.

Also as-insn-check only let's you add to a variable in the success
case, and here I need to do the opposite here.

> > +        CFLAGS += -no-integrated-as
> > +    # Check whether clang keeps .macro-s between asm()-s:
> > +    # https://bugs.llvm.org/show_bug.cgi?id=36110 
> > +    else ifeq ($(if $(shell echo 'void _(void) { asm volatile ( ".macro FOO\n.endm" ); \
> 
> Careful with this; ./README still says "GNU make 3.80 or later" and
> iirc 3.80 doesn't support "else if..." on a single line.

Hm, OK, I'm afraid I don't have access to make 3.80, but I can
certainly expand this to be

else
    ifeq
    endif
endif

Thanks, Roger.

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

Re: [PATCH v4 2/4] x86/clang: restore integrated assembler usage with indirect thunks

[Wei Liu]

On Mon, Feb 19, 2018 at 02:16:18PM +0000, Roger Pau Monne wrote:
> If the required features are meet by the integrated clang assembler
                               ^ met

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

[PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Roger Pau Monne]

When indirect_thunk_asm.h is instantiated directly into assembly files
CONFIG_INDIRECT_THUNK might not be defined, and thus using .if against
it is wrong.

Add a check to define CONFIG_INDIRECT_THUNK to 0 if not defined, so
that using .if CONFIG_INDIRECT_THUNK is always correct.

This suppresses the following clang error:

<instantiation>:8:9: error: expected absolute expression
    .if CONFIG_INDIRECT_THUNK == 1
        ^
<instantiation>:1:1: note: while in macro instantiation
INDIRECT_BRANCH call %rdx
^
entry.S:589:9: note: while in macro instantiation
        INDIRECT_CALL %rdx
        ^

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/include/asm-x86/indirect_thunk_asm.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/include/asm-x86/indirect_thunk_asm.h b/xen/include/asm-x86/indirect_thunk_asm.h
index 96bcc25497..ad83986b2d 100644
--- a/xen/include/asm-x86/indirect_thunk_asm.h
+++ b/xen/include/asm-x86/indirect_thunk_asm.h
@@ -3,6 +3,10 @@
  * usual #ifdef'ary to turn into comments.
  */
 
+.ifndef CONFIG_INDIRECT_THUNK
+    .equ CONFIG_INDIRECT_THUNK, 0
+.endif
+
 .macro INDIRECT_BRANCH insn:req arg:req
 /*
  * Create an indirect branch.  insn is one of call/jmp, arg is a single
-- 
2.16.1


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

Re: [PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Jan Beulich]

>>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> When indirect_thunk_asm.h is instantiated directly into assembly files
> CONFIG_INDIRECT_THUNK might not be defined, and thus using .if against
> it is wrong.
> 
> Add a check to define CONFIG_INDIRECT_THUNK to 0 if not defined, so
> that using .if CONFIG_INDIRECT_THUNK is always correct.
> 
> This suppresses the following clang error:
> 
> <instantiation>:8:9: error: expected absolute expression
>     .if CONFIG_INDIRECT_THUNK == 1
>         ^
> <instantiation>:1:1: note: while in macro instantiation
> INDIRECT_BRANCH call %rdx
> ^
> entry.S:589:9: note: while in macro instantiation
>         INDIRECT_CALL %rdx
>         ^
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

As said previously I'm not overly happy about this, but still
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

Re: [PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Jan Beulich]

>>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> --- a/xen/include/asm-x86/indirect_thunk_asm.h
> +++ b/xen/include/asm-x86/indirect_thunk_asm.h
> @@ -3,6 +3,10 @@
>   * usual #ifdef'ary to turn into comments.
>   */
>  
> +.ifndef CONFIG_INDIRECT_THUNK
> +    .equ CONFIG_INDIRECT_THUNK, 0
> +.endif

I have to withdraw my R-b: This fails to build (I'm getting an error
on the .ifndef above for more than one assembler source). I assume
that's because with indirect thunks enabled the constant gets
replaced by literal 1 by the C preprocessor.

Jan


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

Re: [PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Roger Pau Monné]

On Tue, Feb 20, 2018 at 02:15:24AM -0700, Jan Beulich wrote:
> >>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> > --- a/xen/include/asm-x86/indirect_thunk_asm.h
> > +++ b/xen/include/asm-x86/indirect_thunk_asm.h
> > @@ -3,6 +3,10 @@
> >   * usual #ifdef'ary to turn into comments.
> >   */
> >  
> > +.ifndef CONFIG_INDIRECT_THUNK
> > +    .equ CONFIG_INDIRECT_THUNK, 0
> > +.endif
> 
> I have to withdraw my R-b: This fails to build (I'm getting an error
> on the .ifndef above for more than one assembler source). I assume
> that's because with indirect thunks enabled the constant gets
> replaced by literal 1 by the C preprocessor.

Right, I've just installed gcc7-devel and I'm seeing the same. Thanks
for noticing. Let me try to solve this in another way...

Roger.

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

Re: [PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Roger Pau Monné]

On Tue, Feb 20, 2018 at 10:19:16AM +0000, Roger Pau Monné wrote:
> On Tue, Feb 20, 2018 at 02:15:24AM -0700, Jan Beulich wrote:
> > >>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> > > --- a/xen/include/asm-x86/indirect_thunk_asm.h
> > > +++ b/xen/include/asm-x86/indirect_thunk_asm.h
> > > @@ -3,6 +3,10 @@
> > >   * usual #ifdef'ary to turn into comments.
> > >   */
> > >  
> > > +.ifndef CONFIG_INDIRECT_THUNK
> > > +    .equ CONFIG_INDIRECT_THUNK, 0
> > > +.endif
> > 
> > I have to withdraw my R-b: This fails to build (I'm getting an error
> > on the .ifndef above for more than one assembler source). I assume
> > that's because with indirect thunks enabled the constant gets
> > replaced by literal 1 by the C preprocessor.
> 
> Right, I've just installed gcc7-devel and I'm seeing the same. Thanks
> for noticing. Let me try to solve this in another way...

So far the best way I've found to solve this issue that works with
both gcc (with and without indirect thunk support) and clang is the
following:

diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 6fc13d39d8..ebd2c88a1f 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -15,6 +15,9 @@
 #include <asm/alternative.h>

 #ifdef __ASSEMBLY__
+#ifndef CONFIG_INDIRECT_THUNK
+.equ CONFIG_INDIRECT_THUNK, 0
+#endif
 # include <asm/indirect_thunk_asm.h>
 #else
 asm ( "\t.equ CONFIG_INDIRECT_THUNK, "

It's fairly similar to my first approach, but at least "#ifdef
CONFIG_INDIRECT_THUNK" will still work as expected after this fix.

Roger.

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

Re: [PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Jan Beulich]

>>> On 20.02.18 at 12:01, <roger.pau@citrix.com> wrote:
> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -15,6 +15,9 @@
>  #include <asm/alternative.h>
> 
>  #ifdef __ASSEMBLY__
> +#ifndef CONFIG_INDIRECT_THUNK
> +.equ CONFIG_INDIRECT_THUNK, 0
> +#endif
>  # include <asm/indirect_thunk_asm.h>
>  #else
>  asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
> 
> It's fairly similar to my first approach, but at least "#ifdef
> CONFIG_INDIRECT_THUNK" will still work as expected after this fix.

I've used something similar in backports to old versions, so I
wonder whether what you quote above is right: Assembly files
don't get handed to clang anyway iirc, so the change would
seem to be needed in the #else part of the conditional.

Jan


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

Re: [PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Roger Pau Monné]

On Tue, Feb 20, 2018 at 04:13:42AM -0700, Jan Beulich wrote:
> >>> On 20.02.18 at 12:01, <roger.pau@citrix.com> wrote:
> > --- a/xen/include/asm-x86/asm_defns.h
> > +++ b/xen/include/asm-x86/asm_defns.h
> > @@ -15,6 +15,9 @@
> >  #include <asm/alternative.h>
> > 
> >  #ifdef __ASSEMBLY__
> > +#ifndef CONFIG_INDIRECT_THUNK
> > +.equ CONFIG_INDIRECT_THUNK, 0
> > +#endif
> >  # include <asm/indirect_thunk_asm.h>
> >  #else
> >  asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
> > 
> > It's fairly similar to my first approach, but at least "#ifdef
> > CONFIG_INDIRECT_THUNK" will still work as expected after this fix.
> 
> I've used something similar in backports to old versions, so I
> wonder whether what you quote above is right: Assembly files
> don't get handed to clang anyway iirc, so the change would
> seem to be needed in the #else part of the conditional.

Assembly files do get handed to clang, from xen/Rules.mk:

%.o: %.S Makefile
	$(CC) $(AFLAGS) -c $< -o $@

Xen doesn't call as directly to compile those, or am I missing
something?

Thanks, Roger.

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

Re: [PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Jan Beulich]

>>> On 20.02.18 at 12:22, <roger.pau@citrix.com> wrote:
> On Tue, Feb 20, 2018 at 04:13:42AM -0700, Jan Beulich wrote:
>> >>> On 20.02.18 at 12:01, <roger.pau@citrix.com> wrote:
>> > --- a/xen/include/asm-x86/asm_defns.h
>> > +++ b/xen/include/asm-x86/asm_defns.h
>> > @@ -15,6 +15,9 @@
>> >  #include <asm/alternative.h>
>> > 
>> >  #ifdef __ASSEMBLY__
>> > +#ifndef CONFIG_INDIRECT_THUNK
>> > +.equ CONFIG_INDIRECT_THUNK, 0
>> > +#endif
>> >  # include <asm/indirect_thunk_asm.h>
>> >  #else
>> >  asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
>> > 
>> > It's fairly similar to my first approach, but at least "#ifdef
>> > CONFIG_INDIRECT_THUNK" will still work as expected after this fix.
>> 
>> I've used something similar in backports to old versions, so I
>> wonder whether what you quote above is right: Assembly files
>> don't get handed to clang anyway iirc, so the change would
>> seem to be needed in the #else part of the conditional.
> 
> Assembly files do get handed to clang, from xen/Rules.mk:
> 
> %.o: %.S Makefile
> 	$(CC) $(AFLAGS) -c $< -o $@
> 
> Xen doesn't call as directly to compile those, or am I missing
> something?

Oh, I was referring to the -mno-integrated-as (or whatever it's
called) option. "Handed to clang" wasn't the best way to put it,
I agree. But it's clang's integrated assembler which the problem
is with.

Jan


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

Re: [PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Roger Pau Monné]

On Tue, Feb 20, 2018 at 05:32:17AM -0700, Jan Beulich wrote:
> >>> On 20.02.18 at 12:22, <roger.pau@citrix.com> wrote:
> > On Tue, Feb 20, 2018 at 04:13:42AM -0700, Jan Beulich wrote:
> >> >>> On 20.02.18 at 12:01, <roger.pau@citrix.com> wrote:
> >> > --- a/xen/include/asm-x86/asm_defns.h
> >> > +++ b/xen/include/asm-x86/asm_defns.h
> >> > @@ -15,6 +15,9 @@
> >> >  #include <asm/alternative.h>
> >> > 
> >> >  #ifdef __ASSEMBLY__
> >> > +#ifndef CONFIG_INDIRECT_THUNK
> >> > +.equ CONFIG_INDIRECT_THUNK, 0
> >> > +#endif
> >> >  # include <asm/indirect_thunk_asm.h>
> >> >  #else
> >> >  asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
> >> > 
> >> > It's fairly similar to my first approach, but at least "#ifdef
> >> > CONFIG_INDIRECT_THUNK" will still work as expected after this fix.
> >> 
> >> I've used something similar in backports to old versions, so I
> >> wonder whether what you quote above is right: Assembly files
> >> don't get handed to clang anyway iirc, so the change would
> >> seem to be needed in the #else part of the conditional.
> > 
> > Assembly files do get handed to clang, from xen/Rules.mk:
> > 
> > %.o: %.S Makefile
> > 	$(CC) $(AFLAGS) -c $< -o $@
> > 
> > Xen doesn't call as directly to compile those, or am I missing
> > something?
> 
> Oh, I was referring to the -mno-integrated-as (or whatever it's
> called) option. "Handed to clang" wasn't the best way to put it,
> I agree. But it's clang's integrated assembler which the problem
> is with.

Oh, I see. You are right, this is just a preparatory change for patch
4, which does enable clang's integrated assembler if the right
features are present.

Let me know if you think the above chunk is acceptable.

Thanks, Roger.

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

Re: [PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Jan Beulich]

>>> On 20.02.18 at 13:42, <roger.pau@citrix.com> wrote:
> On Tue, Feb 20, 2018 at 05:32:17AM -0700, Jan Beulich wrote:
>> >>> On 20.02.18 at 12:22, <roger.pau@citrix.com> wrote:
>> > On Tue, Feb 20, 2018 at 04:13:42AM -0700, Jan Beulich wrote:
>> >> >>> On 20.02.18 at 12:01, <roger.pau@citrix.com> wrote:
>> >> > --- a/xen/include/asm-x86/asm_defns.h
>> >> > +++ b/xen/include/asm-x86/asm_defns.h
>> >> > @@ -15,6 +15,9 @@
>> >> >  #include <asm/alternative.h>
>> >> > 
>> >> >  #ifdef __ASSEMBLY__
>> >> > +#ifndef CONFIG_INDIRECT_THUNK
>> >> > +.equ CONFIG_INDIRECT_THUNK, 0
>> >> > +#endif
>> >> >  # include <asm/indirect_thunk_asm.h>
>> >> >  #else
>> >> >  asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
>> >> > 
>> >> > It's fairly similar to my first approach, but at least "#ifdef
>> >> > CONFIG_INDIRECT_THUNK" will still work as expected after this fix.
>> >> 
>> >> I've used something similar in backports to old versions, so I
>> >> wonder whether what you quote above is right: Assembly files
>> >> don't get handed to clang anyway iirc, so the change would
>> >> seem to be needed in the #else part of the conditional.
>> > 
>> > Assembly files do get handed to clang, from xen/Rules.mk:
>> > 
>> > %.o: %.S Makefile
>> > 	$(CC) $(AFLAGS) -c $< -o $@
>> > 
>> > Xen doesn't call as directly to compile those, or am I missing
>> > something?
>> 
>> Oh, I was referring to the -mno-integrated-as (or whatever it's
>> called) option. "Handed to clang" wasn't the best way to put it,
>> I agree. But it's clang's integrated assembler which the problem
>> is with.
> 
> Oh, I see. You are right, this is just a preparatory change for patch
> 4, which does enable clang's integrated assembler if the right
> features are present.
> 
> Let me know if you think the above chunk is acceptable.

It is certainly acceptable if it helps; you now apparently agreeing
I'm more confused though by this question. Iirc you don't enable
the integrated assembler for assembly sources, but only for C ones.

Jan


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

Re: [PATCH v4 3/4] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Roger Pau Monné]

On Tue, Feb 20, 2018 at 05:53:08AM -0700, Jan Beulich wrote:
> >>> On 20.02.18 at 13:42, <roger.pau@citrix.com> wrote:
> > On Tue, Feb 20, 2018 at 05:32:17AM -0700, Jan Beulich wrote:
> >> >>> On 20.02.18 at 12:22, <roger.pau@citrix.com> wrote:
> >> > On Tue, Feb 20, 2018 at 04:13:42AM -0700, Jan Beulich wrote:
> >> >> >>> On 20.02.18 at 12:01, <roger.pau@citrix.com> wrote:
> >> >> > --- a/xen/include/asm-x86/asm_defns.h
> >> >> > +++ b/xen/include/asm-x86/asm_defns.h
> >> >> > @@ -15,6 +15,9 @@
> >> >> >  #include <asm/alternative.h>
> >> >> > 
> >> >> >  #ifdef __ASSEMBLY__
> >> >> > +#ifndef CONFIG_INDIRECT_THUNK
> >> >> > +.equ CONFIG_INDIRECT_THUNK, 0
> >> >> > +#endif
> >> >> >  # include <asm/indirect_thunk_asm.h>
> >> >> >  #else
> >> >> >  asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
> >> >> > 
> >> >> > It's fairly similar to my first approach, but at least "#ifdef
> >> >> > CONFIG_INDIRECT_THUNK" will still work as expected after this fix.
> >> >> 
> >> >> I've used something similar in backports to old versions, so I
> >> >> wonder whether what you quote above is right: Assembly files
> >> >> don't get handed to clang anyway iirc, so the change would
> >> >> seem to be needed in the #else part of the conditional.
> >> > 
> >> > Assembly files do get handed to clang, from xen/Rules.mk:
> >> > 
> >> > %.o: %.S Makefile
> >> > 	$(CC) $(AFLAGS) -c $< -o $@
> >> > 
> >> > Xen doesn't call as directly to compile those, or am I missing
> >> > something?
> >> 
> >> Oh, I was referring to the -mno-integrated-as (or whatever it's
> >> called) option. "Handed to clang" wasn't the best way to put it,
> >> I agree. But it's clang's integrated assembler which the problem
> >> is with.
> > 
> > Oh, I see. You are right, this is just a preparatory change for patch
> > 4, which does enable clang's integrated assembler if the right
> > features are present.
> > 
> > Let me know if you think the above chunk is acceptable.
> 
> It is certainly acceptable if it helps; you now apparently agreeing
> I'm more confused though by this question. Iirc you don't enable
> the integrated assembler for assembly sources, but only for C ones.

So at this point the integrated assembler is only enabled for C
sources.

In patch 4 I enable the integrated assembler for everything if
possible, but in order to enable it I need to solve this issue
first. I can merge patches 3 and 4 if it's going to be clearer.

Hope this help shed some light.

Thanks, Roger.

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

[PATCH v4 4/4] build/clang: add a check whether the assembler supports .skip with labels

[Roger Pau Monne]

Or else switch off the integrated assembler. This is relevant for
older clang versions which integrated assembler don't support .skip
with labels.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/Rules.mk | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index 322e11dba2..415b363859 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -72,7 +72,11 @@ AFLAGS-y                += -D__ASSEMBLY__
 
 # 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
+ifeq ($(clang),y)
+ifeq ($(call as-insn,$(CC) $(AFLAGS),".L0:\n.L1:\n.skip (.L1 - .L0)",y,n),n)
+        AFLAGS += -no-integrated-as
+endif
+endif
 
 ALL_OBJS := $(ALL_OBJS-y)
 
-- 
2.16.1


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

Re: [PATCH v4 4/4] build/clang: add a check whether the assembler supports .skip with labels

[Jan Beulich]

>>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -72,7 +72,11 @@ AFLAGS-y                += -D__ASSEMBLY__
>  
>  # 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
> +ifeq ($(clang),y)
> +ifeq ($(call as-insn,$(CC) $(AFLAGS),".L0:\n.L1:\n.skip (.L1 - .L0)",y,n),n)

Hmm, here you use AFLAGS, so why not as-insn-check?

Jan


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

Re: [PATCH v4 4/4] build/clang: add a check whether the assembler supports .skip with labels

[Roger Pau Monné]

On Mon, Feb 19, 2018 at 09:10:42AM -0700, Jan Beulich wrote:
> >>> On 19.02.18 at 15:16, <roger.pau@citrix.com> wrote:
> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -72,7 +72,11 @@ AFLAGS-y                += -D__ASSEMBLY__
> >  
> >  # 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
> > +ifeq ($(clang),y)
> > +ifeq ($(call as-insn,$(CC) $(AFLAGS),".L0:\n.L1:\n.skip (.L1 - .L0)",y,n),n)
> 
> Hmm, here you use AFLAGS, so why not as-insn-check?

Because as-insn-check only lets you add to a variable on the success
case (ie: when as-insn returns 'y'), and here I need to do the
opposite.

Thanks, Roger.

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