arch-specific reloc.h inconsistencies (bugs?)

arch-specific reloc.h inconsistencies (bugs?)

[Rich Felker]

I'm trying to refactor arch/*/reloc.h to replace the arch-specific
do_single_reloc functions with an arch-generic version in dynlink.c
and a simple remapping in reloc.h from arch-specific reloc types to a
few generic operations that cover everything. This should reduce the
burden of porting to new archs, but the immediate motivation is that
I'm trying to add "gnu2" tls model (aka TLSDESC, see
http://www.fsfla.org/~lxoliva/writeups/TLS/RFC-TLSDESC-x86.txt) and if
I don't refactor this now it's going to lead to some really ugly
complex code duplication whereby each reloc.h file copies logic
that's not arch knowledge but rather an implementation detail of
musl's dynamic linker. And this would be really nasty to maintain in
the future.

Anyway, as the first step for this task, I've been looking over the
reloc types in each arch/*/reloc.h file, and I found a few
inconsistencies that seem like they may be bugs. I'm going to do more
research, but if anyone already has ideas on these, please let me know
so I don't waste time on it:

1. powerpc and sh use RELA (addend in the reloc structure rather than
   inline at the address to be relocated), but the addend is ignored
   and an inline addend is instead read for thread-pointer-relative
   (TPREL) relocations. On sh this also happens for DTPMOD and DTPOFF
   relocations. For DTPMOD that's definitely a bug since an addend
   makes no sense for these whatsoever, but I'm not sure about the
   others. It might be a "historical bug" that has to be duplicated
   for compatibility with toolchains that enshrined it, or it might
   just be a musl bug (likely due to copy-and-paste from other archs).

2. On sh, GLOB_DAT and JMP_SLOT relocations ignore the addend. This
   seems like it would give wrong results for things like:
   char *global2 = &global1[N];

3. Also on sh, there's a strange REL32 relocation that applies
   base_addr as an addend to an expression which already contains a
   symbol value. This seems nonsensical to me just from a "units"
   perspective but maybe it's right since another address (the address
   to which the relocation is being supplied) is subtracted.

4. mips has a REL32 relocation which doubles as both a relative
   (adjust against self's load address) relocation and a symbolic
   (adjust to point to the address a symbol resolves to) one. The
   current logic for which way to treat it as is based on the resolved
   symbol value (sym_val, whether it's zero or nonzero) whereas I
   think it should be based on whether the symbol was found or not
   (i.e. whether def.sym is null or not). This likely does not matter
   in practice but it seems like a bug.

With that said, I have locally a working patch for TLSDESC support on
i386 already, and based on naive cycle counting it looks like it may
be measurably faster than glibc's implementation when dynamic TLS is
used, and possibly fast enough that it's not measurably slower than
static TLS in real-world usage cases. If resolving the reloc.h
refactoring issues doesn't happen immediately I'll go ahead and commit
the code for i386 with the intent of finishing the refactoring before
I add TLSDESC for other archs.

Rich

Re: arch-specific reloc.h inconsistencies (bugs?)

[Rich Felker]

On Tue, Jun 17, 2014 at 01:01:32AM -0400, Rich Felker wrote:
> 4. mips has a REL32 relocation which doubles as both a relative
>    (adjust against self's load address) relocation and a symbolic
>    (adjust to point to the address a symbol resolves to) one. The
>    current logic for which way to treat it as is based on the resolved
>    symbol value (sym_val, whether it's zero or nonzero) whereas I
>    think it should be based on whether the symbol was found or not
>    (i.e. whether def.sym is null or not). This likely does not matter
>    in practice but it seems like a bug.

No, whether the symbol was found is wrong too. I think it should be on
whether there was a symbol reference to begin with (since some relocs
may have references that are only weak and thus don't resolve; these
are still symbolic relocations, not relative ones).

Rich

Re: arch-specific reloc.h inconsistencies (bugs?)

[Rich Felker]

On Tue, Jun 17, 2014 at 01:01:32AM -0400, Rich Felker wrote:
> 1. powerpc and sh use RELA (addend in the reloc structure rather than
>    inline at the address to be relocated), but the addend is ignored
>    and an inline addend is instead read for thread-pointer-relative
>    (TPREL) relocations. On sh this also happens for DTPMOD and DTPOFF
>    relocations. For DTPMOD that's definitely a bug since an addend
>    makes no sense for these whatsoever, but I'm not sure about the
>    others. It might be a "historical bug" that has to be duplicated
>    for compatibility with toolchains that enshrined it, or it might
>    just be a musl bug (likely due to copy-and-paste from other archs).
> 
> 2. On sh, GLOB_DAT and JMP_SLOT relocations ignore the addend. This
>    seems like it would give wrong results for things like:
>    char *global2 = &global1[N];

Based on reading uClibc and glibc dynamic linker sources, these issues
seem to be bugs in musl.

I did however find what seems to be a related bug in glibc: when
processing R_SH_RELATIVE, rather than using the RELA or inline addend
conditionally based on whether the relocation is RELA-form, it uses
the RELA addend if it's nonzero and otherwise uses an inline addend.
In theory this could be a workaround for historical toolchain bugs
(storing an inline addend despite the ELF semantics requiring it to be
in the RELA structure) but I suspect it's just a bug that doesn't
trigger with real toolchains. uClibc does not have this behavior.

> 3. Also on sh, there's a strange REL32 relocation that applies
>    base_addr as an addend to an expression which already contains a
>    symbol value. This seems nonsensical to me just from a "units"
>    perspective but maybe it's right since another address (the address
>    to which the relocation is being supplied) is subtracted.

As far as I can tell, this is also a bug in musl. As I said it makes
no sense to me, and neither uClibc nor glibc is doing this.

So I'll fix these bugs first in the existing framework, then start on
refactoring. This way the refactoring should (at least mostly) avoid
functional changes.

Rich