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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
