--I see there is currently no usage of this in the tree, and I would like to introduce some. To my surprise I can find no evidence of prior discussion.What's the deal? Can we use it? If not, is there a suitable alternative?Specifically of interest is a need to ensure that the lifetime of my thread-local thing is actually bounded by each thread's lifetime, and our //base thread-local support does not appear to be sufficient for such things.
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/415f44d6-88ed-476c-a6e1-cacccfe2cc85%40chromium.org.
(moving to cxx@ per http://chromium-cpp.appspot.com/)On Mon, Aug 14, 2017 at 1:11 PM, Ken Rockot <roc...@chromium.org> wrote:I see there is currently no usage of this in the tree, and I would like to introduce some. To my surprise I can find no evidence of prior discussion.What's the deal? Can we use it? If not, is there a suitable alternative?
----Specifically of interest is a need to ensure that the lifetime of my thread-local thing is actually bounded by each thread's lifetime, and our //base thread-local support does not appear to be sufficient for such things.
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/415f44d6-88ed-476c-a6e1-cacccfe2cc85%40chromium.org.
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiFojdFNd5UcF%2BqiedimL6ATGg6MDox4gPk0LWGO1yx8Eg%40mail.gmail.com.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiFojdFNd5UcF%2BqiedimL6ATGg6MDox4gPk0LWGO1yx8Eg%40mail.gmail.com.
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CA%2BapAgF9Y4_DGwECAfhqWz3aDCkh6NgtiPXU8HB80S_eUsRMCg%40mail.gmail.com.
+aglUsing a thread-local cache for something like base::RandBytes is tricky because it is not safe against fork(), which Chromium does do by way of the zygote. It's probably better off for stuff with crypto
needs[1] to use BoringSSL's implementation which manages thread-locals internally. Also on machines with getrandom(), RAND_bytes will seed with that which avoids some problems /dev/urandom has at very very early boot. We'd need to sort out allowing //base to depend on //third_party/boringssl, but there's no circular dependency, and BoringSSL is static-linker friendly[2], so you wouldn't pull in what you don't need. This would also let us remove //base/sha1.cc which is an unnecessary duplicate SHA-1 implementation.That said, BoringSSL RAND_bytes will also hit an entropy source on each call because we need to be fork-safe, so this won't immediately solve your problem. On machines with RDRAND, RAND_bytes will use that which should avoid the overhead. We do have a "fork-unsafe" mode for machines without RDRAND, but Chromium does actually fork.Can you say a bit more about your needs here? It's not very common to need so much randomness that this overhead matters.
MSVC places thread local variables(*) in a special section (.tls) . A single TLS slot points to the base of the thread-specific copy of this section so accessing a thread_local variable is .tls_base[offset]. I'll let Linux/macOS experts chime in about how things are done on these platforms although it seems very close to the PE/COFF solution.(*) MSVC implements thread_local pretty much by repackaging of the old declspec(thread) with a few rough corners (around initialization) fixed.
On Tue, Aug 15, 2017 at 3:56 PM, Brett Wilson <bre...@chromium.org> wrote:On Tue, Aug 15, 2017 at 3:47 PM, Leonard Mosescu <mos...@chromium.org> wrote:On Tue, Aug 15, 2017 at 3:04 PM, Brett Wilson <bre...@chromium.org> wrote:I don't think we would want to use anything but the base implementation of TLS.On Android, there are very few TLS slots (I found a reference to 60 somewhere) and were hitting the maximum. The base implementation maps a 256-entry TLS array into a single OS TLS slot to avoid this problem. I would assume that an STL implementation of TLS would just call the native OS APIs and if people start using them we would be running out of slots again.There may be some valid concerns about thread_local but this should not be one of them (it is not technically feasible to map thread_local variables to individual TLS slots provided by the host OS).We should probably put this on the explicitly banned list.What's the rule of thumb here? I'm new to Chrome but I'd expect that all thing being equal we'd favor standard language features over hand crafted alternatives. Thread storage class is a first class, fundamental part of C++ since C++11, with well defined semantics (and one would expect first class, solid implementations in all the major toolchains)If the implementation is good and does what we want on all platforms for all compilers we support and there are no specific concerns, then we should use it. I am not so optimistic as you about uniformly good implementations though...Brett
--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANFFm8g_hFQg_M1afPyJ2uWAkBi1cjAdqzHwvykEPF-hKFq27A%40mail.gmail.com.
Also worth mentioning in this thread: base now provides SequenceLocalStorageSlot as a sequence-friendly version of ThreadLocalStorage::Slot. As we move more and more towards sequences, I suspect most TLS usage outside of base internals should migrate to SLS.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANFFm8h8BR04WUzsKX8MY_tu1xV%3Do0QrqPMhJemzXS-NsyMYQA%40mail.gmail.com.
The caveat on Mac is that we cannot access TLS objects during the destructors of other TLS objects, which seems fairly reasonable
The caveat on Mac is that we cannot access TLS objects during the destructors of other TLS objects, which seems fairly reasonableIt's worth noting that this is by design and not an implementation issue. It's pretty much the same constraint that applies to global objects.
What I found is that on Mac, having a TLS object will automatically generate and accessor function, which dynamically allocates memory for the object on first access on that thread. They get destroyed/deallocated on thread destruction, which means we shouldn't access even POD TLS data on the destructors.I still think this is a reasonable limitation given the benefits of improved performance and better generated code.As for the specific issue you mentioned with AtomicStrings, I haven't seen that happening, it's possible that it got fixed. In particular, this would also be bad even without thread_local, since accessing an AtomicString after the destruction on the WTFThreadData would lead to leaked memory, since the WTFThreadData would be re-allocated and nothing will free it afterwards. I'm fine if that gets changed to a crash, since it should be fixed anyway.
static void TLSThreadTest();
static void TLSThreadTest2();
class TLSHelperB {
public:
TLSHelperB() {
LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;
}
~TLSHelperB() {
LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;
TLSThreadTest();
}
};
class WTFTLSHelperB {
public:
WTFTLSHelperB() {
LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;
}
~WTFTLSHelperB() {
LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;
TLSThreadTest2();
}
};
class TLSHelperA {
public:
TLSHelperA() {
static thread_local TLSHelperB tls_helper;
DEFINE_THREAD_SAFE_STATIC_LOCAL(WTF::ThreadSpecific<WTFTLSHelperB>, tls_helper_wtf, ());
*tls_helper_wtf;
LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;
}
~TLSHelperA() {
LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;
}
};
class WTFTLSHelperA {
public:
WTFTLSHelperA() {
static thread_local TLSHelperB tls_helper;
DEFINE_THREAD_SAFE_STATIC_LOCAL(WTF::ThreadSpecific<WTFTLSHelperB>, tls_helper_wtf, ());
*tls_helper_wtf;
LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;
}
~WTFTLSHelperA() {
LOG(ERROR) << this << " " << __PRETTY_FUNCTION__;
}
};
static void TLSThreadTest() {
LOG(ERROR) << "Start " << __PRETTY_FUNCTION__;
static thread_local TLSHelperA tls_helper_a;
DEFINE_THREAD_SAFE_STATIC_LOCAL(WTF::ThreadSpecific<WTFTLSHelperA>, tls_helper_wtf, ());
*tls_helper_wtf;
LOG(ERROR) << "End " << __PRETTY_FUNCTION__;
}
static void TLSThreadTest2() {
LOG(ERROR) << "Start " << __PRETTY_FUNCTION__;
static thread_local TLSHelperA tls_helper_a;
DEFINE_THREAD_SAFE_STATIC_LOCAL(WTF::ThreadSpecific<WTFTLSHelperA>, tls_helper_wtf, ());
*tls_helper_wtf;
LOG(ERROR) << "End " << __PRETTY_FUNCTION__;
}
TEST(..., TLS) {
Thread a("Test");
EXPECT_TRUE(a.Start());
a.task_runner()->PostTask(
FROM_HERE,
base::BindOnce(&TLSThreadTest));
a.task_runner()->PostTask(
FROM_HERE,
base::BindOnce(&TLSThreadTest2));
a.Stop();
}
And here's the output:
Start void WTF::TLSThreadTest()
0x7ffef6e08582 WTF::TLSHelperB::TLSHelperB()
0x27fae18038 WTF::WTFTLSHelperB::WTFTLSHelperB()
0x7ffef6e08580 WTF::TLSHelperA::TLSHelperA()
0x7ffef6e08590 WTF::TLSHelperB::TLSHelperB()
0x27fae18088 WTF::WTFTLSHelperB::WTFTLSHelperB()
0x27fae18060 WTF::WTFTLSHelperA::WTFTLSHelperA()
End void WTF::TLSThreadTest()
Start void WTF::TLSThreadTest2()
0x7ffef6e085a0 WTF::TLSHelperA::TLSHelperA()
0x27fae180b0 WTF::WTFTLSHelperA::WTFTLSHelperA()
End void WTF::TLSThreadTest2()
0x7ffef6e085a0 WTF::TLSHelperA::~TLSHelperA()
0x7ffef6e08590 WTF::TLSHelperB::~TLSHelperB()
Start void WTF::TLSThreadTest()
End void WTF::TLSThreadTest()
0x7ffef6e08580 WTF::TLSHelperA::~TLSHelperA()
0x7ffef6e08582 WTF::TLSHelperB::~TLSHelperB()
Start void WTF::TLSThreadTest()
End void WTF::TLSThreadTest()
0x27fae18038 WTF::WTFTLSHelperB::~WTFTLSHelperB()
Start void WTF::TLSThreadTest2()
0x7ffef6e07d62 WTF::TLSHelperB::TLSHelperB()
0x7ffef6e07d80 WTF::TLSHelperA::TLSHelperA()
End void WTF::TLSThreadTest2()
0x27fae18060 WTF::WTFTLSHelperA::~WTFTLSHelperA()
0x27fae18088 WTF::WTFTLSHelperB::~WTFTLSHelperB()
Start void WTF::TLSThreadTest2()
End void WTF::TLSThreadTest2()
0x27fae180b0 WTF::WTFTLSHelperA::~WTFTLSHelperA()
0x7ffef6e07d80 WTF::TLSHelperA::~TLSHelperA()
0x7ffef6e07d62 WTF::TLSHelperB::~TLSHelperB()
Start void WTF::TLSThreadTest()
0x27fae180b0 WTF::WTFTLSHelperB::WTFTLSHelperB()
0x7ffef6e07d60 WTF::TLSHelperA::TLSHelperA()
0x7ffef6e07d70 WTF::TLSHelperB::TLSHelperB()
0x27fae18060 WTF::WTFTLSHelperB::WTFTLSHelperB()
0x27fae18088 WTF::WTFTLSHelperA::WTFTLSHelperA()
End void WTF::TLSThreadTest()
0x27fae180b0 WTF::WTFTLSHelperB::~WTFTLSHelperB()
Start void WTF::TLSThreadTest2()
0x7ffef6e07d62 WTF::TLSHelperB::TLSHelperB()
0x7ffef6e07d80 WTF::TLSHelperA::TLSHelperA()
0x7ffef6e07d70 WTF::TLSHelperB::TLSHelperB()
0x27fae18038 WTF::WTFTLSHelperA::WTFTLSHelperA()
End void WTF::TLSThreadTest2()
0x27fae18088 WTF::WTFTLSHelperA::~WTFTLSHelperA()
0x27fae18060 WTF::WTFTLSHelperB::~WTFTLSHelperB()
Start void WTF::TLSThreadTest2()
End void WTF::TLSThreadTest2()
0x27fae18038 WTF::WTFTLSHelperA::~WTFTLSHelperA()
0x7ffef6e07d70 WTF::TLSHelperB::~TLSHelperB()
Start void WTF::TLSThreadTest()
0x27fae18038 WTF::WTFTLSHelperB::WTFTLSHelperB()
0x7ffef6e07d60 WTF::TLSHelperA::TLSHelperA()
0x27fae18088 WTF::WTFTLSHelperB::WTFTLSHelperB()
0x27fae18060 WTF::WTFTLSHelperA::WTFTLSHelperA()
End void WTF::TLSThreadTest()
0x7ffef6e07d80 WTF::TLSHelperA::~TLSHelperA()
0x7ffef6e07d62 WTF::TLSHelperB::~TLSHelperB()
Start void WTF::TLSThreadTest()
End void WTF::TLSThreadTest()
0x7ffef6e07d70 WTF::TLSHelperB::~TLSHelperB()
Start void WTF::TLSThreadTest()
End void WTF::TLSThreadTest()
0x7ffef6e07d60 WTF::TLSHelperA::~TLSHelperA()
0x27fae18038 WTF::WTFTLSHelperB::~WTFTLSHelperB()
Start void WTF::TLSThreadTest2()
0x7ffef6e07d62 WTF::TLSHelperB::TLSHelperB()
0x7ffef6e07d80 WTF::TLSHelperA::TLSHelperA()
0x7ffef6e07d70 WTF::TLSHelperB::TLSHelperB()
0x27fae180b0 WTF::WTFTLSHelperA::WTFTLSHelperA()
End void WTF::TLSThreadTest2()
0x27fae18060 WTF::WTFTLSHelperA::~WTFTLSHelperA()
0x27fae18088 WTF::WTFTLSHelperB::~WTFTLSHelperB()
Start void WTF::TLSThreadTest2()
End void WTF::TLSThreadTest2()
0x27fae180b0 WTF::WTFTLSHelperA::~WTFTLSHelperA()
0x7ffef6e07d70 WTF::TLSHelperB::~TLSHelperB()
Start void WTF::TLSThreadTest()
0x27fae180b0 WTF::WTFTLSHelperB::WTFTLSHelperB()
0x7ffef6e07d60 WTF::TLSHelperA::TLSHelperA()
0x27fae18060 WTF::WTFTLSHelperB::WTFTLSHelperB()
0x27fae18088 WTF::WTFTLSHelperA::WTFTLSHelperA()
End void WTF::TLSThreadTest()
0x7ffef6e07d80 WTF::TLSHelperA::~TLSHelperA()
0x7ffef6e07d62 WTF::TLSHelperB::~TLSHelperB()
Start void WTF::TLSThreadTest()
End void WTF::TLSThreadTest()
0x7ffef6e07d60 WTF::TLSHelperA::~TLSHelperA()
0x27fae180b0 WTF::WTFTLSHelperB::~WTFTLSHelperB()
Start void WTF::TLSThreadTest2()
0x7ffef6e07e82 WTF::TLSHelperB::TLSHelperB()
0x7ffef6e07ea0 WTF::TLSHelperA::TLSHelperA()
0x7ffef6e07e90 WTF::TLSHelperB::TLSHelperB()
0x27fae18038 WTF::WTFTLSHelperA::WTFTLSHelperA()
End void WTF::TLSThreadTest2()
0x27fae18088 WTF::WTFTLSHelperA::~WTFTLSHelperA()
0x27fae18060 WTF::WTFTLSHelperB::~WTFTLSHelperB()
Start void WTF::TLSThreadTest2()
End void WTF::TLSThreadTest2()
0x27fae18038 WTF::WTFTLSHelperA::~WTFTLSHelperA()
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13chS9Q_xUCVJ%3D0jkq4sLbVsBYUqni50sY4E-GWNj4%3D-JA%40mail.gmail.com.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALQVofpGyD4LdqkaQE9Fb-8v3Rm2BBN4n2NYfr_4j7o7SJ8sLw%40mail.gmail.com.
OK. I'm a little frustrated that the C++ spec doesn't seem to explicitly spell out what happens in cases like this, but it sounds like this is safe at least for a simple POD like is_main_thread, and we can see what issues we run into in practice from there. If thakis or danakj agrees, I'm happy to approve a CL which moves it to allowed with use by special dispensation and adds a use for a simple such case. I'm reluctant to give carte-blanche for broader use until we have more experience with how this interacts with the various other TLS in Chromium (because it tickles so many weird platform/compiler-specific things), but I would like to see us ultimately use the language-level thread_local if we can, because it's pretty nice. :)
thread_local
, but thread_local
with non-trivial destructors requires run time support that wasn't available until M (and I think the NDK's current libc++abi doesn't have the support either). There was actually a patch recently made to libc++abi adding support for this on platforms that don't have native support, so once that lands and we pick it up we could actually support that on older platforms."""To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACuR13d0FnS0Nga7EEEu%2BwdWciB1%2BRrUjzYQXTGvYdK88GZkSg%40mail.gmail.com.
On Tue, Nov 28, 2017 at 11:08 PM, Jeremy Roman <jbr...@chromium.org> wrote:OK. I'm a little frustrated that the C++ spec doesn't seem to explicitly spell out what happens in cases like this, but it sounds like this is safe at least for a simple POD like is_main_thread, and we can see what issues we run into in practice from there. If thakis or danakj agrees, I'm happy to approve a CL which moves it to allowed with use by special dispensation and adds a use for a simple such case. I'm reluctant to give carte-blanche for broader use until we have more experience with how this interacts with the various other TLS in Chromium (because it tickles so many weird platform/compiler-specific things), but I would like to see us ultimately use the language-level thread_local if we can, because it's pretty nice. :)For the record, I beliieve that thread_local was broken Android before Android M :-( [1]. It is possible to work around this by using a more recent version of the Android NDK and /or libc++abi sources though.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACnJMqqnp6nT-1iiV7oBHNXGO%2BkL6L2B3drGHjVHjXgqK43upQ%40mail.gmail.com.
We're still on r12b. A Skia change went in recently that had a fix we needed for Chromium to compile with r16 (that's probably what you saw). The Chromium-side changes seem to all work, so barring any unexpected blockers, r16 will probably be in shortly after branch point (I would guess early next week?).
OK. I'm a little frustrated that the C++ spec doesn't seem to explicitly spell out what happens in cases like this, but it sounds like this is safe at least for a simple POD like is_main_thread, and we can see what issues we run into in practice from there. If thakis or danakj agrees, I'm happy to approve a CL which moves it to allowed with use by special dispensation and adds a use for a simple such case. I'm reluctant to give carte-blanche for broader use until we have more experience with how this interacts with the various other TLS in Chromium (because it tickles so many weird platform/compiler-specific things), but I would like to see us ultimately use the language-level thread_local if we can, because it's pretty nice. :)
thakis/danakj, want to comment on the above? It seems like this thread is a bit in limbo right now, I'd like to see it move to a resolved place.
PK
--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDK9Y5OuzoG%3DcnGeN%2BC5kYAj3CEms7SWGvPY%2BYuCx7TMA%40mail.gmail.com.
--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/54654263-36c5-46a9-9d73-788185208794%40chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALz_-TYPChvLOzPkHfG947dO7vkZ6Km-nnbSwnvcRThTKwhYnA%40mail.gmail.com.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACuR13cbtMOmOGFe0pg%2B-vOLRHnjhr2FqiJ-PDTN6_B87J9JVQ%40mail.gmail.com.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.
--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFCwU%2BW9Qn1nB2X7EM_VnQjQs1gZkWBKRFxGhFF0gZPjYQ%40mail.gmail.com.
--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/4902fd04-4d69-45eb-a7e4-e42902f701fbn%40chromium.org.
We ran into an issue before where clang assumed incorrect alignment for thread-locals on Windows 7, producing SSE instructions that crashed: https://quiche.git.corp.google.com/quiche/+/cf0f085217c842fb5c04dde1025a3ef8f19fdbbc.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAZdMadS4pnQOGjOtv1%2BagCfwsQ%3DCivMB8eT9prppFRT%3DqKKzw%40mail.gmail.com.
I know that clang produces broken codegen with thread_local on macOS for thread_locals exported by one dylib and used by another: https://github.com/llvm/llvm-project/issues/51045
That's a fairly simple thing that's broken, which to me possibly suggests that `thread_local` isn't terribly widely used, and there might be other issues.
I don't remember the context at this point, since email retention policy has claimed it. What were the original problems with thread_local?
What's the current Chrome alternative? base::SequenceLocalStorageMap/base::SequenceLocalStorageSlot and base::ThreadLocal{Boolean,Pointer} and base::ThreadLocalStorage::StaticSlot/Slot?
Can we give some guidelines for when to use the various alternatives here?
--PK
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFA3VyOzSq2w0BwBhO5k3N5Mm8vaLPpL%2BHsxNzY1a3JNFg%40mail.gmail.com.
On Mon, Jan 2, 2023 at 4:13 PM Daniel Cheng <dch...@chromium.org> wrote:I don't remember the context at this point, since email retention policy has claimed it. What were the original problems with thread_local?jbroman@ 2017: "on Mac I had an issue where the TLS lookup symbol stops resolving (read: segfaults) if you access a thread_local variable past a certain point (which the pthreads thread-specific stuff does not do)"My take: This may be due to the bug Nico referenced. Possible workaround: disallow thread_local for exported variables.
brettw@ 2017: "On Android, there are very few TLS slots (I found a reference to 60 somewhere) and were hitting the maximum. The base implementation maps a 256-entry TLS array into a single OS TLS slot to avoid this problem. I would assume that an STL implementation of TLS would just call the native OS APIs and if people start using them we would be running out of slots again."
mosescu@ 2017: "There may be some valid concerns about thread_local but this should not be one of them (it is not technically feasible to map thread_local variables to individual TLS slots provided by the host OS). MSVC places thread local variables(*) in a special section (.tls) . A single TLS slot points to the base of the thread-specific copy of this section so accessing a thread_local variable is .tls_base[offset]. I'll let Linux/macOS experts chime in about how things are done on these platforms although it seems very close to the PE/COFF solution."My take: Not a concern.
<back and forth between jbroman@ and lfg@ about destruction order/safety/reentrance/etc.>lfg@ 2017: "- thread_local variables are destroyed before WTF::ThreadSpecific (which uses pthreads TLS).- If a thread_local is used after it was destroyed, it'll be re-created. This means that, for example, if you have POD it'll be reinitialized according to its default initialization.- The exact same thing happens to WTF::ThreadSpecific, it'll be re-created if it's used after it has been destroyed. It is also re-constructed according to its default initializers.- If you have a circular dependence between creating TLS inside the destructors, it'll eventually give up.So, basically, thread_local on Mac behaves identically to WTF::ThreadSpecific, with the exception that it is always destroyed first."My take: Not a concern.digit@ 2017: " I beliieve that thread_local was broken [on] Android before Android M"My take: We're now well past this, not a concern.lizeb@ 2020: "For my use case, which had a destructor etc., I encountered weird failures on Windows 7 and Lollipop bots."
My take: This may be due to the bug Victor referenced. Potentially not a concern. Hard to know without more use.What's the current Chrome alternative? base::SequenceLocalStorageMap/base::SequenceLocalStorageSlot and base::ThreadLocal{Boolean,Pointer} and base::ThreadLocalStorage::StaticSlot/Slot?Correct.Can we give some guidelines for when to use the various alternatives here?_If_ thread_local works correctly, I think it could replace all of the Thread* stuff (but not the Sequence* stuff). If it only works for POD/constinit types, then presumably we'd nuke ThreadLocal{Boolean,Pointer} (since those are PODs) and leave ThreadLocalStorage around for non-PODs.PK
--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFA3VyOzSq2w0BwBhO5k3N5Mm8vaLPpL%2BHsxNzY1a3JNFg%40mail.gmail.com.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH3q7_%3DcHjD9YPXEMwxELnR4A%3Dg8XiTbUQOLae5qcsoVm%3DEj1w%40mail.gmail.com.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95Temw0vcK1N7jX9FPY6uo9HgErocDjU_J1pMUeVGBWMMg%40mail.gmail.com.
Has anyone tested the performance characteristics of replacing base::ThreadLocal variables with thread_local variables? Or is this strictly about aligning base:: with the C++ spec?
Is there any worry that devs will use thread_local (or base::ThreadLocal) when they really want sequence-local, leading to hard to find bugs? In my experience many beginners don't well understand the difference between Thread and Sequence (often assuming Sequence is just the Chrome word for Thread or that it's a subtype of thread).I had been assuming that TLS was an advanced enough topic that people without a good grounding in Chrome's threading model wouldn't attempt it, but I have a vague intuition that `thread_local` is easy enough to drop in and standardized enough that people might use it without thinking hard.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95T8qvmbuxTHAS8PjA68kWuDbRqmiss3qgS3mJ20ptCa_A%40mail.gmail.com.
Summary of feedback thus far:
- Known bug on Mac with wrong codegen for exported thread_locals. We should discourage exported variables in general.
- General uncertainty whether there are other bugs.
- thread_local might malloc, and thus cannot safely be used inside a malloc hook, OOM handler, etc. (Is this also true of ThreadLocal*?)
- Thread-affine variables are generally less appropriate than sequence-affine ones. We should make sure there is clear guidance.
- Perf versus ThreadLocal* is unknown, and probably important, since `thread_local` variables are often used in perf-critical systems.
Most of this could probably be dealt with via documentation and possibly PRESUBMITs, but the perf question probably needs addressing before approving this, and it would be nice if we had some general robustness testing (esp. for non-PODs) to see if there are other obvious codegen bugs.Is anyone sufficiently qualified that they could try to do either of those things? I don't feel like I am :(PK
--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFB4_K_VVkc7vb-P0wMgQCg9HAQRuPpwS8Z06_UvyYn5DQ%40mail.gmail.com.
On Wed, Jan 4, 2023 at 1:22 PM 'Joe Mason' via cxx <c...@chromium.org> wrote:Is there any worry that devs will use thread_local (or base::ThreadLocal) when they really want sequence-local, leading to hard to find bugs? In my experience many beginners don't well understand the difference between Thread and Sequence (often assuming Sequence is just the Chrome word for Thread or that it's a subtype of thread).I had been assuming that TLS was an advanced enough topic that people without a good grounding in Chrome's threading model wouldn't attempt it, but I have a vague intuition that `thread_local` is easy enough to drop in and standardized enough that people might use it without thinking hard.Hm.. I don't think we should fix that by avoiding a well known thing, if it's an issue. And it hasn't been raised as one until now with ThreadLocal. Unless this line of question brings out examples of risk and a means to address them, I am not sure that this could (or should?) be actionable, personally.
On Wed, Jan 4, 2023 at 7:41 PM Peter Kasting <pkas...@chromium.org> wrote:Summary of feedback thus far:
- Known bug on Mac with wrong codegen for exported thread_locals. We should discourage exported variables in general.
- General uncertainty whether there are other bugs.
- thread_local might malloc, and thus cannot safely be used inside a malloc hook, OOM handler, etc. (Is this also true of ThreadLocal*?)
- Thread-affine variables are generally less appropriate than sequence-affine ones. We should make sure there is clear guidance.
- Perf versus ThreadLocal* is unknown, and probably important, since `thread_local` variables are often used in perf-critical systems.
On POSIX the ThreadLocal* classes sit on top of pthread_getspecific() and friends. The thread_local would fall back to it only in the worst case, if all the opportunities to rewrite the TLS model in the compiler and the linker fail. Hence at least on Android/Linux/MacOS thread_local has equivalent-or-faster performance.On Windows someone should recheck, I think all Chrome's thread_local variables should live at constant offset from GS (on x64), nothing is faster than that.
--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95SKtJwo9tT9NpB5-LUH3KD7okGg7Hj9n3ULuKOmdqLeFA%40mail.gmail.com.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH3q7_mirNPP5E%2BfT_%3DnACRSPrxLstBW_tvfD_Z2RT8RFEVkvA%40mail.gmail.com.
POD doesn't exist anymore in C++20 (https://en.cppreference.com/w/cpp/named_req/PODType), so could we be a bit more precise about what the requirements are without referring to POD?One or more of:- Default constructible?- Constexpr constructible?- Trivial? Scalar? Or StandardLayout?If someone has a spec reference to explain why we're limiting to POD (pre C++20) that would help and I could propose something.
On Thu, Jan 5, 2023 at 4:13 PM <dan...@chromium.org> wrote:POD doesn't exist anymore in C++20 (https://en.cppreference.com/w/cpp/named_req/PODType), so could we be a bit more precise about what the requirements are without referring to POD?One or more of:- Default constructible?- Constexpr constructible?- Trivial? Scalar? Or StandardLayout?If someone has a spec reference to explain why we're limiting to POD (pre C++20) that would help and I could propose something.Something without destructors? :)
On Thu, Jan 5, 2023 at 11:22 AM Egor Pasko <pa...@chromium.org> wrote:On Thu, Jan 5, 2023 at 4:13 PM <dan...@chromium.org> wrote:POD doesn't exist anymore in C++20 (https://en.cppreference.com/w/cpp/named_req/PODType), so could we be a bit more precise about what the requirements are without referring to POD?One or more of:- Default constructible?- Constexpr constructible?- Trivial? Scalar? Or StandardLayout?If someone has a spec reference to explain why we're limiting to POD (pre C++20) that would help and I could propose something.Something without destructors? :)std::is_trivially_destructible then?
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaSwpDnqQ7g8kA_gf3UuuQXZwr7Ps1Go%2BTmtqozCE1hQKw%40mail.gmail.com.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKrQDSwRzv8H9pL3s9qTZd%2BdmdMeMwpds7UxH90NeoT%3DvQ%40mail.gmail.com.
Isn't TLS essentially static, and the style guide forbids non-PODs with static storage duration?
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFA3gRTZGtXj06c0zLkkdORAYHS4amq%3D5u0EUFnuzPtpBQ%40mail.gmail.com.
Moreover, when a program starts threads that are not joined at exit, those threads may attempt to access objects after their lifetime has ended if their destructor has already run.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sHBSkBcW%3D2UjN_SFrrLSLeMAOLA6-X3c-GjV7eJauX7PQ%40mail.gmail.com.
Ah I figured it out. The static variables section of the guide talks about non-joined threads still running after static destruction, and this potentially UAFing static variables. That particular risk doesn't happen with thread_local objects.
I think we should allow thread_locals as long as they are not exported.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH3q7_%3DxOUJWvWD93bZaLifPEeNySUks%2BPBhS_PkAAFT3fbbKw%40mail.gmail.com.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-6Jout2Uf-KJ5KNPFoeXKfBdY%2BzFMTq6J01godF1gGCug%40mail.gmail.com.
+1 to enforcing the distinction that Benoit proposed, having encountered TLS dragons working with the sampling profiler. It's exceedingly easy to make invalid assumptions about core Chrome services being available (or use APIs that make those assumptions) in non-trivial TLS object destructors. For long-lived threads TLS destruction happens pretty late in Chrome shutdown so use of thread_local objects with non-trivial destructors risks introducing hard-to-debug shutdown crashes.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABJv7p5aMbCWVpRnKyEvdnw_qC2wdfxWcvkDoy3KguqEXmPX%3DQ%40mail.gmail.com.
For context I think the risk falls in three tiers:
- std::is_trivially_destructible thread_local objects: safe
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABJv7p5aMbCWVpRnKyEvdnw_qC2wdfxWcvkDoy3KguqEXmPX%3DQ%40mail.gmail.com.
Since thread_local variables are destroyed in reverse init order, the only way one should attempt to access another that's already been destroyed is if you init A, then B, then hand &B to A. There's nothing really specific to thread locals about this and I don't think it should influence our decision about what kinds of thread locals to allow.
What "this" are you concerned thread_local may not support, that's distinct from what non-thread_local variables support? They can't safely refer to already-destroyed variables either.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDTYx5%2BS7n3UGyy0EV6zAzvJb2XN%2BR7aijNMAiZAy0uQA%40mail.gmail.com.
On Thu, Jan 5, 2023 at 3:01 PM Mike Wittman <wit...@chromium.org> wrote:For context I think the risk falls in three tiers:
- std::is_trivially_destructible thread_local objects: safe
Just *accessing* a TLS variable can cause an allocation when using glibc, which is unexpected and therefore unsafe in some contexts, e.g. allocators and allocation hooks (sampling profiler, GWP-ASAN). One example is the crashes we saw when calling base::PlatformThread::CurrentId() after forking the browser process: the thread id was cached in a TLS variable, but accessing the TLS variable caused an allocation, which is forbidden after forking a multithreaded process. (see CL)
On Thu, Jan 5, 2023 at 3:57 PM Matt Denton <mpde...@google.com> wrote:On Thu, Jan 5, 2023 at 3:01 PM Mike Wittman <wit...@chromium.org> wrote:For context I think the risk falls in three tiers:
- std::is_trivially_destructible thread_local objects: safe
Just *accessing* a TLS variable can cause an allocation when using glibc, which is unexpected and therefore unsafe in some contexts, e.g. allocators and allocation hooks (sampling profiler, GWP-ASAN). One example is the crashes we saw when calling base::PlatformThread::CurrentId() after forking the browser process: the thread id was cached in a TLS variable, but accessing the TLS variable caused an allocation, which is forbidden after forking a multithreaded process. (see CL)Right, but those contexts are obvious (or should be, via code documentation), very rare, and reasonably can be considered expert-only. Having written the allocation-free code for the sampling profiler, I'd argue that we shouldn't be making any assumptions about allocations within standard library code (or any other dependencies) in those contexts without manually verifying that's the case, and also that's likely to continue being the case with future library updates.Since most Chrome engineers will never touch those contexts I think it's reasonable to consider thread_local with trivially-destructible objects safe for general usage.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAFsT0xgVjOHUw4NuOXT2_s1yeZhTz5vfbyJyZ3G_86RyzWy0dw%40mail.gmail.com.
The code you write is guaranteed safe, though. b is destroyed before a, not after it.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAFsT0xjf%3DFNi9L8dLuw3sMxuBsAMgw%2Bh8ncSYL%2BityHYWTEmOQ%40mail.gmail.com.
I do not think that's ub either, especially since a is not static.
(note, though, that trivial destructors don't save you here; this is ub regardless of type or what the function does with the variable.)
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAFsT0xj_Z7On9_HcJJuoO%2Bzow_afNDgyXyXxyZ9iOe0O0dCwrw%40mail.gmail.com.
Yes; I was thinking only that a trivially destructible a does not save you. But you're right that if _all_ thread locals and all static/globals are forced to be trivially destructible, then I think no one can call the function (or anything else) once the thread reaches thread_local-destruction phase.
I probably shouldn't respond to these emails while I'm also trying to parent a five year old :P