Repository navigation
Conversation
The gnulib visibility probe in configure passes for Windows GCC
targets (Cygwin, MSYS2 and MinGW-w64) because it never defines a
symbol with an explicit hidden attribute. visibility.h then used
__attribute__((visibility("hidden"))) for WP11_LOCAL, which PE/COFF
does not support, so every WP11_LOCAL function definition warned
"visibility attribute not supported in this configuration; ignored"
and builds from git, which use -Werror, failed.
Check the Windows targets before HAVE_VISIBILITY, as wolfSSL and
wolfTPM already do, and include __CYGWIN__ in that check.
The newlib ctype.h used by Cygwin and MSYS2 also warns when isdigit()
is given a plain char, which broke the pkcs11mtt build. Cast the
argument to unsigned char there and in the two tests that cast it to
int, which only hid the warning.
Fixes wolfSSL#223
Build wolfPKCS11 on Windows with MSYS2 so Windows GCC regressions such as wolfSSL#223 are caught in CI: - msys-shared: MSYS2's POSIX (Cygwin-based) gcc, full build - msys-static: same toolchain, static build with make check - ucrt64-shared: native MinGW-w64 gcc, library only as the tests and examples do not yet build with MinGW
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The visibility fix is targeted, portability corrections are valid, and the new workflow covers the affected toolchains.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes GCC visibility handling for Windows targets and adds MSYS2 build coverage.
Changes:
- Prioritizes Windows import/export handling over visibility attributes.
- Corrects
isdigit()arguments to avoid undefined behavior. - Adds MSYS2, static, and MinGW CI builds.
| File | Description |
|---|---|
wolfpkcs11/visibility.h |
Handles MinGW and Cygwin visibility correctly. |
tests/pkcs11v3test.c |
Safely casts input for isdigit(). |
tests/pkcs11test.c |
Safely casts input for isdigit(). |
tests/pkcs11mtt.c |
Safely casts input for isdigit(). |
.github/workflows/msys2.yml |
Adds MSYS2 and MinGW build coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gnulib visibility probe in configure passes for Windows GCC
targets (Cygwin, MSYS2 and MinGW-w64) because it never defines a
symbol with an explicit hidden attribute. visibility.h then used
attribute((visibility("hidden"))) for WP11_LOCAL, which PE/COFF
does not support, so every WP11_LOCAL function definition warned
"visibility attribute not supported in this configuration; ignored"
and builds from git, which use -Werror, failed.
Check the Windows targets before HAVE_VISIBILITY, as wolfSSL and
wolfTPM already do, and include CYGWIN in that check.
The newlib ctype.h used by Cygwin and MSYS2 also warns when isdigit()
is given a plain char, which broke the pkcs11mtt build. Cast the
argument to unsigned char there and in the two tests that cast it to
int, which only hid the warning.
Also adds a new CI test so we don't hit this again in future.
Fixes #223