llvm dev - Jun 2012 - [LLVMdev] [llvm-commits] [Patch, RFC] Re: Adding support for explicitly specified TLS models (PR9788)

(To the list this time..)

On Fri, Jun 15, 2012 at 7:38 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
>> The point still stands, though: that code requires the tls_model
>> attribute to be respected; if the compiler chooses local-exec instead,
>> it won't be dlopen-able.
>
> This is a fairly contrived use case, and an extension of a gcc extension.
>
>> I think it adds value to make the behaviour the same as GCC's. And
>> it's not like the difference wouldn't be detectable: the asm is
>> different, the .o files are different, and the code from the non-PIC
>> i386 shared lib example wouldn't load with dlopen.
>
> The gcc behavior is inconsistent with the linker and, as you found
> out, with itself.
Yeah, but I've gotten more attached to the GCC behaviour the more I've
thought about it :/

I respect that you have more experience with both LLVM and the TLS
stuff than me, though :)
> Before you wrote
>
>> I don't have any more compelling reasons than those, and I
don't feel
>> super strongly about this, so I'm willing to give in if others
>> disagree with me :)
>
> What about this:
>
> * We implement this first without a 'default' value. This covers
the
> one real world use case I know of (libc knowing it is not dlopened).
> * I am to blame if we missed some other use case and commit to
> implementing it in the future if/when it is found :-)
OK, let's give it a couple more days and see if someone else chimes in
or something new comes up. In the meantime, I'll rework the patch the
way you've suggested and if nothing's changed we can land it by the
end of next week. Sounds like a plan?

Thanks,
Hans

On Fri, Jun 15, 2012 at 9:56 PM, Hans Wennborg <hans at chromium.org>
wrote:
> (To the list this time..)
>
> On Fri, Jun 15, 2012 at 7:38 PM, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>>> The point still stands, though: that code requires the tls_model
>>> attribute to be respected; if the compiler chooses local-exec
instead,
>>> it won't be dlopen-able.
>>
>> This is a fairly contrived use case, and an extension of a gcc
extension.
>>
>>> I think it adds value to make the behaviour the same as GCC's.
And
>>> it's not like the difference wouldn't be detectable: the
asm is
>>> different, the .o files are different, and the code from the
non-PIC
>>> i386 shared lib example wouldn't load with dlopen.
>>
>> The gcc behavior is inconsistent with the linker and, as you found
>> out, with itself.
>
> Yeah, but I've gotten more attached to the GCC behaviour the more
I've
> thought about it :/
>
> I respect that you have more experience with both LLVM and the TLS
> stuff than me, though :)
>
>> Before you wrote
>>
>>> I don't have any more compelling reasons than those, and I
don't feel
>>> super strongly about this, so I'm willing to give in if others
>>> disagree with me :)
>>
>> What about this:
>>
>> * We implement this first without a 'default' value. This
covers the
>> one real world use case I know of (libc knowing it is not dlopened).
>> * I am to blame if we missed some other use case and commit to
>> implementing it in the future if/when it is found :-)
>
> OK, let's give it a couple more days and see if someone else chimes in
> or something new comes up. In the meantime, I'll rework the patch the
> way you've suggested and if nothing's changed we can land it by the
> end of next week. Sounds like a plan?
Attaching a new patch that has the behaviour we discussed.

The "globaldynamic" and default values have been merged, and LLVM will
start off with the user-specified model, but choose a more specific
one if possible.

Please review.

Thanks,
Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tls_models2.diff
Type: application/octet-stream
Size: 40607 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120620/ac5d72af/attachment.obj>

> Attaching a new patch that has the behaviour we discussed.
>
> The "globaldynamic" and default values have been merged, and LLVM
will
> start off with the user-specified model, but choose a more specific
> one if possible.
>
> Please review.
Awesome, thanks!
I will try to do a more complete review tonight or tomorrow. For now,
just two quick observations

*) This breaks the clang build, it would be nice to for the first
commit to keep the old API. It can be removed as soon as clang is
updated.
*) Please name the most general model GeneralDynamicTLSMode. Elf's
default visibility being called 'default' is already confusing enough
:-)

I was not sure how hard the first item would be, so I just gave it a
try. The resulting patch is attached.
> Thanks,
> Hans
Cheers,
Rafael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t.patch
Type: application/octet-stream
Size: 38564 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120620/61fd4a44/attachment.obj>