This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3 09/19] RISC-V: Startup and Dynamic Loading Code
- From: Darius Rad <darius at bluespec dot com>
- To: Joseph Myers <joseph at codesourcery dot com>, Palmer Dabbelt <palmer at dabbelt dot com>
- Cc: libc-alpha at sourceware dot org, patches at groups dot riscv dot org
- Date: Thu, 4 Jan 2018 13:34:07 -0500
- Subject: Re: [PATCH v3 09/19] RISC-V: Startup and Dynamic Loading Code
- Authentication-results: sourceware.org; auth=none
- References: <20171227060534.3998-1-palmer@dabbelt.com> <20171227060534.3998-10-palmer@dabbelt.com> <alpine.DEB.2.20.1801011450260.394@digraph.polyomino.org.uk>
On 01/01/2018 09:55 AM, Joseph Myers wrote:
> On Tue, 26 Dec 2017, Palmer Dabbelt wrote:
>
>> +/* Return the link-time address of _DYNAMIC. */
>> +static inline ElfW(Addr)
>> +elf_machine_dynamic (void)
>> +{
>> + extern ElfW(Addr) _GLOBAL_OFFSET_TABLE_ __attribute__ ((visibility("hidden")));
>
> Missing space between "visibility" and "(". Please check for and fix such
> issues throughout the patch series.
>
Thanks, I'll fix here and elsewhere in the series.
>> +/* Return the run-time load address of the shared object. */
>> +static inline ElfW(Addr)
>> +elf_machine_load_address (void)
>> +{
>> + ElfW(Addr) load_addr;
>> + asm ("lla %0, _DYNAMIC" : "=r"(load_addr));
>
> Missing space before "(load_addr)". Again, please check for and fix such
> issues in asms throughout the patch series.
>
Will do.
>> + /* Check for unexpected PLT reloc type. */
>> + if (__builtin_expect (r_type == R_RISCV_JUMP_SLOT, 1))
>> + {
>> + if (__builtin_expect (map->l_mach.plt, 0) == 0)
>
> When __builtin_expect is being used for a boolean condition, please use
> __glibc_likely and __glibc_unlikely instead throughout this patch series.
> So that's __glibc_likely (r_type == R_RISCV_JUMP_SLOT) and __glibc_likely
> (map->l_mach.plt == 0), in this case.
>
Understood, thanks. I'll fix here and throughout.
>> +auto inline int
>> +__attribute__ ((always_inline))
>> +elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
>> +{
>> +#ifndef RTLD_BOOTSTRAP
>> + /* If using PLTs, fill in the first two entries of .got.plt. */
>> + if (l->l_info[DT_JMPREL])
>> + {
>> + extern void _dl_runtime_resolve (void) __attribute__ ((visibility("hidden")));
>
> Another missing space in visibility attribute.
>