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

GreenHills Support: add __ARM_ARCH, __ARM_FEATURE_DSP macro definition when build with ghs compiler #14717

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

extinguish
Copy link

Summary

when we build mbedtls in vela with ghs compiler, the mbedtls need to access __ARM_ARCH and __ARM_FEATURE_DSP, and to construct the inline asm code based on these two macros.
With ghs compiler, these two macros are not defined, and will be evaluated as 0 by default, and thus will using to wrong inline asm code, in order to handle this issue, we need to add conversion between the ghs and gcc with __ARM_ARCH, __ARM_FEATURE_DSP

Impact

Has no impact with current gcc toolchain and other toolchain

Testing

  1. has passed the ostest
  2. has tested with ghs and gcc compiler

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small labels Nov 11, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 11, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, although some sections could be more thoroughly filled out. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change. The reasoning for needing the change (incorrect inline assembly due to undefined macros) is well-articulated.
  • Impact Acknowledged: The impact section addresses several points, focusing on the lack of impact on existing toolchains.
  • Testing Provided: The testing section mentions specific tests (ostest) and confirms testing with both GHS and GCC compilers.

Weaknesses & Areas for Improvement:

  • Missing References: The summary should include links to related NuttX issues or PRs if any exist. Even if there are none, stating "N/A" is good practice.
  • Incomplete Impact Assessment: While the impact on existing toolchains is mentioned, the other impact categories are left blank or with a very generic "NO." Even if there's no impact, it's better to explicitly state "NO - No changes to the build process" or similar for each item. This shows that the impact was considered. Pay particular attention to:
    • Documentation: Even small code changes sometimes require documentation updates. If none are needed, explicitly say so.
    • Compatibility: Mentioning backward compatibility is important. Does this change affect any existing code that uses mbedtls?
  • Insufficient Testing Detail: While the testing section mentions ostest, it lacks crucial details. Specify the specific tests within ostest that were run. Also, the "logs" are missing. Provide snippets of the relevant output before and after the change to demonstrate the fix. Include any error messages encountered before the fix and their absence afterward. "Passed" is not enough detail.
  • Missing Build Host/Target Information: The testing section should specify the build host OS, CPU architecture, compiler version, and the target architecture and board configuration used for testing. This ensures reproducibility.

Recommendation:

Revise the PR to address the weaknesses mentioned above. Providing more specific information, particularly in the Impact and Testing sections, will significantly strengthen the PR and make it easier for reviewers to assess.

arch/arm/include/armv6-m/irq.h Outdated Show resolved Hide resolved
when we build mbedtls in vela with ghs compiler, the mbedtls need to
access __ARM_ARCH and __ARM_FEATURE_DSP, and to construct the inline asm
code based on these two macros.
With ghs compiler, these two macros are not defined, and will be
evaluated as 0 by default, and thus will using to wrong inline asm code,
in order to handle this issue, we need to add conversion between the ghs
and gcc with __ARM_ARCH, __ARM_FEATURE_DSP

Signed-off-by: guoshichao <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 6036a31 into apache:master Nov 11, 2024
27 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 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