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 NULL conversion warnings found in Alpine x64 Source-Build #100796

Merged
merged 10 commits into from
Apr 13, 2024

Conversation

jkoritzinsky
Copy link
Member

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@@ -242,21 +242,21 @@ void CustomAssemblyBinder::PrepareForLoadContextRelease(INT_PTR ptrManagedStrong
CustomAssemblyBinder::CustomAssemblyBinder()
{
m_pDefaultBinder = NULL;
m_ptrManagedStrongAssemblyLoadContext = NULL;
m_ptrManagedStrongAssemblyLoadContext = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I worry that this kind of change reduces code readability? Are these new warnings related to a tooling update?

Copy link
Member Author

Choose a reason for hiding this comment

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

With a more recent musl libc (the version in Alpine 3.19), NULL is defined as nullptr directly, making what was suppressable warnings with our current toolsets into unsupressable compiler errors. This PR includes fixes for all the locations that were now procuding errors for an x64 build on Alpine 3.19 (what the source-build pipeline tests).

In the future, I plan to follow up with re-enabling the warnings and fixing the remaining cases

Copy link
Member

Choose a reason for hiding this comment

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

so is there no way to #ifdef NULL to 0 somehow for this platform?

cc: @AaronRobinsonMSFT @janvorli

Copy link
Member

Choose a reason for hiding this comment

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

This PR uses (UINT_PTR)NULL instead of 0 in a similar case, it seems like it would be better here too.

Copy link
Member

Choose a reason for hiding this comment

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

Here it would be actually (INT_PTR)NULL

@jkoritzinsky
Copy link
Member Author

Okay this PR has now included all of the various different changes to handle the three different definitions of NULL that the standard libraries we build against use (and how they interact with our DAC-ized pointer types).

  • #define NULL nullptr as used by newer versions of musl libc (Alpine 3.19)
  • #define NULL __null as used by glibc (GCC/Clang)
  • #define NULL 0 as used by MSVC

This generally involved changing code like TADDR foo = NULL to TADDR foo = (TADDR)NULL or TADDR foo = 0 and changing PTR_Bar foo = NULL to PTR_Bar foo = nullptr.

@jkoritzinsky jkoritzinsky requested review from mangod9 and a team and removed request for a team April 11, 2024 18:12
@jkoritzinsky
Copy link
Member Author

I've taken a different approach here that should still work and cut the diff in half (and removed the majority of the changes required for non Alpine).

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

One suggestion and a few nits to consider. Thanks!

…legating constructors.

Also remove the explicit bool cast as it breaks comparisons against NULL with MSVC's definition.
@jkoritzinsky jkoritzinsky merged commit 93f886f into dotnet:main Apr 13, 2024
110 checks passed
@jkoritzinsky jkoritzinsky deleted the alpine-null-fix branch April 13, 2024 01:14
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alpine Legs failing to build CoreCLR
4 participants