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.

Sign up for GitHub

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

Jump to bottom

[portable-snippets] Fix array length issue #35791

Closed
wants to merge 10 commits into from
Closed

[portable-snippets] Fix array length issue #35791

wants to merge 10 commits into from

Conversation

jimwang118
Copy link
Contributor

@jimwang118 jimwang118 commented Dec 20, 2023 •
edited
Loading 

vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(34): error C2057: expected constant expression
vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(34): error C2466: cannot allocate an array of constant size 0
vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(372): error C2057: expected constant expression
vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(372): error C2466: cannot allocate an array of constant size 0

The function psnip_random_bytes in the header file random.h defines the array data[PSNIP_RANDOM_ARRAY_PARAM(length)]. However, the size of this array is a variable. C language requires that the size of the array be a constant during compilation, so the compilation fails.
So the incoming array will be modified by the incoming pointer.

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

Usage test pass with following triplets:

x86-windows
x64-windows
x64-windows-static

@jimwang118 jimwang118 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Dec 20, 2023
Cheney-W
Cheney-W previously approved these changes Dec 21, 2023
View reviewed changes
@jimwang118 jimwang118 marked this pull request as ready for review December 21, 2023 03:00
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Dec 21, 2023
@jimwang118
Copy link
Contributor Author

This problem has been submitted to the upstream issue 48 for feedback, but there was no reply from the upstream for a long time, so I tried to fix it in vcpkg.

@BillyONeal
Copy link
Member

The function psnip_random_bytes in the header file random.h defines the array data[PSNIP_RANDOM_ARRAY_PARAM(length)]. However, the size of this array is a variable. C language requires that the size of the array be a constant during compilation, so the compilation fails.

This is not true: Variable Length Arrays (VLAs) exist. GCC implements them and I believe Clang implements them for compatibility with GCC, MSVC does not implement them. My understanding is that that feature is widely considered to be a mistake, but it's been in there since 1999.

Are there necessary ci.baseline.txt changes or something? (Why is this not causing the current builds to fail?)

dg0yt
dg0yt reviewed Dec 21, 2023
View reviewed changes
ports/portable-snippets/CMakeLists.txt Outdated Show resolved Hide resolved
ports/portable-snippets/fix_array.patch Outdated
#endif

-static int (* psnip_random_secure_generate)(size_t length, psnip_uint8_t data[PSNIP_RANDOM_ARRAY_PARAM(length)]) = NULL;
+static int (* psnip_random_secure_generate)(size_t length, psnip_uint8_t *data) = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PSNIP_RANDOM_ARRAY_PARAM is defined in the header. Can't it choose a supported definition?
The default definition us empty, i.e.

static int (* psnip_random_secure_generate)(size_t length, psnip_uint8_t data[]) = NULL;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After adding the C11 option, the default options were not entered and a compilation error occurred.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you could simply add a single && !defined(_MSC_VER) to the condition before the second definition alternative for PSNIP_RANDOM_ARRAY_PARAM. Like it already excludes __PGI.

@jimwang118
Copy link
Contributor Author

The function psnip_random_bytes in the header file random.h defines the array data[PSNIP_RANDOM_ARRAY_PARAM(length)]. However, the size of this array is a variable. C language requires that the size of the array be a constant during compilation, so the compilation fails.

This is not true: Variable Length Arrays (VLAs) exist. GCC implements them and I believe Clang implements them for compatibility with GCC, MSVC does not implement them. My understanding is that that feature is widely considered to be a mistake, but it's been in there since 1999.

Are there necessary ci.baseline.txt changes or something? (Why is this not causing the current builds to fail?)

This is an issue of VCPKG testing MSVC.
The issue with VCPKG testing MSVC is as follows:

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.36.32532\include\threads.h(101): error C2054: expected '(' to follow '_Noreturn'
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.36.32532\include\threads.h(101): error C2085: 'thrd_exit': not in formal parameter list
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.36.32532\include\threads.h(104): error C2085: 'thrd_join': not in formal parameter list
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.36.32532\include\threads.h(105): error C2085: '_thrd_sleep32': not in formal parameter list
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.36.32532\include\threads.h(107): error C2085: '_thrd_sleep64': not in formal parameter list
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.36.32532\include\threads.h(116): error C2085: 'thrd_sleep': not in formal parameter list
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.36.32532\include\threads.h(116): error C2143: syntax error: missing ';' before '{'

This problem is due to the lack of compilation option -std:c11, after adding this option, a new problem has appeared.

vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(34): error C2057: expected constant expression
vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(34): error C2466: cannot allocate an array of constant size 0
vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(372): error C2057: expected constant expression
vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(372): error C2466: cannot allocate an array of constant size 0

@BillyONeal
Copy link
Member

This is an issue of VCPKG testing MSVC.

My point is, don't there need to be ci.baseline.txt or similar changes? If MSVC hates it why aren't we seeing it failing in CI builds?

@BillyONeal
Copy link
Member

https://dev.azure.com/vcpkg/public/_build/results?buildId=97842

Installing 1740/2229 portable-snippets:x64-windows@2019-09-20#3...
Building portable-snippets:x64-windows@2019-09-20#3...
-- Note: portable-snippets only supports static library linkage. Building static library.
-- Downloading https://github.com/nemequ/portable-snippets/archive/26496acb37ab46ee249ea19d45381da6955d89c4.tar.gz -> nemequ-portable-snippets-26496acb37ab46ee249ea19d45381da6955d89c4.tar.gz...
-- Extracting source D:/downloads/nemequ-portable-snippets-26496acb37ab46ee249ea19d45381da6955d89c4.tar.gz
-- Using source at D:/b/portable-snippets/src/a6955d89c4-b026d6ec08.clean
-- Found external ninja('1.11.0').
-- Configuring x64-windows
-- Building x64-windows-dbg
-- Building x64-windows-rel
-- Performing post-build validation
Stored binaries in 1 destinations in 94.7 ms.
Elapsed time to handle portable-snippets:x64-windows: 2.1 s

Is the frontend team aware that they regressed this? I don't object to merging this but this seems like the kind of regression our testing is supposed to catch, not paper over.

@BillyONeal
Copy link
Member 

vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(34): error C2057: expected constant expression
vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(34): error C2466: cannot allocate an array of constant size 0
vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(372): error C2057: expected constant expression
vcpkg\buildtrees\portable-snippets\src\a6955d89c4-b026d6ec08\random\random.c(372): error C2466: cannot allocate an array of constant size 0

This looks like a compiler bug to me. PSNIP_RANDOM_ARRAY_PARAM is defined as

#if defined(HEDLEY_ARRAY_PARAM)
#  define PSNIP_RANDOM_ARRAY_PARAM(expr) HEDLEY_ARRAY_PARAM(expr)
#elif defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) && !defined(__cplusplus) && !defined(__PGI)
#  define PSNIP_RANDOM_ARRAY_PARAM(expr) (expr)
#else
#  define PSNIP_RANDOM_ARRAY_PARAM(expr)
#endif

which means that without -std:c11, PSNIP_RANDOM_ARRAY_PARAM(length) should expand to nothing. That means:

static int (* psnip_random_secure_generate)(size_t length, psnip_uint8_t data[PSNIP_RANDOM_ARRAY_PARAM(length)]) = NULL;

should expand to

static int (* psnip_random_secure_generate)(size_t length, psnip_uint8_t data[]) = NULL;

which is valid. The compiler appears wrong to treat a macro expanding to no tokens as a constant zero.

-std:c11 changes __STDC_VERSION__ to be greater than 199901L, which changes the expansion to just (length) which is why you end up needing the subsequent patch.

Has a compiler frontend dev seen this?

@BillyONeal BillyONeal marked this pull request as draft December 21, 2023 19:27
@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Dec 21, 2023
@jimwang118
Copy link
Contributor Author

jimwang118 commented Dec 22, 2023 •
edited
Loading

Has a compiler frontend dev seen this?

There is a related issue 1250747 with compiler development, but this issue has not been fixed yet. Do we need to wait for the compiler developers to fix this?

@LilyWangLL LilyWangLL mentioned this pull request Jan 16, 2024
Merged
@BillyONeal
Copy link
Member

Has a compiler frontend dev seen this?

There is a related issue 1250747 with compiler development, but this issue has not been fixed yet. Do we need to wait for the compiler developers to fix this?

That seems unrelated since there is no static here.

@BillyONeal
Copy link
Member

I believe the correct fix is nemequ/portable-snippets#50

@jimwang118
Copy link
Contributor Author

I believe the correct fix is nemequ/portable-snippets#50

OK, I'll fix it and test it.

@jimwang118
Copy link
Contributor Author

Duplicate of #36143.

@jimwang118 jimwang118 closed this Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dg0yt dg0yt dg0yt left review comments

@Cheney-W Cheney-W Cheney-W left review comments

Assignees

@Cheney-W Cheney-W

@LilyWangLL LilyWangLL

Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jimwang118 @BillyONeal @dg0yt @Cheney-W @LilyWangLL