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

Refactor HAVE_PTHREAD and _POSIX_THREADS #6536

Merged
merged 10 commits into from
Jun 26, 2023
Merged

Conversation

gojimmypi
Copy link
Contributor

@gojimmypi gojimmypi commented Jun 22, 2023

Description

This started as a corner case of cross-compiling testwolfcrypt, in my case for the Xtensa ESP32-S3 Linux kernel.

I didn't have a pthread.h and noticed that several other places (but not all?) are gated with HAVE_PTHREAD.

This PR refactors the HAVE_PTHREAD to not only the test.h that I encountered, but also all other places that the conditional #if defined(_POSIX_THREADS) was used.

Upon merging there will be only one check of _POSIX_THREADS in settings.h and all other code shall be using HAVE_PTHREAD to determine if POSIX threading code should be gated in.

Note that MINGW is not POSIX and is part of the global logic that will disable HAVE_PTHREAD.

The key part of this update is this compiler directive code segment added to wolfssl/wolfcrypt/setthings.h to determine if we have POSIX Threads enabled (aka HAVE_PTHREAD is defined):

/* AFTER user_settings.h is loaded,
** determine if POSIX multi-threaded: HAVE_PTHREAD  */
#if defined(SINGLE_THREADED) || defined(__MINGW32__)
    /* Never HAVE_PTHREAD in single thread, or non-POSIX mode.
    ** Reminder: MING32 is win32 threads, not POSIX threads */
    #undef HAVE_PTHREAD
#else
    #ifdef _POSIX_THREADS
        /* HAVE_PTHREAD == POSIX threads capable and enabled. */
        #undef HAVE_PTHREAD
        #define HAVE_PTHREAD 1
    #else
        /* Not manually disabled, but POSIX threads not found. */
        #undef HAVE_PTHREAD
    #endif
#endif

Fixes zd# n/a/

Testing

How did you test?

Ran the test app in cross-compiled app as well as Linux & ESP-IDF for ESP32.

Default config, threading enabled:

./configure &&  make && ./wolfcrypt/test/testwolfcrypt

Forced single-thread. This was observed to have failed during PRB with prior changes:

 ./configure --enable-singlethreaded --disable-fastmath --enable-smallstack --enable-smallstackcache  --disable-shared  && make && ./wolfcrypt/test/testwolfcrypt 

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@gojimmypi gojimmypi self-assigned this Jun 22, 2023
julek-wolfssl
julek-wolfssl previously approved these changes Jun 22, 2023
wolfssl/test.h Show resolved Hide resolved
@gojimmypi
Copy link
Contributor Author

After seeing the PRB failures, it seems there was some logic missing from a few of the examples. In particular, when the threading capability exists but is explicitly turned off with --enable-singlethreaded to force single-threaded mode.

I sprinkled some && !defined(SINGLE_THREADED) logic in places where if defined(_POSIX_THREADS) is used. It appears this logic was already in use in other places.

There may be an argument to combine all that logic into a single global definition.

@@ -52,6 +52,12 @@
extern "C" {
#endif

/* pick up compiler def; may need to turn on HAVE_PTHREAD for some configs */
#ifdef _POSIX_THREADS
#undef HAVE_PTHREAD
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good to me. Now that this is in settings.h I think it best to change the _POSIX_THREADS elsewhere to use HAVE_PTHREAD. I also think you should add && !defined(SINGLE_THREADED) and remove it from the refactor you did.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool. I've applied your suggested changes and changed the PR description & title to reflect the global nature of this change

@gojimmypi gojimmypi changed the title HAVE_PTHREAD gate in test.h Refactor HAVE_PTHREAD and _POSIX_THREADS Jun 24, 2023
@gojimmypi
Copy link
Contributor Author

@dgarske - After completing the desired changes for HAVE_PTHREAD and _POSIX_THREADS, I noticed there's a potentially undesired WOLFSSL_PTHREADS that should also be refactored? See the codebase for references.

If you agree, changes to (or more specifically: removal of) WOLFSSL_PTHREADS would probably be in a separate PR & I'd be happy to work on that right away.

@gojimmypi gojimmypi requested a review from dgarske June 24, 2023 22:30
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Very nice cleanup!

@dgarske dgarske merged commit 6b240fa into wolfSSL:master Jun 26, 2023
dgarske added a commit that referenced this pull request Jun 27, 2023
gojimmypi added a commit to gojimmypi/wolfssl that referenced this pull request Jun 27, 2023
dgarske added a commit that referenced this pull request Jun 27, 2023
Revert #6536 types.h one line HAVE_PTHREAD
@gojimmypi
Copy link
Contributor Author

See also #6545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants