[OpenRISC] [PATCH v3 04/13] or1k: startup and dynamic linking code

Adhemerval Zanella
2021-12-21T11:09:11Z

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
>>>>> + <https://www.gnu.org/licenses/>. */
>>>>> +
>>>>> +#ifndef _LINK_H
>>>>> +# error "Never include <bits/link.h> directly; use <link.h> 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
>>>>> + <https://www.gnu.org/licenses/>. */
>>>>> +
>>>>> +#ifndef dl_machine_h
>>>>> +#define dl_machine_h
>>>>> +
>>>>> +#define ELF_MACHINE_NAME "or1k"
>>>>> +
>>>>> +#include <sys/cdefs.h>
>>>>> +#include <sys/param.h>
>>>>> +#include <tls.h>
>>>>> +#include <dl-irel.h>
>>>>> +#include <dl-static-tls.h>
>>>>> +#include <dl-machine-rel.h>
>>>>> +
>>>>> +/* 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