Deprecate windows time64_t

[dybucc]

Description

Functionality related to the Windows time64_t has been deprecated in favor of
a single, 64-bit wide time_t. This has also required some work into getting
rid of the conditional compilation uses of time_t on GNU target environments,
and tweaking the max_align_t type. But that was separated onto a different PR
in #5050.

The FIXME comment on the tier 1 platform support for Windows with GNU has also
been removed as no segfaults were observed.

There's not yet an issue open specifically for deprecation of non-64-bit wide
time and offset-related values, so the linked issue in the deprecation notice
for time64_t is still pending.

Sources

• Mingw source code showing how is the bit-width of time_t values is handled
  under Windows GNU.
  https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-headers/crt/time.h#L54-L57
  https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-headers/crt/time.h#L235-L250

Checklist

• Relevant tests in libc-test/semver have been updated
• No placeholder or unstable values like *LAST or *MAX are included (see
  #3131)
• Tested locally (cd libc-test && cargo test --target mytarget);
  especially relevant for platforms that may not be checked in CI

[tgross35]

View changes since the review

Would you mind splitting whatever is needed to fix this to a separate PR? That would be great to have independent of deprecation.

Think that includes this portion of the file, the shell script, and src/windows/gnu/mod.rs.

[dybucc]

Done. Those changes are now in #5050.

[tgross35]

View changes since the review

Have you run any tests before and after this change for whether things work? I'm wondering about this group of functions where it seems like we're already linking the time64 version in some cases

libc/src/windows/mod.rs

Lines 393 to 413 in ff0e616

	pub fn ctime(sourceTime: *const time_t) -> *mut c_char;
	pub fn difftime(timeEnd: time_t, timeStart: time_t) -> c_double;
	#[link_name = "_gmtime64_s"]
	pub fn gmtime_s(destTime: *mut tm, srcTime: *const time_t) -> c_int;
	#[link_name = "_get_daylight"]
	pub fn get_daylight(hours: *mut c_int) -> errno_t;
	#[link_name = "_get_dstbias"]
	pub fn get_dstbias(seconds: *mut c_long) -> errno_t;
	#[link_name = "_get_timezone"]
	pub fn get_timezone(seconds: *mut c_long) -> errno_t;
	#[link_name = "_get_tzname"]
	pub fn get_tzname(
	p_return_value: *mut size_t,
	time_zone_name: *mut c_char,
	size_in_bytes: size_t,
	index: c_int,
	) -> errno_t;
	#[link_name = "_localtime64_s"]
	pub fn localtime_s(tmDest: *mut tm, sourceTime: *const time_t) -> crate::errno_t;
	#[link_name = "_time64"]
	pub fn time(destTime: *mut time_t) -> time_t;

, and am thinking that we might have a bug with the current (before this PR) impl.

To test this you could use the libc from main and pass a pointer to a [time_t; 2] to the time function, both initialized to 0x1234abcd or whatever. If only one of them gets overwritten with the current time, there isn't a bug and things are working despite the link name. If one gets the current time and the other gets zeroed, then we have a bug.

This PR would fix it but we can't backport because of the breakage, so we'll need configure the link_name attributes behind not(x86+gnu).

(I realize that's rather confusing, let me know if anything needs clarification)

[dybucc]

Regarding the first question on tests, I have run the existing libc test suite
both before and after the changes, though admittedly I did not run it prior to
making the changes to src/windows/mod.rs, but without skipping time_t in
build.rs.

I'm not sure what do you mean when you say those routines may be faulty in the
current main worktree. From what I could gather of the Windows CRT docs (e.g.
[1], as all routines explain things similarly in this respect,) there
shouldn't be any issues, considering the non-suffixed variants are just wrappers
for the suffixed, 64-bit variants. So linking the 64-bit symbol with a type that
is also 64-bits wide now should be fine. Still, I understand there may be some
concerns with GNU on Windows, as in those cases, the expected time_t (prior to
this PR) was 32-bits wide, so that could be an issue.

Either way, I'll get back in a few hours once I actually run your test.

[dybucc]

There may be a bug in the current main worktree. Running on Windows x86 GNU,
with a two-element array of 32-bit time_t, a 64-bit store is being performed
on the pointer's pointee that gets passed to one of these 64-bit-linked routines
(tested it out with time() in libc, which links by default on all
targets/environments to _time64()) as one of the elements is zeroed after the
call.

I would personally say the bug is not in linking these routines by default to
their 64-bit variants, but rather in the fact that we assume a 32-bit time_t
on Windows GNU x86. By default, Rust's libc behaves correctly under MSVC, as
time_t is always 64-bit wide unless USE_32BIT_TIME_T is defined, no matter
the target triple, and in libc we opted out of adding support for that feature
test macro. On the other hand, when running under mingw with GNU in x86, we
always define time_t to be 32-bit wide. This, according to [1] and [2], is
incorrect, as behavior in mingw is exactly like that in MSVC when it comes to
bit-width; Namely, define USE_32BIT_TIME_T if you want 32-bit time_t and
non-suffixed variants to default to their 32-bit variants, otherwise, it's
64-bit time_t all the way through.

Running the test with the changes in this PR solves the issues, simply because
there's no 32-bit time_t exposed on Rust's libc under Windows anymore.

[tgross35]

Would you be able to add that test to libc-test/tests?

[dybucc]

Done.

[dybucc]

Just updated the PR description and removed the stable nomination, as there's
clearly no backwards-compatible changes to removing a type of a certain
bit-width, which is what's happening in Windows GNU x86.

@rustbot label -stable-nominated

[rustbot]

Error: Label A-CI can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[tgross35]

Just updated the PR description and removed the stable nomination, as there's clearly no backwards-compatible changes to removing a type of a certain bit-width, which is what's happening in Windows GNU x86.

The current behavior is broken so we do need a fix on stable. Could you put up a PR gating the link_name attributes?

People also need a way to experiment on stable, so I think we need to make this backportable by putting the changes behind a cfg. We're moving away from environment variables, the new way of setting cfg via RUSTFLAGS should look something like this #4977 (you can ignore the verify-build updates).

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[dybucc]

According to the default behavior in both MSVC and GNU on Windows, whether we
even have the link_name attribute linking to the 64-bit routines does not seem
relevant. I just tested again without changing the link symbol in libc with
the same test I submitted to the PR, and there were no changes. This is
expected, considering the default is to define the non-suffixed variants in
terms of their 64-bit variants.

If anything, I'd say the fix should be to have the routine be linked to the
32-bit variant. But I don't think this is a good way of going about stuff,
considering that is not meant to be something Rust's libc does, but rather
something that is defined through the _USE_32BIT_TIME_T feature test macro,
and then conditionally linked to the 32-bit variant if such macro is defined.

The fix on stable may be to just expose a 32-bit time_t under Windows GNU (so
no changes from libc's current main in that respect,) and to have the
link_name instead redirect to the 32-bit variant. This may not be a great idea
because defining _USE_32BIT_TIME_T makes a few more things be conditionally
compiled. But it can be done if all the places in both MSVC and mingw that are
affected (and exposed through Rust's libc) get refactored accordingly. This
shouldn't require exposing a new cfg.

But I could be misunderstanding what you mean by "putting the changes behind a
cfg" and "gating the link_name."

[dybucc]

Alternatively, and judging by the documentation on some of the MSVC CRT docs
[1], it seems as if calling directly onto the suffixed variants is possible
without breaking something. This could make for a quick fix on stable as linking
to the 32-bit variants would be feasible only for 32-bit time_t, which itself
only happens under Windows when running in x86 and GNU. This would make it
easier as no other refactors for other non-32-bit types would be required (as
would otherwise be if we implemented all of the conditionally-compiled changes
that defining _USE_32BIT_TIME_T forces.)

Just opened #5059 with this fix.