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 3608738618E2 for ; Mon, 28 Sep 2020 14:20:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3608738618E2 Received: by mail-il1-x141.google.com with SMTP id s88so1401544ilb.6 for ; Mon, 28 Sep 2020 07:20:38 -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=cgzyFIUPs6kFJPReRfhZjLDBManZZOoJSJ3FClxHhUE=; b=jbK34zqtFSm5VvxFMLXrgkOfMX8mFYs64z+jaso48aGJ+Y3twMXj5z98NyHix/DNJ9 nHtGKE0MrSgAKGjO6CJn7sbzCxIXuRQQ/pu3gc8l0SFDkLKB3HyGqdXTp6cPOxe6yzib Cvr0F0TISHVITM7FAV2xY6AVf+tF/6BqhneeS5plskSuoUPTKxIvD8yqpX4o8gL222H7 1L8ATFHDsrN9VePs19HO46IbsHeChFJyS6GVcup3knwI3gcxPGP6vkCLg2CClZghKH4k 0tukgP+hrcwKkAyhQ1MtmVEJJIfMhuaO0wcuxg6mHRAYlGtxSxOK/EgqMC+SXcQfOrgV AoRA== X-Gm-Message-State: AOAM532+6z8vAbYgSNLA5N1nnIoNyTXprM+HGJgz/jw9tc9C2ZVQq/jK 99QzuDVdxLs2e8JujljUvcrgaQeI0Z0yK1R9mrM= X-Google-Smtp-Source: ABdhPJwGUU6gg519fdK+/C/jp/P/o77gBLZucIyMjltg3GB7GxKyZnd655JkalO02WY9xJeD8t1bm44rEXcCvxojJVY= X-Received: by 2002:a92:1589:: with SMTP id 9mr1420117ilv.292.1601302837585; Mon, 28 Sep 2020 07:20:37 -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> <87y2ku574k.fsf@oldenburg2.str.redhat.com> In-Reply-To: <87y2ku574k.fsf@oldenburg2.str.redhat.com> From: "H.J. Lu" Date: Mon, 28 Sep 2020 07:20:01 -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 14:20:39 -0000 On Mon, Sep 28, 2020 at 7:05 AM Florian Weimer 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 > >> > > >> > +#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 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. -- H.J.