Re: [Xen-devel] [PATCH 1/2] x86/clang: fix build with indirect thunks

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

On Wed, Jan 24, 2018 at 09:40:40AM -0700, Jan Beulich wrote:
> >>> On 24.01.18 at 16:48, <roger.pau@xxxxxxxxxx> wrote:
> > The build with clang is currently broken because clang requires asm
> > macros to be declared inside the same inline asm declaration where
> > they are used.
> 
> I don't understand this: What if I have two asm()-s needing it? Does
> this need to be done in each one? I'd expect this to result in duplicate
> definitions on gas then (which may or may not be benign).

It's quite fun, this approach works fine with clang regardless of the
number of asm()-s needing it. It doesn't complain about duplicate
macros or anything. OTOH gcc complains with "Error: Macro `foo' was
already defined".

One option might be to guard indirect_thunk_asm.h with:

.ifndef INDIRECT_THUNK_ASM
.equ INDIRECT_THUNK_ASM, 1

...

.endif

Not the best, but should do the trick I guess...

> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -66,8 +66,10 @@ endif
> >  
> >  AFLAGS-y                += -D__ASSEMBLY__
> >  
> > -# Clang's built-in assembler can't handle embedded .include's
> > -CFLAGS-$(clang)         += -no-integrated-as
> > +# Clang's built-in assembler doesn't understand assembler directives 
> > without
> > +# an absolute value:
> > +# https://bugs.llvm.org/show_bug.cgi?id=27369 
> > +AFLAGS-$(clang)         += -no-integrated-as
> 
> I also don't understand this - when you switch (back) to AFLAGS, you
> don't affect C files.

Yes, it affects C files because now they are assembled using the
integrated as, not the external one.

> Furthermore without using its integrated assembler
> for C files at present - how is the build broken?

Not using the integrated as is a workaround for using the indirect
thunk. If we can manage to get the indirect thunk to work with the
integrated as there's no need to use the external one for C files.

> Is the description of
> the change perhaps in need of some re-work (and maybe the title as
> well)?
> 
> Nor am I of the opinion that the comment above is really correct -
> I'm sure there are directives where their assembler supports non-
> constant values (.include being the obvious first case in the context
> here).

Right, clang only accepts constant values for assembler directives
like .rept and .skip.

> And finally, if you switch back to use AFLAGS here, you should
> either restore the original comment as well, or explain in the
> description why it isn't applicable anymore.

I will check, but I'm fairly sure that all the clang versions that Xen
supports (>= 3.5) support .code16/.code32/.code64.

> > --- a/xen/arch/x86/extable.c
> > +++ b/xen/arch/x86/extable.c
> > @@ -158,7 +158,8 @@ static int __init stub_selftest(void)
> >          memcpy(ptr, tests[i].opc, ARRAY_SIZE(tests[i].opc));
> >          unmap_domain_page(ptr);
> >  
> > -        asm volatile ( "INDIRECT_CALL %[stb]\n"
> > +        asm volatile ( INCLUDE_INDIRECT_THUNK
> > +                       "INDIRECT_CALL %[stb]\n"
> 
> Besides this being somewhat ugly, having to remember to add
> this going forward, should any new indirect calls be added, is
> surely prone to be forgotten.

I don't have a better solution ATM I'm afraid.

Roger.

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