On 11/12/2013 07:34 PM, Bernhard Reutner-Fischer wrote:
> I take it you double-checked the table below against the defines in
> include/elf.h

Yes.

>> +static const char *_dl_reltypes_tab[] =
>> +{
> +__asm__(
> +    ".section .text                                             \n"
> +    ".balign 4                                                  \n"
> +    ".global  _start                                            \n"
> .hidden _start ?

yes both global + hidden. Otherwise ld can't seem to find it.

>> diff --git a/ldso/ldso/arc/dl-syscalls.h b/ldso/ldso/arc/dl-syscalls.h
>> new file mode 100644
>> index 000000000000..1ab7b552a217
>> --- /dev/null
>> +++ b/ldso/ldso/arc/dl-syscalls.h
>> @@ -0,0 +1,8 @@
>> +/*
>> + * Copyright (C) 2013 Synopsys, Inc. (www.synopsys.com)
>> + *
>> + * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball.
>> + */
>> +
>> +#include "sys/syscall.h"
>> +#include <bits/uClibc_page.h>
> Sounds like it could go with the stub only?

Done.

>> diff --git a/ldso/ldso/arc/dl-sysdep.h b/ldso/ldso/arc/dl-sysdep.h
>> new file mode 100644
>> index 000000000000..642f020f42e1
>> --- /dev/null
>> +++ b/ldso/ldso/arc/dl-sysdep.h
>> @@ -0,0 +1,156 @@
>> +/*
>> + * Copyright (C) 2013 Synopsys, Inc. (www.synopsys.com)
>> + *
>> + * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball.
>> + */
>> +
>> +#include "elf.h"
>> +
>> +/*
>> + * Define this if the system uses RELOCA.
>> + */
>> +#define ELF_USES_RELOCA
>> +
>> +/*
>> + * Dynamic Linking ABI for ARCompact ISA
>> + *
>> + *                PLT
>> + *  --------------------------------
>> + *  |  ld r11, [pcl, off-to-GOT[1]  |  0   (20 bytes)
>> + *  |                               |  4
>> + * plt0     |  ld r10, [pcl, off-to-GOT[2]  |  8
> table layout seems to be broken here
>
>> + *  |                               | 12
>> + *  |  j [r10]                      | 16
>> + *  --------------------------------
>> + *  |    Base address of GOT        | 20
>> + *  --------------------------------
>> + *  |  ld r12, [pcl, off-to-GOT[3]  | 24   (12 bytes each)
>> + * plt1 |                           |
> .. and here.

All fixed.

>
>> + *  |  j_s.d  [r12]                 | 32
>> + *  |  mov_s  r12, pcl              | 34
>> + *  --------------------------------
>> + *  |                               | 36
>> + *      ~                           ~
>> + *      ~                           ~
>> + *  |                               |
>> + *  --------------------------------
>> + *
>> + *       GOT
>> + *  --------------
>> + *  |    [0]      |
>> + *  --------------
>> + *  |    [1]      |  Module info - setup by ldso
>> + *  --------------
>> + *  |    [2]      |  resolver entry point
>> + *  --------------
>> + *  |    [3]      |
>> + *  |    ...      |  Runtime address for function symbols
>> + *  |    [f]      |
>> + *  --------------
>> + *  |    [f+1]    |
>> + *  |    ...      |  Runtime address for data symbols
>> + *  |    [last]   |
>> + *  --------------
>> + */
>> +
>> +/*
>> + * Initialization sequence for a GOT.
>> + * Caller elf_resolve() seeds @GOT_BASE from DT_PLTGOT - which essentially 
>> is
>> + * pointer to first PLT entry. The actual GOT base is 5th word in PLT
>> + *
>> + */
>> +#define INIT_GOT(GOT_BASE,MODULE)                                           
>> \
>> +do {                                                                        
>>         \
>> +    unsigned long *__plt_base = (unsigned long *)GOT_BASE;                  
>> \
>> +    if(MODULE->libtype != program_interpreter)                              
>> \
> you sure you need the above?

Nope, Now removed.

>> +            GOT_BASE = (unsigned long *)(__plt_base[5] +                    
>> \
>> +                                         (unsigned long)MODULE->loadaddr);  
>> \
>> +    GOT_BASE[1] = (unsigned long) MODULE;                                   
>> \
>> +    GOT_BASE[2] = (unsigned long) _dl_linux_resolve;                        
>> \
>> +} while(0)
>> +
>> +/* Here we define the magic numbers that this dynamic loader should accept 
>> */
>> +#define MAGIC1 EM_ARCOMPACT
>> +#undef  MAGIC2
>> +
>> +/* Used for error messages */
>> +#define ELF_TARGET "ARC"
>> +
>> +struct elf_resolve;
>> +extern unsigned long _dl_linux_resolver(struct elf_resolve * tpnt,
>> +                                     unsigned int plt_pc);
>> +
>> +extern unsigned __udivmodsi4 (unsigned, unsigned)
>> +  __attribute ((visibility("hidden")));
> yuck ;)
> And please use __foo__ throughout, for attribute, inline.

OK.

>> +
>> +#define do_rem(result, n, base)  ((result) =                                
>> \
>> +                                                                    \
>> +    __builtin_constant_p (base) ? (n) % (unsigned) (base) :         \
> I'd prefer if you marked that as (__extension__ ({

You mean

-       ({                                                              \
+       __extension__ ({                                       \

If so, done.

However I read man gcc for __extension__, but not sure how it applies here (and
only here).

>> + */
>> +static inline Elf32_Addr elf_machine_dynamic (void) attribute_unused;
>> +static inline Elf32_Addr
> Perhaps you want  __always_inline and in similar spots?

Done for all three

>> +++ b/ldso/ldso/arc/elfinterp.c
>> @@ -0,0 +1,284 @@
>> +/*
>> + * Copyright (C) 2013 Synopsys, Inc. (www.synopsys.com)
>> + *
>> + * Lots of code copied from ../i386/elfinterp.c, so:
>> + * Copyright (c) 1994-2000 Eric Youngdale, Peter MacDonald,
>> + *               David Engel, Hongjiu Lu and Mitch D'Souza
>> + * Copyright (C) 2001-2002, Erik Andersen
>> + * All rights reserved.
>> + *
>> + * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball.
>> + */
>> +#include "ldso.h"
>> +
>> +unsigned long
>> +_dl_linux_resolver(struct elf_resolve *tpnt, unsigned int plt_pc)
>> +{
>> +    ELF_RELOC *this_reloc, *rel_base;
>> +    char *strtab, *symname, *new_addr;
>> +    ElfW(Sym) *symtab;
>> +    int symtab_index;
>> +    unsigned int *got_addr;
>> +    unsigned long plt_base;
>> +    int plt_idx;
>> +
>> +    /* start of .rela.plt */
>> +    rel_base = (ELF_RELOC *)(tpnt->dynamic_info[DT_JMPREL]);
>> +
>> +    /* starts of .plt (addr of PLT0) */
>> +    plt_base = tpnt->dynamic_info[DT_PLTGOT];
>> +
>> +    /*
>> +     * compute the idx of the yet-unresolved PLT entry in .plt
>> +         * Same idx will be used to find the relo entry in .rela.plt
>> +     */
> whitespace damage above

Oops - fixed now.

>> +    plt_idx = (plt_pc - plt_base)/0xc  - 2; /* ignoring 2 dummy PLTs */
> magic 12 ?

Added/used #define ARC_PLT_SIZE  12


>> +
>> +    this_reloc = rel_base + plt_idx;
>> +
>> +    symtab_index = ELF_R_SYM(this_reloc->r_info);
>> +    symtab = (ElfW(Sym) *)(intptr_t) (tpnt->dynamic_info[DT_SYMTAB]);
>> +    strtab = (char *) (tpnt->dynamic_info[DT_STRTAB]);
>> +    symname= strtab + symtab[symtab_index].st_name;
>> +
>> +    /* relo-offset to fixup, shd be a .got entry */
>> +    got_addr = (unsigned int *)(this_reloc->r_offset + tpnt->loadaddr);
>> +
>> +    /* Get the address of the GOT entry */
>> +    new_addr = _dl_find_hash(symname, &_dl_loaded_modules->symbol_scope, 
>> tpnt,
>> +                            ELF_RTYPE_CLASS_PLT, NULL);
>> +
>> +    if (unlikely(!new_addr)) {
>> +            _dl_dprintf(2, "%s: can't resolve symbol '%s'\n", _dl_progname, 
>> symname);
>> +            _dl_exit(1);
>> +    }
>> +
>> +
>> +#if defined (__SUPPORT_LD_DEBUG__)
> please remove braces

All fixed.

>> +static int
>> +_dl_do_lazy_reloc (struct elf_resolve *tpnt, struct r_scope_elem *scope,
>> +               ELF_RELOC *rpnt);
>> +
>> +static int
>> +_dl_do_reloc (struct elf_resolve *tpnt, struct r_scope_elem *scope,
>> +          ELF_RELOC *rpnt, ElfW(Sym) *symtab, char *strtab);
> Please restructure so you don't need these forward declarations.

Done.

>
> Also, it would be nice if you could switch to using a function pointer
> for the real handler, see all other arches.

I consciously did that micro-optimization to avoid the overhead of an indirect
function pointer for processing each symbol relo.

>> +}
>> diff --git a/ldso/ldso/arc/resolve.S b/ldso/ldso/arc/resolve.S
>> new file mode 100644
>> index 000000000000..8609b339effa
>> --- /dev/null
>> +++ b/ldso/ldso/arc/resolve.S
>> @@ -0,0 +1,59 @@
>> +/*
>> + * Copyright (C) 2013 Synopsys, Inc. (www.synopsys.com)
>> + *
>> + * Licensed under the LGPL v2.1, see the file COPYING.LIB in this tarball.
>> + */
>> +
>> +#define __ASSEMBLY__
> please remove that define.

Done.

>> +
>> +#include <bits/asm.h>
> Can you instead of asm.h use ENTRY and PLTJMP et al from sysdep.h please?
> Everywhere.

I was afraid this will come up :-) Fixed now.
Also while at it, converted ENDFUNC -> END to make it more conventional.

Thanks for your review.

-Vineet
_______________________________________________
uClibc mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/uclibc

Reply via email to