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

[Jan Beulich <jbeulich@xxxxxxxx>]

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