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

subsys: testsuite: coverage: support resetting counters #73034

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

srv-meta
Copy link

This continues the work started by Joshua in #65842

Ability to reset gcov counters allows better isolation of tests
coverage. Specifically it allows to:

  1. Not include counters for any code that was executed prior running the
    test, which includes all the code executed during the board boot.
  2. Run multiple tests without firmware reload by resetting counters
    between each.

I also undo renaming of gcov_populate_buffer to gcvo_to_gcda from bc8b7dd as it was actually a bug. The function was only renamed in .c, but not in the corresponding .h. I opted for returning it to original name to avoid breaking the public contract from the header.

Copy link

Hello @srv-meta, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@ycsin ycsin requested a review from jgl-meta May 21, 2024 02:07
@ycsin
Copy link
Collaborator

ycsin commented May 21, 2024

cc @nordic-piks

Copy link
Collaborator

@ycsin ycsin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the indentation issue, commit needs a Signed-off-by: .., otherwise LGTM

@jgl-meta
Copy link
Collaborator

A test would be nice to see.

uint32_t iter_counts;
uint32_t iter_counter_values;

for (iter_functions = 0U;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this traversal code is copy paste? Maybe extract it into it's own function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure it will make the code more readable. And I don't see any other place where it could be reused

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this same file does the traversal of the gcov nodes a couple of times at least.

Last time I talked to @cfriedt we had discussed a function for traversal that took a function pointer to operate of the nodes.

Copy link
Member

@cfriedt cfriedt May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be kind of useful, even in terms of public API. The major benefit I think would be that external code could iterate over coverage data while keeping it in a single, private, location. This would enable coverage backends. Reduces code duplication.

Can be reused by (most?) existing function calls?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 3 nested loops here and there are actions that are needed to be done on each level. Do you propose to define a traversal method that contains these 3 loops and accepts 3 callback functions? And each function will receive a context consisting of multiple variables?
I really don't see how to make it in a simple way that would improve things. Please suggest the exact api you are proposing

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is kind of an interesting proposal. I hadn't thought about it with three callbacks, each one being applied to each level.

What I was thinking is that mostly the "outer" two for loops are just finding out how to walk the counters. I was more thinking about a single function to operate on the specific counters themselves, which are really what the point of the code is in all of the places it is called. So the idea is, to provide something like a for each functionality for each counter.

@ycsin ycsin requested a review from cfriedt May 21, 2024 04:10
@nordic-piks
Copy link
Contributor

@srv-meta Thanks for fixing gcov_populate_buffer to gcvo_to_gcda.

I have one question, when (at which conditions) gcov_reset_counts will be called?
Will it be done automatically and unconditionally? or rather from external trigger (like debugger)?
What you wrote here:

Not include counters for any code that was executed prior running the
test, which includes all the code executed during the board boot.

has downside, especially for testing at HW, as during the board boot there are eg. drivers and peripherals initialized
and IMHO we want to have coverage from that part.

@srv-meta
Copy link
Author

@nordic-piks
This is just a toolkit that one can opt to use or not to use. I don't think any specific pattern should be enforced. One can opt to measuring coverage of the boot sequence as a separate test or not measure it at all. Someone else might not need to use resets at all as they don't have a requirement of measuring coverage of each individual test so precisely.

Ability to reset gcov counters allows better isolation of tests
coverage. Specifically it allows to:
1. Not include counters for any code that was executed prior running the
   test, which includes all the code executed during the board boot.
2. Run multiple tests without firmware reload by resetting counters
   between each.

Signed-off-by: Roman Studenikin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants