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

cmake: Fix some errors when using kasan #13782

Merged
merged 2 commits into from
Oct 2, 2024
Merged

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Oct 2, 2024

Summary

  1. cmake incorrectly identifies the --param xxx option and needs to be changed to --param=xxx
  2. Add arm64 support kasan

Impact

No

Testing

No

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Size: S The size of the change in this PR is small labels Oct 2, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 2, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Here's why and how to improve it:

  • Summary is Insufficient:
    • Missing "Why": Explain the reason for the cmake change. Is it causing build failures? Incompatibility?
    • Unclear Description: "Add arm64 support kasan" is vague. Be specific about what "kasan" is (Kernel Address Sanitizer?) and how the support is being added.
  • Impact is Incomplete:
    • Potentially Inaccurate: While you marked "Impact" as "No", adding arm64 kasan support likely has implications:
      • Build: New build flags/options might be needed.
      • Hardware: This directly affects arm64 architectures.
      • Documentation: Users will need guidance on enabling/using kasan.
  • Testing is Unacceptable:
    • "No" is Not Allowed: You must provide testing evidence.
    • Specifics Required: List the host OS, compiler, target architecture, board, and configurations used for testing.
    • Log Examples are Insufficient: Include actual relevant log snippets demonstrating the issue before the change and the correct behavior after.

How to Fix the PR Description:

  1. Expand the Summary:

    • Reason for cmake fix: E.g., "cmake currently misinterprets the --param xxx option, leading to build errors. This change corrects the syntax to --param=xxx for compatibility..."
    • Detailed kasan description: E.g., "This PR adds support for Kernel Address Sanitizer (kasan) on arm64 targets. It implements..." (be specific about the changes made).
  2. Thoroughly Address Impact:

    • Build: If new build flags are added, document them.
    • Hardware: Specify the arm64 architectures/boards tested.
    • Documentation: Indicate if you've updated documentation or if updates are needed.
  3. Provide Comprehensive Testing Information:

    • Host Details: E.g., "Build Host: Ubuntu 20.04, GCC 9.4.0"
    • Target Details: E.g., "Target: QEMU-based arm64 virt machine, config:..."
    • Relevant Log Snippets: Show the error before the change and the successful output after.

Remember: A good PR description saves reviewers time and increases the chances of your changes being accepted.

@jerpelea
Copy link
Contributor

jerpelea commented Oct 2, 2024

please update the PR title to something more suggestive
ex
cmake: Fix some errors when using kasan

Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

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

please add commit description

@W-M-R W-M-R changed the title Fix some errors when using kasan in cmake cmake: Fix some errors when using kasan Oct 2, 2024
add_compile_options(--param asan-globals=1) is recognized as
--param-lasan-globals=1, which causes compilation exception:

Signed-off-by: W-M-R <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit eb2f661 into apache:master Oct 2, 2024
29 checks passed
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 Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the 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.

5 participants