fix: replace gettimeofday with clock_gettime#3
Conversation
| #ifndef SYS_clock_gettime64 | ||
| #define SYS_clock_gettime64 403 | ||
| #endif |
There was a problem hiding this comment.
This should be put to nolibc/sys.h, and make sure it works on x86-64/aarch64/riscv32/riscv64 + Linux/macOS.
Maybe we need a CI to ensure this.
There was a problem hiding this comment.
I've already added CI to this repo, hope it will work correctly after you update this PR.
| int do_clock_gettime64(int clk_id, struct timespec64 *tp) { | ||
| return SYSCALL2(SYS_clock_gettime64, clk_id, tp); | ||
| } |
There was a problem hiding this comment.
This can be merged into clock_gettime function.
| #else | ||
| #include <sys/time.h> | ||
| #include <unistd.h> | ||
| int clock_gettime(int clk_id, struct timespec *tp); |
There was a problem hiding this comment.
Why is there another clock_gettime declaration here? Is it generated by AI?
| // CLOCK_REALTIME: 0 | ||
| // CLOCK_MONOTONIC: 1 |
There was a problem hiding this comment.
It's best to define them as macros.
| int tv_sec; | ||
| int tv_nsec; |
There was a problem hiding this comment.
Why use int here? They should be time_t and long: https://man7.org/linux/man-pages/man3/clock_gettime.3.html#DESCRIPTION
| struct timeval { | ||
| long tv_sec; | ||
| long tv_usec; | ||
| }; | ||
|
|
||
| int gettimeofday(struct timeval *tv, void *tz); |
There was a problem hiding this comment.
If we never use gettimeofday, we can remove it.
MaxXSoft
force-pushed
the
master
branch
3 times, most recently
from
ca8bb40 to
5b2bef4
Compare
hsqStephenZhang
force-pushed
the
master
branch
6 times, most recently
from
a072406 to
ff73079
Compare
| #ifdef SYS_LINUX | ||
| #define CLOCK_REALTIME 0 | ||
| #define CLOCK_MONOTONIC 1 | ||
| #ifdef SYS_X86_64 |
There was a problem hiding this comment.
Please move these to time.h, just as we did in io.h:
sysy-runtime-lib/src/nolibc/io.h
Lines 7 to 9 in c6b4c34
| #define SYS_WRITE 64 | ||
| #define SYS_EXIT 93 | ||
| #define SYS_GETTIMEOFDAY 169 | ||
| #define SYS_CLOCK_GETTIME64 403 |
There was a problem hiding this comment.
Please ensure that the definition of the SYS_* macros is consistent across all platforms.
I suggest you remove gettimeofday and use clock_gettime to implement starttime and stoptime (for all platforms), just like in your previous commit.
MaxXSoft
left a comment
MaxXSoft
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
I previously had some misunderstandings about clock_gettime on macOS, mistakenly believing it could also be implemented using a system call, but that's not actually the case. Therefore, some of the feedback I provided in previous reviews was unreasonable, and I apologize for making you revise it so many times.
I will fix this part more consistently later.
It's totally ok, I appreciated your comments, my original implementation was not robust enough, thanks for pointing that out. The CI is also useful. |
2 participants
compiler-dev's docker's perf output always shown "time elapsed: 0H-0M-0S-0us", which is caused by usage of
gettimeofdayThis PR aims at replacing
gettimeofdaywithclock_gettimeto make the perf work.