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

libbuiltin: Fix the problem under linbuitin #14679

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

Conversation

W-M-R
Copy link
Contributor

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

Summary

libbuiltin: Fixed the problem that libbuitin cannot be cleaned up
1. $(call CLEAN) will delete all temporary ".o" files
2. bin file and kbin are empty folders and can be created during the compilation process
compiler-rt: Support builtin and profile compilation separately
1. Supports combining rt.builtin in the compiler toolchain with rt.profile builtin
2. Organize the makefile under compiler-rt:
1. Delete unnecessary content
mps/profile: Add mps configuration
1. Use the toolchain's own rt.builtin and libbuildin's rt.profile to instrument the code using parameters such as "-fprofile-generate --coverage"

Impact

no

Testing

Use the toolchain's own rt.builtin and libbuildin's rt.profile to instrument the code using parameters such as "-fprofile-generate --coverage"

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Area: OS Components OS Components issues Board: arm Size: M The size of the change in this PR is medium labels Nov 7, 2024
@W-M-R W-M-R force-pushed the compiler_rt branch 2 times, most recently from 0350b25 to 914675c Compare November 7, 2024 03:53
@nuttxpr
Copy link

nuttxpr commented Nov 7, 2024

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. Here's why and how to improve it:

Missing Information/Improvements Needed:

  • Summary: While it describes what is changed, it lacks a clear explanation of why these changes are necessary. What problem are they solving? What benefit do they bring? Be explicit. For instance, is this for performance analysis, debugging, code coverage reporting, or something else?
  • Impact: The impact section is far too brief. Address each bullet point individually. Even if the answer is "NO," explicitly state "NO." The current "CI supports compiler-rt.profile builder check" statement belongs in the Testing section and doesn't fully explain the impact. Consider these points:
    • User Impact: Will users need to change their configuration or build process to use this feature?
    • Build Impact: Does adding this feature change build times, dependencies, or require any new tools? You mentioned CI changes; elaborate on these.
    • Hardware Impact: Does this affect any specific architectures or boards? Does it increase code size, potentially impacting memory usage on resource-constrained devices?
    • Documentation Impact: Does this new feature require documentation updates? If so, have these updates been made as part of the PR?
    • Security Impact: Does the instrumentation introduce any potential security vulnerabilities?
    • Compatibility Impact: Does this change break compatibility with any existing code or tools?
  • Testing: The testing description is inadequate.
    • Build Hosts: Specify the OS, CPU architecture, and specific compiler versions used for testing. "Linux" isn't enough; say "Ubuntu 20.04" or similar. The same goes for the compiler; specify "GCC 10.2.0" or "Clang 12.0.1".
    • Targets: Specify the exact architectures and boards tested. "sim, RISC-V, ARM" is far too general. Provide specifics, like "QEMU-RV64," "HiFive Unleashed," "STM32F4 Discovery," etc. Include the specific NuttX configurations used.
    • Logs: The placeholder "your testing logs here" needs to be replaced with actual logs demonstrating the functionality before and after the change. Show how the profiling/coverage information is generated and how it's different after your changes. Focus on demonstrating the effect of the change, not just that the build succeeds. If it's about code coverage, show coverage reports before and after. If it's about profiling, show profiling output.

Example of an Improved Summary and Impact Section:

Summary:

This PR introduces support for profiling and code coverage analysis within the NuttX build system using the compiler's built-in profiling capabilities and the compiler-rt runtime library. This addresses the current lack of easy-to-use profiling and coverage tools, enabling developers to identify performance bottlenecks and improve test coverage. This change involves modifying the mps/profile configuration to utilize -fprofile-generate --coverage compiler flags and ensuring compatibility between the toolchain's rt.builtin and compiler-rt's rt.profile. This is related to [NuttX Issue #XXXX] (if applicable).

Impact:

  • New Feature Added: YES, adds profiling and code coverage support.
  • User Impact: YES. Users will need to enable the mps/profile configuration and potentially adjust linker scripts if using custom ones. This will be documented in the updated documentation.
  • Build Impact: YES. The build process will now include linking against the compiler-rt profiling library and will generate profiling data files. The CI system has been updated to build and verify this new configuration. The build time may be slightly increased due to the instrumentation.
  • Hardware Impact: YES. The generated code will be larger due to the instrumentation, potentially increasing memory usage on resource-constrained targets. The impact varies depending on the level of instrumentation and the specific application. Tested on [list specific boards and configurations] and found to have a memory overhead of approximately X%.
  • Documentation Impact: YES. Documentation updates have been included in this PR to describe how to enable and use the new profiling and coverage features.
  • Security Impact: NO. This change does not introduce any known security vulnerabilities.
  • Compatibility Impact: NO. This change is backwards compatible and should not affect existing applications unless they specifically enable the new mps/profile configuration.

By providing more specific information and addressing all the required points, you'll make it much easier for reviewers to understand and approve your PR. Remember to provide concrete evidence of your testing through logs and reports.

@lupyuen
Copy link
Member

lupyuen commented Nov 7, 2024

Sorry I have to cancel the CI Check, there's a problem with CI Test:

@W-M-R W-M-R force-pushed the compiler_rt branch 3 times, most recently from 218942e to 7417a0b Compare November 7, 2024 10:28
@W-M-R W-M-R changed the title compiler-rt: Support builtin and profile compilation separately libbuiltin: Fixed the problem that libbuitin cannot be cleaned up Nov 7, 2024
@github-actions github-actions bot added Size: S The size of the change in this PR is small and removed Area: Tooling Area: CI Arch: arm Issues related to ARM (32-bit) architecture Board: arm Size: M The size of the change in this PR is medium labels Nov 7, 2024
@W-M-R W-M-R force-pushed the compiler_rt branch 2 times, most recently from a40ff0b to 8d6280c Compare November 7, 2024 10:38
1. $(call CLEAN) will delete all temporary ".o" files
2. bin file and kbin are empty folders and can be created during the compilation process

Signed-off-by: wangmingrong1 <[email protected]>
1. Supports combining rt.builtin in the compiler toolchain with rt.profile builtin
2. Organize the makefile under compiler-rt:
	1. Delete unnecessary content

Signed-off-by: wangmingrong1 <[email protected]>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Board: arm labels Nov 7, 2024
@W-M-R W-M-R changed the title libbuiltin: Fixed the problem that libbuitin cannot be cleaned up libbuiltin: Fix the problem under linbuitin Nov 7, 2024
Use the toolchain's own rt.builtin and libbuildin's rt.profile to instrument the code using parameters such as "-fprofile-generate --coverage"

Signed-off-by: wangmingrong1 <[email protected]>
@github-actions github-actions bot added Area: Tooling Area: CI 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 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Area: CI Area: OS Components OS Components issues Area: Tooling Board: arm Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants