Repository navigation
Conversation
WOLF_C99 is on for IAR by default, so IAR (and Keil AC5) got __asm__, which neither accepts.
|
@claude review |
There was a problem hiding this comment.
🔵 Needs a closer look
Clang preprocessing confirms branch selection but cannot establish inline-assembly syntax acceptance, which still requires compilation with IAR (including --strict) and Keil AC5/AC6.
No open issues 🎉
What changed in this PR
This PR adjusts wolfCrypt’s XASM_VOLATILE() compiler selection so IAR and Keil builds use their supported assembly keyword even when WOLF_C99 is defined.
Changes:
- Check IAR and Keil before
WOLF_C99, using__asm volatile(a)for both. - Document the compiler compatibility fix in the header and changelog.
| File | Description |
|---|---|
wolfssl/wolfcrypt/wc_port.h |
Prioritizes IAR and Keil assembly syntax over the generic C99 branch. |
ChangeLog.md |
Records the compatibility fix and affected compiler configurations. |
Review effort: Balanced
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix only matters on IAR and Keil, and neither toolchain runs in CI, so it would still be worth having someone build it with real IAR and Keil AC5 compilers.
What was reviewed:
- The new order in
XASM_VOLATILE(). IAR and__KEIL__now take the__asm volatilebranch even whenWOLF_C99is set. GCC, clang and the default path still expand exactly as before. - Every place that uses it. They are all the
XFENCE()fallbacks inwc_port.h. AC6 (armclang) defines__clang__, so it reaches__atomic_thread_fencebefore any of the asm fallbacks, which means the new order doesn't change what it gets. - The ChangeLog entry, which matches the code change.
Extended reasoning...
The PR changes the order of the compiler checks in the XASM_VOLATILE() macro in wolfssl/wolfcrypt/wc_port.h and adds a ChangeLog entry. Nothing else changes. The macro is used only by the XFENCE() speculation-barrier fallbacks, which are part of wolfCrypt's hardening, so they are crypto-adjacent. The logic is small and looks right, but whether __asm volatile("...") builds depends on the vendor compiler (IAR in strict mode and Keil AC5 armcc), and CI has neither. The author also asks for a real-toolchain test. No CODEOWNERS file covers these paths.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11959
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): ChangeLog.md
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
Description
XASM_VOLATILE() checked WOLF_C99 first. WOLF_C99 is set whenever STDC_VERSION >= 199901L (wc_port.h:38), and IAR defaults to C99 or later. So every IAR build that didn't use --c89 or define NO_WOLF_C99 expanded to asm volatile(...), and IAR doesn't accept asm. Keil's AC5 compiler (armcc) also rejects asm and was affected when built with KEIL. This explains why IDE/IAR-EWARM/Projects/user_settings.h defines NO_WOLF_C99.
IAR and KEIL are now checked first, in a single arm that uses __asm volatile(...). IAR keeps __asm under --strict, where plain asm is not available. Both Keil compilers, AC5 and AC6 (armclang), accept __asm.
Testing
CI has no IAR or Keil toolchain, so I checked the preprocessor output by running clang with the vendor macro defined and -U__GNUC__ -U__clang__. A test with an actual IAR or Keil build would be appreciated.
Checklist