Re: [PATCH v6 11/13] ARC: Build Infrastructure

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 04/06/2020 12:25, Vineet Gupta wrote:
> On 6/3/20 12:58 PM, Adhemerval Zanella via Libc-alpha wrote:
>>
>>>
>>> diff --git a/config.h.in b/config.h.in
>>> index dea43df438f6..08962dfed075 100644
>>> --- a/config.h.in
>>> +++ b/config.h.in
>>> @@ -109,6 +109,9 @@
>>>  /* AArch64 big endian ABI */
>>>  #undef HAVE_AARCH64_BE
>>>  
>>> +/* ARC big endian ABI */
>>> +#undef HAVE_ARC_BE
>>> +
>>
>> Why do you need this define exactly? It is not used anywhere in the code
>> and for C code if is more straightforwar to use endian.h.
> 
> It is used in build system as part of "formal" BE ABI support as pointed to in v4
> series review. This specific thing helps choose the right dynamic linker name for
> BE case.

Ack.

> 
> $ git grep HAVE_ARC_BE
> config.h.in:113:#undef HAVE_ARC_BE
> sysdeps/arc/configure:175:  $as_echo "#define HAVE_ARC_BE 1" >>confdefs.h
> sysdeps/arc/configure.ac:22:  AC_DEFINE(HAVE_ARC_BE)
> sysdeps/unix/sysv/linux/arc/shlib-versions:3:%ifdef HAVE_ARC_BE
> 
> I looked at other ports and they seem to follow similar patters: csky for CSKYABI,
> riscv for RISCV_ABI_XLEN etc.

Right, it is the usual way indeed. This is fine.

> 
> But I can rework if there's a simpler/better way.
> 
>>> +++ b/sysdeps/arc/Versions
>>> @@ -0,0 +1,8 @@
>>> +libc {
>>> +  GLIBC_2.32 {
>>> +    __mcount;
>>> +  }
>>
>> Hum, does ARC require a different symbol name than the one provided
>> by gmon/Versions?
> 
> ARC gcc generates __mcount and _mcount with different ABIs and we use the newer
> __mcount.

Ack.

> 
>>> +AC_DEFINE(PI_STATIC_AND_HIDDEN)
>>> +libc_cv_have_sdata_section=no
>>
>> The libc_cv_have_sdata_section is done by configure.ac, why do you need
>> to explicit set it here?
> 
> We inhibit small data explicitly which by default kicks in.

Ok, is it some limitation for loader bootstrap or something else?

> 
>>> +if test $libc_cv_arc_be = yes; then
>>> +  # For shlib-versions.
>>> +  AC_DEFINE(HAVE_ARC_BE)
>>> +  LIBC_CONFIG_VAR([default-abi], [ilp32_be])
>>> +else
>>> +  LIBC_CONFIG_VAR([default-abi], [ilp32])
>>> +fi
>>
>> The ilp32 naming is usually set for ILP32 architectures that uses 
>> 64-bit registers type on 32 bit VMA (for instance mips64n32, x32,
>> or aarch64_ilp32).  I don't think this is the case for ARC, so I think
>> this naming might be confusing.
>>> +abi-variants := ilp32 ilp32_be
> 
> arcle arcbe ?

LGTM.

> 
>>> +
>>> +ifeq (,$(filter $(default-abi),$(abi-variants)))
>>> +$(error Unknown ABI $(default-abi), must be one of $(abi-variants))
>>> +endif
>>> +
>>> +abi-ilp32-condition	:= !defined __BIG_ENDIAN__
>>> +abi-ilp32_be-condition	:= defined __BIG_ENDIAN__
>>
>> Ok with the 'ilp32' naming module described above.
> 
> 
>>> diff --git a/sysdeps/unix/sysv/linux/arc/Versions b/sysdeps/unix/sysv/linux/arc/Versions
>>> new file mode 100644
>>> index 000000000000..292f1974b02a
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/arc/Versions
>>> @@ -0,0 +1,16 @@
>>> +ld {
>>> +  GLIBC_PRIVATE {
>>> +    # used for loading by static libraries
>>> +    _dl_var_init;
>>> +  }
>>> +}
>>> +libc {
>>> +  GLIBC_2.32 {
>>> +    _flush_cache;
>>> +    cacheflush;
>>> +  }
>>> +  GLIBC_PRIVATE {
>>> +    # A copy of sigaction lives in libpthread, and needs these.
>>> +    __default_rt_sa_restorer;
>>> +  }
>>> +}
>>
>> Afaik all other ABIs that requires the sa_restores uses a local symbol in
>> libpthread. I don't have a strong preference here.
> 
> Do you mean add following to sysdeps/unix/sysv/linux/arc/Makefile
> 
> ifeq ($(subdir),nptl)
> libpthread-routines += sigrestorer
> libpthread-shared-only-routines += sigrestorer
> endif

Yeap.

> 
> And that is to optimize the reference to restorer as a direct PC relative access
> vs a got reference ?

My understanding is this specific optimization does not really matter: 
sigaction is hardly a hotspot and the symbol will be issue by the 
kernel itself. AFAIU is more a way to optimize the exported symbols
and simplify the GLIBC_PRIVATE namespace (since the sa_restore usually
has small text size).

> 
> It seems even in libc, this is currently not optimal. It seems we need
> libc_hidden_* on restorer.
> 
> 0002b020 <__GI___libc_sigaction>:
>  2b020: std.aw	r14r15,[sp,-8]
>  2b024: push_s	r13
>  2b026: sub_s	sp,sp,0x28
> ...
>  2b02e: mov_s	r3,r1
>  2b030: ld r13,[pcl,0xd4e9c] <__default_rt_sa_restorer@@GLIBC_PRIVATE+0xd4558>
> 

If you define it as attribute_hidden and add on both libc and libpthread it
should not require the libc_hidden_{proto,def}.

> 
> 
>>> diff --git a/sysdeps/unix/sysv/linux/arc/shlib-versions b/sysdeps/unix/sysv/linux/arc/shlib-versions
>>> new file mode 100644
>>> index 000000000000..343c0a04500e
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/arc/shlib-versions
>>> @@ -0,0 +1,7 @@
>>> +DEFAULT                 GLIBC_2.32
>>> +
>>> +%ifdef HAVE_ARC_BE
> 
> This is where the BE define is used.
> 
>>> +ld=ld-linux-arceb.so.2
>>> +%else
>>> +ld=ld-linux-arc.so.2
>>> +%endif
> 

_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc