This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: V3 [PATCH] x86: Add <sys/platform/x86.h>


On Fri, Aug 31, 2018 at 7:08 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 08/30/2018 11:20 PM, H.J. Lu wrote:
>>
>> On Thu, Aug 30, 2018 at 12:47 AM, Florian Weimer <fweimer@redhat.com>
>> wrote:
>>>
>>> On 08/29/2018 05:57 PM, H.J. Lu wrote:
>>>
>>>>> CPUID and XGETBV do not need relocations, so they will work where this
>>>>> approach will not.
>>>>
>>>>
>>>>
>>>> Do you have a testcase to show that this claim is true?
>>>
>>>
>>>
>>> I expect the effect to be similar to this bug:
>>>
>>>    <https://bugzilla.redhat.com/show_bug.cgi?id=1377895>
>>>
>>> And this one, which has more analysis:
>>>
>>>    <https://sourceware.org/bugzilla/show_bug.cgi?id=20019>
>>>
>>> This shows that the IFUNC resolvers inside libc itself (which use the
>>> proposed mechanism) are unsafe.
>>
>>
>> This particular issue involves a shared library which references libc.so,
>> but is not linked against libc.so.6.
>>
>> x86_get_arch_feature,  x86_get_cpu_kind and x86_get_cpuid_registers
>> are different since they are provided by ld.so.  They don't have any
>> dependency
>> issues.  I added 2 new testcases to verify that they can be used in shared
>> libraries with or without lazy binding.
>
>
> Isn't there another problem?  The functions you listed have been fully
> relocated, but I don't see anything that ensures that the IFUNC resolver
> which calls these functions has been relocated when it is called.
>
> I think that, collectively, we are still undecided whether an IFUNC resolver
> may rely on run-time relocations.

This issue is orthogonal to <sys/platform/x86.h>.  Why should it block
<sys/platform/x86.h>?

>>>>> You also need to check for XSAVE support, I think.  This is easy to
>>>>> miss,
>>>>> and the proposed interface does not prevent this.
>>>>
>>>>
>>>>
>>>> That is why I added:
>>>>
>>>> @defmac HAS_ARCH_FEATURE(name)
>>>
>>>
>>>
>>> There should only be a single feature to query, to avoid confusion.
>>
>>
>> Good point.  I added more XXX_Usable definitions and updated
>> document.
>
>
> To be frank, I think only the HAS_ARCH_FEATURE should exist. Programmers who
> need the actual CPUID bits can use <cpuid.h> from GCC.

Using <cpuid.h> requires many specific steps.  We have taken all these
steps and information is readily accessible by glibc.  I'd like to make it
available to programmers.  This is one of motivations for this header file.

> I also suggest to use a different interface for the implementation.
> Something like this:
>
>   unsigned int x86_feature_word (int idx)  __attribute__ ((__const__));
>
> This would return 32 feature bits in one call, and the caller would select
> the appropriate bit from the result.
>
> Or we could have a function call which returns the feature bit directly, but
> this could be less efficient if multiple features are queried.
>
> The advantage of doing it this way is that we do not need to add a new
> symbol version when backporting support for new CPU features.
> x86_feature_word would just return zero for unknown indexes.

My implementation doesn't require symbol version either:

x86_get_arch_feature returns 0 if architecture index >=
FEATURE_INDEX_MAX.  This happens when the run-time glibc
doesn't support the newly added feature index.

I don't see any significant difference other than the function name.

-- 
H.J.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]