windows: expose cfg for 64-bit time_t

[dybucc]

Description

Under Windows GNU x86, time_t doesn't have the right size and alignment. This is a backwards-incompatible change, so for folks to experiment on stable, a cfg has been exposed to allow exposing a 64-bit time_t instead. Tests don't fail because we still skip time_t and records containing some value of such type whenever we're under this target platform/environment.

In #5059, we address the issue with routines involving time_t values by making them link to their 32-bit symbols on that platform/environment.

This should allow not breaking existing code (even though incorrect,) while still allowing new code to experiment with correct size/alignment for time_t values. The cfg tries to follow the new practices introduced in #4977.

An additional job was not added to the test matrix in CI because it was instead introduced as part of the tests in ci/run.sh that run only under 32-bit machine word and GNU-hosted targets.

Sources

None, if any the same as #5032 as this stems from it.

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

@rustbot label +stable-nominated

[dybucc]

@rustbot ready

[tgross35]

It shouldn't, right? That should only be applicable on win-msvc, and on MSVC targets time_t is already always 64 bits. I believe this should only affect i686-win-gnu, which uses the same _TIME_BITS=64.

View changes since the review

[dybucc]

That's not right. My comment is also wrong. Let me clarify.

In both MSVC and MinGW headers time_t is always 64-bit. In both of them one can use _USE_32BIT_TIME_T to get a 32-bits time_t. The _TIME_BITS feature test macro is sort of supported on MinGW. Though it's useless. It's only checked before checking that _USE_32BIT_TIME_T is defined [1].

What we expose is wrong. This PR is more of a bugfix on our side.

[tgross35]

What do you mean by useless? It appears to control what time_t is via _USE_32BIT_TIME_T https://github.com/mingw-w64/mingw-w64/blob/31bd54ab7d5fe03c67ed2bb1a57e531b9c7f8cc4/mingw-w64-headers/crt/time.h#L52-L59

[dybucc]

I'm referring to _TIME_BITS.

[tgross35]

There's no need to use then when there is no return value, if is more clear. But this can be simplified anyway:

if let Ok(tb) = env::var("CARGO_CFG_LIBC_UNSTABLE_GNU_TIME_BITS") {
    assert!(
        tb == "64" || tb == "32",
        "Invalid value for libc_unstable_gnu_time_bits. Must be 32, 64 or unset"
    );

    if gnu && i686 && tb == "64" {
        cfg.cfg("gnu_time_bits64", None);
    }
}

Do we need a cfg.define here? The other instances of CARGO_CFG_LIBC_UNSTABLE_GNU_TIME_BITS have one.

View changes since the review

[dybucc]

We don't. Windows with GNU (under MinGW) has the same behavior as MSVC. It exposes a 64-bit time_t by default. The feature test macro to use a 32-bit time_t is also the same as in MSVC.

Recall this cfg is meant as a bugfix to faulty bindings in the libc crate. time_t is currently 32-bits in main when in Windows x86 with GNU. This doesn't match MinGW headers. We just so happen to be "reusing" gnu_time_bits64.

_TIME_BITS is incompatible with _USE_32BIT_TIME_T in MinGW. The former is sort of supported. They check it's not defined when checking for the latter [1].

[dybucc]

Seems done. We don't check for time_t on Windows x86 GNU if gnu_time_bits64 is not set.

@rustbot ready

[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]

I had to add a cfg in the libc-test build script. The Windows test added on #5059 needs it with the changes in this PR.

I'm not sure if this is the way to go.

[tgross35]

Could replace the false chaining with:

let tb_env = env::var("CARGO_CFG_LIBC_UNSTABLE_GNU_TIME_BITS");
let redirect_time_t = match tb_env {
    Ok(tb) if tb == "64" => true,
    Ok(tb) if tb == "32" => false,
    Ok(x) => panic!("Invalid value`{x}` for ... "),
    Err(_) => false,
};

println!("rustc-check-cfg...");
if redirect_time_t {
    println!(...);
    cfg.cfg(...);
}

A name like win32_time64 would be more descriptive than redirect_time_t.

View changes since the review

[dybucc]

Done.

[tgross35]

Try to be more precise with wording like faulty/wrong/broken. In this case there should be nothing broken in Rust since the link name fix merged, it's just a mismatch to what users get on C. (Applies to the commit message and a few other PRs too).

Also 64-bit time_t wasn't even always an option until last year mingw-w64/mingw-w64@fe21b7e and uclibc a few years before that - libc's bindings just happen to predate that. (Even glibc and musl support didn't fully exist until ~2020.)

View changes since the review

[dybucc]

Noted.

[tgross35]

I think you may be able to delete the time_t exclusion entirely with:

if i686 && !redirect_time_t {
    cfg.define("_TIME_BITS", Some("32"));
}

View changes since the review

[dybucc]

Doesn't seem like it. It may be that the MinGW pre-defined macros target the x86_64 host even if the binary targets x86. That would make the conditional in ¹ evaluate false.

Footnotes

1. https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-headers/crt/_mingw.h.in#L393 ↩