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

Fix Broken time And times Built-ins #1327

Closed
wants to merge 1 commit into from

Conversation

etscrivner
Copy link

Currently, the time and times built-ins result in an assertion failure
causing a crash. This is due to an erroneous assertion from commit
595f499 which attempted to fix a linter
warning about the usage of an uninitialized timeval variable tb.

This change removes the assertion and instead runs memset on tb in the case
where it would have been used without having been initialized. This fixes the
cppcheck linter warning and restores the time and times built-ins to
working order.

A simple set of tests is added to ensure that these built-ins do not break in
the future.

Currently, the `time` and `times` built-ins result in an assertion failure
causing a crash. This is due to an erroneous assertion from commit
595f499 which attempted to fix a linter
warning about the usage of an uninitialized `timeval` variable `tb`.

This change removes the assertion and instead runs `memset` on `tb` in the case
where it would have been used without having been initialized. This fixes the
`cppcheck` linter warning and restores the `time` and `times` built-ins to
working order.

A simple set of tests is added to ensure that these built-ins do not break in
the future.
@krader1961
Copy link
Contributor

@etscrivner Thanks for taking a stab at fixing this but the fix isn't correct. The memset() is equivalent to tb.tv_sec = tb.tv_usec = 0;. All you've done to "fix" this is remove the assert. The assert is meant to ensure that tb has been initialized to a non-zero value so that the calculation of at in the statements following the assert produces a meaningful result. The question is whether tb should be initialized to zero (and the assert removed) or should it be initialized via timeofday(&tb).

Simply removing the assert and adding DPRINTF("at %lld", (long long)at); after the assignment to at shows that it has an absurdly large value when tb is initialized to zero. If instead of initializing it to zero we do timeofday(&tb); then the calculation of at produces a much more reasonable answer; i.e., zero. So that appears to be the correct fix.

Also, simply running time to ensure it doesn't blow up isn't a particularly useful test. Because you can't capture the output of time using simple redirection the only viable way to test it is to write an "expect" based unit test.

@krader1961
Copy link
Contributor

Also, we now always #define timeofday so all those #ifdef timeofday can be eliminated. If we ever find ourselves needing timeofday() to work on a platform without gettimeofday() we'll probably want to create a fallback implementation that does something reasonable using whatever mechanism is available for getting the current wall clock time with high precision.

@krader1961
Copy link
Contributor

Sigh! And, of course, the fact that tb wasn't initialized in one branch prior to commit 595f499 appears to be harmless because the value of at based on it isn't used in that case. Which is exhibit 666 for how badly this code is structured.

@etscrivner
Copy link
Author

@krader1961 Thanks for the review, if you don't mind I'd like to take another stab at fixing this since the functionality is currently completely broken. It sounds like the fix here is twofold:

  1. Remove #ifdef timeofday checks.
  2. Initialize tb with timeofday(&tb).

Definitely understand how having a test that checks functionality isn't completely broken seems minimally useful, but in this particular case the lack of such a test definitely allowed this functionality to remain broken for over a year. Happy to add more "correct" seeming tests. Do we currently do any expect based on testing that I could base this on?

@krader1961
Copy link
Contributor

@etscrivner Please review #1329

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.

2 participants