Repository navigation
Shrink the wc_GetErrorString() lookup table - #11861
michael-membrowse wants to merge 3 commits into
Conversation
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
🟢 Approval recommended
The remapping preserves error-string selection and boundary handling, with existing tests covering both spans and unknown-code ranges.
No open issues 🎉
What changed in this PR
This PR reduces the memory footprint of wolfCrypt’s error-string lookup by internally remapping the second error-code span into a compact range, without changing public error codes or returned strings.
Changes:
- Remaps span 2 before the lookup switch to reduce compiler-generated table size.
- Adds an unused enum switch to retain compiler warnings for missing error-code cases.
- Shares the unknown-error string across fallback paths.
| File | Description |
|---|---|
wolfcrypt/src/error.c |
Compacts error-string lookup while preserving enum coverage checks and unknown-code handling. |
Review effort: Balanced
|
Hey @michael-membrowse, are you able and willing to sign a contributor agreement so that we can get things moving? Details on how to do it are here, but basically you need to email support@wolfssl.com referencing this pull request and the team will help you out: https://github.com/wolfSSL/wolfssl/blob/master/CONTRIBUTING.md#contributor-agreement-required |
|
Okay to test. Contributor agreement "In progress". Let's see what our CI does with this optimization. Thank you @michael-membrowse |
|
Contributor Agreement has been approved. Please move forward with this review. |
|
Hi @michael-membrowse I am reviewing this. For some reason I can't mark the PR ready for review, so would you do that when its "ready"? Thank you |
|
Hi, @dgarske. I have another PR open (MemBrowse action version bump). |
|
Hi @michael-membrowse , Nice find on the sparse range, and the output is provably unchanged. I linked both versions into one binary under different symbol names and compared
The table does not shrink from 4172 to 1376 on GCC 13, it disappears, and is replaced by more code. On master, so the switch falls through to Moving the switch into its own function, so the pass sees a body that is nothing but the switch, recovers it: static const char* wc_GetErrorStringIdx(int error)
{
switch (error) {
/* ... body unchanged ... */
default:
return wc_unknown_error_str;
}
}
WOLFSSL_ABI
const char* wc_GetErrorString(int error)
{
if ((error <= WC_SPAN2_FIRST_E) && (error >= WC_SPAN2_MIN_CODE_E)) {
error = WC_SPAN2_TO_IDX(error);
}
else if (error < WC_SPAN1_MIN_CODE_E) {
return wc_unknown_error_str;
}
return wc_GetErrorStringIdx(error);
}With that, Cortex-M4 Thanks, |
|
Hi @dgarske, Thanks for your detailed review. I applied your suggestion of moving the switch to a separate function. here are the updated results with your suggestion:
|
|
retest this please |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Jenkins retest this please. FIPS test "cmd" missing (not related to this PR) |
|
wolfCrypt error codes sit in two spans, -97..-300 and -1000..-1999. The switch in wc_GetErrorString() covers both, so compilers that lower it to a lookup table (GCC -O2/-Os, clang) build one table from 0 to the last span 2 code, with an entry for each of the ~800 unused codes, most of them between the spans. Move span 2 to just below span 1 before the switch, so the table covers one dense range. Output is unchanged for every input. Error code values are not changed; the remapping is internal to the function. On Cortex-M4 with arm-none-eabi-gcc -Os, the table shrinks from 4172 to 1376 bytes and error.o from 12911 to 10135 bytes. The switch now runs on remapped values, so the enum cast that lets -Wswitch-enum catch a code without a string moves to a new, never called wc_ErrorStringsCheck() that lists every code. error_test() still checks that every code in both spans has a string.
GCC 12 and later stop turning the switch into a lookup table when the span 2 remap comes before it in the same function. They emit a jump table and one block per case instead, which saves less on Thumb and grows the code in ARM mode and on 32-bit PowerPC. Move the switch into wc_GetErrorStringIdx(), so it compiles to the small table again on every compiler measured. Output is unchanged.
Add a static assert that span 2 starts below span 1, which the remap relies on. Move wc_ErrorStringsCheck() below wc_GetErrorStringIdx(), so a -Wswitch-enum warning for a new code points past the switch that also needs its string. Fix a comment that named the wrong function.
f5d8a6f to
258f06e
Compare
wolfCrypt error codes sit in two spans, -97..-300 and -1000..-1999. The switch in wc_GetErrorString() covers both, so compilers that lower it to a lookup table (GCC -O2/-Os, clang) build one table from 0 to the last span 2 code, with an entry for each of the ~800 unused codes, most of them between the spans.
Move span 2 to just below span 1 before the switch, so the table covers one dense range. Output is unchanged for every input. Error code values are not changed; the remapping is internal to the function. On Cortex-M4 with arm-none-eabi-gcc -Os, the table shrinks from 4172 to 1376 bytes and error.o from 12911 to 10135 bytes.
The switch now runs on remapped values, so the enum cast that lets -Wswitch-enum catch a code without a string moves to a new, never called wc_ErrorStringsCheck() that lists every code. error_test() still checks that every code in both spans has a string.
ZD 22676