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 pal_assert.h when PAL_STDCPP_COMPAT defined #64631

Merged
merged 10 commits into from
Apr 14, 2022

Conversation

kant2002
Copy link
Contributor

@kant2002 kant2002 commented Feb 1, 2022

Improves #37310

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 1, 2022
@kant2002
Copy link
Contributor Author

kant2002 commented Feb 1, 2022

I prefer to fix mentioned issue in piecemeal fashion, so this is to check if I understand ropes properly. Mentioned issue important for NativeAOT-LLVM, so you can this about this as of the rationale for this PR.

@kant2002 kant2002 changed the title Fix pal_assert.h for PAL_STDCPP_COMPAT Fix pal_assert.h when PAL_STDCPP_COMPAT defined Feb 1, 2022
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.

Yes, this looks good.

@jkotas
Copy link
Member

jkotas commented Feb 1, 2022

The profiler test failures seem to be caused by the change in this PR.

@jkotas jkotas added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 1, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 2, 2022
@kant2002
Copy link
Contributor Author

kant2002 commented Feb 2, 2022

I need advice. Errors coming from https://github.com/dotnet/runtime/blob/d4f06a9c524819dfd1345745a17b3cc6e060ba8b/src/coreclr/inc/corhlpr.cpp where _ASSERTE is used. _ASSERTE require me to link to corepal which seems to be not an option for tests.

Personally I would favor link to corepal in this particular test, but modifying CMakeLists.txt without some dity hacks is not trivial for me in this specific case.

@kant2002
Copy link
Contributor Author

kant2002 commented Feb 2, 2022

My understanding that PAL is source level component which does not have physical representation on the disk. Adding files from PAL to tests seems to be too much.

@mangod9
Copy link
Member

mangod9 commented Mar 14, 2022

Hi @kant2002, thanks for your contribution appears that there are still CI failures with this change. Do you plan on investigating further?

@kant2002
Copy link
Contributor Author

Hi @kant2002, thanks for your contribution appears that there are still CI failures with this change. Do you plan on investigating further?

Yes. Thanks for remind me about that PR

@mangod9
Copy link
Member

mangod9 commented Apr 11, 2022

@kant2002, gentle ping again.

@jkotas jkotas added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 11, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 12, 2022
@kant2002
Copy link
Contributor Author

Thank you @mangod9 ! Without that ping, I would probably again forgot about this PR.

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 32e03eb into dotnet:main Apr 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 14, 2022
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.

4 participants