From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x629.google.com (mail-pl1-x629.google.com [IPv6:2607:f8b0:4864:20::629]) by sourceware.org (Postfix) with ESMTPS id 3B388385841D for ; Mon, 20 Dec 2021 21:40:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3B388385841D Received: by mail-pl1-x629.google.com with SMTP id y7so9133149plp.0 for ; Mon, 20 Dec 2021 13:40:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=uLXIWd/6Hk0fPnHCcv5atk/2G64UQ0PHBV62QJkJRiE=; b=kHZLSWUGSpSE5bgT3654iQnOc+ZSHiePvWeF7mSTvI1OoY4hyPxrhQV2HR1D8W5RZ4 h/ho3O3Um0pNxhb+fcPZ0GxVbgIibO3ww5SyBPANrQbw9S7DUl4B5GM+KfszlSdY2Iiw FHadTxRDBsVhKQi2ahQ7p7tL25tuHs7FHeM6L3JxHGkVt/rN9k/lv9D52+v/DZsvPZ62 hcVctJFlhnTobmGfmOvTfRf0z97w6m7F0QydjoLp//WLvtIYXuGQ3OcLaw0b189e1CDS aR9d1+j6+H7Cjvkx93lyCjXt0ARB80zc1A75b62FeRoDtsZbgZIbDjtbpI3pNm61xJyP qELw== X-Gm-Message-State: AOAM531dGvZaCLJY71uGSRmQ6/dvfmZxwFtuWmYKEGUlWNmwsG0RAGNp xKNw5AQiYlOadvMfrl5Hu+A= X-Google-Smtp-Source: ABdhPJwtnsBM5rUIF/vTgrtrRePRcmXWEUd5Eu2383fZFQrcAlFAIk13A05ZRvgp1qIUk+gWUinGaw== X-Received: by 2002:a17:90b:4ad0:: with SMTP id mh16mr74296pjb.176.1640036445252; Mon, 20 Dec 2021 13:40:45 -0800 (PST) Received: from localhost ([2409:10:24a0:4700:e8ad:216a:2a9d:6d0c]) by smtp.gmail.com with ESMTPSA id j8sm21118802pfc.8.2021.12.20.13.40.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Dec 2021 13:40:44 -0800 (PST) Date: Tue, 21 Dec 2021 06:40:42 +0900 From: Stafford Horne To: Adhemerval Zanella Cc: libc-alpha@sourceware.org, Openrisc Subject: Re: [PATCH v3 04/13] or1k: startup and dynamic linking code Message-ID: References: <20211210233456.4146479-1-shorne@gmail.com> <20211210233456.4146479-5-shorne@gmail.com> <3aec1cdb-54af-af1c-acec-38efe55c63cf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, 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: Mon, 20 Dec 2021 21:40:51 -0000 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. > > > >>> +/* 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: 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