Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: posix: common: do not overspecify pthread_attr_t #67094

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Jan 2, 2024

Originally, Zephyr only supported statically defined thread stacks.

That is actually the exception in terms of how pthreads are typically spawned. The norm is simply passing a NULL pthread_attr_t * and using a stack allocated by the implementation.

For the vast majority of test cases involving typically trivial workloads, there is zero need to manually specify stack sizes, priorities, scheduling parameters, etc, unless actually testing that functionality, and therefore no reason to specify pthread_attr_t, since NULL will simply pull from the pool of preallocated thread stacks.

This highlights an aspect of the testsuite that needs to be fixed as part of #67091

Moreover, virtually everywhere we have used pthread_attr_init(), there is insufficient care taken to call pthread_attr_destroy() in the various failure cases. That complexity is mitigated by using automatic thread stacks, as the POSIX API manages the thread stack for us. The significance of this is quite obvious once those functions are modified to manage thread stacks (rather than funnelling it all into pthread_create()), because not calling pthread_attr_destroy() translates to memory leaks.

By passing in a NULL pthread_attr_t * to pthread_create() we can avoid retrofitting dozens of pthread_attr_destroy() instances throughout the testsuite while simultaneously making the tests more concise and easier to maintain.

Additionally, make tests more concise / accurate by switching to e.g. zassert_ok(), rather than zassert_false(), zassert_not_null() rather than zassert_equal((int)x, (int)y), etc.

This PR fixes a difficult-to-enumerate number of bugs in the POSIX testsuite.

(Partially)
Fixes #67178

@cfriedt cfriedt changed the title tests: posix: common: pthread: do not overspecify pthread_attr_t tests: posix: common: do not overspecify pthread_attr_t Jan 2, 2024
@cfriedt cfriedt force-pushed the do-not-overspecify-pthread-attr-t-in-tests branch 3 times, most recently from 1d18cf9 to aed0301 Compare January 2, 2024 12:17
Much of tests/posix/common still overspecifies pthread_attr_t
options.

Default thread attributes are perfectly fine for the vast
majority of spawned threads in this testsuite, so simply use
a NULL pthread_attr_t* argument.

This fixes piles of bugs because we have not properly used
pthread_attr_destroy() appropriately.

Signed-off-by: Christopher Friedt <[email protected]>
Much of tests/posix/common still overspecifies pthread_attr_t
options.

Default thread attributes are perfectly fine for the vast
majority of spawned threads in this testsuite, so simply use
a NULL pthread_attr_t* argument.

This fixes piles of bugs because we have not properly used
pthread_attr_destroy() appropriately.

Signed-off-by: Christopher Friedt <[email protected]>
Much of tests/posix/common still overspecifies pthread_attr_t
options.

Default thread attributes are perfectly fine for the vast
majority of spawned threads in this testsuite, so simply use
a NULL pthread_attr_t* argument.

This fixes piles of bugs because we have not properly used
pthread_attr_destroy() appropriately.

Signed-off-by: Christopher Friedt <[email protected]>
Much of tests/posix/common still overspecifies pthread_attr_t
options.

Default thread attributes are perfectly fine for the vast
majority of spawned threads in this testsuite, so simply use
a NULL pthread_attr_t* argument.

This fixes piles of bugs because we have not properly used
pthread_attr_destroy() appropriately.

This is only a partial cleanup for pthread.c

Signed-off-by: Christopher Friedt <[email protected]>
Much of tests/posix/common still overspecifies pthread_attr_t
options.

Default thread attributes are perfectly fine for the vast
majority of spawned threads in this testsuite, so simply use
a NULL pthread_attr_t* argument.

This fixes piles of bugs because we have not properly used
pthread_attr_destroy() appropriately.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt cfriedt force-pushed the do-not-overspecify-pthread-attr-t-in-tests branch from aed0301 to c629ef3 Compare January 2, 2024 12:33
@cfriedt cfriedt marked this pull request as ready for review January 2, 2024 12:51
@zephyrbot zephyrbot added the area: POSIX POSIX API Library label Jan 2, 2024
@cfriedt cfriedt requested review from ycsin and jgl-meta January 2, 2024 12:52
@cfriedt cfriedt added the area: Tests Issues related to a particular existing or missing test label Jan 2, 2024
keith-packard
keith-packard previously approved these changes Jan 2, 2024
Copy link
Collaborator

@keith-packard keith-packard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good cleanup in general, just one suggestion -- I'd leave the active code outside the zassert calls so that they just verify values and don't have the action include. e.g.
instead of

zassert_ok(pthread_setspecific(key, value), "pthread_setspecific failed");

use

ret = pthread_setspecific(key, value);
zassert_ok(ret, "pthread_setspecific failed");

@cfriedt
Copy link
Member Author

cfriedt commented Jan 2, 2024

Good cleanup in general, just one suggestion -- I'd leave the active code outside the zassert calls so that they just verify values and don't have the action include.

I do see the usefulness in keeping temporary variables around but also feel it's a matter of personal preference / utility. Definitely agree if the value is read / written more than once in a test.

Worthwhile suggestion in any case.

Much of tests/posix/common still overspecifies pthread_attr_t
options.

Default thread attributes are perfectly fine for the vast
majority of spawned threads in this testsuite, so simply use
a NULL pthread_attr_t* argument.

This fixes piles of bugs because we have not properly used
pthread_attr_destroy() appropriately.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
Copy link
Member Author

cfriedt commented Jan 2, 2024

  • the same for rwlock.c

@keith-packard
Copy link
Collaborator

Good cleanup in general, just one suggestion -- I'd leave the active code outside the zassert calls so that they just verify values and don't have the action include.

I do see the usefulness in keeping temporary variables around but also feel it's a matter of personal preference / utility. Definitely agree if the value is read / written more than once in a test.

I was thinking more as a way to ensure the 'real' code was visible by separating it from the 'test framework' bits. Kinda like making sure that your 'assert' statements never have side-effects.

Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cfriedt
Copy link
Member Author

cfriedt commented Jan 3, 2024

I was thinking more as a way to ensure the 'real' code was visible by separating it from the 'test framework' bits. Kinda like making sure that your 'assert' statements never have side-effects.

It would be beneficial to do that if this were an __ASSERT() or __ASSERT_NO_MSG() or just plain old assert() in regular application code because one needs to be careful that the assert() could be a macro that is switched off (and therefore the internal call might not work), or that somehow the contents of the function call might interfere with the assert macro.

But this is an integration test intentionally written for the ZTest framework. So there is no need to be cautious that zasserts would evaluate to nothing. Functions also definitely do evaluate correctly from within zassert statements.

@cfriedt cfriedt requested a review from keith-packard January 3, 2024 19:01
@cfriedt cfriedt added the bug The issue is a bug, or the PR is fixing a bug label Jan 3, 2024
@cfriedt
Copy link
Member Author

cfriedt commented Jan 4, 2024

@keith-packard - could you please re-approve?

@cfriedt cfriedt merged commit 80e3f4a into zephyrproject-rtos:main Jan 4, 2024
23 checks passed
@cfriedt cfriedt deleted the do-not-overspecify-pthread-attr-t-in-tests branch January 4, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests: posix: pthread_attr_destroy() not consistently called after pthread_attr_init()
4 participants