Skip to content

Conversation

@W-M-R
Copy link
Contributor

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

Summary

1. enable CONFIG_BUILTIN_COMPILER_RT  to built libclang_rt.builtins-xxx.a and no longer use the compiler's built-in
2. Modify clang version acquisition to get two decimal points
3. It has been ported to support four architectures: ARM, ARM64, RISCV, and x86_64, among which ARM has been validated
4. Align the ported compile-rt library with the clang version downloaded by ci to 17.0.1

Impact

Testing

 There is a problem with testbuild.sh, which causes the config I uploaded using the clang compiler to fail verification. I will fix this problem in the next patch and upload a defconfig that supports compiling this builtin.

@github-actions github-actions bot added Area: Tooling Area: Build system Arch: arm Issues related to ARM (32-bit) architecture Area: OS Components OS Components issues Size: M The size of the change in this PR is medium labels Oct 23, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 23, 2024

[Experimental Bot, please feedback here]

This PR summary is incomplete to assess if it meets NuttX requirements. Here's a breakdown of what's missing and why it's important:

Missing Information in Summary

  • Why is this change necessary? Is there a bug with the compiler's built-in implementation? Does this improve performance? A clear reason helps reviewers understand the PR's motivation.
  • What functional part of the code is being changed? Be specific. Which files are affected? Mentioning "CONFIG_TOOLCHAINS_BUILTIN" gives a hint, but more context is needed.
  • How does the change work? What are the technical steps involved? Simply stating "no longer use the compiler's built-in" is too vague.

Missing Information in Impact

  • ALL sections are blank! This is the most critical part. The impact section helps assess:
    • Risk: Are there potential side effects on users, builds, hardware, etc.?
    • Testing Scope: What needs to be tested thoroughly based on the areas affected?
    • Documentation: Will users need guidance on this change?

Missing Information in Testing

  • No build host details: What OS, CPU architecture, and compiler were used for testing?
  • No target details: Which simulator or physical hardware was this tested on?
  • No logs: Provide "before" and "after" logs demonstrating the change's effect.

Conclusion

This PR, in its current state, does not meet NuttX requirements. The missing information makes it impossible to evaluate the change effectively. Please provide the missing details to ensure your PR receives proper review.

@xiaoxiang781216
Copy link
Contributor

@W-M-R please add a config to ensure the porting really work.

@W-M-R
Copy link
Contributor Author

W-M-R commented Oct 23, 2024

@W-M-R please add a config to ensure the porting really work.

OK, I'll turn on builtin by default

@github-actions github-actions bot added Size: L The size of the change in this PR is large and removed Size: M The size of the change in this PR is medium labels Oct 24, 2024
@xiaoxiang781216
Copy link
Contributor

please rebase to fix the ci error

@W-M-R
Copy link
Contributor Author

W-M-R commented Oct 24, 2024

This CI result is a bit strange. The compiler clang used by Docker is 17.0.1, but it is inconsistent with the local compilation result of mine. I will to build a Docker to verify it latter.

1. enable CONFIG_BUILTIN_COMPILER_RT  to built libclang_rt.builtins-xxx.a and no longer use the compiler's built-in
2. Modify clang version acquisition to get two decimal points
3. It has been ported to support four architectures: ARM, ARM64, RISCV, and x86_64, among which ARM has been validated

Signed-off-by: wangmingrong1 <wangmingrong1@xiaomi.com>
@W-M-R
Copy link
Contributor Author

W-M-R commented Oct 28, 2024

There is a problem with testbuild.sh, which causes the config I uploaded using the clang compiler to fail verification. I will fix this problem in the next patch and upload a defconfig that supports compiling this builtin.

@W-M-R W-M-R changed the title clang: libclang_rt.builtins-xxx.a supports builtin clang:libclang_rt.builtins-xxx.a supports builtin Oct 28, 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: Build system Area: OS Components OS Components issues Area: Tooling Size: L The size of the change in this PR is large

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants