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

On Thu, Jun 14, 2012 at 9:33 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
>> Right, that's a good point.
>>
>> On i386 Linux, the following code,
>>
>>  static __thread int x[100000];
>>  int* get_x() { return x; }
>>
>> compiled with "clang -shared a.c -o a.so" won't be
possible do dlopen.
>
> I get exactly the same result with gcc 4.7 with and without
> -ftls-model=global-dynamic:
>
> movq %gs:0, %eax
> addl $x at ntpoff, %eax
Thanks for bearing with me on this. I think it's important that we get it
right.

I could have sworn I tried with -ftls-model yesterday, but apparently
I didn't. Seems it works differently than I assumed (I guess I should
have learnt not too assume things by now). Sorry about that.

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.

The -ftls-model command-line flag has the behavior you describe: the
user sets what the default model should be (global-dynamic if nothing
else is specified), but if the compiler can choose a better model,
that will be used instead. I want to implement this flag too, but that
will be the subject of another patch.


I still think the behaviour in my patch is right. This is an
implementation of a GCC attribute, so unless we have a very good
reason not to, we should make it behave in the same way. And I think
that behaviour makes sense: the attribute is a way for the programmer
to ask for something very specific, so that's what he should get; the
attribute is more like a requirement than a suggestion. AFAIK, that's
how other attributes work too. If I specified the always_inline
attribute on a function, I'd expect the compiler to do its best to
inline it, even if it might not be profitable.
>> Rafael, I think your argument has been that if we're unsure if
there
>> is any reason to strictly preserve the user's choice, we should
just
>> let LLVM choose a better model (and not have to specify any difference
>> between the "default" and global-dynamic models in the IR).
>
> Not quiet. My main argument is that we are not adding value, since the
> linker also does optimizations and the ELF format doesn't list if the
> choice of model was explicit or not.
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 question is what value it adds to make it work differently from GCC.
> Secondary arguments for going without a "default" value are:
>
> * It is much easier to add it if we find that we do need it than it is
> to remove it if we convince ourselves that it is redundant.
I don't think it is redundant.

(And removing the "default" value while preserving backwards
compatibility just means we've wasted one value in the bitcode
encoding.)
> * It adds complexity. For example, LLVM would have to distinguish
> being run in "compiler mode" versus "linker mode", as
the optimization
> is clearly valid at LTO time even if we find a reason for not doing it
> during compilation.
I don't know enough about how the LTO code works to comment on this one.

Thanks,
Hans

> 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.
> The question is what value it adds to make it work differently from GCC.
>
>> Secondary arguments for going without a "default" value are:
>>
>> * It is much easier to add it if we find that we do need it than it is
>> to remove it if we convince ourselves that it is redundant.
>
> I don't think it is redundant.
>
> (And removing the "default" value while preserving backwards
> compatibility just means we've wasted one value in the bitcode
> encoding.)
No, to do the upgrade without changing the final result, the value
that "default" should map too depends a lot on the context. It depends
if we are in a "compiler context" or a "linker context". It
depends on
-fPIC or not, and that information is not normally available during an
upgrade.
>
> I don't know enough about how the LTO code works to comment on this
one.
The summary is that in LTO we are part of the linker, so if the linker
can do an optimization, so can we.

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 :-)
> Thanks,
> Hans
Cheers,
Rafael

(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