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

Improve build error on systems that have a 32-bit time_t #104368

Merged
merged 4 commits into from
Jul 4, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,11 @@ void InitializeOpenSSLShim(void)
}

#if defined(TARGET_ARM) && defined(TARGET_LINUX)
c_static_assert_msg(sizeof(time_t) == 8, "Build requires 64-bit time_t.");

// This value will represent a time in year 2038 if 64-bit time is used,
// or 1901 if the lower 32 bits are interpreted as a 32-bit time_t value.
time_t timeVal = (time_t)INT_MAX + 1;
time_t timeVal = (time_t)0x80000000U;
Copy link
Member

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-bit time_t, it can't be used to detect whether openssl was built with 64-bit time_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?

Copy link
Member Author

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.

The logic was assuming we use 64-bit time_t.

Is that a reasonable assumption? Should we just set g_libSslUses32BitTime to 1 if sizeof(time_t) == sizeof(int32_t) and not bother asking OpenSSL?

Could we instead fix this by updating the armv6 build to use 64-bit time_t?

That would perhaps work for our CI, but what about other community builds where time_t is 32-bit?

Copy link
Member

Choose a reason for hiding this comment

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

Should we just set g_libSslUses32BitTime to 1 if sizeof(time_t) == sizeof(int32_t) and not bother asking OpenSSL?

I think it's possible we built with 32-bit time_t, but are running on a system where openssl uses 64-bit time_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-bit time_t?

Copy link
Member Author

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-bit time_t.

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.

static_assert(sizeof(time_t) == 8, "64-bit time_t is required.");

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?

or do the community builds need to continue targeting 32-bit time_t?

I don't know much about those builds or who owns them. @ViktorHofer or @akoeplinger may know?

Copy link
Member

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.

Copy link
Member

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-bit time_t.

But that does not work right now, either, since you said "The logic was assuming we use 64-bit time_t."

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.

Are we basically saying that .NET requires a 64-bit time_t now?

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.

Copy link
Member Author

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.

struct tm tmVal = { 0 };

// Detect whether openssl is using 32-bit or 64-bit time_t.
Expand Down
Loading