Page MenuHomeFreeBSD
Differential D55893

libc: Fix cxa_thread_atexit test.
ClosedPublic
Actions

Authored by aokblast on Tue, Mar 17, 7:48 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 3, 12:22 AM
Unknown Object (File)
Thu, Apr 2, 8:28 PM
Unknown Object (File)
Thu, Apr 2, 5:02 PM
Unknown Object (File)
Thu, Apr 2, 4:26 PM
Unknown Object (File)
Thu, Apr 2, 5:48 AM
Unknown Object (File)
Thu, Apr 2, 5:34 AM
Unknown Object (File)
Thu, Apr 2, 1:56 AM
Unknown Object (File)
Tue, Mar 31, 12:33 AM
View All 21 Files
Subscribers
imp
lwhsu

Details

Reviewers
kib
markj
Commits
rGc25976f0a9a3: libc: Fix cxa_thread_atexit{,nothr} test.
Summary

After patch 9d26b82, we don't provide recursive call protection anymore.
Therefore, to pass the test, we adjust the testcase by protecting on
caller and the testcase is to make sure the dtors is properly handled.

Diff Detail

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

Event Timeline

aokblast created this revision.Tue, Mar 17, 7:48 AM
Herald added a subscriber: imp. · View Herald TranscriptTue, Mar 17, 7:48 AM
aokblast requested review of this revision.Tue, Mar 17, 7:48 AM
Harbormaster completed remote builds in B71451: Diff 173792.Tue, Mar 17, 7:48 AM
aokblast added reviewers: kib, markj.Tue, Mar 17, 7:48 AM
kib added a comment.Tue, Mar 17, 2:33 PM

No, I mean to keep this test, adjusted.
Create a global static var that is incremented on each again() call, and do not call __cxa_thread_atexit() if the counter greater than some value.
As an additional check, you might assert that the counter indeed counted to the specified value.

aokblast added a comment.Tue, Mar 17, 2:48 PM
In D55893#1278865, @kib wrote:

No, I mean to keep this test, adjusted.
Create a global static var that is incremented on each again() call, and do not call __cxa_thread_atexit() if the counter greater than some value.
As an additional check, you might assert that the counter indeed counted to the specified value.

In D55893#1278865, @kib wrote:

No, I mean to keep this test, adjusted.
Create a global static var that is incremented on each again() call, and do not call __cxa_thread_atexit() if the counter greater than some value.
As an additional check, you might assert that the counter indeed counted to the specified value.

What about the nothr case? Since there is only one main thread. The dtos will be called after the ATF_TEST_CASE_BODY, which means we are unable to use assert since the main thread is destroyed.

aokblast updated this revision to Diff 173808.Tue, Mar 17, 2:55 PM

Adjust the testcase insteada of deleting the testcase.

Harbormaster completed remote builds in B71459: Diff 173808.Tue, Mar 17, 2:55 PM
aokblast updated this revision to Diff 173809.Tue, Mar 17, 2:56 PM

Formatting

Harbormaster completed remote builds in B71460: Diff 173809.Tue, Mar 17, 2:56 PM
aokblast added a comment.Tue, Mar 17, 2:57 PM
In D55893#1278868, @aokblast wrote:
In D55893#1278865, @kib wrote:

No, I mean to keep this test, adjusted.
Create a global static var that is incremented on each again() call, and do not call __cxa_thread_atexit() if the counter greater than some value.
As an additional check, you might assert that the counter indeed counted to the specified value.

In D55893#1278865, @kib wrote:

No, I mean to keep this test, adjusted.
Create a global static var that is incremented on each again() call, and do not call __cxa_thread_atexit() if the counter greater than some value.
As an additional check, you might assert that the counter indeed counted to the specified value.

What about the nothr case? Since there is only one main thread. The dtos will be called after the ATF_TEST_CASE_BODY, which means we are unable to use assert since the main thread is destroyed.

I don't add the ATF_REQUIRE_EQ in nothr case. I think it at least check the dtor is handled properly.

aokblast edited the summary of this revision. (Show Details)Tue, Mar 17, 2:58 PM
kib added a comment.Tue, Mar 17, 2:59 PM
In D55893#1278868, @aokblast wrote:
In D55893#1278865, @kib wrote:

No, I mean to keep this test, adjusted.
Create a global static var that is incremented on each again() call, and do not call __cxa_thread_atexit() if the counter greater than some value.
As an additional check, you might assert that the counter indeed counted to the specified value.

In D55893#1278865, @kib wrote:

No, I mean to keep this test, adjusted.
Create a global static var that is incremented on each again() call, and do not call __cxa_thread_atexit() if the counter greater than some value.
As an additional check, you might assert that the counter indeed counted to the specified value.

What about the nothr case? Since there is only one main thread. The dtos will be called after the ATF_TEST_CASE_BODY, which means we are unable to use assert since the main thread is destroyed.

I think that this is the right concern, and the right answer would be to significantly reorg the test. Since we are testing something that checks C runtime behavior outside the scope of normal execution (inside the main) for ATF, we need to create a helper program.
The test should spawn that program and check the effects of it, instead of trying to do everything inline.

aokblast added a comment.Tue, Mar 17, 3:45 PM
In D55893#1278882, @kib wrote:
In D55893#1278868, @aokblast wrote:
In D55893#1278865, @kib wrote:

No, I mean to keep this test, adjusted.
Create a global static var that is incremented on each again() call, and do not call __cxa_thread_atexit() if the counter greater than some value.
As an additional check, you might assert that the counter indeed counted to the specified value.

In D55893#1278865, @kib wrote:

No, I mean to keep this test, adjusted.
Create a global static var that is incremented on each again() call, and do not call __cxa_thread_atexit() if the counter greater than some value.
As an additional check, you might assert that the counter indeed counted to the specified value.

What about the nothr case? Since there is only one main thread. The dtos will be called after the ATF_TEST_CASE_BODY, which means we are unable to use assert since the main thread is destroyed.

I think that this is the right concern, and the right answer would be to significantly reorg the test. Since we are testing something that checks C runtime behavior outside the scope of normal execution (inside the main) for ATF, we need to create a helper program.
The test should spawn that program and check the effects of it, instead of trying to do everything inline.

Yes, re-organize the testcase is necessary. I also see something strange in the testcase like there is a 1 second sleep after thread::join(), which I don't think is necessary. But in the short term (at least for this week), I think we can fix the CI first.

kib added a comment.Tue, Mar 17, 4:00 PM

For now, I suggest to #ifdef 0 the ADD_TEST(), then revert it when the updated test is written.

lwhsu added a subscriber: lwhsu.Tue, Mar 17, 5:25 PM
In D55893#1278897, @kib wrote:

For now, I suggest to #ifdef 0 the ADD_TEST(), then revert it when the updated test is written.

For the tests we plan to rewrite, I suggest using atf_tc_skip() with a reason to mark this test case is problematic and have a reminder in the test output.

aokblast updated this revision to Diff 173857.Wed, Mar 18, 4:21 PM

Skip the again test

Harbormaster completed remote builds in B71487: Diff 173857.Wed, Mar 18, 4:21 PM
kib accepted this revision.Wed, Mar 18, 7:05 PM
This revision is now accepted and ready to land.Wed, Mar 18, 7:05 PM
markj added a comment.Fri, Mar 20, 2:04 AM

BTW, is it possible to create a test case for commit 9d26b82826d9?

lib/libc/tests/stdlib/cxa_thread_atexit_nothr_test.cc
33

AGAIN_CALL_LIMIT is a better name.

85–88
94

Do we still need to skip this?

aokblast added a comment.Fri, Mar 20, 2:57 AM
In D55893#1280330, @markj wrote:

BTW, is it possible to create a test case for commit 9d26b82826d9?

Yes, I think we can have some tests for this. But I think we should do it after re-org the tests. At least for now we can rely on the llvm testcase as I run it biweekly with latest CURRENT.

lib/libc/tests/stdlib/cxa_thread_atexit_nothr_test.cc
94

Yes, we only have main thread so we are unable to use any ATF_CHECK

aokblast updated this revision to Diff 173974.Fri, Mar 20, 2:58 AM

Rename macro

This revision now requires review to proceed.Fri, Mar 20, 2:58 AM
Harbormaster completed remote builds in B71568: Diff 173974.Fri, Mar 20, 2:58 AM
markj added inline comments.Fri, Mar 20, 3:14 AM
lib/libc/tests/stdlib/cxa_thread_atexit_nothr_test.cc
94

Well, we do not really need ATF_CHECK. Just let the test run. If it hangs forever, or crashes, kyua will still report a problem.

It would be nice to have ATF_CHECK verify again_count, but even without it this test is better than nothing.

aokblast added inline comments.Fri, Mar 20, 5:23 AM
lib/libc/tests/stdlib/cxa_thread_atexit_nothr_test.cc
94

I am fine if @kib think so. The thr_test has it already.

markj added inline comments.Fri, Mar 20, 6:20 AM
lib/libc/tests/stdlib/cxa_thread_atexit_nothr_test.cc
94

I just don't want to leave this test in this state. Either it should be useful, or it should be deleted entirely.

kib added inline comments.Sat, Mar 21, 1:06 AM
lib/libc/tests/stdlib/cxa_thread_atexit_nothr_test.cc
94

The promise was that it would be fixed. IMO it is better to leave it as is for some time, because I prefer to see the diff against the current baseline, both in review and in the commit history, instead of the revert and then commit consisting of the additions (we loose useful change history).

markj accepted this revision.Sat, Mar 21, 2:07 AM
markj added inline comments.
lib/libc/tests/stdlib/cxa_thread_atexit_nothr_test.cc
94

I am not proposing to revert anything, I just do not like the skip() here. But ok, we can fix it separately, it would be good to fix the tests in the meantime.

This revision is now accepted and ready to land.Sat, Mar 21, 2:07 AM
Closed by commit rGc25976f0a9a3: libc: Fix cxa_thread_atexit{,nothr} test. (authored by aokblast). · Explain WhySun, Mar 22, 2:48 AM
This revision was automatically updated to reflect the committed changes.
aokblast added a commit: rGc25976f0a9a3: libc: Fix cxa_thread_atexit{,nothr} test..

Revision Contents
Changeset List

PathSize
lib/
libc/
tests/
stdlib/
11 lines
10 lines

Diff 173974

View Options

lib/libc/tests/stdlib/cxa_thread_atexit_nothr_test.cc

Loading...
View Options

lib/libc/tests/stdlib/cxa_thread_atexit_test.cc

Loading...