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

Modify get_time_info() to return seconds and nanoseconds #445

Merged
merged 2 commits into from
May 22, 2024

Conversation

visitorckw
Copy link
Collaborator

Modify get_time_info() to return seconds and nanoseconds to get higher time resolution. Additionally, remove the outdated TODO comments as it was solved in newlib v4.4.0 [1].

Link: https://sourceware.org/git/?p=newlib-cygwin.git;a=commit;h=5f15d7c5817b07a6b18cbab17342c95cb7b42be4 [1]

Removed outdated "TODO: Clarify newlib's handling of time units"
comments as the time unit conversion error in newlib was fixed in
v4.4.0 [1].

Link: https://sourceware.org/git/?p=newlib-cygwin.git;a=commit;h=5f15d7c5817b07a6b18cbab17342c95cb7b42be4 [1]
The current get_time_info() helper function returns seconds and
microseconds, which reduces the resolution of rv_clock_gettime() (which
has nanosecond resolution). This change updates get_time_info() to
return seconds and nanoseconds to maintain higher resolution.
@jserv jserv merged commit f3c4d50 into sysprog21:master May 22, 2024
8 checks passed
@jserv
Copy link
Contributor

jserv commented May 22, 2024

Thank @visitorckw for contributing!

@visitorckw visitorckw deleted the update-time-info-resolution branch May 22, 2024 10:40
@vacantron
Copy link
Collaborator

vacantron commented May 29, 2024

@visitorckw, I found the results of some tests like dhrystone and coremark being weird after applying the changes of timespec. Should we also update the binary to reflect the change of newlib?

@visitorckw
Copy link
Collaborator Author

visitorckw commented May 29, 2024

@visitorckw, I found the results of some tests like drystone and coremark being weird after applying the changes of timespec. Should we also update the binary to reflect the change of newlib?

Absolutely. When I discovered the errors in newlib libgloss, @jserv asked me to correct the time-related system calls in rv32emu and also update all the binaries. However, I remember that when I applied my newlib patch and compiled the riscv gnu toolchain to generate the test binaries for rv32emu, I encountered an undefined reference to `nanosleep' error. Since I couldn't figure out the cause of this error at that time, I haven't made this update.

@visitorckw
Copy link
Collaborator Author

visitorckw commented May 29, 2024

@visitorckw, I found the results of some tests like drystone and coremark being weird after applying the changes of timespec. Should we also update the binary to reflect the change of newlib?

Additionally, I believe the weird results you observed are due to the change in time units introduced by PR #360 , not this PR. This PR merely optimized the precision without changing the time units.

@vacantron
Copy link
Collaborator

Additionally, I believe the weird results you observed are due to the change in time units introduced by PR #360 , not this PR. This PR merely optimized the precision without changing the time units.

Yes, I have found that I replied the wrong one. I sorted the PRs with created time and not realized it. 🥲

@visitorckw
Copy link
Collaborator Author

Yes, I have found that I replied the wrong one. I sorted the PRs with created time and not realized it. 🥲

That's fine. Replying to either one is fine.
I just tried using the updated riscv gnu toolchain to compile nyancat.c and still encountered the following error:

nyancat.c: In function 'main':
nyancat.c:940:9: warning: implicit declaration of function 'nanosleep' [-Wimplicit-function-declaration]
  940 |         nanosleep(&ts, &ts);
      |         ^~~~~~~~~
in function `main':
nyancat.c:(.text+0x34c): undefined reference to `nanosleep'
collect2: error: ld returned 1 exit status

Do you have any idea about this error?

@visitorckw
Copy link
Collaborator Author

That's fine. Replying to either one is fine. I just tried using the updated riscv gnu toolchain to compile nyancat.c and still encountered the following error:

nyancat.c: In function 'main':
nyancat.c:940:9: warning: implicit declaration of function 'nanosleep' [-Wimplicit-function-declaration]
  940 |         nanosleep(&ts, &ts);
      |         ^~~~~~~~~
in function `main':
nyancat.c:(.text+0x34c): undefined reference to `nanosleep'
collect2: error: ld returned 1 exit status

Do you have any idea about this error?

I encountered this error back in November or December of last year. This issue has indeed been pending for quite a while without resolution. If we still can't identify the cause of this error, perhaps I could write a script to perform a bisection on the riscv gnu toolchain to try and find the cause.

@vacantron
Copy link
Collaborator

vacantron commented Jun 5, 2024

I encountered this error back in November or December of last year. This issue has indeed been pending for quite a while without resolution. If we still can't identify the cause of this error, perhaps I could write a script to perform a bisection on the riscv gnu toolchain to try and find the cause.

I add -lsemihost to the compilation option and the problem is solved. It seems they turned some libgloss's static linked libraries into dynamic ones.

vestata pushed a commit to vestata/rv32emu that referenced this pull request Jan 24, 2025
…olution

Modify get_time_info() to return seconds and nanoseconds
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 this pull request may close these issues.

3 participants