Timo Teräs <[email protected]> wrote on 2010/04/17 12:09:22:
>
> Joakim Tjernlund wrote:
> >> Finally looked at the ldso patch and I am not entirely happy with it.
> >> I think the general structure should be different to make it easier
> >> to follow the code and port it to other archs. I can probably
> >> take a stab at it during the weekend but I have no env. to test in
> >> so I will have to do it blindly.
> >
> > So this is what I came up with. I do wonder if not linux_resolver
> > need PROTECTED support too? Have you tested with LAZY relocation too?
>
> Have not tested lazy yet.
>
> > --- a/ldso/ldso/i386/elfinterp.c
> > +++ b/ldso/ldso/i386/elfinterp.c
> > @@ -175,11 +175,16 @@ _dl_do_reloc(struct elf_resolve *tpnt, struct dyn_elf
> > *scope,
> > symbol_addr = 0;
> > symname = strtab + symtab[symtab_index].st_name;
> >
> > - if (symtab_index &&
> > - (ELF32_ST_VISIBILITY(symtab[symtab_index].st_other)
> > - != STV_PROTECTED)) {
> > - symbol_addr = (unsigned long)_dl_find_hash(symname, scope, tpnt,
> > - elf_machine_type_class(reloc_type), &tls_tpnt);
> > + if (symtab_index) {
> > + if (ELF32_ST_VISIBILITY(symtab[symtab_index].st_other)
> > + != STV_PROTECTED)
> > + symbol_addr = (unsigned long)_dl_find_hash(symname, scope,
> > + tpnt,
> > + elf_machine_type_class(reloc_type),
> > + &tls_tpnt);
> > + else
> > + symbol_addr = DL_FIND_HASH_VALUE(tpnt,
> > elf_machine_type_class(reloc_type),
> > + &symtab[symtab_index]);
> >
> > /*
> > * We want to allow undefined references to weak symbols - this
> > @@ -190,11 +195,7 @@ _dl_do_reloc(struct elf_resolve *tpnt, struct dyn_elf
> > *scope,
> > && ELF32_ST_BIND(symtab[symtab_index].st_info) != STB_WEAK))
> > return 1;
> > } else {
> > - if (symtab_index)
> > - symbol_addr = DL_FIND_HASH_VALUE(tpnt,
> > elf_machine_type_class(reloc_type),
> > - &symtab[symtab_index]);
> > - else
> > - symbol_addr = symtab[symtab_index].st_value;
> > + symbol_addr = symtab[symtab_index].st_value;
> > tls_tpnt = tpnt;
> > }
>
> This looks functionally equivalent. And yes, looks like this is more
> consistent
> approach.
>
> Related, I noticed that _dl_lookup_hash that uses DL_FIND_HASH_VALUE does
> actually
> first a check for TLS symbols: if ELF_ST_TYPE(sym->st_info) == STT_TLS the
> DL_FIND_HASH_VALUE is never called. I would suspect that protected TLS symbols
> are broken currently.
hmm, looks like it. Either one can add a hack for that or just not support
protected for TLS ATM. Protected is very rare so we could get away with
just supporting it for internal uClibc needs.
The LAZY relocs is a bigger problem though. That has to work so it would be
great if
you could test that too.
>
> Maybe all this should be split into some new function?
>
> > diff --git a/ldso/ldso/i386/elfinterp.c b/ldso/ldso/i386/elfinterp.c
> > index 7e05c14..b04febc 100644
> > --- a/ldso/ldso/i386/elfinterp.c
> > +++ b/ldso/ldso/i386/elfinterp.c
> > @@ -194,11 +194,7 @@ _dl_do_reloc(struct elf_resolve *tpnt, struct dyn_elf
> > *scope,
> > if (unlikely(!symbol_addr && (ELF_ST_TYPE(symtab
> [symtab_index].st_info) != STT_TLS)
> > && ELF32_ST_BIND(symtab[symtab_index].st_info) != STB_WEAK))
> > return 1;
> > - } else {
> > - symbol_addr = symtab[symtab_index].st_value;
> > - tls_tpnt = tpnt;
> > }
> > -
>
> This part was added as part of support for TLS symbols. Other parts of dlso
> doing the same thing have comment such as:
> /* Relocs against STN_UNDEF are usually treated as using a
> symbol value of zero, and using the module containing the
> reloc itself. */
>
> Austin, since you did 534661b9 adding this, could you explain why this is
> needed?
Ahh, I see now. This is not 100% correct but it probably works because
symtab_index is only 0 for TLS and RELATIVE relocs so one wold not end up
in the other reloc types. I can't be sure though.
One might do this but I can't say for sure if one needs to:
diff --git a/ldso/ldso/i386/elfinterp.c b/ldso/ldso/i386/elfinterp.c
index 7e05c14..e44ac52 100644
--- a/ldso/ldso/i386/elfinterp.c
+++ b/ldso/ldso/i386/elfinterp.c
@@ -194,11 +194,10 @@ _dl_do_reloc(struct elf_resolve *tpnt, struct dyn_elf
*scope,
if (unlikely(!symbol_addr &&
(ELF_ST_TYPE(symtab[symtab_index].st_info) != STT_TLS)
&&
ELF32_ST_BIND(symtab[symtab_index].st_info) != STB_WEAK))
return 1;
- } else {
+ } else if (ELF_ST_TYPE(symtab[symtab_index].st_info) == STT_TLS) {
symbol_addr = symtab[symtab_index].st_value;
tls_tpnt = tpnt;
}
-
#if defined (__SUPPORT_LD_DEBUG__)
old_val = *reloc_addr;
@@ -237,22 +236,28 @@ _dl_do_reloc(struct elf_resolve *tpnt, struct dyn_elf
*scope,
break;
#if defined USE_TLS && USE_TLS
case R_386_TLS_DTPMOD32:
- *reloc_addr = tls_tpnt->l_tls_modid;
+ if (tls_tpnt)
+ *reloc_addr = tls_tpnt->l_tls_modid;
break;
case R_386_TLS_DTPOFF32:
/* During relocation all TLS symbols are defined and
used.
* Therefore the offset is already correct. */
- *reloc_addr = symbol_addr;
+ if (tls_tpnt)
+ *reloc_addr = symbol_addr;
break;
case R_386_TLS_TPOFF32:
/* The offset is positive, backward from the thread
pointer. */
- CHECK_STATIC_TLS((struct link_map*) tls_tpnt);
- *reloc_addr += tls_tpnt->l_tls_offset - symbol_addr;
+ if (tls_tpnt) {
+ CHECK_STATIC_TLS((struct link_map*) tls_tpnt);
+ *reloc_addr += tls_tpnt->l_tls_offset -
symbol_addr;
+ }
break;
case R_386_TLS_TPOFF:
/* The offset is negative, forward from the thread
pointer. */
- CHECK_STATIC_TLS((struct link_map*) tls_tpnt);
- *reloc_addr += symbol_addr - tls_tpnt->l_tls_offset;
+ if (tls_tpnt) {
+ CHECK_STATIC_TLS((struct link_map*) tls_tpnt);
+ *reloc_addr += symbol_addr -
tls_tpnt->l_tls_offset;
+ }
break;
#endif
default:
>
> Looking also */elfinterp.c, there seems to be a lot of common code. I wonder
> if these could be merged somehow.
Oh yes, it is getting hard to maintain this code.
_______________________________________________
uClibc mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/uclibc
