Re: [PATCH] PPC64 tls fixes

[Roland McGrath <roland at redhat dot com>]

> The problem was that none of the R_PPC64_JMP_SLOT relocations where done 
> for ld64.so itself. 

I see.  Thanks for locating that problem.  I would have thought that the
compiler would warn about the uninitialized variable (sym_map) and so this
could be seen and fixed early on (as I said, I wasn't able to test building
it myself).

> Along the way I had to fix up some compile failures and warnings: 
> include dl-tls.h, remove the duplicate declare of sym_map, and add 
> parentheses to BIT_INSERT to insure the correct operation order and 
> elliminate the related compiler warning.

Like I said, there were bound to be typos and other errors since I didn't test.

> However (at least for ppc64) the DTPREL16* relocs will never generate 
> dynamic endties.

I'm assuming this is a typo for "entries" and you meant dynamic relocs.  
I asked about this earlier and Alan Modra answered my question on Monday
with this answer contrary to what you've just said:

> As far as I'm aware, there's nothing in the ppc64 ABI that prohibits
> position dependent code.  Thus it's valid to generate the ADDR16 relocs
> in a shared library.

> 2) The full expansion of CHECK_STATIC_TLS and TLS_TPREL_VALUE is large
> (128 bytes for ppc64). Expanding this code 5 (ppc32) to 10 (ppc64) times 
> inline seems excessive. Especially when elf_machine_rela is itself 
> expanded several times. A (out of line) static function 
> (elf_machine_tprel) would be appropriate to call from within the case 
> statements that requires this logic.

In fact elf_machine_rela is expanded twice where this code is used at all.
Those are dl-reloc.c and dl-conflict.c; in the latter, CHECK_STATIC_TLS is
empty.  Moving code out of macros into functions is fine when it both works
and in fact improves things.  I concentrate first on correctness for all cases.

> 3) The whole implementation seems overly complicated and hard to understand.

You'll have to be more specific.  If you mean the dynamic linker overall,
then tough beans.  You try to write one portable to a dozen machines, more
than one operating system, with the full set of features, and with minimal
cut&paste code duplication.  If you mean my use of inlines and macros in
powerpc32/dl-machine.h, then this is exactly the kind of technique that we
use to minimize error-prone code duplication and it has saved us much more
time and headache than it has cost you, trust me.  Almost every extant port
has been bitten at one time or another by careless copying of code when
factoring was possible, resulting in the same bugs in multiple places and
not always getting fixed everywhere at once.

> I'll hold my patch till tomorrow to give you a chance to respond to the 
> comments above.

It never makes sense to withhold code or other technical information.
Always post what you have, even if you don't expect it to be used.  If you
don't expect it to be the final version, then you don't need to bother
writing up log entries or anything else that might be time-consuming.  It
only takes a second to generate and include a patch, and saves incalculable
amounts of speculation and duplicated work arising from not knowing what
you have already done.  To wit, I have just done and checked in fixes for
the bugs that you did report in this message, but have no way of knowing if
I might have overlooked anything else you touched in your changes since you
haven't shown them to me.  

> This comment is unwarranted and unfair since you never bothered to look 
> at my improved implementation.

I indeed did look at the patch you sent and my comments were about the code
both before and after them.  If you have had anything improved to the point
that my comments no longer apply, then I have not yet seen it.