20240215-benchmark-smallstack-refactors#7252
Merged
dgarske merged 3 commits intowolfSSL:masterfrom Feb 16, 2024
Merged
Conversation
dgarske
requested changes
Feb 16, 2024
Member
dgarske
left a comment
There was a problem hiding this comment.
Looks like this still needs some work. Scan-build is unhappy:
Example:
./configure --enable-singlethreaded --enable-sp=small --enable-smallstack --enable-smallstackcache CPPFLAGS="-DECC_CACHE_CURVE -DHAVE_WOLF_BIGINT -DWOLFSSL_OLD_PRIME_CHECK"
wolfcrypt/benchmark/benchmark.c: In function ‘bench_dh’:
./wolfssl/wolfcrypt/types.h:575:97: error: ‘priv$’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
575 | #define XFREE(p, h, t) {void* xp = (p); (void)(h); (void)(t); if (xp) wolfSSL_Free(xp);}
| ^~~~~~~~~~~~
wolfcrypt/benchmark/benchmark.c:8938:22: note: ‘priv$’ was declared here
8938 | WC_DECLARE_ARRAY(priv, byte, BENCH_MAX_PENDING,
| ^~~~
./wolfssl/wolfcrypt/types.h:588:19: note: in definition of macro ‘WC_DECLARE_HEAP_ARRAY’
588 | VAR_TYPE* VAR_NAME[VAR_ITEMS]; \
| ^~~~~~~~
wolfcrypt/benchmark/benchmark.c:8938:5: note: in expansion of macro ‘WC_DECLARE_ARRAY’
8938 | WC_DECLARE_ARRAY(priv, byte, BENCH_MAX_PENDING,
| ^~~~~~~~~~~~~~~~
* fix overallocation in WC_DECLARE_ARRAY() macro in the !WOLFSSL_SMALL_STACK path. * rename WC_INIT_ARRAY() to WC_ALLOC_ARRAY() for clarity (it doesn't initialize any memory). * rename WC_DECLARE_ARRAY_DYNAMIC_DEC(), WC_DECLARE_ARRAY_DYNAMIC_EXE(), and WC_FREE_ARRAY_DYNAMIC() to WC_DECLARE_HEAP_ARRAY(), WC_ALLOC_HEAP_ARRAY(), and WC_FREE_HEAP_ARRAY(), respectively, also for clarity, and refactor out the duplicate definitions. * add WC_ALLOC_VAR(), and move the XMALLOC() in smallstack WC_DECLARE_VAR() into it. smallstack WC_DECLARE_VAR() now initializes the pointer to NULL, like smallstack WC_DECLARE_ARRAY(), assuring all pointers are valid upon shortcircuit to cleanup for a failed allocation (see WC_ALLOC_DO_ON_FAILURE below). * add a new hook "WC_ALLOC_DO_ON_FAILURE" in WC_ALLOC_VAR(), WC_ALLOC_ARRAY(), and WC_DECLARE_ARRAY_DYNAMIC_EXE(), which is invoked when an allocation fails. by default the hook is defined to WC_DO_NOTHING. * add basic safety to WC_*_HEAP_ARRAY() by recording/detecting allocation state via idx##VAR_NAME. * add macros WC_ARRAY_OK() and WC_HEAP_ARRAY_OK() to test if allocation succeeded. * add macros WC_CALLOC_ARRAY() and WC_CALLOC_HEAP_ARRAY() which zero the objects. * add macro WC_CALLOC_VAR() which zeros the object. ED448: smallstack refactor of ge448_scalarmult_base(). src/tls.c tests/api.c wolfcrypt/test/test.c: update WC_DECLARE_VAR()s with now-required matching WC_ALLOC_VAR()s. wolfcrypt/benchmark/benchmark.c: * no functional changes in default error-free behavior. * add definition of WC_ALLOC_DO_ON_FAILURE() that prints error message, sets ret, and does goto exit. * add BENCH_NTIMES and BENCH_AGREETIMES overrideeable macros, to allow fast sanitizer runs and slow high-precision runs. * smallstack refactor of all declarations of stack arrays of the form foo[BENCH_MAX_PENDING], using WC_DECLARE_ARRAY() (35 in all). * additional smallstack refactors, using WC_DECLARE_VAR(), for bench_aesxts(), bench_ed448KeyGen(), bench_eccsi*(), and bench_sakke*(). * fixes for various unhandled error conditions around malloc failures. wolfcrypt/test/test.c: opportunistically constify several (42) static constants, moving them to the readonly data segment. linuxkm/Makefile: if ENABLED_LINUXKM_BENCHMARKS, add wolfcrypt/benchmark/benchmark.o to WOLFSSL_OBJ_FILES. linuxkm/Kbuild: enable FPU for benchmark.o, and remove enablement for module_hooks.o. linuxkm/module_hooks.c: remove inline include of benchmark.c.
…n several false positives around WC_DECLARE_ARRAY().
4683b83 to
3676dc0
Compare
Contributor
Author
|
@dgarske I addressed the apparently-false positives from |
dgarske
previously approved these changes
Feb 16, 2024
Contributor
Author
|
retest this please |
…ck WC_ALLOC_VAR().
Contributor
Author
|
retest this please |
dgarske
approved these changes
Feb 16, 2024
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.
wolfssl/wolfcrypt/types.h:WC_DECLARE_ARRAY()macro in the !WOLFSSL_SMALL_STACKpath.WC_INIT_ARRAY()toWC_ALLOC_ARRAY()for clarity (it doesn't initialize any memory).WC_DECLARE_ARRAY_DYNAMIC_DEC(),WC_DECLARE_ARRAY_DYNAMIC_EXE(), andWC_FREE_ARRAY_DYNAMIC()toWC_DECLARE_HEAP_ARRAY(),WC_ALLOC_HEAP_ARRAY(), andWC_FREE_HEAP_ARRAY(), respectively, also for clarity, and refactor out the duplicate definitions.WC_ALLOC_VAR(), and move theXMALLOC()in smallstackWC_DECLARE_VAR()into it. smallstackWC_DECLARE_VAR()now initializes the pointer toNULL, like smallstackWC_DECLARE_ARRAY(), assuring all pointers are valid upon shortcircuit to cleanup for a failed allocation (seeWC_ALLOC_DO_ON_FAILUREbelow).WC_ALLOC_DO_ON_FAILUREinWC_ALLOC_VAR(),WC_ALLOC_ARRAY(), andWC_ALLOC_HEAP_ARRAY(), which is invoked when an allocation fails. by default the hook is defined toWC_DO_NOTHINGpreserving the incumbent behavior.WC_*_HEAP_ARRAY()by recording/detecting allocation state viaidx##VAR_NAME.WC_ARRAY_OK()andWC_HEAP_ARRAY_OK()to test if allocation succeeded.WC_CALLOC_ARRAY()andWC_CALLOC_HEAP_ARRAY()which zero the objects.WC_CALLOC_VAR()which zeros the object.ED448: smallstack refactor of
ge448_scalarmult_base().src/tls.ctests/api.cwolfcrypt/test/test.c: updateWC_DECLARE_VAR()s with now-required matchingWC_ALLOC_VAR()s.wolfcrypt/benchmark/benchmark.c:WC_ALLOC_DO_ON_FAILURE()that prints error message, setsret, and doesgoto exit.BENCH_NTIMESandBENCH_AGREETIMESoverrideeable macros, to allow fast sanitizer runs and slow high-precision runs.foo[BENCH_MAX_PENDING], usingWC_DECLARE_ARRAY()` (35 in all).WC_DECLARE_VAR(), forbench_aesxts(),bench_ed448KeyGen(),bench_eccsi*(), andbench_sakke*().mallocfailures.wolfcrypt/test/test.c: opportunisticallyconstify several (42)staticconstants, moving them to the readonly data segment.linuxkm/Makefile: ifENABLED_LINUXKM_BENCHMARKS, addwolfcrypt/benchmark/benchmark.otoWOLFSSL_OBJ_FILES.linuxkm/Kbuild: enable FPU forbenchmark.o, and remove enablement formodule_hooks.o.linuxkm/module_hooks.c: remove inline include ofbenchmark.c.tested with
wolfssl-multi-test.sh ... super-quick-check all-max-func-stack-2k linuxkm-benchmarks sanitize-all-wolfcrypt-benchmark sanitize-all-wolfcrypt-benchmark-smallstack.sanitize-all-wolfcrypt-benchmarkandsanitize-all-wolfcrypt-benchmark-smallstackare new scenarios that runbenchmarkunder the sanitizer.all-max-func-stack-2kis modified from--disable-benchmarkto--enable-benchmark.before fix to WC_DECLARE_ARRAY(), bench_aescbc_internal() (for example) allocated 1218816 bytes on stack, versus needed 1104 bytes.