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

For MSVC use alloca() instead of VLA #26

Merged
merged 1 commit into from
Aug 3, 2022
Merged

For MSVC use alloca() instead of VLA #26

merged 1 commit into from
Aug 3, 2022

Conversation

bwesterb
Copy link
Contributor

Closes #20

@thomwiggers

@bwesterb bwesterb requested review from kste and sfluhrer December 29, 2021 12:18
@thomwiggers
Copy link
Contributor

Was this tested? AFAICT MSVC doesn't have alloca, but it does have _alloca (deprecated) in malloc.h (for maximum confusion).

@sfluhrer
Copy link
Contributor

I'm not certain that allocating variable length automatic arrays (either with VLAs or using alloca) is the Right Thing. Two examples:

  • haraka_S_absorb - the rate is always 32 - why do we need a variable length array?
  • thash (the robust versions) - currently, we allocate the full array, generate the full mask, and xor it in. An alternative approach would be generate the mask one block at a time, and process it incrementally - see https://github.com/sphincs/parallel-sphincsplus/blob/main/shake256_robust.cpp for an example of that being done.

@thomwiggers
Copy link
Contributor

Even though this perhaps isn't at all the best way to solve this, it would be great if we can actually merge some form of this PR so that we can proceed with updating PQClean (and OQS downstream).

@bwesterb
Copy link
Contributor Author

bwesterb commented Aug 2, 2022

@sfluhrer I want to propose to merge a rebased version of this patch together with Thom's suggestion for now. Then, later, we can get rid of the variable sized arrays alltogether.

@bwesterb
Copy link
Contributor Author

bwesterb commented Aug 2, 2022

@thomwiggers Rebased. If builds pass, please give it a double-check.

Closes #20

Co-authored-by: Thom Wiggers <thom@thomwiggers.nl>
@thomwiggers
Copy link
Contributor

thomwiggers commented Aug 3, 2022

Seems to compile just fine using MSVC. There's some more work needed before this repo will produce useful binaries (mostly because the test binaries here use OpenSSL and the random number generator isn't set up for Windows) -- but I'm not getting VLA problems here.

@bwesterb bwesterb merged commit 7b6cc5f into master Aug 3, 2022
thomwiggers added a commit to thomwiggers/sphincsplus that referenced this pull request Jan 23, 2023
Continuation of sphincs#26
@thomwiggers thomwiggers mentioned this pull request Jan 23, 2023
bwesterb pushed a commit that referenced this pull request Jan 27, 2023
Continuation of #26
@bwesterb bwesterb deleted the novla branch January 27, 2023 15:04
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.

Variable-length arrays are not compatible with MSVC
3 participants