Add localtime_r shim #3461

tiif · 2024-04-11T14:48:09Z

Implement localtime_r shim as mentioned in Shim wishlist #2057

Note:

tm_zone, tm_gmtoff might not be consistent with libc::localtime_r as custom implementation is provided through chrono. Due to the lack of daylight saving information in chrono, is_dst value will always be -1.

src/shims/time.rs

tiif · 2024-04-14T06:22:35Z

@RalfJung @oli-obk If memory leak in tm_zone string is a concern, would it be better if we just don't update the tm_zone field at all? The implementation provided is not accurate most of the time anyway since I didn't manage to use timezone abbreviation, I put something like +08 instead.

Cargo.toml

src/shims/time.rs

tiif · 2024-04-15T02:42:54Z

Thanks for the thorough review! I need a little bit more time to fully address them, and will mark this pr as ready after that.

src/shims/time.rs

tiif · 2024-04-16T07:09:10Z

Sorry for submitting so many comments one go, I didn't notice that these comments are under pending, and only visible to me :(

rustbot · 2024-04-16T07:14:15Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

Cargo.toml

RalfJung

Thanks! We're entering the final polishing stage of this PR. :)

@rustbot author

src/shims/time.rs

RalfJung · 2024-04-18T19:23:30Z

src/shims/time.rs

+        let tm_hour = dt.hour();
+        let tm_mday = dt.day();
+        let tm_mon = dt.month0();
+        let tm_year = dt.year().saturating_sub(1900);


Is it even possible for this to overflow? Why use saturating arithmetic?

Since i32 maximum value is 2147483647, should be impossible to overflow, at least not soon :). But I use this because the compiler will produce arithmetic side effect warning if I do

let tm_year = dt.year() - 1900;

So this is mainly to eliminate the warning.

The intended way to eliminate the warning is checked_sub().unwrap()

tests/pass-dep/shims/libc-misc.rs

rustbot · 2024-04-20T06:26:46Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

src/shims/time.rs

tiif · 2024-04-20T07:32:58Z

@rustbot ready

RalfJung · 2024-04-21T21:03:54Z

tests/pass-dep/shims/libc-misc.rs

+    let res_tm: libc::tm;
+    unsafe {
+        res_tm = *res;
+    }
    // The returned value should match the pointer passed in.


Instead of comparing everything a second time, can't you just check that res is equal to &mut tm (as in, ptr::eq)?

Yeah, this makes sense.

src/shims/time.rs

rustbot · 2024-04-22T03:58:35Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

d2a0ab2

RalfJung · 2024-04-22T05:39:56Z

All right, looks good. Thanks! Please rebase and squash.

RalfJung · 2024-04-22T06:40:00Z

@bors r+

bors · 2024-04-22T06:40:03Z

📌 Commit 90b408c has been approved by RalfJung

It is now in the queue for this repository.

bors · 2024-04-22T06:41:09Z

⌛ Testing commit 90b408c with merge 8870881...

tiif · 2024-04-22T06:41:16Z

Thanks @RalfJung and @oli-obk for mentoring this PR! I really learned a lot from the detailed suggestion and discovered a lot of new Rust syntax that I never used before. I will be away preparing for final exams until 1 May, and I will start working on socketpair by then.

bors · 2024-04-22T07:12:47Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 8870881 to master...

bors · 2024-04-22T07:12:47Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 8870881 to master...

oli-obk reviewed Apr 11, 2024

View reviewed changes