Page MenuHomeFreeBSD
Differential D55826

libc: Fix dtor order in __cxa_thread_atexit
ClosedPublic
Actions

Authored by aokblast on Thu, Mar 12, 2:39 PM.
Tags
None
Referenced Files
F148638921: D55826.id173609.diff
Thu, Mar 19, 7:26 AM
Unknown Object (File)
Wed, Mar 18, 2:03 AM
Unknown Object (File)
Mon, Mar 16, 10:44 PM
Unknown Object (File)
Fri, Mar 13, 3:45 PM
Subscribers
dim
imp
siva

Details

Reviewers
kib
markj
imp
lwhsu
Commits
rG9d26b82826d9: libc: Fix dtor order in __cxa_thread_atexit
Summary

The thread_local variable may creates another thread_local variable
inside its dtor. This new object is immediately be registered in
__cxa_thread_atexit() and need to be freed before processing another
variable.

This fixes the libcxx test thread_local_destruction_order.pass.cpp.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 71365
Build 68248: arc lint + arc unit

Event Timeline

aokblast created this revision.Thu, Mar 12, 2:39 PM
Herald added a subscriber: imp. · View Herald TranscriptThu, Mar 12, 2:39 PM
aokblast requested review of this revision.Thu, Mar 12, 2:39 PM
Harbormaster completed remote builds in B71364: Diff 173607.Thu, Mar 12, 2:39 PM
aokblast added reviewers: kib, markj, imp.Thu, Mar 12, 2:40 PM

Test case in here.
https://github.com/llvm/llvm-project/blob/5eaf19a151296f770a1a2bd8514a76530234d933/libcxxabi/test/thread_local_destruction_order.pass.cpp

kib added inline comments.Thu, Mar 12, 2:45 PM
lib/libc/stdlib/cxa_thread_atexit_impl.c
125

This is

dtor = LIST_FIRST(&dtors);

But usually it is written as

    while ((dtor = LIST_FIRST(&dtors)) != NULL) {
          LIST_REMOVE();
...
aokblast updated this revision to Diff 173609.Thu, Mar 12, 2:50 PM

Style fix

Harbormaster completed remote builds in B71365: Diff 173609.Thu, Mar 12, 2:50 PM
kib accepted this revision.Thu, Mar 12, 3:02 PM
This revision is now accepted and ready to land.Thu, Mar 12, 3:02 PM
aokblast added a comment.EditedThu, Mar 12, 3:07 PM

I think we need to refactor these stuff to dynamic array in the future. It makes no sense to use linked_list if we only need to push and pop from the top.

aokblast added a reviewer: lwhsu.Fri, Mar 13, 10:31 AM
lwhsu accepted this revision.Fri, Mar 13, 3:38 PM
Closed by commit rG9d26b82826d9: libc: Fix dtor order in __cxa_thread_atexit (authored by aokblast). · Explain WhySat, Mar 14, 4:31 AM
This revision was automatically updated to reflect the committed changes.
aokblast added a commit: rG9d26b82826d9: libc: Fix dtor order in __cxa_thread_atexit.
siva added a subscriber: siva.EditedMon, Mar 16, 1:42 PM

@aokblast this commit breaks the following tests in CI:

lib/libc/stdlib/cxa_thread_atexit_nothr_test:cxx__thread_inf_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_inf_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_add_while_calling_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_after
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_before

See https://ci.freebsd.org/view/Test/job/FreeBSD-main-amd64-test/28044/testReport/.

aokblast added a subscriber: dim.Mon, Mar 16, 3:53 PM
In D55826#1278375, @siva wrote:

@aokblast this commit breaks the following tests in CI:

lib/libc/stdlib/cxa_thread_atexit_nothr_test:cxx__thread_inf_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_inf_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_add_while_calling_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_after
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_before

See https://ci.freebsd.org/view/Test/job/FreeBSD-main-amd64-test/28044/testReport/.

I don't think the test actually make sense with my change.
The following code in the test makes it unables to terminate

static void                                                                                                                                        
again(void *arg)                                                                                                                                   
{                                                                                                                                                  
                                                                                                                                                   
    __cxa_thread_atexit(again, arg, &output);                                                                                                      
}

This makes again(void *) called himself and create a infinite loop. In my opnion, the user of the __cxa_thread_atexit should take the responsibility on the infinite loop instead of setting the hard limit on iteration.

I just take a look at the original commit which add this test and would like to ask @kib and @dim on their opinions.

Should we remove the again function or if there is any suggestions on this?

aokblast added a comment.Mon, Mar 16, 3:53 PM

Also, thanks for your remind:). @siva

kib added a comment.Mon, Mar 16, 4:00 PM
In D55826#1278380, @aokblast wrote:
In D55826#1278375, @siva wrote:

@aokblast this commit breaks the following tests in CI:

lib/libc/stdlib/cxa_thread_atexit_nothr_test:cxx__thread_inf_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_inf_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_add_while_calling_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_after
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_before

See https://ci.freebsd.org/view/Test/job/FreeBSD-main-amd64-test/28044/testReport/.

I don't think the test actually make sense with my change.
The following code in the test makes it unables to terminate

static void                                                                                                                                        
again(void *arg)                                                                                                                                   
{                                                                                                                                                  
                                                                                                                                                   
    __cxa_thread_atexit(again, arg, &output);                                                                                                      
}

This makes again(void *) called himself and create a infinite loop. In my opnion, the user of the __cxa_thread_atexit should take the responsibility on the infinite loop instead of setting the hard limit on iteration.

I just take a look at the original commit which add this test and would like to ask @kib and @dim on their opinions.

Should we remove the again function or if there is any suggestions on this?

Indeed this test is perhaps not too useful as is. Might be, it should limit the number of registrations, then it would make some reasonable checks.
I do think that your change that broke the test is right.

aokblast added a comment.Mon, Mar 16, 4:28 PM
In D55826#1278383, @kib wrote:
In D55826#1278380, @aokblast wrote:
In D55826#1278375, @siva wrote:

@aokblast this commit breaks the following tests in CI:

lib/libc/stdlib/cxa_thread_atexit_nothr_test:cxx__thread_inf_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_inf_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_add_while_calling_dtors
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_after
lib/libc/stdlib/cxa_thread_atexit_test:cxx__thread_local_before

See https://ci.freebsd.org/view/Test/job/FreeBSD-main-amd64-test/28044/testReport/.

I don't think the test actually make sense with my change.
The following code in the test makes it unables to terminate

static void                                                                                                                                        
again(void *arg)                                                                                                                                   
{                                                                                                                                                  
                                                                                                                                                   
    __cxa_thread_atexit(again, arg, &output);                                                                                                      
}

This makes again(void *) called himself and create a infinite loop. In my opnion, the user of the __cxa_thread_atexit should take the responsibility on the infinite loop instead of setting the hard limit on iteration.

I just take a look at the original commit which add this test and would like to ask @kib and @dim on their opinions.

Should we remove the again function or if there is any suggestions on this?

Indeed this test is perhaps not too useful as is. Might be, it should limit the number of registrations, then it would make some reasonable checks.
I do think that your change that broke the test is right.

Thanks! I will try to unbreak the tes tomorrow first. I don't object any check on this and think that it would be somehow useful for guarding our system. But notice that __cxa_thread_atexit is expected to use as a syntax sugar for C++ thread_local keyword. So any C++ program with enough complexity might cause simple check breaks.

Revision Contents
Changeset List

PathSize
lib/
libc/
stdlib/
4 lines

Diff 173609

View Options

lib/libc/stdlib/cxa_thread_atexit_impl.c

Loading...