Re: [PATCH] build/xen: fix symbol generation with LLVM LD

[Jan Beulich <jbeulich@xxxxxxxx>]

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