From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x236.google.com (mail-oi1-x236.google.com [IPv6:2607:f8b0:4864:20::236]) by sourceware.org (Postfix) with ESMTPS id 177C63858D39 for ; Mon, 14 Feb 2022 11:44:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 177C63858D39 Received: by mail-oi1-x236.google.com with SMTP id u3so16972540oiv.12 for ; Mon, 14 Feb 2022 03:44:54 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=P0JYaMJCD3252b7dgOKbr51MoE0PsKu9fXXY0+8W9Dk=; b=VOed3q4hhyyI8wzC+ISvU9cpCc7PwOkPVscJzawp2QbMQ+GDa/++LR0vptrtm4iowF pXGZK6hofnFGORm6kqbXwK+Ub2ogLUP24X+23XqsOZZFtfXwuNatb9GHclLmknp93O94 BkjqjNnYgiyTGoaaJKfQFBFyY/69jyZPj+Ws/JuPQPMYkAqP5C1LomIWNOyEH7f+SaWz meAoUf3IGrNk+GpVQeaPc2b2izakgd488yvZqRj5EZmeBZwISI1Rf33I3d1VEjnTn+te qpiO14AhGdifIYso4/LAnrA6upCfR+ok16luJIKyJshf8p6hEY3DfoHGmSf6vi1j+eJI MNJQ== X-Gm-Message-State: AOAM531Frqe9Lw5JzHJAVZvEmK5Ex932+Y0b308LU/vFX5QHNZ0i5uep TUd7SlafTv23uGjfx6rsdG7a1HpuGk5f+A== X-Google-Smtp-Source: ABdhPJzXmQ+3oG4yQCaiL5VJPjycD2A72H3wbHCOMEzYJjwWkeKFlnINCP6Yxn+tokttgvpnLAfC1Q== X-Received: by 2002:a05:6808:d47:: with SMTP id w7mr5174458oik.268.1644839093053; Mon, 14 Feb 2022 03:44:53 -0800 (PST) Received: from ?IPV6:2804:431:c7cb:6747:70ce:2039:9f74:f513? ([2804:431:c7cb:6747:70ce:2039:9f74:f513]) by smtp.gmail.com with ESMTPSA id b21sm2628613ots.50.2022.02.14.03.44.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 14 Feb 2022 03:44:52 -0800 (PST) Message-ID: <662fface-5af4-5094-a0ae-b37e56de837d@linaro.org> Date: Mon, 14 Feb 2022 08:44:50 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.0 Subject: Re: [PATCH] Linux: Consolidate auxiliary vector parsing (redo) Content-Language: en-US To: Florian Weimer , libc-alpha@sourceware.org References: <87y22hqh2w.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella In-Reply-To: <87y22hqh2w.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 14 Feb 2022 11:44:57 -0000 On 11/02/2022 17:33, Florian Weimer via Libc-alpha wrote: > And optimize it slightly. > > This is commit 8c8510ab2790039e58995ef3a22309582413d3ff revised. > > In _dl_aux_init in elf/dl-support.c, use an explicit loop > and -fno-tree-loop-distribute-patterns to avoid memset. > > Tested on aarch64-linux-gnu, i686-linux-gnu, x86_64-linux-gnu. > Built with build-many-glibcs.py. > > --- > elf/Makefile | 5 ++ > elf/dl-support.c | 85 +++----------------- > sysdeps/unix/sysv/linux/alpha/dl-auxv.h | 18 ++--- > sysdeps/unix/sysv/linux/dl-parse_auxv.h | 61 +++++++++++++++ > sysdeps/unix/sysv/linux/dl-sysdep.c | 112 ++++++++------------------- > sysdeps/unix/sysv/linux/powerpc/dl-auxv.h | 14 +--- > sysdeps/unix/sysv/linux/powerpc/dl-support.c | 4 + > 7 files changed, 117 insertions(+), 182 deletions(-) > > diff --git a/elf/Makefile b/elf/Makefile > index bff94954c9..65ba06d8b8 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -160,6 +160,11 @@ ifeq (yes,$(have-loop-to-function)) > CFLAGS-rtld.c += -fno-tree-loop-distribute-patterns > endif > > +ifeq (yes,$(have-loop-to-function)) > +# Likewise, during static library startup, memset is not yet available. > +CFLAGS-dl-support.c = -fno-tree-loop-distribute-patterns > +endif > + I still think this approach is still fragile since we don't how if this will be the only optimization option gcc will make loops to memset. We already have a similar problem on _dl_start, but it is slight different since the clear memory should be done before self-relocation. Also for memset.c itself there is no much we can do, so we need to resort on -fno-tree-loop-distribute-patterns. However in this case I think it would be better to avoid hacking the compiler and expect it does not generate broken code and instead assume memset *might* be generate and work around it. We already have the hack on sysdeps/generic/symbol-hacks.h, so I think we should do something similiar: 1. Add a __memset_generic internal symbol on all ports to avoid the iFUNC plt (which seems to be cause of this issue). 2. Add a symbol redirection to call such symbol when required (most likely in the dl-start.h as H.J has suggested. So we don't need to handle this specific issue everytime we hack into the loader code. > # Compile rtld itself without stack protection. > # Also compile all routines in the static library that are elided from > # the shared libc because they are in libc.a in the same way. > diff --git a/elf/dl-support.c b/elf/dl-support.c > index 6d2c4baf81..153dd57ad2 100644 > --- a/elf/dl-support.c > +++ b/elf/dl-support.c > @@ -44,6 +44,7 @@ > #include > #include > #include > +#include > > extern char *__progname; > char **_dl_argv = &__progname; /* This is checked for some error messages. */ > @@ -240,93 +241,25 @@ __rtld_lock_define_initialized_recursive (, _dl_load_tls_lock) > > > #ifdef HAVE_AUX_VECTOR > +#include > + > int _dl_clktck; > > void > _dl_aux_init (ElfW(auxv_t) *av) > { > - int seen = 0; > - uid_t uid = 0; > - gid_t gid = 0; > - > #ifdef NEED_DL_SYSINFO > /* NB: Avoid RELATIVE relocation in static PIE. */ > GL(dl_sysinfo) = DL_SYSINFO_DEFAULT; > #endif > > _dl_auxv = av; > - for (; av->a_type != AT_NULL; ++av) > - switch (av->a_type) > - { > - case AT_PAGESZ: > - if (av->a_un.a_val != 0) > - GLRO(dl_pagesize) = av->a_un.a_val; > - break; > - case AT_CLKTCK: > - GLRO(dl_clktck) = av->a_un.a_val; > - break; > - case AT_PHDR: > - GL(dl_phdr) = (const void *) av->a_un.a_val; > - break; > - case AT_PHNUM: > - GL(dl_phnum) = av->a_un.a_val; > - break; > - case AT_PLATFORM: > - GLRO(dl_platform) = (void *) av->a_un.a_val; > - break; > - case AT_HWCAP: > - GLRO(dl_hwcap) = (unsigned long int) av->a_un.a_val; > - break; > - case AT_HWCAP2: > - GLRO(dl_hwcap2) = (unsigned long int) av->a_un.a_val; > - break; > - case AT_FPUCW: > - GLRO(dl_fpu_control) = av->a_un.a_val; > - break; > -#ifdef NEED_DL_SYSINFO > - case AT_SYSINFO: > - GL(dl_sysinfo) = av->a_un.a_val; > - break; > -#endif > -#ifdef NEED_DL_SYSINFO_DSO > - case AT_SYSINFO_EHDR: > - GL(dl_sysinfo_dso) = (void *) av->a_un.a_val; > - break; > -#endif > - case AT_UID: > - uid ^= av->a_un.a_val; > - seen |= 1; > - break; > - case AT_EUID: > - uid ^= av->a_un.a_val; > - seen |= 2; > - break; > - case AT_GID: > - gid ^= av->a_un.a_val; > - seen |= 4; > - break; > - case AT_EGID: > - gid ^= av->a_un.a_val; > - seen |= 8; > - break; > - case AT_SECURE: > - seen = -1; > - __libc_enable_secure = av->a_un.a_val; > - __libc_enable_secure_decided = 1; > - break; > - case AT_RANDOM: > - _dl_random = (void *) av->a_un.a_val; > - break; > - case AT_MINSIGSTKSZ: > - _dl_minsigstacksize = av->a_un.a_val; > - break; > - DL_PLATFORM_AUXV > - } > - if (seen == 0xf) > - { > - __libc_enable_secure = uid != 0 || gid != 0; > - __libc_enable_secure_decided = 1; > - } > + dl_parse_auxv_t auxv_values; > + /* Use an explicit initialization loop here because memset may not > + be available yet. */ > + for (int i = 0; i < array_length (auxv_values); ++i) > + auxv_values[i] = 0; > + _dl_parse_auxv (av, auxv_values); > } > #endif > > diff --git a/sysdeps/unix/sysv/linux/alpha/dl-auxv.h b/sysdeps/unix/sysv/linux/alpha/dl-auxv.h > index 81d90da095..fcec743239 100644 > --- a/sysdeps/unix/sysv/linux/alpha/dl-auxv.h > +++ b/sysdeps/unix/sysv/linux/alpha/dl-auxv.h > @@ -20,16 +20,8 @@ > > extern long __libc_alpha_cache_shape[4]; > > -#define DL_PLATFORM_AUXV \ > - case AT_L1I_CACHESHAPE: \ > - __libc_alpha_cache_shape[0] = av->a_un.a_val; \ > - break; \ > - case AT_L1D_CACHESHAPE: \ > - __libc_alpha_cache_shape[1] = av->a_un.a_val; \ > - break; \ > - case AT_L2_CACHESHAPE: \ > - __libc_alpha_cache_shape[2] = av->a_un.a_val; \ > - break; \ > - case AT_L3_CACHESHAPE: \ > - __libc_alpha_cache_shape[3] = av->a_un.a_val; \ > - break; > +#define DL_PLATFORM_AUXV \ > + __libc_alpha_cache_shape[0] = auxv_values[AT_L1I_CACHESHAPE]; \ > + __libc_alpha_cache_shape[1] = auxv_values[AT_L1D_CACHESHAPE]; \ > + __libc_alpha_cache_shape[2] = auxv_values[AT_L2_CACHESHAPE]; \ > + __libc_alpha_cache_shape[3] = auxv_values[AT_L3_CACHESHAPE]; > diff --git a/sysdeps/unix/sysv/linux/dl-parse_auxv.h b/sysdeps/unix/sysv/linux/dl-parse_auxv.h > new file mode 100644 > index 0000000000..b3d82f6994 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/dl-parse_auxv.h > @@ -0,0 +1,61 @@ > +/* Parse the Linux auxiliary vector. > + Copyright (C) 1995-2022 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include > +#include > +#include > +#include > +#include > + > +typedef ElfW(Addr) dl_parse_auxv_t[AT_MINSIGSTKSZ + 1]; > + > +/* Copy the auxiliary vector into AUX_VALUES and set up GLRO > + variables. */ > +static inline > +void _dl_parse_auxv (ElfW(auxv_t) *av, dl_parse_auxv_t auxv_values) > +{ > + auxv_values[AT_ENTRY] = (ElfW(Addr)) ENTRY_POINT; > + auxv_values[AT_PAGESZ] = EXEC_PAGESIZE; > + auxv_values[AT_FPUCW] = _FPU_DEFAULT; > + > + /* NB: Default to a constant CONSTANT_MINSIGSTKSZ. */ > + _Static_assert (__builtin_constant_p (CONSTANT_MINSIGSTKSZ), > + "CONSTANT_MINSIGSTKSZ is constant"); > + auxv_values[AT_MINSIGSTKSZ] = CONSTANT_MINSIGSTKSZ; > + > + for (; av->a_type != AT_NULL; av++) > + if (av->a_type <= AT_MINSIGSTKSZ) > + auxv_values[av->a_type] = av->a_un.a_val; > + > + GLRO(dl_pagesize) = auxv_values[AT_PAGESZ]; > + __libc_enable_secure = auxv_values[AT_SECURE]; > + GLRO(dl_platform) = (void *) auxv_values[AT_PLATFORM]; > + GLRO(dl_hwcap) = auxv_values[AT_HWCAP]; > + GLRO(dl_hwcap2) = auxv_values[AT_HWCAP2]; > + GLRO(dl_clktck) = auxv_values[AT_CLKTCK]; > + GLRO(dl_fpu_control) = auxv_values[AT_FPUCW]; > + _dl_random = (void *) auxv_values[AT_RANDOM]; > + GLRO(dl_minsigstacksize) = auxv_values[AT_MINSIGSTKSZ]; > + GLRO(dl_sysinfo_dso) = (void *) auxv_values[AT_SYSINFO_EHDR]; > +#ifdef NEED_DL_SYSINFO > + if (GLRO(dl_sysinfo_dso) != NULL) > + GLRO(dl_sysinfo) = auxv_values[AT_SYSINFO]; > +#endif > + > + DL_PLATFORM_AUXV > +} > diff --git a/sysdeps/unix/sysv/linux/dl-sysdep.c b/sysdeps/unix/sysv/linux/dl-sysdep.c > index 8ad72c4b7b..71cf1968b5 100644 > --- a/sysdeps/unix/sysv/linux/dl-sysdep.c > +++ b/sysdeps/unix/sysv/linux/dl-sysdep.c > @@ -18,15 +18,15 @@ > > #include <_itoa.h> > #include > +#include > #include > #include > +#include > #include > #include > #include > -#include > #include > #include > -#include > #include > #include > #include > @@ -45,8 +45,6 @@ > #include > > #ifdef SHARED > -# include > - > extern char **_environ attribute_hidden; > extern char _end[] attribute_hidden; > > @@ -64,20 +62,20 @@ void *_dl_random attribute_relro = NULL; > # define DL_STACK_END(cookie) ((void *) (cookie)) > #endif > > -ElfW(Addr) > -_dl_sysdep_start (void **start_argptr, > - void (*dl_main) (const ElfW(Phdr) *phdr, ElfW(Word) phnum, > - ElfW(Addr) *user_entry, ElfW(auxv_t) *auxv)) > +/* Arguments passed to dl_main. */ > +struct dl_main_arguments > { > - const ElfW(Phdr) *phdr = NULL; > - ElfW(Word) phnum = 0; > + const ElfW(Phdr) *phdr; > + ElfW(Word) phnum; > ElfW(Addr) user_entry; > - ElfW(auxv_t) *av; > -#ifdef NEED_DL_SYSINFO > - uintptr_t new_sysinfo = 0; > -#endif > +}; > > - __libc_stack_end = DL_STACK_END (start_argptr); > +/* Separate function, so that dl_main can be called without the large > + array on the stack. */ > +static void > +_dl_sysdep_parse_arguments (void **start_argptr, > + struct dl_main_arguments *args) > +{ > _dl_argc = (intptr_t) *start_argptr; > _dl_argv = (char **) (start_argptr + 1); /* Necessary aliasing violation. */ > _environ = _dl_argv + _dl_argc + 1; > @@ -89,75 +87,26 @@ _dl_sysdep_start (void **start_argptr, > break; > } > > - user_entry = (ElfW(Addr)) ENTRY_POINT; > - GLRO(dl_platform) = NULL; /* Default to nothing known about the platform. */ > + dl_parse_auxv_t auxv_values = { 0, }; > + _dl_parse_auxv (GLRO(dl_auxv), auxv_values); > > - /* NB: Default to a constant CONSTANT_MINSIGSTKSZ. */ > - _Static_assert (__builtin_constant_p (CONSTANT_MINSIGSTKSZ), > - "CONSTANT_MINSIGSTKSZ is constant"); > - GLRO(dl_minsigstacksize) = CONSTANT_MINSIGSTKSZ; > + args->phdr = (const ElfW(Phdr) *) auxv_values[AT_PHDR]; > + args->phnum = auxv_values[AT_PHNUM]; > + args->user_entry = auxv_values[AT_ENTRY]; > +} > > - for (av = GLRO(dl_auxv); av->a_type != AT_NULL; av++) > - switch (av->a_type) > - { > - case AT_PHDR: > - phdr = (void *) av->a_un.a_val; > - break; > - case AT_PHNUM: > - phnum = av->a_un.a_val; > - break; > - case AT_PAGESZ: > - GLRO(dl_pagesize) = av->a_un.a_val; > - break; > - case AT_ENTRY: > - user_entry = av->a_un.a_val; > - break; > - case AT_SECURE: > - __libc_enable_secure = av->a_un.a_val; > - break; > - case AT_PLATFORM: > - GLRO(dl_platform) = (void *) av->a_un.a_val; > - break; > - case AT_HWCAP: > - GLRO(dl_hwcap) = (unsigned long int) av->a_un.a_val; > - break; > - case AT_HWCAP2: > - GLRO(dl_hwcap2) = (unsigned long int) av->a_un.a_val; > - break; > - case AT_CLKTCK: > - GLRO(dl_clktck) = av->a_un.a_val; > - break; > - case AT_FPUCW: > - GLRO(dl_fpu_control) = av->a_un.a_val; > - break; > -#ifdef NEED_DL_SYSINFO > - case AT_SYSINFO: > - new_sysinfo = av->a_un.a_val; > - break; > -#endif > - case AT_SYSINFO_EHDR: > - GLRO(dl_sysinfo_dso) = (void *) av->a_un.a_val; > - break; > - case AT_RANDOM: > - _dl_random = (void *) av->a_un.a_val; > - break; > - case AT_MINSIGSTKSZ: > - GLRO(dl_minsigstacksize) = av->a_un.a_val; > - break; > - DL_PLATFORM_AUXV > - } > +ElfW(Addr) > +_dl_sysdep_start (void **start_argptr, > + void (*dl_main) (const ElfW(Phdr) *phdr, ElfW(Word) phnum, > + ElfW(Addr) *user_entry, ElfW(auxv_t) *auxv)) > +{ > + __libc_stack_end = DL_STACK_END (start_argptr); > + > + struct dl_main_arguments dl_main_args; > + _dl_sysdep_parse_arguments (start_argptr, &dl_main_args); > > dl_hwcap_check (); > > -#ifdef NEED_DL_SYSINFO > - if (new_sysinfo != 0) > - { > - /* Only set the sysinfo value if we also have the vsyscall DSO. */ > - if (GLRO(dl_sysinfo_dso) != 0) > - GLRO(dl_sysinfo) = new_sysinfo; > - } > -#endif > - > __tunables_init (_environ); > > /* Initialize DSO sorting algorithm after tunables. */ > @@ -188,8 +137,9 @@ _dl_sysdep_start (void **start_argptr, > if (__builtin_expect (__libc_enable_secure, 0)) > __libc_check_standard_fds (); > > - (*dl_main) (phdr, phnum, &user_entry, GLRO(dl_auxv)); > - return user_entry; > + (*dl_main) (dl_main_args.phdr, dl_main_args.phnum, > + &dl_main_args.user_entry, GLRO(dl_auxv)); > + return dl_main_args.user_entry; > } > > void > diff --git a/sysdeps/unix/sysv/linux/powerpc/dl-auxv.h b/sysdeps/unix/sysv/linux/powerpc/dl-auxv.h > index 594371940a..ce2281cf11 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/dl-auxv.h > +++ b/sysdeps/unix/sysv/linux/powerpc/dl-auxv.h > @@ -16,15 +16,5 @@ > License along with the GNU C Library; if not, see > . */ > > -#include > - > -#if IS_IN (libc) && !defined SHARED > -int GLRO(dl_cache_line_size); > -#endif > - > -/* Scan the Aux Vector for the "Data Cache Block Size" entry and assign it > - to dl_cache_line_size. */ > -#define DL_PLATFORM_AUXV \ > - case AT_DCACHEBSIZE: \ > - GLRO(dl_cache_line_size) = av->a_un.a_val; \ > - break; > +#define DL_PLATFORM_AUXV \ > + GLRO(dl_cache_line_size) = auxv_values[AT_DCACHEBSIZE]; > diff --git a/sysdeps/unix/sysv/linux/powerpc/dl-support.c b/sysdeps/unix/sysv/linux/powerpc/dl-support.c > new file mode 100644 > index 0000000000..abe68a7049 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/powerpc/dl-support.c > @@ -0,0 +1,4 @@ > +#include > + > +/* Populated from the auxiliary vector. */ > +int _dl_cache_line_size; >