Fix embedded images built with the clang compiler#730
Fix embedded images built with the clang compiler#730danielinux wants to merge 23 commits intowolfSSL:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses oversized/sparse embedded images produced when building with clang by adjusting linking and objcopy behavior for clang-based builds.
Changes:
- Add clang-specific
objcopysection filtering to avoid producing huge sparse binary/hex/srec outputs. - Switch clang builds to use the GNU linker driver to preserve LMAs in the linker script.
- Relax clang build warnings around unknown attributes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test-app/Makefile | Introduces clang-specific objcopy flags and applies them to image artifact generation to prevent sparse outputs. |
| arch.mk | Changes clang toolchain linking to use GNU linker driver and adds clang warning suppressions for unknown attributes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
danielinux
force-pushed
the
clang-linker
branch
from
9c7d22d to
aeee273
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mattia-moffa
force-pushed
the
clang-linker
branch
from
0f77eac to
a0a3dae
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 2 posted, 2 skipped
Posted findings
- [High] Missing
||operator between CI condition lines causes expression parse error —.github/workflows/test-build.yml:56-57 - [Medium] Consider using
CLANG_MULTILIB_FLAGSas a shared pattern to reduce duplication —options.mk:807-809,860-862
Skipped findings
- [Info] Whitespace-only change in stm32l4.ld adds blank line
- [Medium] Consider using
CLANG_MULTILIB_FLAGSas a shared pattern to reduce duplication
Review generated by Skoll via openclaw
.github/workflows/test-build.yml
Outdated
| inputs.config-file == './config/examples/stm32u5.config' || | ||
| inputs.config-file == './config/examples/stm32u5-wolfcrypt-tz.config' || | ||
| inputs.config-file == './config/examples/stm32u5-nonsecure-dualbank.config' || | ||
| inputs.config-file == './config/examples/stm32n6.config' |
There was a problem hiding this comment.
🟠 [High] Missing || operator between CI condition lines causes expression parse error
🚫 BLOCK bug
Lines 56-57 of the Rebuild wolfboot with Clang step's if: condition are missing the || (OR) operator between two comparisons. All other lines (51-55) correctly end with ||, but line 56 ends without one. In GitHub Actions, the | YAML block scalar concatenates these lines into a single expression string, resulting in two comparisons placed side-by-side without an operator: ... == './config/examples/stm32n6.config' inputs.config-file == .... This is a syntax error in the GitHub Actions expression parser. When the expression fails to parse, the if: condition evaluates to false, meaning the Clang rebuild step will never run for any config — silently defeating the purpose of this new CI step.
Suggestion:
| inputs.config-file == './config/examples/stm32n6.config' | |
| inputs.config-file == './config/examples/stm32n6.config' || | |
| inputs.config-file == './config/examples/stm32n6-tz.config' |
zd21378