-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Improve build error on systems that have a 32-bit time_t #104368
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
The logic was assuming we use 64-bit
time_t
. If this is 32-bittime_t
, it can't be used to detect whether openssl was built with 64-bittime_t
below - to do the detection, we must pass the value 0x80000000U to openssl and see how it gets interpreted.Could we instead fix this by updating the armv6 build to use 64-bit
time_t
?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.
Okay. Now I understand the intention behind this check.
Is that a reasonable assumption? Should we just set
g_libSslUses32BitTime
to1
ifsizeof(time_t) == sizeof(int32_t)
and not bother asking OpenSSL?That would perhaps work for our CI, but what about other community builds where time_t is 32-bit?
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 it's possible we built with 32-bit
time_t
, but are running on a system where openssl uses 64-bittime_t
.I am not sure how we decide the OS compatibility for armv6, but for the arm32 builds we updated our minimum supported glibc to one that supports 64-bit
time_t
. Could we do the same for armv6, or do the community builds need to continue targeting 32-bittime_t
?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 does not work right now, either, since you said "The logic was assuming we use 64-bit time_t." If we require a 64-bit time_t then we static assert it so folks have a better compilation message.
If we do that, then we can still switch to using
(time_t)0x80000000
so that people using a 32-bit time_t do not see error messages around overflows.That doesn't help us with what to do about the armv6 build. Are we basically saying that .NET requires a 64-bit time_t now?
I don't know much about those builds or who owns them. @ViktorHofer or @akoeplinger may know?
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.
After jogging my memory I am remembering that this came up in #102775 (comment).
In response I added a new armv6 image that supports 64-bit time_t: dotnet/dotnet-buildtools-prereqs-docker#1077. It should let us build the product, but tests will fail if they run on older raspbian.
Unfortunately I haven't had time to figure out how to build a helix image for armv6 using newer raspbian. I had a start in dotnet/dotnet-buildtools-prereqs-docker#1083, but eventually ran into issues building the helix client similar to dotnet/dnceng#2808.
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 meant if we fix this to build with 32-bit time_t, it's still possible that openssl is using 64-bit time_t, so just setting g_libSslUses32BitTime to 1 isn't the right fix. A static assert sounds like a good idea to me.
Yes, that's what we did for the officially supported arm32 builds. I am not sure who in practice owns the community-supported raspbian builds, but I would suggest doing the same.
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.
Okay. I added a static assert and undid some comment changes. I think replacing INT_MAX + 1 is still helpful so that it avoids UB on systems where time_t is 32-bit, that way the only error they see is the static assert when building.
This puts us back in to a broken build, but at least now the build is broken with a better error message.