[llvm] r227300 - [LPM] Rip all of ManagedStatic and ThreadLocal out of the pretty stack

[Chris Bieneman]

A few comments below inline.

> On Jan 28, 2015, at 5:23 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> 
> 
> On Wed, Jan 28, 2015 at 4:53 PM, Owen Anderson <resistor at mac.com <mailto:resistor at mac.com>> wrote:
> 
>> On Jan 28, 2015, at 3:05 PM, Chandler Carruth <chandlerc at gmail.com <mailto:chandlerc at gmail.com>> wrote:
>> 
>> 
>> On Wed, Jan 28, 2015 at 2:14 PM, Owen Anderson <resistor at mac.com <mailto:resistor at mac.com>> wrote:
>> Hi Chandler,
>> 
>>> On Jan 28, 2015, at 1:52 AM, Chandler Carruth <chandlerc at gmail.com <mailto:chandlerc at gmail.com>> wrote:
>>> 
>>> -static ManagedStatic<sys::ThreadLocal<const PrettyStackTraceEntry> > PrettyStackTraceHead;
>>> +// We need a thread local pointer to manage the stack of our stack trace
>>> +// objects, but we *really* cannot tolerate destructors running and do not want
>>> +// to pay any overhead of synchronizing. As a consequence, we use a raw
>>> +// thread-local variable. Some day, we should be able to use a limited subset
>>> +// of C++11's thread_local, but compilers aren't up for it today.
>>> +// FIXME: This should be moved to a Compiler.h abstraction.
>>> +#ifdef _MSC_VER // MSVC supports this with a __declspec.
>>> +static __declspec(thread) const PrettyStackTraceEntry
>>> +    *PrettyStackTraceHead = nullptr;
>>> +#else // Clang, GCC, and all compatible compilers tend to use __thread.
>>> +static __thread const PrettyStackTraceEntry *PrettyStackTraceHead = nullptr;
>>> +#endif
>> 
>> Darwin doesn’t support __thread or thread_local on armv7 or arm64.  This is breaking all Darwin embedded builds.  :-(
>> 
>> So, this is part of C++11 and threading... I don't know how to support a system which wants a threaded LLVM but doesn't support TLS. I'm happy to make these compile out when threading is disabled of course. Do you have any other suggestions?
> 
> Not particularly, other than restoring (part of?) the old behavior.  Threaded LLVM is required by multiple users on embedded Darwin.
> 
> Ok dude.
> 
> You complained about compile time hit from fixing a race condition. I didn't know how to fix it and it didn't significantly impact any other users. Still, I did my absolute best, put off other work, and found a way to speed things up. It even helps other users a small amount. Awesome. I'm actually still working on getting all the platforms working that we have active build bots for.
> 
> Now some of the functionality doesn't work on a some other platform I don't have access to, we don't have build bots for, and I can't possibly debug.

This is a real problem. Unfortunately I don’t have a solution for it yet, but I am working on getting public LLVM sources to build with publicly available iOS tools. I’m close, and I’ll hopefully have it working this week. Once I have it working we can hopefully get a bot setup.

> 
> Please propose patches which address your needs. Personally, I would suggest teaching Clang to support thread_local when targeting iOS. LLVM requires C++11 in large part for threading. This requirement seems unlikely to go away, and the need for TLS in that context is *huge*. This is just the first occasion.

I’m a little nervous in general about using C++11 threading (and in particular TLS) in LLVM. This is probably in part because I got bit by it pretty hard with the whole init_once thing...

It isn’t just iOS that has problems with TLS. Microsoft lists TLS support as “Partial” even in the latest revisions of Visual Studio (https://msdn.microsoft.com/en-us/library/hh567368.aspx <https://msdn.microsoft.com/en-us/library/hh567368.aspx>), and they have a pretty big article on the rules and limitations for using it (https://msdn.microsoft.com/en-us/library/2s9wt68x.aspx <https://msdn.microsoft.com/en-us/library/2s9wt68x.aspx>). If past experience means anything, when the MSDN has an article about issues with a feature there’s usually a lot more problems that aren’t even mentioned.

I certainly think it is a noble goal to require full C++11 conformance on all our supported platforms, but I think we are a VERY long way off from being able to require it.

> 
> Still, if that is simply impossible, my suggestion would be to go into PrettyStackPrinter.cpp (and any other future user of thread_local) and wrap the access to the pointer in a function that does completely manual pthread_create, pthread_set_specific, and pthread_get_specific. It's almost impossible to even wrap this up in a helper class because of the lifetime issues that cause the destructor to run while the TLS is being used. At least for other users of TLS we could potentially just have the embedded darwin target use the slow managed static of an llvm::ThreadLocal<T> with all its destructor madness.
> 
> Maybe the latter will even be done for you because I may not be able to fix PPC64 any other way.
> 
> That's about all I've got. I'm somewhat regretting investing as much time as I have in this issue.

<politically correct hat> I’m sorry that this has been a less than ideal interaction. </politically correct hat>

Thank you for all your help with this. We do really appreciate it.

-Chris

> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150129/8c5a2812/attachment.html>