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

gmtime is still 32 bit (wrapping to 1900 after 2038) #19694

Closed
mmarczell-graphisoft opened this issue Jun 23, 2023 · 5 comments · Fixed by #19700
Closed

gmtime is still 32 bit (wrapping to 1900 after 2038) #19694

mmarczell-graphisoft opened this issue Jun 23, 2023 · 5 comments · Fixed by #19700

Comments

@mmarczell-graphisoft
Copy link
Contributor

Parts of the time.h / ctime API still don't support 64 bit time, as demonstrated by the following snippet:

#include <iostream>
#include <ctime>

int main () {
	struct tm tm;
	memset (&tm, 0, sizeof (tm));
	tm.tm_mon = 1;
	tm.tm_mday = 2;
	tm.tm_hour = 3;
	tm.tm_min = 4;
	tm.tm_sec = 5;
	tm.tm_isdst = -1;
	tm.tm_zone = "UTC";

	for (int year :  { 2035, 2040 }) {
		tm.tm_year = year - 1900;
		
		time_t timeVal = timegm (&tm);

		struct tm* ptm = gmtime (&timeVal);
		if (ptm) {
			std::cout << ptm->tm_year + 1900 << " should equal " << year << std::endl;
		}
	}	
}

It prints

2035 should equal 2035
1903 should equal 2040

Related:
#17393, #17401, #17471

I understand the reasons behind this with JS Number being less than 64 bit, but I still think it's a bug.

Version of emscripten/emsdk:

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.34 (57b21b8fdcbe3ebb523178b79465254668eab408)
clang version 17.0.0 (https://github.com/llvm/llvm-project a031f72187ce495b9faa4ccf99b1e901a3872f4b)
Target: wasm32-unknown-emscripten
Thread model: posix
InstalledDir: /Volumes/SSD/git/emsdk/upstream/bin

But I believe the bug is present in latest too.

@sbc100
Copy link
Collaborator

sbc100 commented Jun 23, 2023

JS Number can hold up to 53bits, so that should be plenty.

Any idea where the remaining bug lies.. it not in the definition of time_t..

@sbc100
Copy link
Collaborator

sbc100 commented Jun 23, 2023

I found the source of the problem:

// Declare these functions `int` rather than time_t to avoid int64 at the wasm
// boundary (avoids 64-bit complexity at the boundary when WASM_BIGINT is
// missing).
// TODO(sbc): Covert back to `time_t` before 2038 ...
int _timegm_js(struct tm* tm);
int _mktime_js(struct tm* tm);

I wonder if this is actually urgent to fix? i.e. does real world code care about this today? Fixing would likely involve a slight code size bump for all users.

@sbc100
Copy link
Collaborator

sbc100 commented Jun 23, 2023

The reason I ask is that in a few months when we have wasm-bigint enabled by default this should be trivial to fix.

sbc100 added a commit that referenced this issue Jun 23, 2023
Seems like there is no reason to pass a pointer here.

I noticed this while investigating #19694.
sbc100 added a commit that referenced this issue Jun 23, 2023
Seems like there is no reason to pass a pointer here.

I noticed this while investigating #19694.
sbc100 added a commit that referenced this issue Jun 23, 2023
Seems like there is no reason to pass a pointer here.

I noticed this while investigating #19694.
@kripken
Copy link
Member

kripken commented Jun 23, 2023

Good find. Sgtm to wait for WASM_BIGINT by default - that will surely happen before 2038! 🤣

@sbc100
Copy link
Collaborator

sbc100 commented Jun 23, 2023

Actually it looks like we can fix without regressing code size for most programs..

sbc100 added a commit that referenced this issue Jun 23, 2023
sbc100 added a commit that referenced this issue Jun 23, 2023
Seems like there is no reason to pass a pointer here.

I noticed this while investigating #19694.
sbc100 added a commit that referenced this issue Jun 23, 2023
sbc100 added a commit that referenced this issue Jun 23, 2023
Seems like there is no reason to pass a pointer here.

I noticed this while investigating #19694.
sbc100 added a commit that referenced this issue Jun 23, 2023
sbc100 added a commit that referenced this issue Jun 23, 2023
Seems like there is no reason to pass a pointer here.

I noticed this while investigating #19694.
sbc100 added a commit that referenced this issue Jun 23, 2023
sbc100 added a commit that referenced this issue Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants