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

Clang/gcov: Supports gcc standard output interface "__gcov_dump" #14877

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

W-M-R
Copy link
Contributor

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

Summary

Clang/gcov: Supports gcc standard output interface "__gcov_dump"
mps/gcov: gcov app is enabled by default

Impact

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

Testing

Compile "boards/arm/mps/mps3-an547/configs/gcov", find the module that needs code coverage analysis, add the parameter "-fprofile-instr-generate -fcoverage-mapping", run qemu-system-arm -M mps3-an547 -nographic -kernel ./nuttx/nuttx, execute the app that calls __llvm_profile_dump or nsh run "gcov -d /tmp/xxx ", if you set the dump to memory, read the memory in your own way, such as gdb. If you use the default dump to file, please dump the file to the host, and finally use the following command to run:

"llvm-profdata merge -sparse default.profraw -o result.profdata"
"llvm-cov show -format=html ./nuttx/nuttx -instr-profile=result.profdata -output-dir=./coverage/"

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Nov 20, 2024
@W-M-R W-M-R force-pushed the gcov branch 2 times, most recently from 118c5c6 to abbea93 Compare November 20, 2024 11:19
@nuttxpr
Copy link

nuttxpr commented Nov 20, 2024

[Experimental Bot, please feedback here]

The PR description, as written, does not fully meet the NuttX requirements. Here's why and how to fix it:

Missing Information in Summary:

  • Why is this change necessary? The summary states what it does but not why. Is this for testing purposes? To improve debugging capabilities? To support systems without filesystems for code coverage? Explain the motivation.
  • What functional part of the code is changed? Be specific. Which files/modules are affected? Is this a change to the core OS, a specific driver, a build system modification?
  • How does the change exactly work? "Execute the app that calls __llvm_profile_dump" is vague. Explain the mechanism. Does it use a dedicated memory region? Does it hook into existing functionality? How is the data retrieved if not using a filesystem?

Incomplete Impact Assessment:

The impact section is essentially empty. Address all the points:

  • New/Changed Feature: Yes, new feature (data storage for code coverage without a filesystem). Describe the feature briefly.
  • Impact on User: Potentially YES. How will users enable/use this feature? Are there any configuration options? If the answer is NO, explicitly state it and justify why.
  • Impact on Build: Possibly YES if compiler flags need to be set. Describe how the build process is affected. If NO, explicitly state so.
  • Impact on Hardware: Probably NO. If so, state NO. If specific architectures are affected, explain how.
  • Impact on Documentation: YES. Documentation is required to explain how to use this new feature. State that documentation is included in the PR or will be provided in a follow-up.
  • Impact on Security: Potentially YES or NO. Does this change introduce any security risks (e.g., memory corruption)? Does it improve security by aiding in vulnerability discovery? Justify your answer.
  • Impact on Compatibility: Potentially YES or NO. Does this break existing code? Does it affect compatibility with different LLVM versions? Explain.
  • Anything else to consider: Are there any performance implications? Resource usage considerations?

Insufficient Testing Information:

  • Build Host(s): Provide details about the build host operating system, CPU architecture, and compiler version used for testing.
  • Target(s): Be specific about the target architecture, board, and configuration. "qemu-system-arm -M mps3-an547 -nographic" is a good start, but mention the NuttX configuration used (e.g., gcov).
  • Testing logs before change: Include actual logs or output demonstrating the behavior before the change. What happens when you try to get coverage data without a filesystem before this PR? Likely an error. Show that.
  • Testing logs after change: Show logs demonstrating the successful retrieval of coverage data using the new method. Include example output from the llvm-cov tools to show that it works.

Example of an Improved Summary:

This PR introduces support for collecting code coverage data without relying on a file system. This is essential for environments where a file system is unavailable or undesirable, such as deeply embedded systems or during early boot stages. The change modifies the arch/arm/src/xxx and libs/xxx modules to store coverage data in a dedicated memory region. The data is then retrieved via a new system call or by examining memory directly using a debugger. This feature is enabled by setting CONFIG_XXX and requires compiling with -fprofile-instr-generate -fcoverage-mapping. This addresses issue #[NuttX Issue Number].

By providing more complete and specific information in each section, the PR will be much easier to review and understand, increasing the likelihood of it being accepted. Remember, clarity and detail are key to a good PR.

@W-M-R W-M-R changed the title clang/gcov: Supports saving data by dumping it directly into memory clang/gcov: Supports saving data by dumping it directly into memory and compatibility with gcc standard gcov output Nov 20, 2024
libs/libbuiltin/Kconfig Outdated Show resolved Hide resolved
@W-M-R W-M-R force-pushed the gcov branch 3 times, most recently from ab234d4 to e73ed8b Compare November 21, 2024 01:53
libs/libbuiltin/compiler-rt/coverage.c Outdated Show resolved Hide resolved
libs/libbuiltin/compiler-rt/coverage.c Outdated Show resolved Hide resolved
libs/libbuiltin/compiler-rt/coverage.c Outdated Show resolved Hide resolved
libs/libbuiltin/compiler-rt/coverage.c Outdated Show resolved Hide resolved
@github-actions github-actions bot added Size: M The size of the change in this PR is medium and removed Size: S The size of the change in this PR is small labels Nov 21, 2024
@W-M-R W-M-R force-pushed the gcov branch 4 times, most recently from b111e79 to 99b51f3 Compare November 21, 2024 08:42
@github-actions github-actions bot added Size: S The size of the change in this PR is small and removed Board: arm Size: M The size of the change in this PR is medium labels Nov 21, 2024
@W-M-R W-M-R changed the title clang/gcov: Supports saving data by dumping it directly into memory and compatibility with gcc standard gcov output Clang/gcov: Supports gcc standard output interface "__gcov_dump" Nov 21, 2024
Signed-off-by: wangmingrong1 <wangmingrong1@xiaomi.com>
Signed-off-by: wangmingrong1 <wangmingrong1@xiaomi.com>
libs/libbuiltin/compiler-rt/coverage.c Outdated Show resolved Hide resolved
@@ -225,14 +219,9 @@ void __llvm_profile_register_names_function(void *names_start,
* llvm-prof. See the clang profiling documentation for details.
*/

void __llvm_profile_dump(const char *path)
void __llvm_profile_dump(lib_puts_t puts, FAR void *stream)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove puts

}

/* Data */
puts(stream, &hdr, sizeof(hdr));
Copy link
Contributor

Choose a reason for hiding this comment

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

call lib_stream_puts instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Board: arm 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