aarch64: add -mno-outline-atomics compilation flag #232
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @jmaksymowicz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses a system hang issue on Zynq Ultrascale+ after updating to GCC 14.2. The newer GCC version uses a function for atomic operations that relies on the LSE extension instructions, determined by a flag in memory (__aarch64_have_lse_atomics). Since glibc is not used, this flag remains uninitialized, causing non-deterministic hangs on real hardware. To resolve this, the pull request adds the -mno-outline-atomics flag to the CFLAGS in target/aarch64.mk. This ensures that GCC only uses LSE instructions when they are mandatory for the architecture specified by the -mcpu flag (Arm v8.1 and later). Disabling "outline atomics" is deemed a better approach as Phoenix-RTOS applications are compiled for specific platforms, making the target instruction set known at compile time.
Highlights
- Bug Fix: Addresses a system hang on Zynq Ultrascale+ after updating to GCC 14.2 due to uninitialized LSE atomics flag.
- Compilation Flag: Adds the
-mno-outline-atomicsflag toCFLAGSintarget/aarch64.mkto control LSE instruction usage. - Optimization: Disables "outline atomics" feature as Phoenix-RTOS applications are compiled for specific platforms, making the target instruction set known at compile time.
Changelog
- target/aarch64.mk
- Added
-mno-outline-atomicstoCFLAGSto prevent GCC from using LSE instructions based on an uninitialized flag. - Line 18 was modified to include the new flag.
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A flag to control,
Atom's dance, a kernel's soul,
Hangs no more in wait.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces a compilation flag -mno-outline-atomics to address a system hang issue on Zynq Ultrascale+ after updating to GCC 14.2. The change seems reasonable given the explanation in the pull request description. I have a few suggestions to improve the code clarity and maintainability.
Summary of Findings
- Compiler Flag Placement: The
-mno-outline-atomicsflag is added directly toCFLAGS. Consider whether this flag should also be added toCXXFLAGSseparately, or if it's intended to be applied to both C and C++ compilations viaCFLAGS.
Merge Readiness
The pull request addresses a critical bug and the provided solution seems reasonable. However, before merging, it's important to ensure that the implications of this change are fully understood and that it doesn't introduce any regressions. I recommend addressing the review comments before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
There was a problem hiding this comment.
- maybe add short comment to the code regarding WHY this option is needed
- maybe include the full issue description (from the PR) in the commit message body - that way it would be easier for the future developer to understand the change without too much digging.
Other than that - LGTM.
IMHO the PR description is sufficient to tell why we don't want to do it right now (or maybe ever) - that's why I insist on putting it in the commit message body. (tu summarize: it would be necessary to patch toolchain to support this feature (and introduce necessary changes in AUXVECTOR API), and right now we're not distributing "universal" target binaries so runtime check is not needed) |
GCC 14.2 may implement atomic operations using a function that may use LSE extension instructions. It decided whether to use these instructions using a flag in memory. However, because we do not use glibc, the flag was never initialized. The "outline atomics" feature would require additional changes in kernel, libphoenix and plo and possibly toolchain. Disabling the feature seems like a better choice because Phoenix-RTOS applications are compiled for a specific platform, so the target instruction set (and extensions) are known at compile time. With -mno-outline-atomics flag, GCC only uses LSE instructions if they are mandatory for the architecture given in -mcpu flag. LSE is mandatory starting from Arm v8.1. If Phoenix-RTOS is ever ported to an Arm v8.0 system with LSE and enabling support would be beneficial, we can do it per-target. JIRA: RTOS-1050
jmaksymowicz
force-pushed
the
jmaksymowicz/RTOS-1050
branch
from
0e17413 to
95f350d
Compare
|
I put the rationale for the changes in the commit message and added a comment in the Make script. |
This fixes system hang on Zynq Ultrascale+ after update to GCC 14.2.
Description
The newer GCC version implements atomic operations using a function that may use LSE extension instructions. It decided whether to use these instructions using a flag in memory (
__aarch64_have_lse_atomics). However, because we do not use glibc, the flag was never initialized. Additionally, because in kernel the.bsssegment is not initialized to 0, system would hang non-deterministically on real hardware (on QEMU uninitialized memory reads as 0, so it would never hang).With
-mno-outline-atomicsflag, GCC only uses LSE instructions if they are mandatory for the architecture given in-mcpuflag. LSE is mandatory starting from Arm v8.1.Because we do not use glibc, the "outline atomics" feature would require additional changes to work. The changes would have to be in kernel, libphoenix and plo. Disabling the feature seems like a better choice because Phoenix-RTOS applications are compiled for a specific platform, so the target instruction set (and extensions) are known at compile time. If Phoenix-RTOS is ever ported to an Arm v8.0 system with LSE and enabling support would be beneficial, we can do it per-target by adding
+lseto the-mcpuflag.Motivation and Context
Closes phoenix-rtos/phoenix-rtos-project#1297
DONE: RTOS-1050
Types of changes
How Has This Been Tested?
Checklist:
Special treatment