From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22b.google.com (mail-oi1-x22b.google.com [IPv6:2607:f8b0:4864:20::22b]) by sourceware.org (Postfix) with ESMTPS id E53B13858D1E for ; Tue, 8 Feb 2022 20:19:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E53B13858D1E Received: by mail-oi1-x22b.google.com with SMTP id s185so297149oie.3 for ; Tue, 08 Feb 2022 12:19:45 -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=yKH0lhS1uQV9FVR7UQkHYaIaNDSX/bKlgWeVmKbBdbw=; b=ZgO8jTEWV7SYf414EehFuSYN79gZrFOomj15J0H9Ljq04jGxVd+Lc3VrOleKrfj2rO o34gLpJibuyUlLKtjLUTYvCbKHRLQc6QF5ZM1HW3EkaM5VLrTngI09sLi4cUyru775+R RVl345VEeMyEDH/k3PNsOc95VLBAOZMVxKAh5e0iWGQw5IEbKI7p1y/RLhTa2p19BZml 6sXkIPUqCMK2nFLJAja/Kk7XZOlRhqzcdU+Jo6dLvcey2IkmL/t5Px5Vh6xKueHQX64u pYYOg8tzbY2veH+2l7Z7fUdQUQPGI3mHjXEoH1sZEwimdm4NNJGEUls1WtQLhuENdrUe b8HA== X-Gm-Message-State: AOAM533eKyHk8vfblcp14W6YpG3Xml1ty9jl1AVJfiXJunx2bAWSIXZt ZKfgnpjXowk4F2seSYrenPOi9g== X-Google-Smtp-Source: ABdhPJzNAVl3BqP5zzdkdJ7/jEjOZzBvymZch1gcfmmu+D7nggDOjdpj4knOv+QTm0vaGi/Bk4kLRw== X-Received: by 2002:a05:6808:1b2a:: with SMTP id bx42mr1401750oib.131.1644351585113; Tue, 08 Feb 2022 12:19:45 -0800 (PST) Received: from ?IPV6:2804:431:c7ca:733:86a9:ad5:adef:2f3e? ([2804:431:c7ca:733:86a9:ad5:adef:2f3e]) by smtp.gmail.com with ESMTPSA id x19sm5729508otj.59.2022.02.08.12.19.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 08 Feb 2022 12:19:44 -0800 (PST) Message-ID: <2b39ebaf-98ee-d0c2-39f6-6b0a87d0741a@linaro.org> Date: Tue, 8 Feb 2022 17:19:42 -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 v2 5/5] Linux: Consolidate auxiliary vector parsing Content-Language: en-US To: Florian Weimer , libc-alpha@sourceware.org References: <151c5398adbbe538a275ea5ac77705b0abd0d748.1643886336.git.fweimer@redhat.com> From: Adhemerval Zanella In-Reply-To: <151c5398adbbe538a275ea5ac77705b0abd0d748.1643886336.git.fweimer@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: Tue, 08 Feb 2022 20:19:48 -0000 On 03/02/2022 08:08, Florian Weimer via Libc-alpha wrote: > And optimize it slightly. > > The large switch statement in _dl_sysdep_start can be replaced with > a large array. This reduces source code and binary size. On > i686-linux-gnu: > > Before: > > text data bss dec hex filename > 7791 12 0 7803 1e7b elf/dl-sysdep.os > > After: > > text data bss dec hex filename > 7135 12 0 7147 1beb elf/dl-sysdep.os LGTM with a couple comments below. Reviewed-by: Adhemerval Zanella > --- > elf/dl-support.c | 80 +------------ > 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 | 111 ++++++------------- > sysdeps/unix/sysv/linux/powerpc/dl-auxv.h | 14 +-- > sysdeps/unix/sysv/linux/powerpc/dl-support.c | 4 + > 6 files changed, 107 insertions(+), 181 deletions(-) > create mode 100644 sysdeps/unix/sysv/linux/dl-parse_auxv.h > create mode 100644 sysdeps/unix/sysv/linux/powerpc/dl-support.c > > diff --git a/elf/dl-support.c b/elf/dl-support.c > index fb64765537..0fff62064a 100644 > --- a/elf/dl-support.c > +++ b/elf/dl-support.c > @@ -241,93 +241,21 @@ __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 = { 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..f450c6c5ce > --- /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; Extra whitespace. > + 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"); Shouldn't it warn that CONSTANT_MINSIGSTKSZ is not a 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 I think you can now assume NEED_DL_SYSINFO is always defined. > + 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 2bda76b820..71cf1968b5 100644 > --- a/sysdeps/unix/sysv/linux/dl-sysdep.c > +++ b/sysdeps/unix/sysv/linux/dl-sysdep.c > @@ -21,13 +21,12 @@ > #include > #include > #include > +#include > #include > #include > #include > -#include > #include > #include > -#include > #include > #include > #include > @@ -63,24 +62,24 @@ 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; > - for (char **tmp = _environ + 1; ; ++tmp) > + for (char **tmp = _environ; ; ++tmp) > if (*tmp == NULL) > { > /* Another necessary aliasing violation. */ > @@ -88,74 +87,25 @@ _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); > > - dl_hwcap_check (); > + struct dl_main_arguments dl_main_args; > + _dl_sysdep_parse_arguments (start_argptr, &dl_main_args); > > -#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 > + dl_hwcap_check (); > > __tunables_init (_environ); > > @@ -187,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;