diff mbox series

[1/4] x86: Initialize CPU info via IFUNC relocation [BZ 26203]

Message ID 20200918160709.949608-2-hjl.tools@gmail.com
State Superseded
Headers show
Series [1/4] x86: Initialize CPU info via IFUNC relocation [BZ 26203] | expand

Commit Message

H.J. Lu Sept. 18, 2020, 4:07 p.m. UTC
X86 CPU features in ld.so are initialized by init_cpu_features, which is
invoked by DL_PLATFORM_INIT from _dl_sysdep_start.  But when ld.so is
loaded by static executable, DL_PLATFORM_INIT is never called.  Also
x86 cache info in libc.o and libc.a is initialized by a constructor
which may be called too late.  Since _rtld_global_ro in ld.so is
initialized by dynamic relocation,  we can also initialize x86 CPU
features in _rtld_global_ro in ld.so and cache info in libc.so by
initializing dummy function pointers in ld.so and libc.so via IFUNC
relocation.

Note: _dl_x86_init_cpu_features can be called more than once from
DL_PLATFORM_INIT and during relocation in ld.so.
---
 sysdeps/i386/dl-machine.h          |  3 +--
 sysdeps/x86/cacheinfo.c            | 10 +++++++++-
 sysdeps/x86/dl-get-cpu-features.c  | 25 ++++++++++++++++++++++++-
 sysdeps/x86/include/cpu-features.h |  1 +
 sysdeps/x86_64/dl-machine.h        |  3 +--
 5 files changed, 36 insertions(+), 6 deletions(-)

Comments

Florian Weimer Sept. 28, 2020, 1:08 p.m. UTC | #1
* H. J. Lu via Libc-alpha:

> X86 CPU features in ld.so are initialized by init_cpu_features, which is
> invoked by DL_PLATFORM_INIT from _dl_sysdep_start.  But when ld.so is
> loaded by static executable, DL_PLATFORM_INIT is never called.  Also
> x86 cache info in libc.o and libc.a is initialized by a constructor
> which may be called too late.  Since _rtld_global_ro in ld.so is
> initialized by dynamic relocation,  we can also initialize x86 CPU
> features in _rtld_global_ro in ld.so and cache info in libc.so by
> initializing dummy function pointers in ld.so and libc.so via IFUNC
> relocation.

_rtld_global_ro is *partially* initialized by relocation.  Most of it is
not initialized (see the need for rtld_active).

Please make this a little bit clearer in the commit message.

> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> index 0f08079e48..5e22e795cc 100644
> --- a/sysdeps/i386/dl-machine.h
> +++ b/sysdeps/i386/dl-machine.h
> @@ -25,7 +25,6 @@
>  #include <sysdep.h>
>  #include <tls.h>
>  #include <dl-tlsdesc.h>
> -#include <cpu-features.c>
>  
>  /* Return nonzero iff ELF header is compatible with the running host.  */
>  static inline int __attribute__ ((unused))
> @@ -250,7 +249,7 @@ dl_platform_init (void)
>  #if IS_IN (rtld)
>    /* init_cpu_features has been called early from __libc_start_main in
>       static executable.  */
> -  init_cpu_features (&GLRO(dl_x86_cpu_features));
> +  _dl_x86_init_cpu_features ();

Is the commented outdated?

> diff --git a/sysdeps/x86/cacheinfo.c b/sysdeps/x86/cacheinfo.c
> index 217c21c34f..7a325ab70e 100644
> --- a/sysdeps/x86/cacheinfo.c
> +++ b/sysdeps/x86/cacheinfo.c

> +  assert (cpu_features->basic.kind != arch_kind_unknown);

Why doesn't this assert fire occasionally?  How do you ensure that
relocation processing is correctly ordered?

> +/* NB: Call init_cacheinfo by initializing a dummy function pointer via
> +   IFUNC relocation.  */
> +extern void __x86_cacheinfo (void) attribute_hidden;
> +const void (*__x86_cacheinfo_p) (void) attribute_hidden
> +  = __x86_cacheinfo;
> +
> +__ifunc (__x86_cacheinfo, __x86_cacheinfo, NULL, void, init_cacheinfo);
>  #endif

Please expand the comment and mention that is used to initialize the
dormant copy of ld.so after static dlopen.  The comment in
sysdeps/x86/dl-get-cpu-features.c is good.

> diff --git a/sysdeps/x86/dl-get-cpu-features.c b/sysdeps/x86/dl-get-cpu-features.c
> index 5f9e46b0c6..da4697b895 100644
> --- a/sysdeps/x86/dl-get-cpu-features.c
> +++ b/sysdeps/x86/dl-get-cpu-features.c
> @@ -1,4 +1,4 @@
> -/* This file is part of the GNU C Library.
> +/* Initialize CPU feature data via IFUNC relocation.
>     Copyright (C) 2015-2020 Free Software Foundation, Inc.
>  
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -18,6 +18,29 @@
>  
>  #include <ldsodefs.h>
>  
> +#ifdef SHARED
> +# include <cpu-features.c>
> +
> +/* NB: Normally, DL_PLATFORM_INIT calls init_cpu_features to initialize
> +   CPU features.  But when loading ld.so inside of static executable,
> +   DL_PLATFORM_INIT isn't called.  Call init_cpu_features by initializing
> +   a dummy function pointer via IFUNC relocation for ld.so.  */
> +extern void __x86_cpu_features (void) attribute_hidden;
> +const void (*__x86_cpu_features_p) (void) attribute_hidden
> +  = __x86_cpu_features;
> +
> +void
> +_dl_x86_init_cpu_features (void)
> +{
> +  struct cpu_features *cpu_features = __get_cpu_features ();
> +  if (cpu_features->basic.kind == arch_kind_unknown)
> +    init_cpu_features (cpu_features);
> +}
> +
> +__ifunc (__x86_cpu_features, __x86_cpu_features, NULL, void,
> +	 _dl_x86_init_cpu_features);
> +#endif
> +
>  #undef __x86_get_cpu_features

Why do we need both the conditional check and the function pointer hack?

I expect that one of the function pointers can go, probably the one
here.  The cache hierarchy data might be used by a string function that
has not been selected by IFUNC.

Thanks,
Florian
H.J. Lu Sept. 28, 2020, 1:48 p.m. UTC | #2
On Mon, Sep 28, 2020 at 6:08 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu via Libc-alpha:
>
> > X86 CPU features in ld.so are initialized by init_cpu_features, which is
> > invoked by DL_PLATFORM_INIT from _dl_sysdep_start.  But when ld.so is
> > loaded by static executable, DL_PLATFORM_INIT is never called.  Also
> > x86 cache info in libc.o and libc.a is initialized by a constructor
> > which may be called too late.  Since _rtld_global_ro in ld.so is
> > initialized by dynamic relocation,  we can also initialize x86 CPU
> > features in _rtld_global_ro in ld.so and cache info in libc.so by
> > initializing dummy function pointers in ld.so and libc.so via IFUNC
> > relocation.
>
> _rtld_global_ro is *partially* initialized by relocation.  Most of it is
> not initialized (see the need for rtld_active).
>
> Please make this a little bit clearer in the commit message.

Will do

> > diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> > index 0f08079e48..5e22e795cc 100644
> > --- a/sysdeps/i386/dl-machine.h
> > +++ b/sysdeps/i386/dl-machine.h
> > @@ -25,7 +25,6 @@
> >  #include <sysdep.h>
> >  #include <tls.h>
> >  #include <dl-tlsdesc.h>
> > -#include <cpu-features.c>
> >
> >  /* Return nonzero iff ELF header is compatible with the running host.  */
> >  static inline int __attribute__ ((unused))
> > @@ -250,7 +249,7 @@ dl_platform_init (void)
> >  #if IS_IN (rtld)
> >    /* init_cpu_features has been called early from __libc_start_main in
> >       static executable.  */
> > -  init_cpu_features (&GLRO(dl_x86_cpu_features));
> > +  _dl_x86_init_cpu_features ();
>
> Is the commented outdated?

I will fix it.

> > diff --git a/sysdeps/x86/cacheinfo.c b/sysdeps/x86/cacheinfo.c
> > index 217c21c34f..7a325ab70e 100644
> > --- a/sysdeps/x86/cacheinfo.c
> > +++ b/sysdeps/x86/cacheinfo.c
>
> > +  assert (cpu_features->basic.kind != arch_kind_unknown);
>
> Why doesn't this assert fire occasionally?  How do you ensure that

See

https://sourceware.org/bugzilla/show_bug.cgi?id=26203

It only happens in dlopen from a static executable.

> relocation processing is correctly ordered?

cpu_features is also initialized by IFUNC relocation in ld.so which
is relocated before libc.so.

> > +/* NB: Call init_cacheinfo by initializing a dummy function pointer via
> > +   IFUNC relocation.  */
> > +extern void __x86_cacheinfo (void) attribute_hidden;
> > +const void (*__x86_cacheinfo_p) (void) attribute_hidden
> > +  = __x86_cacheinfo;
> > +
> > +__ifunc (__x86_cacheinfo, __x86_cacheinfo, NULL, void, init_cacheinfo);
> >  #endif
>
> Please expand the comment and mention that is used to initialize the
> dormant copy of ld.so after static dlopen.  The comment in
> sysdeps/x86/dl-get-cpu-features.c is good.

Will do.

> > diff --git a/sysdeps/x86/dl-get-cpu-features.c b/sysdeps/x86/dl-get-cpu-features.c
> > index 5f9e46b0c6..da4697b895 100644
> > --- a/sysdeps/x86/dl-get-cpu-features.c
> > +++ b/sysdeps/x86/dl-get-cpu-features.c
> > @@ -1,4 +1,4 @@
> > -/* This file is part of the GNU C Library.
> > +/* Initialize CPU feature data via IFUNC relocation.
> >     Copyright (C) 2015-2020 Free Software Foundation, Inc.
> >
> >     The GNU C Library is free software; you can redistribute it and/or
> > @@ -18,6 +18,29 @@
> >
> >  #include <ldsodefs.h>
> >
> > +#ifdef SHARED
> > +# include <cpu-features.c>
> > +
> > +/* NB: Normally, DL_PLATFORM_INIT calls init_cpu_features to initialize
> > +   CPU features.  But when loading ld.so inside of static executable,
> > +   DL_PLATFORM_INIT isn't called.  Call init_cpu_features by initializing
> > +   a dummy function pointer via IFUNC relocation for ld.so.  */
> > +extern void __x86_cpu_features (void) attribute_hidden;
> > +const void (*__x86_cpu_features_p) (void) attribute_hidden
> > +  = __x86_cpu_features;
> > +
> > +void
> > +_dl_x86_init_cpu_features (void)
> > +{
> > +  struct cpu_features *cpu_features = __get_cpu_features ();
> > +  if (cpu_features->basic.kind == arch_kind_unknown)
> > +    init_cpu_features (cpu_features);
> > +}
> > +
> > +__ifunc (__x86_cpu_features, __x86_cpu_features, NULL, void,
> > +      _dl_x86_init_cpu_features);
> > +#endif
> > +
> >  #undef __x86_get_cpu_features
>
> Why do we need both the conditional check and the function pointer hack?

Because _dl_x86_init_cpu_features is called both indirectly and by IFUNC
reloc in dynamic executable, but it is only called by IFUNC reloc when
dlopen in static executable.

> I expect that one of the function pointers can go, probably the one
> here.  The cache hierarchy data might be used by a string function that
> has not been selected by IFUNC.
>

There are one IFUNC reloc in ld.so and the other in libc.so.  We need
both.
Florian Weimer Sept. 28, 2020, 2:05 p.m. UTC | #3
* H. J. Lu:

>> > diff --git a/sysdeps/x86/cacheinfo.c b/sysdeps/x86/cacheinfo.c
>> > index 217c21c34f..7a325ab70e 100644
>> > --- a/sysdeps/x86/cacheinfo.c
>> > +++ b/sysdeps/x86/cacheinfo.c
>>
>> > +  assert (cpu_features->basic.kind != arch_kind_unknown);
>>
>> Why doesn't this assert fire occasionally?  How do you ensure that
>
> See
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=26203
>
> It only happens in dlopen from a static executable.

Sorry, I don't understand how this answers my question.

Do you mean that for the non-static case, initialization has already
happened.

>> relocation processing is correctly ordered?
>
> cpu_features is also initialized by IFUNC relocation in ld.so which
> is relocated before libc.so.

Is that really true in all cases?  Even if libc.so is preloaded?
(Static dlopen probably ignores LD_PRELOAD.)

Maybe put this information as a comment next to the assert?

But since cacheinfo.os is linked into libc.so, I don't really think the
assert is correct.

>> > diff --git a/sysdeps/x86/dl-get-cpu-features.c b/sysdeps/x86/dl-get-cpu-features.c
>> > index 5f9e46b0c6..da4697b895 100644
>> > --- a/sysdeps/x86/dl-get-cpu-features.c
>> > +++ b/sysdeps/x86/dl-get-cpu-features.c
>> > @@ -1,4 +1,4 @@
>> > -/* This file is part of the GNU C Library.
>> > +/* Initialize CPU feature data via IFUNC relocation.
>> >     Copyright (C) 2015-2020 Free Software Foundation, Inc.
>> >
>> >     The GNU C Library is free software; you can redistribute it and/or
>> > @@ -18,6 +18,29 @@
>> >
>> >  #include <ldsodefs.h>
>> >
>> > +#ifdef SHARED
>> > +# include <cpu-features.c>
>> > +
>> > +/* NB: Normally, DL_PLATFORM_INIT calls init_cpu_features to initialize
>> > +   CPU features.  But when loading ld.so inside of static executable,
>> > +   DL_PLATFORM_INIT isn't called.  Call init_cpu_features by initializing
>> > +   a dummy function pointer via IFUNC relocation for ld.so.  */
>> > +extern void __x86_cpu_features (void) attribute_hidden;
>> > +const void (*__x86_cpu_features_p) (void) attribute_hidden
>> > +  = __x86_cpu_features;
>> > +
>> > +void
>> > +_dl_x86_init_cpu_features (void)
>> > +{
>> > +  struct cpu_features *cpu_features = __get_cpu_features ();
>> > +  if (cpu_features->basic.kind == arch_kind_unknown)
>> > +    init_cpu_features (cpu_features);
>> > +}
>> > +
>> > +__ifunc (__x86_cpu_features, __x86_cpu_features, NULL, void,
>> > +      _dl_x86_init_cpu_features);
>> > +#endif
>> > +
>> >  #undef __x86_get_cpu_features
>>
>> Why do we need both the conditional check and the function pointer hack?
>
> Because _dl_x86_init_cpu_features is called both indirectly and by IFUNC
> reloc in dynamic executable, but it is only called by IFUNC reloc when
> dlopen in static executable.

I think we always need to call it eventually, as a dependency of filling
in the cacheinfo data?

>> I expect that one of the function pointers can go, probably the one
>> here.  The cache hierarchy data might be used by a string function that
>> has not been selected by IFUNC.
>>
>
> There are one IFUNC reloc in ld.so and the other in libc.so.  We need
> both.

libc.so should not need the relocation hack because we have
__libc_early_init, which is also called after static dlopen and before
constructors.

Thanks,
Florian
H.J. Lu Sept. 28, 2020, 2:20 p.m. UTC | #4
On Mon, Sep 28, 2020 at 7:05 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> >> > diff --git a/sysdeps/x86/cacheinfo.c b/sysdeps/x86/cacheinfo.c
> >> > index 217c21c34f..7a325ab70e 100644
> >> > --- a/sysdeps/x86/cacheinfo.c
> >> > +++ b/sysdeps/x86/cacheinfo.c
> >>
> >> > +  assert (cpu_features->basic.kind != arch_kind_unknown);
> >>
> >> Why doesn't this assert fire occasionally?  How do you ensure that
> >
> > See
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=26203
> >
> > It only happens in dlopen from a static executable.
>
> Sorry, I don't understand how this answers my question.
>
> Do you mean that for the non-static case, initialization has already
> happened.

Yes, it is initialized by DL_PLATFORM_INIT in a dynamic executable.

> >> relocation processing is correctly ordered?
> >
> > cpu_features is also initialized by IFUNC relocation in ld.so which
> > is relocated before libc.so.
>
> Is that really true in all cases?  Even if libc.so is preloaded?

Not if init_cacheinfo is a constructor function.

> (Static dlopen probably ignores LD_PRELOAD.)

Correct.

> Maybe put this information as a comment next to the assert?

Will do.

> But since cacheinfo.os is linked into libc.so, I don't really think the
> assert is correct.

When init_cacheinfo is called, cpu_features must be initialized.

> >> > diff --git a/sysdeps/x86/dl-get-cpu-features.c b/sysdeps/x86/dl-get-cpu-features.c
> >> > index 5f9e46b0c6..da4697b895 100644
> >> > --- a/sysdeps/x86/dl-get-cpu-features.c
> >> > +++ b/sysdeps/x86/dl-get-cpu-features.c
> >> > @@ -1,4 +1,4 @@
> >> > -/* This file is part of the GNU C Library.
> >> > +/* Initialize CPU feature data via IFUNC relocation.
> >> >     Copyright (C) 2015-2020 Free Software Foundation, Inc.
> >> >
> >> >     The GNU C Library is free software; you can redistribute it and/or
> >> > @@ -18,6 +18,29 @@
> >> >
> >> >  #include <ldsodefs.h>
> >> >
> >> > +#ifdef SHARED
> >> > +# include <cpu-features.c>
> >> > +
> >> > +/* NB: Normally, DL_PLATFORM_INIT calls init_cpu_features to initialize
> >> > +   CPU features.  But when loading ld.so inside of static executable,
> >> > +   DL_PLATFORM_INIT isn't called.  Call init_cpu_features by initializing
> >> > +   a dummy function pointer via IFUNC relocation for ld.so.  */
> >> > +extern void __x86_cpu_features (void) attribute_hidden;
> >> > +const void (*__x86_cpu_features_p) (void) attribute_hidden
> >> > +  = __x86_cpu_features;
> >> > +
> >> > +void
> >> > +_dl_x86_init_cpu_features (void)
> >> > +{
> >> > +  struct cpu_features *cpu_features = __get_cpu_features ();
> >> > +  if (cpu_features->basic.kind == arch_kind_unknown)
> >> > +    init_cpu_features (cpu_features);
> >> > +}
> >> > +
> >> > +__ifunc (__x86_cpu_features, __x86_cpu_features, NULL, void,
> >> > +      _dl_x86_init_cpu_features);
> >> > +#endif
> >> > +
> >> >  #undef __x86_get_cpu_features
> >>
> >> Why do we need both the conditional check and the function pointer hack?
> >
> > Because _dl_x86_init_cpu_features is called both indirectly and by IFUNC
> > reloc in dynamic executable, but it is only called by IFUNC reloc when
> > dlopen in static executable.
>
> I think we always need to call it eventually, as a dependency of filling
> in the cacheinfo data?

Yes.  _dl_x86_init_cpu_features is called twice by DL_PLATFORM_INIT
and IFUNC reloc in dynamic executable.  It is called once by IFUNC reloc
via dlopen in static executable.

> >> I expect that one of the function pointers can go, probably the one
> >> here.  The cache hierarchy data might be used by a string function that
> >> has not been selected by IFUNC.
> >>
> >
> > There are one IFUNC reloc in ld.so and the other in libc.so.  We need
> > both.
>
> libc.so should not need the relocation hack because we have
> __libc_early_init, which is also called after static dlopen and before
> constructors.
>

We want to call init_cacheinfo as early as possible.  __libc_early_init is
still too late.
Florian Weimer Sept. 28, 2020, 2:22 p.m. UTC | #5
* H. J. Lu:

> We want to call init_cacheinfo as early as possible.  __libc_early_init is
> still too late.

My point is that we should call it from IFUNC resolvers that need it,
and from __libc_early_init.  That should cover all cases, no?

It would also settle the matter of the assert.

Thanks,
Florian
H.J. Lu Sept. 28, 2020, 2:39 p.m. UTC | #6
On Mon, Sep 28, 2020 at 7:22 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > We want to call init_cacheinfo as early as possible.  __libc_early_init is
> > still too late.
>
> My point is that we should call it from IFUNC resolvers that need it,
> and from __libc_early_init.  That should cover all cases, no?

Why call it from __libc_early_init after it has been called by
IFUNC reloc?  IFUNC relocations are processed before
__libc_early_init is called.

> It would also settle the matter of the assert.
>
>
Florian Weimer Sept. 28, 2020, 2:47 p.m. UTC | #7
* H. J. Lu:

> On Mon, Sep 28, 2020 at 7:22 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > We want to call init_cacheinfo as early as possible.  __libc_early_init is
>> > still too late.
>>
>> My point is that we should call it from IFUNC resolvers that need it,
>> and from __libc_early_init.  That should cover all cases, no?
>
> Why call it from __libc_early_init after it has been called by
> IFUNC reloc?  IFUNC relocations are processed before
> __libc_early_init is called.

IFUNC relocations might not existing in a --disable-multi-arch build,
but it may still need the cacheinfo data if the hard-coded
implementations need them.

We would still run the IFUNC resolver for the artificial IFUNC resolver
with its function pointer, but:

My concern is that you seem to have a specific ordering dependency on
IFUNC resolvers, and I would like to get rid of that: Initialize the
necessary data on demand (for string function selection), and during
__libc_early_init for potential use from string functions.

I hope this clarifies what I'm looking for.  Please let me know if this
is not reasonable.

Thanks,
Florian
H.J. Lu Sept. 28, 2020, 5:54 p.m. UTC | #8
On Mon, Sep 28, 2020 at 7:47 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Mon, Sep 28, 2020 at 7:22 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> > We want to call init_cacheinfo as early as possible.  __libc_early_init is
> >> > still too late.
> >>
> >> My point is that we should call it from IFUNC resolvers that need it,
> >> and from __libc_early_init.  That should cover all cases, no?
> >
> > Why call it from __libc_early_init after it has been called by
> > IFUNC reloc?  IFUNC relocations are processed before
> > __libc_early_init is called.
>
> IFUNC relocations might not existing in a --disable-multi-arch build,
> but it may still need the cacheinfo data if the hard-coded
> implementations need them.

IFUNC is always supported on x86.  multi-arch uses IFUNC, not
the other way around.

> We would still run the IFUNC resolver for the artificial IFUNC resolver
> with its function pointer, but:
>
> My concern is that you seem to have a specific ordering dependency on
> IFUNC resolvers, and I would like to get rid of that: Initialize the
> necessary data on demand (for string function selection), and during
> __libc_early_init for potential use from string functions.
>
> I hope this clarifies what I'm looking for.  Please let me know if this
> is not reasonable.
>

Here is the updated patch.  Does it address your concerns?

Thanks.
Florian Weimer Sept. 29, 2020, 7:53 a.m. UTC | #9
* H. J. Lu via Libc-alpha:

> Here is the updated patch.  Does it address your concerns?

“git am” fails to apply it for me:

Applying: x86: Initialize CPU info via IFUNC relocation [BZ 26203]
error: corrupt patch at line 41
Patch failed at 0001 x86: Initialize CPU info via IFUNC relocation [BZ 26203]

The patch program does not like it, either:

patching file sysdeps/i386/dl-machine.h
patching file sysdeps/x86/cacheinfo.c
patch: **** malformed patch at line 73: @@ -770,6 +769,8 @@ init_cacheinfo (void)

I don't think it's the Red Hat mail system that's at fault this time. 8-/

Thanks,
Florian
H.J. Lu Sept. 29, 2020, 11:44 a.m. UTC | #10
On Tue, Sep 29, 2020 at 12:53 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu via Libc-alpha:
>
> > Here is the updated patch.  Does it address your concerns?
>
> “git am” fails to apply it for me:
>
> Applying: x86: Initialize CPU info via IFUNC relocation [BZ 26203]
> error: corrupt patch at line 41
> Patch failed at 0001 x86: Initialize CPU info via IFUNC relocation [BZ 26203]
>
> The patch program does not like it, either:
>
> patching file sysdeps/i386/dl-machine.h
> patching file sysdeps/x86/cacheinfo.c
> patch: **** malformed patch at line 73: @@ -770,6 +769,8 @@ init_cacheinfo (void)
>
> I don't think it's the Red Hat mail system that's at fault this time. 8-/
>

Here is the updated patch with the correct commit log.

Thanks.
Florian Weimer Oct. 1, 2020, 8:46 a.m. UTC | #11
* H. J. Lu:

> diff --git a/sysdeps/x86/cacheinfo.c b/sysdeps/x86/cacheinfo.c
> index dadec5d58f..65ab29123d 100644
> --- a/sysdeps/x86/cacheinfo.c
> +++ b/sysdeps/x86/cacheinfo.c
> @@ -16,7 +16,9 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#if IS_IN (libc)
> +/* NB: In libc.a, this file is included in libc-static.c.  In libc.so,
> +   this file is standalone.  */
> +#if IS_IN (libc) && (defined SHARED || defined _PRIVATE_CPU_FEATURES_H)

libc-static.c should be libc-start.c, I believe.  The “defined
_PRIVATE_CPU_FEATURES_H” part seems rather indirect.  What exactly are
you trying to accomplish here?

It looks to me as if this file should included in libc.so, but not
pulled into ld.so via the rebuild, so maybe you can add an empty
sysdeps/x86/rtld-cacheinfo.c file instead?

Thanks,
Florian
diff mbox series

Patch

diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index 0f08079e48..5e22e795cc 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -25,7 +25,6 @@ 
 #include <sysdep.h>
 #include <tls.h>
 #include <dl-tlsdesc.h>
-#include <cpu-features.c>
 
 /* Return nonzero iff ELF header is compatible with the running host.  */
 static inline int __attribute__ ((unused))
@@ -250,7 +249,7 @@  dl_platform_init (void)
 #if IS_IN (rtld)
   /* init_cpu_features has been called early from __libc_start_main in
      static executable.  */
-  init_cpu_features (&GLRO(dl_x86_cpu_features));
+  _dl_x86_init_cpu_features ();
 #else
   if (GLRO(dl_platform) != NULL && *GLRO(dl_platform) == '\0')
     /* Avoid an empty string which would disturb us.  */
diff --git a/sysdeps/x86/cacheinfo.c b/sysdeps/x86/cacheinfo.c
index 217c21c34f..7a325ab70e 100644
--- a/sysdeps/x86/cacheinfo.c
+++ b/sysdeps/x86/cacheinfo.c
@@ -756,7 +756,6 @@  intel_bug_no_cache_info:
 
 
 static void
-__attribute__((constructor))
 init_cacheinfo (void)
 {
   /* Find out what brand of processor.  */
@@ -770,6 +769,8 @@  init_cacheinfo (void)
   unsigned int threads = 0;
   const struct cpu_features *cpu_features = __get_cpu_features ();
 
+  assert (cpu_features->basic.kind != arch_kind_unknown);
+
   if (cpu_features->basic.kind == arch_kind_intel)
     {
       data = handle_intel (_SC_LEVEL1_DCACHE_SIZE, cpu_features);
@@ -894,4 +895,11 @@  init_cacheinfo (void)
 # endif
 }
 
+/* NB: Call init_cacheinfo by initializing a dummy function pointer via
+   IFUNC relocation.  */
+extern void __x86_cacheinfo (void) attribute_hidden;
+const void (*__x86_cacheinfo_p) (void) attribute_hidden
+  = __x86_cacheinfo;
+
+__ifunc (__x86_cacheinfo, __x86_cacheinfo, NULL, void, init_cacheinfo);
 #endif
diff --git a/sysdeps/x86/dl-get-cpu-features.c b/sysdeps/x86/dl-get-cpu-features.c
index 5f9e46b0c6..da4697b895 100644
--- a/sysdeps/x86/dl-get-cpu-features.c
+++ b/sysdeps/x86/dl-get-cpu-features.c
@@ -1,4 +1,4 @@ 
-/* This file is part of the GNU C Library.
+/* Initialize CPU feature data via IFUNC relocation.
    Copyright (C) 2015-2020 Free Software Foundation, Inc.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -18,6 +18,29 @@ 
 
 #include <ldsodefs.h>
 
+#ifdef SHARED
+# include <cpu-features.c>
+
+/* NB: Normally, DL_PLATFORM_INIT calls init_cpu_features to initialize
+   CPU features.  But when loading ld.so inside of static executable,
+   DL_PLATFORM_INIT isn't called.  Call init_cpu_features by initializing
+   a dummy function pointer via IFUNC relocation for ld.so.  */
+extern void __x86_cpu_features (void) attribute_hidden;
+const void (*__x86_cpu_features_p) (void) attribute_hidden
+  = __x86_cpu_features;
+
+void
+_dl_x86_init_cpu_features (void)
+{
+  struct cpu_features *cpu_features = __get_cpu_features ();
+  if (cpu_features->basic.kind == arch_kind_unknown)
+    init_cpu_features (cpu_features);
+}
+
+__ifunc (__x86_cpu_features, __x86_cpu_features, NULL, void,
+	 _dl_x86_init_cpu_features);
+#endif
+
 #undef __x86_get_cpu_features
 
 const struct cpu_features *
diff --git a/sysdeps/x86/include/cpu-features.h b/sysdeps/x86/include/cpu-features.h
index dcf29b6fe8..f62be0b9b3 100644
--- a/sysdeps/x86/include/cpu-features.h
+++ b/sysdeps/x86/include/cpu-features.h
@@ -159,6 +159,7 @@  struct cpu_features
 /* Unused for x86.  */
 #  define INIT_ARCH()
 #  define __x86_get_cpu_features(max) (&GLRO(dl_x86_cpu_features))
+extern void _dl_x86_init_cpu_features (void) attribute_hidden;
 # endif
 
 # ifdef __x86_64__
diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
index ca73d8fef9..773e94c8bb 100644
--- a/sysdeps/x86_64/dl-machine.h
+++ b/sysdeps/x86_64/dl-machine.h
@@ -26,7 +26,6 @@ 
 #include <sysdep.h>
 #include <tls.h>
 #include <dl-tlsdesc.h>
-#include <cpu-features.c>
 
 /* Return nonzero iff ELF header is compatible with the running host.  */
 static inline int __attribute__ ((unused))
@@ -225,7 +224,7 @@  dl_platform_init (void)
 #if IS_IN (rtld)
   /* init_cpu_features has been called early from __libc_start_main in
      static executable.  */
-  init_cpu_features (&GLRO(dl_x86_cpu_features));
+  _dl_x86_init_cpu_features ();
 #else
   if (GLRO(dl_platform) != NULL && *GLRO(dl_platform) == '\0')
     /* Avoid an empty string which would disturb us.  */