From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-il1-x141.google.com (mail-il1-x141.google.com [IPv6:2607:f8b0:4864:20::141]) by sourceware.org (Postfix) with ESMTPS id E48BC38618E2 for ; Mon, 28 Sep 2020 13:48:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E48BC38618E2 Received: by mail-il1-x141.google.com with SMTP id f15so1288626ilj.2 for ; Mon, 28 Sep 2020 06:48:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=oeykyRfstmPVxlR4KAnrdVcUrZsI3aCwWLeg5aI0XfM=; b=olX9iLTVh2m78XZ6Qeo3fbBBjgMUNRzyvCv0aM7WzH2zBBq2rvHSDXbClAszq46v79 eu1vscDljTM0GvpxjX8sYLAGAY+MVkYFx0rfcb12sfaItpaeZr6g91nTi0hNcPTJpYz7 2Ye1X6cxSc/8F1HblWEX/51fPv3GNqe+Gvv/PbajaFEPc1nCJe0Murf94CBJS4zP3MbM 7MCZFV1cPoRJbKD1BnRzSAVwwn4XaLm7HrBPBHFnjD0UgrFSkA0P5bGxuo3hHvwhuOxh MBob5CEWY98OVkRedJA+RHA8zCPSNgoMKML8Szm7dfk4f4I6LxMl3m5XpGBY54CHkX+t hrMA== X-Gm-Message-State: AOAM532RuvzWLqn+iz21Fp1aQo0wpAChlmtt5KRtw7YSiocsIb4hdpMi 8qg7eDvtB2pcJqlGgNnRE8tfOVX3sm2Yo7NKrEs= X-Google-Smtp-Source: ABdhPJy1VIfZJwdce6vwDmgPlhteB/2bD7ekvus4ekykgfa5yjt7A6xdrPZOmTG5yGfm1na/Jav1x/tnhep1YaNdFSc= X-Received: by 2002:a92:6a09:: with SMTP id f9mr1247760ilc.273.1601300936293; Mon, 28 Sep 2020 06:48:56 -0700 (PDT) MIME-Version: 1.0 References: <20200918160709.949608-1-hjl.tools@gmail.com> <20200918160709.949608-2-hjl.tools@gmail.com> <87imby6obw.fsf@oldenburg2.str.redhat.com> In-Reply-To: <87imby6obw.fsf@oldenburg2.str.redhat.com> From: "H.J. Lu" Date: Mon, 28 Sep 2020 06:48:20 -0700 Message-ID: Subject: Re: [PATCH 1/4] x86: Initialize CPU info via IFUNC relocation [BZ 26203] To: Florian Weimer Cc: "H.J. Lu via Libc-alpha" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3036.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, URIBL_BLACK autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Sep 2020 13:48:58 -0000 On Mon, Sep 28, 2020 at 6:08 AM Florian Weimer 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 > > #include > > #include > > -#include > > > > /* 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 > > > > +#ifdef SHARED > > +# include > > + > > +/* 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. -- H.J.