Current LLVM LD implementation will turn global hidden symbols in
object files into local ones when generating the .symtab of the Xen
binary image.

This is different from GNU ld implementation, that will only do the
conversion (or remove the symbols) when generation .dynsym but not
.symtab.  Such conversion breaks the processing of symbols done by
tools/symbols.

Use protected symbol visibility instead of hidden, as that preserves
the symbol binding while not generating GOT or PLT indirections that
are not compatible with some of the inline assembly constructs
currently used.

While there also make the visibility setting compiler support
non-optional: compilers not supporting it won't be able to build Xen
anyway, and will just throw a compiler error sooner rather than later
during the build.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/Kconfig                | 4 ----
 xen/include/xen/compiler.h | 9 +++++----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 134e6e68ad..a9182fb13d 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -29,10 +29,6 @@ config LD_IS_GNU
 config LD_IS_LLVM
 	def_bool $(success,$(LD) --version | head -n 1 | grep -q "^LLD")
 
-# -fvisibility=hidden reduces -fpic cost, if it's available
-config CC_HAS_VISIBILITY_ATTRIBUTE
-	def_bool $(cc-option,-fvisibility=hidden)
-
 # Use -f{function,data}-sections compiler parameters
 config CC_SPLIT_SECTIONS
 	bool
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 933aec09a9..c144b17217 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -125,10 +125,11 @@
 #define __must_be_array(a) \
   BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
 
-#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE
-/* Results in more efficient PIC code (no indirections through GOT or PLT). */
-#pragma GCC visibility push(hidden)
-#endif
+/*
+ * Results in more efficient PIC code (no indirections through GOT or PLT)
+ * and is also required by some of the assembly constructs.
+ */
+#pragma GCC visibility push(protected)
 
 /* Make the optimizer believe the variable can be manipulated arbitrarily. */
 #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) )
-- 
2.36.0

Hi,

It seems that this patch has been stale for more than 2 months and from
the discussion in this thread I think some actions need to be taken from the
author. So I am sending this email as a gentle reminder.

Kind regards,
Henry

> -----Original Message-----
> Subject: [PATCH] build/xen: fix symbol generation with LLVM LD
> 
> Current LLVM LD implementation will turn global hidden symbols in
> object files into local ones when generating the .symtab of the Xen
> binary image.
> 
> This is different from GNU ld implementation, that will only do the
> conversion (or remove the symbols) when generation .dynsym but not
> .symtab.  Such conversion breaks the processing of symbols done by
> tools/symbols.
> 
> Use protected symbol visibility instead of hidden, as that preserves
> the symbol binding while not generating GOT or PLT indirections that
> are not compatible with some of the inline assembly constructs
> currently used.
> 
> While there also make the visibility setting compiler support
> non-optional: compilers not supporting it won't be able to build Xen
> anyway, and will just throw a compiler error sooner rather than later
> during the build.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/Kconfig                | 4 ----
>  xen/include/xen/compiler.h | 9 +++++----
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/Kconfig b/xen/Kconfig
> index 134e6e68ad..a9182fb13d 100644
> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -29,10 +29,6 @@ config LD_IS_GNU
>  config LD_IS_LLVM
>  	def_bool $(success,$(LD) --version | head -n 1 | grep -q "^LLD")
> 
> -# -fvisibility=hidden reduces -fpic cost, if it's available
> -config CC_HAS_VISIBILITY_ATTRIBUTE
> -	def_bool $(cc-option,-fvisibility=hidden)
> -
>  # Use -f{function,data}-sections compiler parameters
>  config CC_SPLIT_SECTIONS
>  	bool
> diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
> index 933aec09a9..c144b17217 100644
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -125,10 +125,11 @@
>  #define __must_be_array(a) \
>    BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a),
> typeof(&a[0])))
> 
> -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE
> -/* Results in more efficient PIC code (no indirections through GOT or PLT). */
> -#pragma GCC visibility push(hidden)
> -#endif
> +/*
> + * Results in more efficient PIC code (no indirections through GOT or PLT)
> + * and is also required by some of the assembly constructs.
> + */
> +#pragma GCC visibility push(protected)
> 
>  /* Make the optimizer believe the variable can be manipulated arbitrarily. */
>  #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) )
> --
> 2.36.0
> 

On 05.05.2022 16:21, Roger Pau Monne wrote:
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -125,10 +125,11 @@
>  #define __must_be_array(a) \
>    BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
>  
> -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE
> -/* Results in more efficient PIC code (no indirections through GOT or PLT). */
> -#pragma GCC visibility push(hidden)
> -#endif
> +/*
> + * Results in more efficient PIC code (no indirections through GOT or PLT)
> + * and is also required by some of the assembly constructs.
> + */
> +#pragma GCC visibility push(protected)
>  
>  /* Make the optimizer believe the variable can be manipulated arbitrarily. */
>  #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) )

This has failed my pre-push build test, with massive amounts of errors
about asm() constraints in the alternative call infrastructure. This
was with gcc 11.3.0.

Jan

On Fri, May 06, 2022 at 02:56:56PM +0200, Jan Beulich wrote:
> On 05.05.2022 16:21, Roger Pau Monne wrote:
> > --- a/xen/include/xen/compiler.h
> > +++ b/xen/include/xen/compiler.h
> > @@ -125,10 +125,11 @@
> >  #define __must_be_array(a) \
> >    BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
> >  
> > -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE
> > -/* Results in more efficient PIC code (no indirections through GOT or PLT). */
> > -#pragma GCC visibility push(hidden)
> > -#endif
> > +/*
> > + * Results in more efficient PIC code (no indirections through GOT or PLT)
> > + * and is also required by some of the assembly constructs.
> > + */
> > +#pragma GCC visibility push(protected)
> >  
> >  /* Make the optimizer believe the variable can be manipulated arbitrarily. */
> >  #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) )
> 
> This has failed my pre-push build test, with massive amounts of errors
> about asm() constraints in the alternative call infrastructure. This
> was with gcc 11.3.0.

Hm, great. I guess I will have to use protected with clang and hidden
with gcc then, for lack of a better solution.

I'm slightly confused as to why my godbolt example:

https://godbolt.org/z/chTnMWxeP

Seems to work with gcc 11 then.  I will have to investigate a bit I
think.

Thanks, and sorry for the trouble.

On Fri, May 06, 2022 at 03:31:12PM +0200, Roger Pau Monné wrote:
> On Fri, May 06, 2022 at 02:56:56PM +0200, Jan Beulich wrote:
> > On 05.05.2022 16:21, Roger Pau Monne wrote:
> > > --- a/xen/include/xen/compiler.h
> > > +++ b/xen/include/xen/compiler.h
> > > @@ -125,10 +125,11 @@
> > >  #define __must_be_array(a) \
> > >    BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
> > >  
> > > -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE
> > > -/* Results in more efficient PIC code (no indirections through GOT or PLT). */
> > > -#pragma GCC visibility push(hidden)
> > > -#endif
> > > +/*
> > > + * Results in more efficient PIC code (no indirections through GOT or PLT)
> > > + * and is also required by some of the assembly constructs.
> > > + */
> > > +#pragma GCC visibility push(protected)
> > >  
> > >  /* Make the optimizer believe the variable can be manipulated arbitrarily. */
> > >  #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) )
> > 
> > This has failed my pre-push build test, with massive amounts of errors
> > about asm() constraints in the alternative call infrastructure. This
> > was with gcc 11.3.0.
> 
> Hm, great. I guess I will have to use protected with clang and hidden
> with gcc then, for lack of a better solution.
> 
> I'm slightly confused as to why my godbolt example:
> 
> https://godbolt.org/z/chTnMWxeP
> 
> Seems to work with gcc 11 then.  I will have to investigate a bit I
> think.

So it seems the problem is explicitly with constructs like:

void (*foo)(void);

void test(void)
{
    asm volatile (".long [addr]" :: [addr] "i" (&(foo)));
}

See:

https://godbolt.org/z/TYqeGdWsn

AFAICT gcc will consider the function pointer foo to go through the
GOT/PLT redirection table, while clang will not.  I think gcc behavior
is correct because in theory foo could be set from a different module?
protect only guarantees that references to local functions cannot be
overwritten, but not external ones.

I don't really see a good way to fix this, rather that setting
different visibilities based on the compiler.  clang would use
protected and gcc would use hidden.  I think it's unlikely to have a
toolstack setup to use gcc as the compiler and LLVM LD as the
linker, which would be the problematic configuration, and even in that
case it's kind of a cosmetic issue with symbol resolution, binary
output from the linker would still be correct.

Let me know if that seems acceptable.

Thanks, Roger.

On 06.05.2022 17:35, Roger Pau Monné wrote:
> On Fri, May 06, 2022 at 03:31:12PM +0200, Roger Pau Monné wrote:
>> On Fri, May 06, 2022 at 02:56:56PM +0200, Jan Beulich wrote:
>>> On 05.05.2022 16:21, Roger Pau Monne wrote:
>>>> --- a/xen/include/xen/compiler.h
>>>> +++ b/xen/include/xen/compiler.h
>>>> @@ -125,10 +125,11 @@
>>>>  #define __must_be_array(a) \
>>>>    BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
>>>>  
>>>> -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE
>>>> -/* Results in more efficient PIC code (no indirections through GOT or PLT). */
>>>> -#pragma GCC visibility push(hidden)
>>>> -#endif
>>>> +/*
>>>> + * Results in more efficient PIC code (no indirections through GOT or PLT)
>>>> + * and is also required by some of the assembly constructs.
>>>> + */
>>>> +#pragma GCC visibility push(protected)
>>>>  
>>>>  /* Make the optimizer believe the variable can be manipulated arbitrarily. */
>>>>  #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) )
>>>
>>> This has failed my pre-push build test, with massive amounts of errors
>>> about asm() constraints in the alternative call infrastructure. This
>>> was with gcc 11.3.0.
>>
>> Hm, great. I guess I will have to use protected with clang and hidden
>> with gcc then, for lack of a better solution.
>>
>> I'm slightly confused as to why my godbolt example:
>>
>> https://godbolt.org/z/chTnMWxeP
>>
>> Seems to work with gcc 11 then.  I will have to investigate a bit I
>> think.
> 
> So it seems the problem is explicitly with constructs like:
> 
> void (*foo)(void);
> 
> void test(void)
> {
>     asm volatile (".long [addr]" :: [addr] "i" (&(foo)));
> }
> 
> See:
> 
> https://godbolt.org/z/TYqeGdWsn
> 
> AFAICT gcc will consider the function pointer foo to go through the
> GOT/PLT redirection table, while clang will not.  I think gcc behavior
> is correct because in theory foo could be set from a different module?
> protect only guarantees that references to local functions cannot be
> overwritten, but not external ones.

Right, since there's no way to tell the compiler that the symbol will
be resolved in the same "module".

> I don't really see a good way to fix this, rather that setting
> different visibilities based on the compiler.  clang would use
> protected and gcc would use hidden.

If gcc's behavior is indeed correct, then moving to protected with
clang would set us up for going through GOT/PLT there - either right
away (if the implement this like gcc), or once they correct their
behavior. I don't think we want that. Therefore I think we want to
alter visibility between compilation and linking (i.e. presumably
right in prelink.o), going from compile-time hidden to link-time
protected. That would likely be closer to what your original patch
did (sadly there's no "convert <visibility1> to <visibility2> option
to objcopy, and making it have one wouldn't really help us here;
it's also not clear to me whether llvm comes with its own objcopy,
or whether they re-use GNU's).

>  I think it's unlikely to have a
> toolstack setup to use gcc as the compiler and LLVM LD as the
> linker, which would be the problematic configuration, and even in that
> case it's kind of a cosmetic issue with symbol resolution, binary
> output from the linker would still be correct.

While likely moot with the above, I agree that we could make such an
assumption if need be.

Jan

> Let me know if that seems acceptable.
> 
> Thanks, Roger.
> 

On Sun, May 08, 2022 at 10:34:43AM +0200, Jan Beulich wrote:
> On 06.05.2022 17:35, Roger Pau Monné wrote:
> > On Fri, May 06, 2022 at 03:31:12PM +0200, Roger Pau Monné wrote:
> >> On Fri, May 06, 2022 at 02:56:56PM +0200, Jan Beulich wrote:
> >>> On 05.05.2022 16:21, Roger Pau Monne wrote:
> >>>> --- a/xen/include/xen/compiler.h
> >>>> +++ b/xen/include/xen/compiler.h
> >>>> @@ -125,10 +125,11 @@
> >>>>  #define __must_be_array(a) \
> >>>>    BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
> >>>>  
> >>>> -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE
> >>>> -/* Results in more efficient PIC code (no indirections through GOT or PLT). */
> >>>> -#pragma GCC visibility push(hidden)
> >>>> -#endif
> >>>> +/*
> >>>> + * Results in more efficient PIC code (no indirections through GOT or PLT)
> >>>> + * and is also required by some of the assembly constructs.
> >>>> + */
> >>>> +#pragma GCC visibility push(protected)
> >>>>  
> >>>>  /* Make the optimizer believe the variable can be manipulated arbitrarily. */
> >>>>  #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) )
> >>>
> >>> This has failed my pre-push build test, with massive amounts of errors
> >>> about asm() constraints in the alternative call infrastructure. This
> >>> was with gcc 11.3.0.
> >>
> >> Hm, great. I guess I will have to use protected with clang and hidden
> >> with gcc then, for lack of a better solution.
> >>
> >> I'm slightly confused as to why my godbolt example:
> >>
> >> https://godbolt.org/z/chTnMWxeP
> >>
> >> Seems to work with gcc 11 then.  I will have to investigate a bit I
> >> think.
> > 
> > So it seems the problem is explicitly with constructs like:
> > 
> > void (*foo)(void);
> > 
> > void test(void)
> > {
> >     asm volatile (".long [addr]" :: [addr] "i" (&(foo)));
> > }
> > 
> > See:
> > 
> > https://godbolt.org/z/TYqeGdWsn
> > 
> > AFAICT gcc will consider the function pointer foo to go through the
> > GOT/PLT redirection table, while clang will not.  I think gcc behavior
> > is correct because in theory foo could be set from a different module?
> > protect only guarantees that references to local functions cannot be
> > overwritten, but not external ones.
> 
> Right, since there's no way to tell the compiler that the symbol will
> be resolved in the same "module".
> 
> > I don't really see a good way to fix this, rather that setting
> > different visibilities based on the compiler.  clang would use
> > protected and gcc would use hidden.
> 
> If gcc's behavior is indeed correct, then moving to protected with
> clang would set us up for going through GOT/PLT there - either right
> away (if the implement this like gcc), or once they correct their
> behavior. I don't think we want that. Therefore I think we want to
> alter visibility between compilation and linking (i.e. presumably
> right in prelink.o), going from compile-time hidden to link-time
> protected. That would likely be closer to what your original patch
> did (sadly there's no "convert <visibility1> to <visibility2> option
> to objcopy, and making it have one wouldn't really help us here;
> it's also not clear to me whether llvm comes with its own objcopy,
> or whether they re-use GNU's).

So I've raised the difference in protected behavior between gcc and
clang:

https://discourse.llvm.org/t/gcc-vs-clang-differences-in-protected-visibility-implementation

It's no clear to me whether clang would switch it's implementation,
but it also seems fragile to rely on global protected function
pointers not going through the GOT.

Do you have any recommendation as to how to change symbol visibility?
I've been looking at objcopy, but I don't seem to find a way to do
it.

Thanks, Roger.

On 16.05.2022 10:01, Roger Pau Monné wrote:
> On Sun, May 08, 2022 at 10:34:43AM +0200, Jan Beulich wrote:
>> On 06.05.2022 17:35, Roger Pau Monné wrote:
>>> On Fri, May 06, 2022 at 03:31:12PM +0200, Roger Pau Monné wrote:
>>>> On Fri, May 06, 2022 at 02:56:56PM +0200, Jan Beulich wrote:
>>>>> On 05.05.2022 16:21, Roger Pau Monne wrote:
>>>>>> --- a/xen/include/xen/compiler.h
>>>>>> +++ b/xen/include/xen/compiler.h
>>>>>> @@ -125,10 +125,11 @@
>>>>>>  #define __must_be_array(a) \
>>>>>>    BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
>>>>>>  
>>>>>> -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE
>>>>>> -/* Results in more efficient PIC code (no indirections through GOT or PLT). */
>>>>>> -#pragma GCC visibility push(hidden)
>>>>>> -#endif
>>>>>> +/*
>>>>>> + * Results in more efficient PIC code (no indirections through GOT or PLT)
>>>>>> + * and is also required by some of the assembly constructs.
>>>>>> + */
>>>>>> +#pragma GCC visibility push(protected)
>>>>>>  
>>>>>>  /* Make the optimizer believe the variable can be manipulated arbitrarily. */
>>>>>>  #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) )
>>>>>
>>>>> This has failed my pre-push build test, with massive amounts of errors
>>>>> about asm() constraints in the alternative call infrastructure. This
>>>>> was with gcc 11.3.0.
>>>>
>>>> Hm, great. I guess I will have to use protected with clang and hidden
>>>> with gcc then, for lack of a better solution.
>>>>
>>>> I'm slightly confused as to why my godbolt example:
>>>>
>>>> https://godbolt.org/z/chTnMWxeP
>>>>
>>>> Seems to work with gcc 11 then.  I will have to investigate a bit I
>>>> think.
>>>
>>> So it seems the problem is explicitly with constructs like:
>>>
>>> void (*foo)(void);
>>>
>>> void test(void)
>>> {
>>>     asm volatile (".long [addr]" :: [addr] "i" (&(foo)));
>>> }
>>>
>>> See:
>>>
>>> https://godbolt.org/z/TYqeGdWsn
>>>
>>> AFAICT gcc will consider the function pointer foo to go through the
>>> GOT/PLT redirection table, while clang will not.  I think gcc behavior
>>> is correct because in theory foo could be set from a different module?
>>> protect only guarantees that references to local functions cannot be
>>> overwritten, but not external ones.
>>
>> Right, since there's no way to tell the compiler that the symbol will
>> be resolved in the same "module".
>>
>>> I don't really see a good way to fix this, rather that setting
>>> different visibilities based on the compiler.  clang would use
>>> protected and gcc would use hidden.
>>
>> If gcc's behavior is indeed correct, then moving to protected with
>> clang would set us up for going through GOT/PLT there - either right
>> away (if the implement this like gcc), or once they correct their
>> behavior. I don't think we want that. Therefore I think we want to
>> alter visibility between compilation and linking (i.e. presumably
>> right in prelink.o), going from compile-time hidden to link-time
>> protected. That would likely be closer to what your original patch
>> did (sadly there's no "convert <visibility1> to <visibility2> option
>> to objcopy, and making it have one wouldn't really help us here;
>> it's also not clear to me whether llvm comes with its own objcopy,
>> or whether they re-use GNU's).
> 
> So I've raised the difference in protected behavior between gcc and
> clang:
> 
> https://discourse.llvm.org/t/gcc-vs-clang-differences-in-protected-visibility-implementation
> 
> It's no clear to me whether clang would switch it's implementation,
> but it also seems fragile to rely on global protected function
> pointers not going through the GOT.

I agree, and I don't view it as likely that they would change their
code. GNU objcopy offers a separate step for that conversion
(--localize-hidden), but ..

> Do you have any recommendation as to how to change symbol visibility?
> I've been looking at objcopy, but I don't seem to find a way to do
> it.

... kind of unexpectedly doesn't offer means to alter visibility.
So I guess you/we need to turn back to your original RFC approach,
no matter that it wasn't really pretty.

Jan

On 05.05.2022 16:21, Roger Pau Monne wrote:
> Current LLVM LD implementation will turn global hidden symbols in
> object files into local ones when generating the .symtab of the Xen
> binary image.
> 
> This is different from GNU ld implementation, that will only do the
> conversion (or remove the symbols) when generation .dynsym but not
> .symtab.  Such conversion breaks the processing of symbols done by
> tools/symbols.
> 
> Use protected symbol visibility instead of hidden, as that preserves
> the symbol binding while not generating GOT or PLT indirections that
> are not compatible with some of the inline assembly constructs
> currently used.
> 
> While there also make the visibility setting compiler support
> non-optional: compilers not supporting it won't be able to build Xen
> anyway, and will just throw a compiler error sooner rather than later
> during the build.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>