unix: fix utime/futime timestamp rounding errors by bnoordhuis · Pull Request #2747 · libuv/libuv

bnoordhuis · 2020-03-21T12:11:11Z

uv_fs_utime() and uv_fs_futime() receive the timestamp as
a double and then convert it to struct timeval or struct timespec
where necessary but the calculation for the sub-second part exhibited
rounding errors for dates in the deep past or the far-flung future,
causing the timestamps to be off by sometimes over half a second.

Fixes: nodejs/node#32369 (partially)
~~CI: https://ci.nodejs.org/job/libuv-test-commit/1806/~~
~~CI: https://ci.nodejs.org/job/libuv-test-commit/1817/~~
CI: https://ci.nodejs.org/job/libuv-test-commit/1825/

cjihrig · 2020-03-24T13:46:37Z

+  t.tv_sec = x;
+  t.tv_nsec = 1e9 * uv__frac(x);
+
+  /* TODO(bnoordhuis) Remove this. utimesat() has nanosecond resolution but we


Out of curiosity, when can we remove this? I'm only asking because WASI wants nanosecond precision.

I'll merge this and open a PR that removes it.

bnoordhuis · 2020-03-28T09:37:11Z

FTR, this is how the test fails on Windows:

19:15:03 # Assertion failed in c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2017\test\test-fs.c on line 821:
`s->st_atim.tv_sec + (s->st_atim.tv_nsec / 1000000000.0) == atime`
(2119191946.955162 == -14245440.000000)

stale · 2020-05-06T00:27:37Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

vtjnash · 2020-07-28T19:01:59Z

Perhaps our time computation is overflowing in a similar way here https://github.com/libuv/libuv/blame/master/src/win/fs.c#L146? or just doesn't like the casts to unsigned? or the filesystem doesn't support negative times?

`uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second. Fixes: nodejs/node#32369 (partially) PR-URL: libuv#2747

`uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second on unix, or to be reinterpreted as unsigned and end up off by more than just sign but many also decades. Fixes: nodejs/node#32369 (partially) PR-URL: libuv#2747 Co-authored-by: Jameson Nash <vtjnash@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Without `volatile`, the x87 hardware may re-organize the comparison math and end up with the wrong answer. Fixes: one bullet point item of libuv#2655 and the tests from libuv#849 in certain build configurations PR-URL: libuv#2747

vtjnash · 2020-11-06T18:41:59Z

I ended up changing some of the details of the PR (in particular, fmod was the wrong function to emulate here—we needed floor), and also fixed the equivalent problems on Windows

CI: https://ci.nodejs.org/job/libuv-test-commit/2085/

`check_utime()` already calls `uv_fs_stat()`, no point in doing it twice. PR-URL: libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

`uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second on unix, or to be reinterpreted as unsigned and end up off by more than just sign but many also decades. Fixes: nodejs/node#32369 (partially) PR-URL: libuv#2747 Co-authored-by: Jameson Nash <vtjnash@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Without `volatile`, the x87 hardware may re-organize the comparison math and end up with the wrong answer. Fixes: one bullet point item of libuv#2655 and the tests from libuv#849 in certain build configurations PR-URL: libuv#2747

vtjnash · 2020-11-10T02:57:33Z

Tests are a bit too aggressive for some of our CI filesystems, but intended to show that it works correctly on at least some of the configurations. Will need to back off on those slightly then merge this. Anyone willing to review?

vtjnash · 2020-11-16T19:18:13Z

bump for (re)review

`check_utime()` already calls `uv_fs_stat()`, no point in doing it twice. PR-URL: libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

`uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second on unix, or to be reinterpreted as unsigned and end up off by more than just sign but many also decades. Fixes: nodejs/node#32369 (partially) PR-URL: libuv#2747 Co-authored-by: Jameson Nash <vtjnash@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Without `volatile`, the x87 hardware may re-organize the comparison math and end up with the wrong answer. Fixes: one bullet point item of libuv#2655 and the tests from libuv#849 in certain build configurations PR-URL: libuv#2747

`uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second on unix, or to be reinterpreted as unsigned and end up off by more than just sign but many also decades. Fixes: nodejs/node#32369 (partially) PR-URL: libuv#2747 Co-authored-by: Jameson Nash <vtjnash@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Without `volatile`, the x87 hardware may re-organize the comparison math and end up with the wrong answer. Fixes: one bullet point item of libuv#2655 and the tests from libuv#849 in certain build configurations PR-URL: libuv#2747

cjihrig · 2020-11-18T21:04:05Z

I already signed off on Ben's changes, but my LGTM still stands if you want to move this forward.

vtjnash · 2020-11-18T21:31:11Z

Thanks. I'd changed enough about it that I felt it needed a re-confirmation before merging.

I've got most tests functioning now (https://ci.nodejs.org/job/libuv-test-commit/2088/), but need to update a couple more.

`uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second on unix, or to be reinterpreted as unsigned and end up off by more than just sign but many also decades. Fixes: nodejs/node#32369 (partially) PR-URL: libuv#2747 Co-authored-by: Jameson Nash <vtjnash@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Without `volatile`, the x87 hardware may re-organize the comparison math and end up with the wrong answer. Fixes: one bullet point item of libuv#2655 and the tests from libuv#849 in certain build configurations PR-URL: libuv#2747

vtjnash · 2020-11-19T22:28:15Z

Final CI, for most recent fixup commit is not showing any related failures: https://ci.nodejs.org/job/libuv-test-commit/2090/. Should be ready for someone to land now!

`check_utime()` already calls `uv_fs_stat()`, no point in doing it twice. PR-URL: libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

`uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second on unix, or to be reinterpreted as unsigned and end up off by more than just sign but many also decades. Fixes: nodejs/node#32369 (partially) PR-URL: libuv#2747 Co-authored-by: Jameson Nash <vtjnash@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Without `volatile`, the x87 hardware may re-organize the comparison math and end up with the wrong answer. Fixes: one bullet point item of libuv#2655 and the tests from libuv#849 in certain build configurations PR-URL: libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

`check_utime()` already calls `uv_fs_stat()`, no point in doing it twice. PR-URL: libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

`uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second on unix, or to be reinterpreted as unsigned and end up off by more than just sign but many also decades. Fixes: nodejs/node#32369 (partially) PR-URL: libuv#2747 Co-authored-by: Jameson Nash <vtjnash@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Without `volatile`, the x87 hardware may re-organize the comparison math and end up with the wrong answer. Fixes: one bullet point item of libuv#2655 and the tests from libuv#849 in certain build configurations PR-URL: libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

`check_utime()` already calls `uv_fs_stat()`, no point in doing it twice. PR-URL: libuv/libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

`uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second on unix, or to be reinterpreted as unsigned and end up off by more than just sign but many also decades. Fixes: nodejs/node#32369 (partially) PR-URL: libuv/libuv#2747 Co-authored-by: Jameson Nash <vtjnash@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Without `volatile`, the x87 hardware may re-organize the comparison math and end up with the wrong answer. Fixes: one bullet point item of libuv/libuv#2655 and the tests from libuv/libuv#849 in certain build configurations PR-URL: libuv/libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

`check_utime()` already calls `uv_fs_stat()`, no point in doing it twice. PR-URL: libuv/libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

`uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second on unix, or to be reinterpreted as unsigned and end up off by more than just sign but many also decades. Fixes: nodejs/node#32369 (partially) PR-URL: libuv/libuv#2747 Co-authored-by: Jameson Nash <vtjnash@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Without `volatile`, the x87 hardware may re-organize the comparison math and end up with the wrong answer. Fixes: one bullet point item of libuv/libuv#2655 and the tests from libuv/libuv#849 in certain build configurations PR-URL: libuv/libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

bnoordhuis mentioned this pull request Mar 21, 2020

src: make fs functions handle pre-epoch timestamps nodejs/node#32408

Closed

bnoordhuis requested a review from cjihrig March 24, 2020 10:59

cjihrig approved these changes Mar 24, 2020

View reviewed changes

bnoordhuis force-pushed the node-fix32369 branch 2 times, most recently from 3745b86 to c57bf54 Compare March 27, 2020 23:13

stale Bot added the stale label May 6, 2020

vtjnash force-pushed the node-fix32369 branch from c57bf54 to 03d712b Compare November 6, 2020 18:39

stale Bot removed the stale label Nov 6, 2020

vtjnash pushed a commit to bnoordhuis/libuv that referenced this pull request Nov 10, 2020

test: remove unnecessary uv_fs_stat() calls

17b4156

`check_utime()` already calls `uv_fs_stat()`, no point in doing it twice. PR-URL: libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

vtjnash force-pushed the node-fix32369 branch from 03d712b to 3e61a15 Compare November 10, 2020 02:55

vtjnash pushed a commit to bnoordhuis/libuv that referenced this pull request Nov 18, 2020

test: remove unnecessary uv_fs_stat() calls

e846c3b

`check_utime()` already calls `uv_fs_stat()`, no point in doing it twice. PR-URL: libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

vtjnash force-pushed the node-fix32369 branch from 3e61a15 to d8feea7 Compare November 18, 2020 19:59

vtjnash force-pushed the node-fix32369 branch from d8feea7 to 703c7ee Compare November 18, 2020 21:00

vtjnash force-pushed the node-fix32369 branch 3 times, most recently from 5fed6f5 to bed5761 Compare November 19, 2020 22:16

vtjnash force-pushed the node-fix32369 branch from bed5761 to a3a1669 Compare November 19, 2020 22:17

bnoordhuis and others added 3 commits November 28, 2020 10:48

test: remove unnecessary uv_fs_stat() calls

e823d72

`check_utime()` already calls `uv_fs_stat()`, no point in doing it twice. PR-URL: libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

vtjnash force-pushed the node-fix32369 branch from a3a1669 to dde9815 Compare November 28, 2020 15:49

vtjnash merged commit dde9815 into libuv:v1.x Nov 28, 2020

richardlau mentioned this pull request Jun 9, 2021

Change in node v14.17.0 broke the mtimeMs comparison for source/target files in gulp-changed nodejs/node#38981

Open

JeffroMF pushed a commit to JeffroMF/libuv that referenced this pull request May 16, 2022

test: remove unnecessary uv_fs_stat() calls

af3f315

`check_utime()` already calls `uv_fs_stat()`, no point in doing it twice. PR-URL: libuv#2747 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unix: fix utime/futime timestamp rounding errors#2747

unix: fix utime/futime timestamp rounding errors#2747
vtjnash merged 3 commits into
libuv:v1.xfrom
bnoordhuis:node-fix32369

bnoordhuis commented Mar 21, 2020 •

edited

Loading

Uh oh!

cjihrig Mar 24, 2020

Uh oh!

bnoordhuis Mar 24, 2020

Uh oh!

bnoordhuis commented Mar 28, 2020

Uh oh!

stale Bot commented May 6, 2020

Uh oh!

vtjnash commented Jul 28, 2020

Uh oh!

vtjnash commented Nov 6, 2020

Uh oh!

vtjnash commented Nov 10, 2020

Uh oh!

vtjnash commented Nov 16, 2020

Uh oh!

cjihrig commented Nov 18, 2020

Uh oh!

vtjnash commented Nov 18, 2020

Uh oh!

vtjnash commented Nov 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bnoordhuis commented Mar 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Mar 28, 2020

Uh oh!

stale Bot commented May 6, 2020

Uh oh!

vtjnash commented Jul 28, 2020

Uh oh!

vtjnash commented Nov 6, 2020

Uh oh!

vtjnash commented Nov 10, 2020

Uh oh!

vtjnash commented Nov 16, 2020

Uh oh!

cjihrig commented Nov 18, 2020

Uh oh!

vtjnash commented Nov 18, 2020

Uh oh!

vtjnash commented Nov 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bnoordhuis commented Mar 21, 2020 •

edited

Loading