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 08/31/2018 05:11 PM, H.J. Lu wrote:
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>?

I think it's a replacement for <cpuid.h> and lots of manual checking of bits. <cpuid.h> does not have this problem because everything is inlined.

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.

Right, and programmers using the new interface want to know “can I execute an FMA3 instruction?”, which means support for the register file and the instruction itself. So I think we should hide the CPUID details for this interface because they only add confusion and do not provide value.

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.

I'm concerned about this bit, from the commit message:

“
1. x86_get_cpu_kind will get a new version via symbol versioning if a
 new CPU kind is added to cpu_features_kind.
”

I think we can avoid that.

And I'm also not convinced that CPUID is the arbiter of all things. It's really combination of several factors (initial CPUID probing, actual CPUID data fetch, maybe XGETBV), so a register-based interface strikes me as wrong.

The interface also does not really support different probing mechanisms for different CPU vendors, even for features which are exactly the same otherwise.

Thanks,
Florian


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