Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Share compiler configurations across libs & coreclr #58096
Share compiler configurations across libs & coreclr #58096
Changes from 1 commit
2ceaa4b
3f1f787
ef1f861
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@am11 why have you removed this line and the same one in the configure.cmake and inb the hosts CMakeLists? It is not correct, we want to support 64 bit file offsets on all platforms. The PAL tests on ARM are now failing due to this (see #58481). I don't see this removal mentioned in the PR description. I have not noticed this before, I was OOF when this PR was created and merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janvorli, it was moved at common place:
runtime/eng/native/configurecompiler.cmake
Lines 280 to 281 in 5181e1c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that doesn't influence the compiler options in the configure.cmake. So while this place and the hosts are likely ok, the configure.cmake change causes the problem, as we detect off_t as 4 bytes due to the missing define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can double-check, but I am pretty sure global defines are used in cmake introspection unless explicitly stripped off.
Besides, #58481 was opened ~11 hours before this PR was merged..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I mixed up; was checking on mobile, but it does seem like first occurrence of
File sizes do not match up.
happened after this was merged. The one on 24th of August is different errorCall to SetEndOfFile failed with 112
. Will check it out shortly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just verified that adding that line back to configure.cmake fixes the failing tests (I've tried without it to eliminate the possibility that something else is in play here). And the global defines are really not used in the introspection, I've checked the CMakeOutput.txt and the only definitions passed to the compiler were the ones in the CMAKE_REQUIRED_DEFINITIONS:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, we set
CMAKE_REQUIRED_DEFINITIONS
so whatever it had before is reset. #59504 for the fix. Thanks for spotting it! :)