From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf31.google.com (mail-qv1-xf31.google.com [IPv6:2607:f8b0:4864:20::f31]) by sourceware.org (Postfix) with ESMTPS id 49D6B3858400 for ; Tue, 21 Dec 2021 11:09:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 49D6B3858400 Received: by mail-qv1-xf31.google.com with SMTP id kj16so8088421qvb.2 for ; Tue, 21 Dec 2021 03:09:15 -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:cc:references:from:in-reply-to :content-transfer-encoding; bh=UN38VvTHQnyzExiEHZxAtrAE3hrpwv1MueJMJN0Ygng=; b=mtLGrhvA9rQjF23ZvdaQIv79zJ+AcBJkbfY+h4fIRG3L5TlCQgU9MDh7gIbYB8CJRr CuVJC8Ze8EQUTtXHVau98a4QiA7XoM+RVcrASHkTlTd3o20LoyZl+reCyvoeSesHQa+5 HyFm/0aY/RfqUVUSo4VygHHHobG8Ccwwsd7hvAL4Qh+SNSjA23/7TEv7g9VZ6JXWlrG/ KLxqDNzDl/MqxPmjqM/McapSKD4WohP6mCUWWyjXIBUZHSeyAAAlNre3zdfh3DGDrPKV sOM7Cz9VH6uP2CYT22idaGNEeJCFYdQQrZWjGIhS+v14W5g/OXaQ2rHz6EQJb0MKxl32 gtEQ== X-Gm-Message-State: AOAM532i//kJxdAefLoNog+23PxYSR8SV6tKjUqcjz4f29/kHvdDTe+N duMsveisjPYwjXJiB4wKCAIYFP7hKVCmPQ== X-Google-Smtp-Source: ABdhPJxCiof//YIOWB30arAzAcgVX3HWfqaWFCaGBW4OU2L5bc4N7zk9RTVzNiMlwlxH6M5OU++NVA== X-Received: by 2002:a05:6214:27cd:: with SMTP id ge13mr1799286qvb.24.1640084954771; Tue, 21 Dec 2021 03:09:14 -0800 (PST) Received: from ?IPV6:2804:431:c7cb:3b1e:8bd2:32a9:e2a3:1842? ([2804:431:c7cb:3b1e:8bd2:32a9:e2a3:1842]) by smtp.gmail.com with ESMTPSA id n13sm14826801qkp.19.2021.12.21.03.09.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 21 Dec 2021 03:09:14 -0800 (PST) Message-ID: Date: Tue, 21 Dec 2021 08:09:11 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH v3 04/13] or1k: startup and dynamic linking code Content-Language: en-US To: Stafford Horne Cc: libc-alpha@sourceware.org, Openrisc References: <20211210233456.4146479-1-shorne@gmail.com> <20211210233456.4146479-5-shorne@gmail.com> <3aec1cdb-54af-af1c-acec-38efe55c63cf@linaro.org> From: Adhemerval Zanella In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.2 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 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, 21 Dec 2021 11:09:17 -0000 On 20/12/2021 18:40, Stafford Horne wrote: > On Mon, Dec 20, 2021 at 04:45:21PM -0300, Adhemerval Zanella wrote: >> >> >> On 17/12/2021 20:03, Stafford Horne wrote: >>> On Thu, Dec 16, 2021 at 07:42:46AM -0300, Adhemerval Zanella wrote: >>>> >>>> >>>> On 10/12/2021 20:34, Stafford Horne via Libc-alpha wrote: >>>>> Code for C runtime startup and dynamic loading including PLT layout. >>>> >>>> Patch looks ok, although I comment much on ABI and assembly code. Some >>>> comments below. >>>> >>>>> --- >>>>> sysdeps/or1k/bits/link.h | 51 ++++++ >>>>> sysdeps/or1k/dl-machine.h | 323 +++++++++++++++++++++++++++++++++++++ >>>>> sysdeps/or1k/dl-start.S | 98 +++++++++++ >>>>> sysdeps/or1k/ldsodefs.h | 40 +++++ >>>>> sysdeps/or1k/sotruss-lib.c | 51 ++++++ >>>>> sysdeps/or1k/start.S | 99 ++++++++++++ >>>>> sysdeps/or1k/tst-audit.h | 24 +++ >>>>> 7 files changed, 686 insertions(+) >>>>> create mode 100644 sysdeps/or1k/bits/link.h >>>>> create mode 100644 sysdeps/or1k/dl-machine.h >>>>> create mode 100644 sysdeps/or1k/dl-start.S >>>>> create mode 100644 sysdeps/or1k/ldsodefs.h >>>>> create mode 100644 sysdeps/or1k/sotruss-lib.c >>>>> create mode 100644 sysdeps/or1k/start.S >>>>> create mode 100644 sysdeps/or1k/tst-audit.h >>>>> >>>>> diff --git a/sysdeps/or1k/bits/link.h b/sysdeps/or1k/bits/link.h >>>>> new file mode 100644 >>>>> index 0000000000..ad183c9625 >>>>> --- /dev/null >>>>> +++ b/sysdeps/or1k/bits/link.h >>>>> @@ -0,0 +1,51 @@ >>>>> +/* Declarations for dynamic linker interface. OpenRISC version. >>>>> + Copyright (C) 2021 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 >>>>> + . */ >>>>> + >>>>> +#ifndef _LINK_H >>>>> +# error "Never include directly; use instead." >>>>> +#endif >>>>> + >>>>> +/* Registers for entry into PLT. */ >>>>> +typedef struct La_or1k_regs >>>>> +{ >>>>> + uint32_t lr_reg[31]; >>>>> +} La_or1k_regs; >>>>> + >>>> >>>> So is the intent is to provide caller all the register, no only the caller-saved >>>> registers? >>> >>> As I see it... >>> >>> Shouldn't these usually be the argument registers used in la_pltenter to print >>> our the function call? i.e. >>> >>> print_enter (refcook, defcook, symname, >>> regs->lr_reg[0], regs->lr_reg[1], regs->lr_reg[2], >>> *flags); >>> >>> These registers get setup in sysdeps/{arch}/dl-trampoline.S by >>> _dl_runtime_profile, which then pass them to _dl_profile_fixup. >>> >>> The problem is openrisc doesn't implement _dl_runtime_profile so we haven't >>> defined what the registers are. >>> >>> I noticed the same for arc, csky, riscv. >>> >>> These are needed to be defined for sotruss, but I dont seem them being used. >>> >>> Is that correct? >> >> The register are really dependend of the function calling ABI of the architecture, >> if you check other architecture like x86 and powerpc you will see that La_i86_regs >> and La_ppc64_regs only saves the caller-saved ones. It is mainly because for >> PLT tracking, as done by audit modules, other register are not really useful >> (since they are just internal ones to the caller function). > > I see, but isn't this register saving done in _dl_profile_resolve? > > OpenRISC doesn't define _dl_profile_resolve. > > I can change the definition of La_or1k_regs, but as I see it it is currently > not used other than building sotruss-lib and audit modules. But I don't see > how pltenter will be called without implementing _dl_profile_resolve. The issue is the definition you set now will define the current audit ABI (LAV_CURRENT), which means if you eventually implement _dl_profile_resolve support for or1k you will need to either save/restore all registers or zero the non calles-saved ones. That's why modeling the audit PLT register structure over the ABI calling functions makes more sense. > >>> >>>>> +/* Return values for calls from PLT. */ >>>>> +typedef struct La_or1k_retval >>>>> +{ >>>>> + uint32_t lrv_r3; >>>>> +} La_or1k_retval; >>>>> + >>>>> +__BEGIN_DECLS >>>>> + >>>>> +extern ElfW(Addr) la_or1k_gnu_pltenter (ElfW(Sym) *__sym, unsigned int __ndx, >>>>> + uintptr_t *__refcook, >>>>> + uintptr_t *__defcook, >>>>> + La_or1k_regs *__regs, >>>>> + unsigned int *__flags, >>>>> + const char *__symname, >>>>> + long int *__framesizep); >>>>> +extern unsigned int la_or1k_gnu_pltexit (ElfW(Sym) *__sym, unsigned int __ndx, >>>>> + uintptr_t *__refcook, >>>>> + uintptr_t *__defcook, >>>>> + const La_or1k_regs *__inregs, >>>>> + La_or1k_retval *__outregs, >>>>> + const char *__symname); >>>>> + >>>>> +__END_DECLS >>>>> diff --git a/sysdeps/or1k/dl-machine.h b/sysdeps/or1k/dl-machine.h >>>>> new file mode 100644 >>>>> index 0000000000..d41554769e >>>>> --- /dev/null >>>>> +++ b/sysdeps/or1k/dl-machine.h >>>>> @@ -0,0 +1,323 @@ >>>>> +/* Machine-dependent ELF dynamic relocation inline functions. OpenRISC version. >>>>> + Copyright (C) 2021 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 >>>>> + . */ >>>>> + >>>>> +#ifndef dl_machine_h >>>>> +#define dl_machine_h >>>>> + >>>>> +#define ELF_MACHINE_NAME "or1k" >>>>> + >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> + >>>>> +/* Return nonzero iff ELF header is compatible with the running host. */ >>>>> +static inline int __attribute__ ((unused)) >>>>> +elf_machine_matches_host (const Elf32_Ehdr *ehdr) >>>>> +{ >>>>> + return ehdr->e_machine == EM_OPENRISC; >>>>> +} >>>>> + >>>>> +static inline Elf32_Addr * >>>>> +or1k_get_got (void) >>>>> +{ >>>>> + Elf32_Addr *got; >>>>> + register long int linkreg asm ("r9"); >>>> >>>> Is the 'r9' a required register by the ABI to use on this assembly snippet? >>>> I am asking because local name 'registers' are only a hint to gcc, as we >>>> found recently [1], if you really need 'r9' you will probably need to use >>>> a global name resgister. >>> >>> The r9 is to avoid the snippet from clobbering r9. The result of l.jal will >>> update r9 (link register) we want to make sure that is what we used for %1. >>> >>> I think there were issues with other ways I tried. >> >> Should you add it the clobbered list instead? >> >>> >>> Let me look into it more. >>> >>>>> + asm ("l.jal 0x8\n" >>>>> + " l.movhi %0, gotpchi(_GLOBAL_OFFSET_TABLE_-4)\n" >>>>> + "l.ori %0, %0, gotpclo(_GLOBAL_OFFSET_TABLE_+0)\n" >>>>> + "l.add %0, %0, %1\n" >>>>> + : "=r" (got), "=r" (linkreg)); >> >> With something like (unstested): >> >> >> static inline Elf32_Addr * >> or1k_get_got (void) >> { >> Elf32_Addr *got; >> asm ("l.jal 0x8\n" >> " l.movhi %0, gotpchi(_GLOBAL_OFFSET_TABLE_-4)\n" >> "l.ori %0, %0, gotpclo(_GLOBAL_OFFSET_TABLE_+0)\n" >> "l.add %0, %0, %1\n" >> : "=r" (got) >> : >> : "r9"); >> } > > Yes, this is what I ended up changing it to. > > Is the indentation important here? I had it as: I think either one is fine. > > static inline Elf32_Addr * > or1k_get_got (void) > { > Elf32_Addr *got; > > asm ("l.jal 0x8\n" > " l.movhi %0, gotpchi(_GLOBAL_OFFSET_TABLE_-4)\n" > "l.ori %0, %0, gotpclo(_GLOBAL_OFFSET_TABLE_+0)\n" > "l.add %0, %0, r9\n" > : "=r" (got) : : "r9"); > > return got; > } > > -Stafford