-
Notifications
You must be signed in to change notification settings - Fork 31
Timestamped logging macro for elapsed physical time #546
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
Open
adi-65-ray
wants to merge
14
commits into
lf-lang:main
Choose a base branch
from
adi-65-ray:logging-timestamped
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
4f51612
Timestamped logging macro
adi-65-ray 2e60dc9
Added unit test
adi-65-ray 7aa0ff7
unit test for logging clang-format fix
adi-65-ray 995e05e
logging_macros format fix
adi-65-ray 9564112
Update test/general/logging_test.c
hokeun e5ccae5
Merge branch 'main' into logging-timestamped
hokeun eb05623
Remove inclusion of tag.h
hokeun 578111d
Enable LOG_LEVEL_DEBUG for log UT
adi-65-ray 948a758
CI fix
adi-65-ray dba5062
trace low-level-platform dependency
adi-65-ray 9ca2499
Revert unnecessary change to CMakeLists
hokeun ba37edb
Merge branch 'main' into logging-timestamped
adi-65-ray 8782543
added unit-tests-debug for running test/debug_tests
adi-65-ray 280c7b4
Fix linking libraries. Link low-level-platforms directly to lf-loggin…
Jakio815 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| #include "logging_macros.h" | ||
| #include <stdio.h> | ||
| #include <stdlib.h> | ||
| #include <string.h> | ||
| #include <assert.h> | ||
| #include <unistd.h> | ||
| #include <regex.h> | ||
|
|
||
| #define TIMEED_DEBUG_CHAR_LEN 30 | ||
| #define SOME_EXTRA_SPACE 10 | ||
| /** | ||
| * @brief unit for LF_PRINT_DEBUG macro | ||
| * LOG_LEVEL must be in LOG_LEVEL_DEBUG | ||
| */ | ||
| void test_logging_macro(const char* expected, int st_len) { | ||
|
|
||
| FILE* tmp = tmpfile(); // auto-deletes when closed | ||
| char* buffer; | ||
| char pattern[256]; | ||
| regex_t re; | ||
| int result; | ||
|
|
||
| // Computing the buffer size based on strlen("DEBUG: ") + \0 + \n + extra space | ||
| int buffer_size = st_len + TIMEED_DEBUG_CHAR_LEN + SOME_EXTRA_SPACE; | ||
|
|
||
| if (!tmp) { | ||
| perror("tmpfile"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Redirect stdout -> tmp | ||
| fflush(stdout); | ||
| int fd = fileno(tmp); | ||
| if (fd == -1 || dup2(fd, STDOUT_FILENO) == -1) { | ||
| perror("redirect stdout"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Call code under test | ||
| LF_PRINT_DEBUG("%s", expected); | ||
|
|
||
| fflush(stdout); // flush so data goes into tmp | ||
| rewind(tmp); // reset read position | ||
|
|
||
| // Read back | ||
| buffer = (char*)malloc(buffer_size); | ||
| size_t n = fread(buffer, 1, buffer_size - 1, tmp); | ||
| buffer[n] = '\0'; | ||
|
|
||
| // Regex to check format: DEBUG: [number]expected\n | ||
| snprintf(pattern, sizeof(pattern), "^DEBUG: \\[-?[0-9]+\\]%s\\n?$", expected); | ||
|
|
||
| if (regcomp(&re, pattern, REG_EXTENDED | REG_NEWLINE) != 0) { | ||
| perror("regcomp"); | ||
| exit(1); | ||
| } | ||
|
|
||
| result = regexec(&re, buffer, 0, NULL, 0); | ||
| regfree(&re); | ||
|
|
||
| assert(result == 0); // match succeeded | ||
|
|
||
| fclose(tmp); // deletes the file | ||
| free(buffer); | ||
| } | ||
|
|
||
| int main() { | ||
| char* str_test = "Hello World"; | ||
| test_logging_macro(str_test, strlen(str_test)); | ||
| return 0; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to set LOG_LEVEL globally for all tests? What are the consequences of this? Can it be set within your one test that needs it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried redefining LOG_LEVEL locally in the test file but it gave waring for redeclaration and -Werror is set so it gets stuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this and I came up with two solutions:
I wanted to take the 2nd option but the CMakeLists.txt file of core has lot of conditional dependencies and I don't know should include all or is it fine if I link only a few of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adi-65-ray It will be great if you can use LOG_LEVEL=4 only for the unit tests involving debug-level logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't sufficient to just set
logging: DEBUGas a target parameter in the relevant tests?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Prof. Lee, @edwardalee
I think maybe you are confused.
This PR is adding a unit test, which is just run as
make unit-testsright inreactor-c/.There is no
.lffile, so there are no target properties (e.g., logging: DEBUG). So adding this flag does not affect most tests, such as the long tests likelf-default.The CI for this part, which is the
unit-tests-singlesuch as here, only takes 9 to 15 seconds to finish, so I don't think adding this will increase the CI test's time.If we still really want to separate this Aditya suggested two ways.
Makefile.However, I say this is not the right way to do, and complicate things more.
test/general/logging_test.cWe can override the log level using
#define LOG_LEVEL 4, but it shows a warning, and warnings are set as errors due to-Werrorflag.Do you still suggest fixing this problem? I say this is too trivial, and fixing this will complicate the
CMake.I’m open to your guidance if you think this should still be addressed or if there is a cleaner solution I may have missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just talked with @Hokuen and he suggested us to create separate binaries on the test such as I mentioned as method 1.
@adi-65-ray will work on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But why does this need to be a unit test rather than a regular LF test? It seems the same trick of redirecting stdout could work in a startup reaction and the check could be performed in a shutdown reaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that makes sense. Would you like to create a LF test @adi-65-ray?