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

gcov: Add gcov gcc implementation and toolchain to ci compilation #14970

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Nov 28, 2024

Summary

When running qemu/nsh_smp or sim/nsh configuration, run gcov -d /tmp/xxx to save all gcov generated data and export them to the host. Use ./tools/gcov.sh to parse and generate reports. For details, see: Documentation/applications/system/gcov/index.rst

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

When running qemu/nsh_smp or sim/nsh configuration, run gcov -d /tmp/xxx to save all gcov generated data and export them to the host. Use ./tools/gcov.sh to parse and generate reports. For details, see: Documentation/applications/system/gcov/index.rst

@github-actions github-actions bot added Board: arm64 Board: simulator Size: S The size of the change in this PR is small labels Nov 28, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 28, 2024

[Experimental Bot, please feedback here]

This PR does not fully meet the NuttX requirements as presented. While it provides a summary of the change and some testing information, it is missing crucial details.

Here's a breakdown of what's missing and how to improve it:

Summary:

  • Missing Clarity on Necessity: The summary states why gcov data is being saved and exported, but not why this change is necessary. Was gcov data not accessible before? Was it difficult to access? Was the previous method unreliable? This needs clarification.
  • Functional Part: The summary mentions tools and configurations affected, but not the specific code files or modules changed. e.g., "Changes were made to the NSH builtin commands and the qemu/sim configurations."
  • Exact Mechanism: The summary describes the user-facing steps but not how the code achieves this. Does it involve new NSH commands? Modifications to existing code? New configuration options?

Impact:

  • The impact section is completely unaddressed. This is a major deficiency. The PR author must consider and document the impact on users (do they need new commands? different workflow?), build process (new dependencies?), hardware (any specific requirements?), documentation (updated to reflect the new feature?), security (does exporting this data pose any risks?), compatibility (does this break anything existing?), and any other considerations. Even if the answer is "NO" for each, it must be explicitly stated.

Testing:

  • Insufficient Test Logs: Simply stating how to run the tests is not enough. Actual output from the tests before and after the change is required. This allows reviewers to see the difference and verify the functionality. The current text describes how to test, not the results of testing.
  • Limited Testing Scope: While testing on qemu/nsh_smp and sim/nsh is mentioned, are there other configurations where this change might have an impact that should be tested?
  • Missing Build Host Information: The PR must specify the development environment used for testing, including the OS, CPU architecture, and compiler version.

Example of an improved summary:

"This change adds support for easily exporting gcov data from qemu/nsh_smp and sim/nsh configurations. Previously, accessing gcov data from these configurations required manual intervention and was prone to errors. This change introduces a new NSH command 'gcov_export' and modifies the qemu and sim startup scripts to automatically save gcov data to a specified location. This data can then be processed using the existing tools/gcov.sh script. This addresses issue #[Insert Issue Number Here]".

By addressing these missing points, the PR will significantly improve in quality and make it easier for reviewers to understand and approve the changes.

@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 28, 2024

Sorry, the configuration of qemu64 is turned on incorrectly. I will make the necessary changes later.

@W-M-R W-M-R closed this Nov 28, 2024
When running qemu/nsh_smp or sim/nsh configuration, run gcov -d /tmp/xxx
to save all gcov generated data and export them to the host. Use
./tools/gcov.sh to parse and generate reports. For details, see:
Documentation/applications/system/gcov/index.rst

Signed-off-by: wangmingrong1 <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 07493f1 into apache:master Nov 28, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: arm64 Board: simulator Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants