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

Fix build issues with clang18 #94782

Merged
merged 5 commits into from
Nov 16, 2023
Merged

Conversation

Raphtaliyah
Copy link
Contributor

Clang 18 introduced some changes that prevent building CoreCLR:

  • The warning for VLAs being a C++ extension is now on by default which causes errors with -Werror. Enabled -Wvla-extension by default in C++ compilation modes llvm/llvm-project#62836
  • stdarg.h now has a guard that depends on va_arg not being defined, currently the PAL internal doesn't undefined that macro (despite PAL defining it) which prevent stdarg.h from being included again and causes missing symbols (va_start, va_end, va_arg).

This PR fixes the warning by disabling it to match the behaviour of previous clang versions and undefines va_arg in palinternal.h.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 15, 2023
@Raphtaliyah
Copy link
Contributor Author

@dotnet-policy-service agree

@@ -606,6 +606,10 @@ if (CLR_CMAKE_HOST_UNIX)
# other clang 16.0 suppressions
add_compile_options(-Wno-single-bit-bitfield-constant-conversion)
add_compile_options(-Wno-cast-function-type-strict)

# clang 18.0 warns about VLAs being a C++ extension by default.
Copy link
Member

Choose a reason for hiding this comment

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

Where do we use VLAs?

Copy link
Contributor Author

@Raphtaliyah Raphtaliyah Nov 15, 2023

Choose a reason for hiding this comment

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

There are two instance when building for Linux:

unsigned long nodeMask[nodeMaskLength];

and
char TempBuffer[nBufferLength > 0 ? nBufferLength : 1];

Copy link
Member

Choose a reason for hiding this comment

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

If it is just these two, I think it is better to convert them to use alloca.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be easy, I'll change them tomorrow! Should I also check if there are more in code that's only used for different platforms? Also if there are no notes in the coding guidelines it might be worth mentioning that VLAs shouldn't be used (and maybe enable warnings for them?)

Copy link
Member

Choose a reason for hiding this comment

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

That should be easy, I'll change them tomorrow!

Thanks!

Should I also check if there are more in code that's only used for different platforms?

I do not expect that you will find any. VLAs are not supported on Windows that prohibits their use in majority of the code. And there is very little code in the repo that is non-Windows non-Linux.

there are no notes in the coding guidelines it might be worth mentioning that VLAs shouldn't be used (and maybe enable warnings for them

VLAs are very niche. I do not think it is worth mentioning them in the coding guidelines. Enabling the warnings for them on older compilers sounds good if it is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted them to alloca and enabled warnings for unix hosts. If I understand it correctly only MSVC is supported for Windows which doesn't support VLAs (for C++), for Linux there is Clang and GCC, both of which support -Wvla, this should cover every supported compiler/platform.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit 487d7f0 into dotnet:main Nov 16, 2023
188 of 190 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants