Page MenuHomePhabricator

Fix race conditions in Core/Timer
ClosedPublic

Authored by tberghammer on Oct 21 2015, 6:36 AM.

Details

Summary

Fix race conditions in Core/Timer

The Timer class already had some support for multi-threaded access
but it still contained several race conditions. This CL fixes them
in preparation of adding multi-threaded dwarf parsing (and other
multi-threaded parts later).

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer retitled this revision from to Fix race conditions in Core/Timer.
tberghammer updated this object.
tberghammer added reviewers: labath, clayborg.
tberghammer added a subscriber: lldb-commits.
labath accepted this revision.Oct 21 2015, 6:57 AM
labath edited edge metadata.

lgtm

This revision is now accepted and ready to land.Oct 21 2015, 6:57 AM
clayborg requested changes to this revision.Oct 21 2015, 10:25 AM
clayborg edited edge metadata.

"thread_local" isn't well supported and has performance issues. See inlined comments.

include/lldb/Core/Timer.h
89 ↗(On Diff #38005)

Not sure if all platforms support thread_local correctly. MacOSX might not. I also spoke with some compiler folks on using "thread_local" and they warned that the C++11 spec requires a bunch of other stuff gets initialized when _any_ thread_local variable gets used, so I was told that using the "thread" instead of much better to do. So switch this from thread_local to thread and it should work on most platforms. Not sure if windows will have a problem with this.

So the best fix is probably to make a LLDB_THREAD_LOCAL #define in lldb-defines.h and define it to "__thread" by default. Windows can fix up as needed.

source/Core/Timer.cpp
27 ↗(On Diff #38005)

Use LLDB_THREAD_LOCAL instead of "thread_local".

This revision now requires changes to proceed.Oct 21 2015, 10:25 AM
zturner added inline comments.
include/lldb/Core/Timer.h
89 ↗(On Diff #38005)

Actually can you use llvm::ThreadLocal instead?

__thread doesn't exist on Windows, and neither does thread_local. ThreadLocal<T> in llvm is the only thing I know that works everywhere.

Once I get the Python 3 stuff finished, we can move to MSVC 2015, which *does* support thread_local. Then we can use the LLDB_THREAD_LOCAL define as you suggest.

Does that work for everyone?

Yes, please use any llvm system utilities when possible. And scratch the need for LLDB_THREAD_LOCAL at any point in the future and just use llvm::ThreadLocal all the time as thread local variables have already been abstracted by llvm.

Sad as it may seem, I actually think we may *need* this LLDB_THREAD_LOCAL at some point in the future. The reason is that there is at least one place in LLDB (I think it might even be in Timer, but it's been a while since I found this) that relies on thread local destructors. It is possible to make this work on Windows, but the code is absolutely crazy. You have to use pragmas to manually inject magic sections into the binary. Anyway, this actually works using the C++11 thread_local keyword under MSVC 2015, so the fact that we don't have those semantics on Windows is currently a bug, and the only way I know of to get them in a sane manner is to use thread_local.

Anyway, TL;DR - If you need Thread Local destructors, you can't use llvm::ThreadLocal

tberghammer edited edge metadata.

We already used some thread local storage in the same file to store the current Timer stack so I decided to move the depth to the same storage object (it is using the Host::ThreadLocalStorage* methods).

Originally I decided to go with the thread_local keyword because I expected it to be more efficient for integral types then the other methods and based on my understanding it should be supported since MSVC 2010 for integral types.

clayborg accepted this revision.Oct 22 2015, 9:30 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Oct 22 2015, 9:30 AM
This revision was automatically updated to reflect the committed changes.