On 5/27/20 11:26 AM, Adhemerval Zanella via Libc-alpha wrote: > > > On 22/04/2020 22:41, Vineet Gupta via Libc-alpha wrote: >> This code deals with the ARC ABI. >> >> Signed-off-by: Vineet Gupta <vgupta@xxxxxxxxxxxx> > > We do not use DCO, but rather copyright assignment. Right, removed that now. > Looks ok in general, with some comments below. Thx for taking a look. >> +;@ r1 = value that setjmp( ) will return due to this longjmp > > Since all .S files are processed by gcc assembly implementation usually > use C style comment (/* ... */). Same applies to other assembly > implementations. OK, I can update throughout, although I like the small assembler comments which are on the same line. >> diff --git a/sysdeps/arc/dl-runtime.c b/sysdeps/arc/dl-runtime.c >> new file mode 100644 >> index 000000000000..b28da38329f1 >> --- /dev/null >> +++ b/sysdeps/arc/dl-runtime.c >> @@ -0,0 +1,33 @@ >> +/* dl-runtime helpers for ARC. >> + Copyright (C) 2017-2020 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/>. */ >> + >> +/* PLT jump into resolver passes PC of PLTn, while _dl_fixup expects the >> + address of corresponding .rela.plt entry. */ >> + >> +#define reloc_index \ >> +({ \ >> + unsigned long int plt0 = D_PTR (l, l_info[DT_PLTGOT]); \ >> + unsigned long int pltn = reloc_arg; \ >> + /* Exclude PLT0 and PLT1. */ \ >> + unsigned long int idx = ((pltn - plt0) / 16) - 2; \ >> + idx; \ >> +}) >> + >> +#define reloc_offset reloc_index * sizeof (PLTREL) >> + >> +#include <elf/dl-runtime.c> > > Ok, although this kid of macro are really error-prone: it relies on > not specified arguments (l, reloc_arg) and its variable definition might > clash. > > I wonder if it would be better to refactor both reloc_index and > reloc_offset to inline function that might be overriden by the > architecture where required. Something like: > > diff --git a/elf/dl-runtime.c b/elf/dl-runtime.c > index cf5f1d3e82..62f9057937 100644 > --- a/elf/dl-runtime.c > +++ b/elf/dl-runtime.c > @@ -27,6 +27,7 @@ > #include "dynamic-link.h" > #include <tls.h> > #include <dl-irel.h> > +#include <dl-runtime.h> > > > #if (!ELF_MACHINE_NO_RELA && !defined ELF_MACHINE_PLT_REL) \ > @@ -42,13 +43,6 @@ > # define ARCH_FIXUP_ATTRIBUTE > #endif > > -#ifndef reloc_offset > -# define reloc_offset reloc_arg > -# define reloc_index reloc_arg / sizeof (PLTREL) > -#endif > - > - > - > /* This function is called through a special trampoline from the PLT the > first time each PLT entry is called. We must perform the relocation > specified in the PLT of the given shared object, and return the resolved > @@ -68,8 +62,11 @@ _dl_fixup ( > = (const void *) D_PTR (l, l_info[DT_SYMTAB]); > const char *strtab = (const void *) D_PTR (l, l_info[DT_STRTAB]); > > + const uintptr_t pltgot = (uintptr_t) D_PTR (l, l_info[DT_PLTGOT]); > + > const PLTREL *const reloc > - = (const void *) (D_PTR (l, l_info[DT_JMPREL]) + reloc_offset); > + = (const void *) (D_PTR (l, l_info[DT_JMPREL]) > + + reloc_offset (pltgot, reloc_arg)); > const ElfW(Sym) *sym = &symtab[ELFW(R_SYM) (reloc->r_info)]; > const ElfW(Sym) *refsym = sym; > void *const rel_addr = (void *)(l->l_addr + reloc->r_offset); > @@ -182,7 +179,8 @@ _dl_profile_fixup ( > > /* This is the address in the array where we store the result of previous > relocations. */ > - struct reloc_result *reloc_result = &l->l_reloc_result[reloc_index]; > + struct reloc_result *reloc_result > + = &l->l_reloc_result[reloc_index (reloc_arg, sizeof (PLTREL))]; > > /* CONCURRENCY NOTES: > > @@ -219,8 +217,11 @@ _dl_profile_fixup ( > = (const void *) D_PTR (l, l_info[DT_SYMTAB]); > const char *strtab = (const char *) D_PTR (l, l_info[DT_STRTAB]); > > + const uintptr_t pltgot = (uintptr_t) D_PTR (l, l_info[DT_PLTGOT]); > + > const PLTREL *const reloc > - = (const void *) (D_PTR (l, l_info[DT_JMPREL]) + reloc_offset); > + = (const void *) (D_PTR (l, l_info[DT_JMPREL]) > + + reloc_offset (pltgot, reloc_arg)); > const ElfW(Sym) *refsym = &symtab[ELFW(R_SYM) (reloc->r_info)]; > const ElfW(Sym) *defsym = refsym; > lookup_t result; > @@ -489,7 +490,8 @@ _dl_call_pltexit (struct link_map *l, ElfW(Word) reloc_arg, > relocations. */ > // XXX Maybe the bound information must be stored on the stack since > // XXX with bind_not a new value could have been stored in the meantime. > - struct reloc_result *reloc_result = &l->l_reloc_result[reloc_index]; > + struct reloc_result *reloc_result = > + &l->l_reloc_result[reloc_index (reloc_arg, sizeof (PLTREL))]; > ElfW(Sym) *defsym = ((ElfW(Sym) *) D_PTR (reloc_result->bound, > l_info[DT_SYMTAB]) > + reloc_result->boundndx); > diff --git a/elf/dl-runtime.h b/elf/dl-runtime.h > new file mode 100644 > index 0000000000..901249f912 > --- /dev/null > +++ b/elf/dl-runtime.h > @@ -0,0 +1,11 @@ > +static inline uintptr_t > +reloc_offset (uintptr_t plt0, uintptr_t pltn) > +{ > + return pltn; > +} > + > +static inline uintptr_t > +reloc_index (uintptr_t pltn, size_t size) > +{ > + return pltn / size; > +} > diff --git a/sysdeps/hppa/dl-runtime.c b/sysdeps/hppa/dl-runtime.c > index 885a3f1837..2d061b150f 100644 > --- a/sysdeps/hppa/dl-runtime.c > +++ b/sysdeps/hppa/dl-runtime.c > @@ -17,10 +17,6 @@ > Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA > 02111-1307 USA. */ > > -/* Clear PA_GP_RELOC bit in relocation offset. */ > -#define reloc_offset (reloc_arg & ~PA_GP_RELOC) > -#define reloc_index (reloc_arg & ~PA_GP_RELOC) / sizeof (PLTREL) > - > #include <elf/dl-runtime.c> > > /* The caller has encountered a partially relocated function descriptor. > diff --git a/sysdeps/hppa/dl-runtime.h b/sysdeps/hppa/dl-runtime.h > new file mode 100644 > index 0000000000..dc12fd1071 > --- /dev/null > +++ b/sysdeps/hppa/dl-runtime.h > @@ -0,0 +1,12 @@ > +/* Clear PA_GP_RELOC bit in relocation offset. */ > +static inline uintptr_t > +reloc_offset (uintptr_t plt0, uintptr_t pltn) > +{ > + return pltn & ~PA_GP_RELOC; > +} > + > +static inline uintptr_t > +reloc_index (uintptr_t pltn, size_t size) > +{ > + return (pltn & ~PA_GP_RELOC )/ size; > +} > diff --git a/sysdeps/x86_64/dl-runtime.c b/sysdeps/x86_64/dl-runtime.h > similarity index 60% > rename from sysdeps/x86_64/dl-runtime.c > rename to sysdeps/x86_64/dl-runtime.h > index b625d1e882..494ff47b70 100644 > --- a/sysdeps/x86_64/dl-runtime.c > +++ b/sysdeps/x86_64/dl-runtime.h > @@ -3,7 +3,14 @@ > also use the index. Therefore it is wasteful to compute the offset > in the trampoline just to reverse the operation immediately > afterwards. */ > -#define reloc_offset reloc_arg * sizeof (PLTREL) > -#define reloc_index reloc_arg > +static inline uintptr_t > +reloc_offset (uintptr_t plt0, uintptr_t pltn) > +{ > + return pltn * sizeof (ElfW(Rela)); > +} > > -#include <elf/dl-runtime.c> > +static inline uintptr_t > +reloc_index (uintptr_t pltn, size_t size) > +{ > + return pltn; > +} Indeed looks much sane now. Do you want me to break out this code as a separate patch, ahead of the series ? >> + >> +.macro RESTORE_CALLER_SAVED_BUT_R0 >> + ld.ab blink,[sp, 4] >> + cfi_adjust_cfa_offset (-4) >> + cfi_restore (blink) >> + ld.ab r9, [sp, 4] >> + ld.ab r8, [sp, 4] >> + ld.ab r7, [sp, 4] >> + ld.ab r6, [sp, 4] >> + ld.ab r5, [sp, 4] >> + ld.ab r4, [sp, 4] >> + pop_s r3 >> + pop_s r2 >> + pop_s r1 >> + cfi_adjust_cfa_offset (-36) >> +.endm >> + >> +/* Upon entry, PLTn, which led us here, sets up the following regs >> + r11 = Module info (tpnt pointer as expected by resolver) >> + r12 = PC of the PLTn itself - needed by resolver to find >> + corresponding .rela.plt entry. */ >> + >> +ENTRY (_dl_runtime_resolve) >> + ; args to func being resolved, which resolver might clobber >> + SAVE_CALLER_SAVED >> + >> + mov_s r1, r12 >> + bl.d _dl_fixup >> + mov r0, r11 >> + >> + RESTORE_CALLER_SAVED_BUT_R0 >> + j_s.d [r0] /* r0 has resolved function addr. */ >> + pop_s r0 /* restore first arg to resolved call. */ >> + cfi_adjust_cfa_offset (-4) >> + cfi_restore (r0) >> +END (_dl_runtime_resolve) > > Ok, although I am not seeing why exactly you need asm macros here. Yes this is just a relic of code from the past, I can opencode it. >> diff --git a/sysdeps/arc/gccframe.h b/sysdeps/arc/gccframe.h >> new file mode 100644 >> index 000000000000..5d547fd40a6c >> --- /dev/null >> +++ b/sysdeps/arc/gccframe.h >> @@ -0,0 +1,21 @@ >> +/* Definition of object in frame unwind info. ARC version. >> + Copyright (C) 2017-2020 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/>. */ >> + >> +#define FIRST_PSEUDO_REGISTER 40 >> + >> +#include <sysdeps/generic/gccframe.h> > > Ok. > >> diff --git a/sysdeps/arc/jmpbuf-offsets.h b/sysdeps/arc/jmpbuf-offsets.h >> new file mode 100644 >> + >> +/* Callee Regs. */ >> +#define JB_R13 0 >> +#define JB_R14 1 >> +#define JB_R15 2 >> +#define JB_R16 3 >> +#define JB_R17 4 >> +#define JB_R18 5 >> +#define JB_R19 6 >> +#define JB_R20 7 >> +#define JB_R21 8 >> +#define JB_R22 9 >> +#define JB_R23 10 >> +#define JB_R24 11 >> +#define JB_R25 12 I'll probably drop these as these are not used anyways. Also will help declutter on the next architecture where the ABI for callee regs is changed from r13-25 to r14-r26. >> + >> +/* Frame Pointer, Stack Pointer, Branch-n-link. */ >> +#define JB_FP 13 >> +#define JB_SP 14 >> +#define JB_BLINK 15 >> + >> +/* We save space for some extra state to accommodate future changes >> + This is number of words. */ >> +#define JB_NUM 32 >> + >> +/* Helper for generic ____longjmp_chk. */ >> +#define JB_FRAME_ADDRESS(buf) ((void *) (unsigned long int) (buf[JB_SP])) > > Ok. > >> diff --git a/sysdeps/arc/memusage.h b/sysdeps/arc/memusage.h >> + >> +#define GETSP() ({ register uintptr_t stack_ptr asm ("sp"); stack_ptr; }) >> + >> +#define uatomic32_t unsigned int > > Not sure if this is really required now that we are moving to C11 atomic > model withing glibc itself. Maybe we could just use uint32_t on > malloc/memusage.c and rely on atomic macros instead. But that would be much bigger change, and orthogonal to the port. So perhaps we add it for now and then do the bigger/sweeping change. _______________________________________________ linux-snps-arc mailing list linux-snps-arc@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/linux-snps-arc
