tavianator marked 5 inline comments as done.
================
Comment at: src/cxa_thread_atexit.cpp:70
@@ +69,3 @@
+ while (auto head = dtors) {
+ dtors = head->next;
+ head->dtor(head->obj);
----------------
EricWF wrote:
> 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.
Yep, done! I guess that was the point of `thread_alive` from your original
patch-to-my-patch, sorry for stripping it out.
https://reviews.llvm.org/D21803
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
