[PATCH] D21803: [libcxxabi] Provide a fallback __cxa_thread_atexit() implementation

[Eric Fiselier via cfe-commits]

EricWF added a comment.

LGTM after addressing the inline comments.


================
Comment at: src/cxa_thread_atexit.cpp:70
@@ +69,3 @@
+    while (auto head = dtors) {
+      dtors = head->next;
+      head->dtor(head->obj);
----------------
tavianator wrote:
> EricWF wrote:
> > tavianator wrote:
> > > EricWF wrote:
> > > > There is a bug here. If `head->next == nullptr` and if `head->dtor(head->obj))` creates a TL variable in the destructor then that destructor will not be invoked.
> > > > 
> > > > Here's an updated test case which catches the bug: https://gist.github.com/EricWF/3bb50d4f28b91aa28d2adefea0e94a0e
> > > I can't reproduce that failure here, your exact test case passes (even with `#undef HAVE___CXA_THREAD_ATEXIT_IMPL` and the weak symbol test commented out).
> > > 
> > > Tracing the implementation logic, it seems correct.  If `head->next == nullptr` then this line does `dtors = nullptr`.  Then if `head->dtor(head->obj)` registers a new `thread_local`, `__cxa_thread_atexit()` does `head = malloc(...); ... dtors = head;`.  Then the next iteration of the loop `while (auto head = dtors) {` picks up that new node.
> > > 
> > > Have I missed something?
> > I can't reproduce this morning either, I must have been doing something funny. I'll look at this with a fresh head tomorrow. If I can't find anything this will be good to go. Thanks for working on this. 
> No problem!  I can integrate your updated test case anyway if you want.
Yeah I would like to see the upgraded test case applied. At least that way we're testing the case in question.

So I agree with your above analysis of what happens, and that all destructors are correctly called during the first iteration of pthread key destruction. My one issues is that we still register a new non-null key which forces pthread to run the destructor for the key again. I would like to see this fixed.


https://reviews.llvm.org/D21803