[PATCH] D37590: [scudo] RFC thread specific data refactoring

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added a comment.

In https://reviews.llvm.org/D37590#867632, @dvyukov wrote:

> So this is basically no functional changes, just rebranding what we had for linux as "context per thread" and what we had for android as "several shared contexts", right?


This is correct. Rebranding and reorganization of the code.



================
Comment at: lib/scudo/scudo_allocator.cpp:255
 
-AllocatorCache *getAllocatorCache(ScudoThreadContext *ThreadContext) {
-  return &ThreadContext->Cache;
+AllocatorCache *getAllocatorCache(ScudoTSD *TSD) {
+  return &TSD->Cache;
----------------
dvyukov wrote:
> This and getPrng look unnecessary now.
I think I had them for accessor consistency  more than anything else.


================
Comment at: lib/scudo/scudo_allocator.cpp:385
+      ScudoTSD *TSD = getTSDAndLock();
+      if (LIKELY(TSD)) {
+        Salt = getPrng(TSD)->getU8();
----------------
dvyukov wrote:
> It feels that in this new model getTSDAndLock() should just never return NULL. If some TSD implementation can't return a thread-local descriptor, it should be _its_ problem to allocate and return a fallback descriptor instead. Since we now have a notion of getTSDAndLock()/TSD->unlock(), global mutex-protected fallback descriptor fits into into this model well.
> But the "global hashed set of TSDs" implementation just does not have this problem.
I think I tried that at some point.
I got into some issues with the fact that the fallback TSD is a shared one (eg: with mutex) while using exclusive per-thread TSDs (no locks).
This ended up with both TSD versions having to be present in the code (current it's one or the other) and added a layer of complexity that ended up being counter intuitive to me.
There is likely some more clever way to do it that I haven't thought about though. I'll have another look.


================
Comment at: lib/scudo/scudo_tsd.h:27
+
+#if !SCUDO_TSD_EXCLUSIVE
+struct ScudoTSDPlatform {
----------------
dvyukov wrote:
> I think this file and the cc will be much easier to understand if split into multiple files (one per model).
I think I will indeed have to keep the previous multilple .h/.inc/.cpp organization.


https://reviews.llvm.org/D37590