Skip to content

use semihosting for collecting coverage data #94079

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

use semihosting for collecting coverage data #94079

wants to merge 6 commits into from
+207 −119

Conversation

nashif
Copy link
Member

@nashif nashif commented Aug 4, 2025

  • testsuite: coverage: Support semihosting
  • testsuite: split coverage kconfig into own file
  • tests: workqueue: increase stacks to allow coverage
  • tests: increase stack sizes to support coverage
  • samples: increase stack sizes to support coverage
  • kernel: set DYNAMIC_THREAD_STACK_SIZE to 4096 for coverage
  • tests: increase stacks to allow coverage
  • tests: kernel: fatal: fix test name for no-mt

@nashif nashif mentioned this pull request Aug 4, 2025
Merged
@nashif nashif force-pushed the topic/coverage/semihosting branch 2 times, most recently from 54e7404 to 91fec41 Compare August 4, 2025 17:44
JordanYates
JordanYates reviewed Aug 4, 2025
View reviewed changes
Copy link
Contributor

@JordanYates JordanYates left a comment

Choose a reason for hiding this comment

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

For the majority of use cases this seems like the better solution.
I believe this wouldn't work as is for the reboot case (#72561), since each reboot would just overwrite the previous file.
I'm not sure if there's an easy way to support that, since the console version is relying on the gcov tool to merge the files to the expected path.

@nashif
Copy link
Member Author

nashif commented Aug 4, 2025

I believe this wouldn't work as is for the reboot case (#72561), since each reboot would just overwrite the previous file.
I'm not sure if there's an easy way to support that, since the console version is relying on the gcov tool to merge the files to the expected path.

hmm, something to look at, I tried semihosting with both qemu and hardware and it seems to work, now sure how this works with gcov across multiple runs in general.

@nashif nashif marked this pull request as ready for review August 13, 2025 11:48
@nashif nashif force-pushed the topic/coverage/semihosting branch 2 times, most recently from 1db8789 to 9ae86bf Compare September 8, 2025 20:55
cfriedt
cfriedt reviewed Sep 12, 2025
View reviewed changes
scripts/pylib/twister/twisterlib/coverage.py Outdated
merge_tool = self.gcov_tool + '-tool'
for d1, d2 in zip(dirs[:-1], dirs[1:], strict=False):
cmd = [merge_tool, 'merge', d1, d2, '--output', d2]
print(f"Running command: {' '.join(cmd)}")
Copy link
Member

Choose a reason for hiding this comment

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

Should this stay in? Unsure if it's for debug purposes.

@nashif nashif force-pushed the topic/coverage/semihosting branch from 9ae86bf to c736671 Compare September 12, 2025 12:51
@zephyrbot zephyrbot added area: Networking Buffers net_buf/net_buf_simple API & implementation area: Architectures area: ARM ARM (32-bit) Architecture area: Debugging labels Sep 12, 2025
JordanYates
JordanYates reviewed Sep 13, 2025
View reviewed changes
kernel/Kconfig Outdated
default 4096 if X86
default 1024 if !X86 && !64BIT
default 2048 if !X86 && 64BIT
default 4096 if COVERAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

Defaults are picked up on a first match basis, I think this needs to be up the top, unless you want the stack size to be 1024 when !X86 && !64BIT && COVERAGE

@nashif nashif force-pushed the topic/coverage/semihosting branch from c736671 to bda0116 Compare September 13, 2025 06:58
@PerMac
Copy link
Member

PerMac commented Sep 17, 2025

@nordic-piks Maybe you could have a look at this? you have quite some experience with coverage

JordanYates
JordanYates requested changes Sep 17, 2025
View reviewed changes
Copy link
Contributor

@JordanYates JordanYates left a comment

Choose a reason for hiding this comment

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

In its current state this leaves CONFIG_COVERAGE_DUMP enabled when running twister tests. This would change if rebased on #96126.
It will also need its own conditional as added in #96042.

I used this PR as a base and added reboot support downstream in Embeint@3920b82
Not saying it needs to be part of this PR, but it is possible.

@nashif nashif force-pushed the topic/coverage/semihosting branch 2 times, most recently from af51d15 to 38fc026 Compare September 17, 2025 12:07
@nashif nashif requested a review from JordanYates September 17, 2025 12:08
@nashif nashif force-pushed the topic/coverage/semihosting branch from 38fc026 to d02fe08 Compare September 23, 2025 17:00
@nashif
Copy link
Member Author

nashif commented Sep 26, 2025

@JordanYates please revisit

JordanYates
JordanYates previously approved these changes Sep 29, 2025
View reviewed changes
Copy link
Contributor

@JordanYates JordanYates left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. There are two separate commits that are increasing test stack sizes that could be merged together, but won't block on that.

@nashif nashif force-pushed the topic/coverage/semihosting branch from d02fe08 to 2a0553c Compare October 9, 2025 08:54
mbolivar
mbolivar reviewed Oct 9, 2025
View reviewed changes
@nashif nashif force-pushed the topic/coverage/semihosting branch 2 times, most recently from b372f58 to 98f3b09 Compare October 10, 2025 11:36
mbolivar
mbolivar reviewed Oct 10, 2025
View reviewed changes
nashif added 6 commits October 10, 2025 13:23
@nashif
testsuite: coverage: Support semihosting
7bccafe 
Use semihosting to collect coverage data instead of dumping data to
serial console.

Signed-off-by: Anas Nashif <[email protected]>
@nashif
testsuite: split coverage kconfig into own file
9cc4072 
Coverage options deserve their own kconfig, otherwise the testsuite
Kconfig will be very crowded and difficult to read.

Signed-off-by: Anas Nashif <[email protected]>
@nashif
tests: workqueue: increase stacks to allow coverage
6797bcd 
Test was failing to build on some scenarios when coverage was enabled.

Signed-off-by: Anas Nashif <[email protected]>
@nashif
tests: increase stack sizes to support coverage
741f14b 
Increase stack sizes to allow coverage to complete.

Signed-off-by: Anas Nashif <[email protected]>
@nashif
kernel: set DYNAMIC_THREAD_STACK_SIZE to 4096 for coverage
98ff5e0 
Increase stack sizes to allow coverage to complete.

Signed-off-by: Anas Nashif <[email protected]>
@nashif
tests: increase stacks to allow coverage
9fa6eb8 
Increase stack sizes to allow coverage to complete.

Signed-off-by: Anas Nashif <[email protected]>
@nashif nashif force-pushed the topic/coverage/semihosting branch from 98f3b09 to 9fa6eb8 Compare October 10, 2025 19:36
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cfriedt cfriedt cfriedt left review comments

@mbolivar mbolivar mbolivar approved these changes

@JordanYates JordanYates JordanYates left review comments

@aaronemassey aaronemassey Awaiting requested review from aaronemassey

@andyross andyross Awaiting requested review from andyross

@asemjonovs asemjonovs Awaiting requested review from asemjonovs

@carlocaione carlocaione Awaiting requested review from carlocaione

@ceolin ceolin Awaiting requested review from ceolin

@dcpleung dcpleung Awaiting requested review from dcpleung

@jeremybettis jeremybettis Awaiting requested review from jeremybettis

@microbuilder microbuilder Awaiting requested review from microbuilder

@npitre npitre Awaiting requested review from npitre

@peter-mitsis peter-mitsis Awaiting requested review from peter-mitsis

@TaiJuWu TaiJuWu Awaiting requested review from TaiJuWu

@wearyzen wearyzen Awaiting requested review from wearyzen

@yperess yperess Awaiting requested review from yperess

+1 more reviewer

@nordic-piks nordic-piks nordic-piks approved these changes

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

@andyross andyross

@peter-mitsis peter-mitsis

Labels

area: Architectures area: ARM ARM (32-bit) Architecture area: Debugging area: Kernel area: Networking Buffers net_buf/net_buf_simple API & implementation area: Networking area: Sockets Networking sockets area: Testsuite Testsuite area: Userspace Userspace

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@nashif @PerMac @mbolivar @cfriedt @JordanYates @nordic-piks @andyross @zephyrbot @peter-mitsis