Re: [PATCH v6 01/13] ARC: ABI Implementation

[Vineet Gupta <Vineet.Gupta1@synopsys.com>]

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@synopsys.com>
> 
> 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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc