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

sim: Align LLVM target info across CMake and Makefile #15706

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Jan 27, 2025

Summary

  • Unified the specification of LLVM architecture and ABI types across the CMake and Makefile configurations.
  • Introduced LLVM_ARCHTYPE and LLVM_CPUTYPE variables in the CMake configuration to match the Makefile's approach.
  • Standardized the ABI type (LLVM_ABITYPE) to sysv for both Linux and macOS host configurations.

Impact

  • Ensures consistent LLVM target definitions across build systems (CMake and Makefile).
  • Simplifies future maintenance by avoiding divergence in LLVM-related configurations.
  • No functional changes to the build output; only the internal representation of LLVM target information is aligned.

Testing

GitHub CI and local build

Summary:
- Unified the specification of LLVM architecture and ABI types across the CMake and Makefile configurations.
- Introduced `LLVM_ARCHTYPE` and `LLVM_CPUTYPE` variables in the CMake configuration to match the Makefile's approach.
- Standardized the ABI type (`LLVM_ABITYPE`) to `sysv` for both Linux and macOS host configurations.

Impact:
- Ensures consistent LLVM target definitions across build systems (CMake and Makefile).
- Simplifies future maintenance by avoiding divergence in LLVM-related configurations.
- No functional changes to the build output; only the internal representation of LLVM target information is aligned.

Signed-off-by: Huang Qi <[email protected]>
@github-actions github-actions bot added Arch: simulator Issues related to the SIMulator Board: simulator Size: S The size of the change in this PR is small labels Jan 27, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 27, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although some sections could be more detailed.

Strengths:

  • The summary clearly explains the why, what, and how of the changes.
  • The impact section addresses most of the required points, highlighting the lack of user-facing changes.
  • Reference to GitHub CI and local builds demonstrates testing effort.

Areas for improvement:

  • Testing: While mentioning CI and local builds is good, providing specific details is crucial. List the specific build hosts (OS, CPU, compiler) and targets (arch, board, config) used for testing. Include snippets of relevant log output demonstrating the change's effect. The current "Testing logs before/after change" sections are empty.
  • Impact - Build: While the impact states "no functional changes," explicitly stating "NO" for "Impact on build" would be clearer. If there are any nuances to the build process change (even if internal), briefly mention them.
  • Impact - Other: Consider explicitly stating "NO" for hardware, documentation, security, and compatibility impact if there are truly none. This removes ambiguity. "Anything else to consider?" is also unanswered.
  • Issues/PRs: If this PR addresses a specific issue in either the NuttX or NuttX Apps repositories, provide the link.

By adding these specifics, the PR will be more comprehensive and easier for reviewers to assess.

Summary:
- Updated `Rust.defs` to use `gnu` as the target for Linux systems and `darwin` for macOS systems
- This change aligns with Rust toolchain conventions, where `gnu` is used to declare the system environment rather than the actual ABI name (e.g., `sysv`)

Impact:
- No functional changes - the Rust toolchain interprets `gnu` correctly for Linux targets
- Improves consistency with Rust toolchain conventions and reduces potential confusion
- Maintains compatibility with existing Rust builds on Linux and macOS systems

Signed-off-by: Huang Qi <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit abcbb1e into apache:master Jan 28, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: simulator Issues related to the SIMulator Area: Tooling Board: 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