atf: once again no-warn-rwx-segment woes

[rpardini]

• turns out everybody was wrong, including me
• some (older?) ATF sources won't work, ever; thus
  • introduce ATF_SKIP_LDFLAGS=yes to skip it completely
  • introduce ATF_SKIP_LDFLAGS_WL=yes to only skip the -Wl, prefix
    • this is for ATF's that pass flag directly to linker, not gcc
• artifact-uboot: hash atf-building code into artifact version

Summary by CodeRabbit

• Build Changes
  • U-Boot artifact versioning updated with expanded hash calculation, resulting in modified artifact versions.
  • ATF compilation now uses dynamic feature detection for compiler flag compatibility instead of static version checks, improving robustness across different toolchains.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR expands the U-Boot artifact hashing to include the compile_atf function and replaces static binutils version checks with dynamic feature detection for the --no-warn-rwx-segment linker flag in ATF compilation, introducing helper functions to test compiler and linker flag support.

Changes

Cohort / File(s)	Summary
U-Boot Artifact Hashing lib/functions/artifacts/artifact-uboot.sh	Extends the hashing scope for U-Boot versioning by including compile_atf function in the hash calculation alongside existing functions; updates comment to reflect inclusion of ATF code in the hashing process.
ATF Compilation Feature Detection lib/functions/compilation/atf.sh	Replaces hardcoded binutils version gating with dynamic feature checks via new gcc_accepts_flag and ld_supports_flag helper functions; adds two-step verification for --no-warn-rwx-segment flag support, respects skip flags, and includes debug logging and cleanup.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• armbian/build#8757: Modifies ATF compilation flag handling for --no-warn-rwx-seg(s) with feature-detection and linker prefix forwarding.
• armbian/build#8583: Adjusts ATF compilation logic for --no-warn-rwx-segment flag selection and detection mechanisms.
• armbian/build#8097: Modifies ATF linker flag handling by adding -Wl, prefix to the --no-warn-rwx-segment flag.

Suggested reviewers

• clee
• prahal
• igorpecovnik
• EvilOlaf
• chainsx

Poem

🐰 The hashing grows wider, ATF joins the dance,
Feature-checks replace the version's old stance,
Flags flow dynamically through GCC and LD's prance,
Static walls crumble as detection takes its chance!
✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly refers to addressing recurring linker flag compatibility issues with ATF, which is the primary focus of the changes in atf.sh and the artifact versioning update.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[EvilOlaf]

Ah yes. Groundhog Day

[rpardini]

Draft, as this needs consensus. I tried my hand at it, but would like to get everyone who's tried before insights: 🙏

• rockchip64: fix atf build #8097 @EvilOlaf
• [bugfix] Update binutils version checks in atf.sh #8583 @chainsx
• build machinery - fixup compilation for uboot/atf BRANCH=edge, pass --no-warn-rwx-segments to linker, not compiler #8757 @tabrisnet
• Revitalize Espressobin - fix atf compilation bug, bump kernels #8873 @igorpecovnik

🙏 Please chime in. The gist of this:

• Sometimes the ld to be affected is called via cc - in those cases (mainline AT-F), we need a -Wl, prefix
• sometimes, gcc doesn't support that flag - can't rely on gcc version
• sometimes, ld itself doesn't support the flag - can't rely on ld version
• sometimes (vendor/older ATF) the ld is called directly
• sometimes, both, or neither, or some of the above

🙏 So please check your previous changes with this code, as the current state is broken.

• In most cases of non-mainline builds could be fixed by setting ATF_SKIP_LDFLAGS_WL=yes in board/family code
• Such fixes are needed before or during this PR merge, otherwise builds will break, because:
  • 💥 This also fixes "we not knowing the current state is broken" due to changes in compile_atf() not being hashed and thus not causing rebuilds of affected u-boot packages

[rpardini]

Let's not invite any AI here. This is strictly for humans.

[EvilOlaf]

Well as for my part I only know that adjusting the flag fixed the immediate issue but as I guess this left others broken which were not discovered(?). Also I admit not really diving into this (mostly because due to lack of understand what's going on) and focused on getting it to build (and perhaps even run).
I don't think I can be very helpful in sorting this once and for all.

[tabrisnet]

My concern would be that there seems to be two versions of the flag.
no-warn-rwx-segment & no-warn-rwx-segments, and when I went digging for #8757 I could find no information as to whether there was a meaningful difference. & yes, AI will probably be equally confused or confabulate.

[rpardini]

My concern would be that there seems to be two versions of the flag. no-warn-rwx-segment & no-warn-rwx-segments, and when I went digging for #8757 I could find no information as to whether there was a meaningful difference.

I'm not sure (needs confirmation across multiple versions) but I think the plural form is (also?) supported by cc, not ld, and thus only in the case where cc is involved.

[rpardini]

Not much feedback in 3 days, while stuff is broken. Undrafted.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @lib/functions/compilation/atf.sh:
- Around line 73-80: The helper functions gcc_accepts_flag and ld_supports_flag
are invoking "${ATF_COMPILER}gcc" without the toolchain on PATH; update both so
the compiler and the resolved linker are executed with PATH prefixed by
"${toolchain}" (i.e., use env PATH="${toolchain}:${PATH}" when calling
"${ATF_COMPILER}gcc" and when invoking the linker returned by
-print-prog-name=ld). Specifically, in gcc_accepts_flag wrap the compiler
invocation with env PATH="${toolchain}:${PATH}" "${ATF_COMPILER}gcc" so the test
uses the same toolchain compiler, and in ld_supports_flag first resolve the
linker using env PATH="${toolchain}:${PATH}" "${ATF_COMPILER}gcc"
-print-prog-name=ld into a variable and then call that linker with env
PATH="${toolchain}:${PATH}" ... --help | grep -q -- "$1".

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 7f9d663 and a9a49cc.

📒 Files selected for processing (2)

• lib/functions/artifacts/artifact-uboot.sh
• lib/functions/compilation/atf.sh

🧰 Additional context used

🧠 Learnings (10)

📓 Common learnings

Learnt from: glneo
Repo: armbian/build PR: 8913
File: config/sources/families/include/k3_common.inc:57-60
Timestamp: 2025-11-11T20:56:20.303Z
Learning: In config/sources/families/include/k3_common.inc, the OP-TEE build command at line 59 should be updated in a future PR to explicitly set CROSS_COMPILE64=aarch64-linux-gnu- and CROSS_COMPILE32=arm-linux-gnueabihf- instead of relying on OP-TEE's internal defaults, for better clarity and maintainability. User glneo agreed to address this in a separate PR.

Learnt from: rpardini
Repo: armbian/build PR: 9159
File: patch/u-boot/u-boot-genio/0026-dts-configs-add-Grinn-GenioSBC-510.patch:161-161
Timestamp: 2026-01-03T20:46:29.189Z
Learning: For the Armbian genio family (config/sources/families/genio.conf and patch/u-boot/u-boot-genio/), when reviewing PRs that include vendor U-Boot patches from Collabora, avoid flagging potential issues in board configurations that are out of scope for the PR's primary focus (e.g., don't flag Genio 510/700 board issues when the PR is focused on radxa-nio-12l/Genio 1200). The maintainer prioritizes keeping vendor patches close to upstream for easier re-copying and maintenance, even if secondary board configs have potential mismatches.
<!-- </add_learning>

Learnt from: igorpecovnik
Repo: armbian/build PR: 9087
File: .github/workflows/pr-check-pictures.yml:138-146
Timestamp: 2025-12-16T13:40:07.649Z
Learning: In the Armbian build repository, when introducing new requirements or checks (like the board assets verification workflow), the project prefers an initial educational-only period where violations post helpful PR comments and warnings but don't block merges. This allows contributors to become familiar with new requirements before enforcement is enabled (typically after ~6 months).

Learnt from: tabrisnet
Repo: armbian/build PR: 8661
File: lib/functions/compilation/armbian-kernel.sh:194-199
Timestamp: 2025-09-25T18:37:00.330Z
Learning: In PR armbian/build#8661, line 235 of lib/functions/compilation/armbian-kernel.sh already contains the corrected comment "BPF link support for netfilter hooks" for NETFILTER_BPF_LINK, not the misleading "BPF_SYSCALL" comment that was flagged during review.

Learnt from: tabrisnet
Repo: armbian/build PR: 8661
File: lib/functions/compilation/armbian-kernel.sh:194-199
Timestamp: 2025-09-25T18:37:00.330Z
Learning: In PR armbian/build#8661, line 235 of lib/functions/compilation/armbian-kernel.sh already contains the corrected comment "BPF link support for netfilter hooks" for NETFILTER_BPF_LINK, not the misleading "BPF_SYSCALL" comment that was flagged during review.

Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: In lib/functions/rootfs/rootfs-create.sh, the FIXME comment about mmdebstrap usage with --aptopt is a future note related to PR #8785, which hasn't been merged yet.

Learnt from: EvilOlaf
Repo: armbian/build PR: 8968
File: patch/u-boot/u-boot-sunxi/arm64-dts-sun50i-h6-orangepi.dtsi-Rollback-r_rsb-to-r_i2c.patch:36-36
Timestamp: 2025-11-20T18:20:11.985Z
Learning: The rewrite-patches tool (REWRITE_PATCHES=yes) in the Armbian build system can inadvertently introduce semantic changes when the u-boot/kernel git base revision differs from expected state. The tool applies patches, commits them, and re-exports them using git format-patch, which can cause the re-exported patch to reflect the base revision's state rather than preserving the original patch intent. This is particularly problematic for device tree changes like interrupt specifications. The tool currently lacks validation mechanisms to detect such semantic drift, and affected patches must be manually corrected after rewriting.

Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.

📚 Learning: 2025-08-30T06:48:09.091Z

Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-30T06:48:09.091Z
Learning: In lib/functions/compilation/armbian-kernel.sh, the user prefers flexible grep patterns over anchored ones for BTRFS configuration checks, but agrees to use quiet grep (-q) to avoid polluting build logs.

Applied to files:

• lib/functions/compilation/atf.sh

📚 Learning: 2025-12-18T23:40:41.627Z

Learnt from: rpardini
Repo: armbian/build PR: 9101
File: lib/functions/image/compress-checksum.sh:44-48
Timestamp: 2025-12-18T23:40:41.627Z
Learning: In the Armbian build framework, scripts run with set -e, so a non-zero exit status will abort the script. During reviews, assume failures will stop execution unless explicitly handled. Verify that commands that must fail are checked, errors are propagated, and any critical steps have proper error handling (e.g., using pipefail where appropriate, checking exit codes, and not masking failures).

Applied to files:

• lib/functions/compilation/atf.sh
• lib/functions/artifacts/artifact-uboot.sh

📚 Learning: 2025-12-19T13:56:45.124Z

Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.

Applied to files:

• lib/functions/artifacts/artifact-uboot.sh

📚 Learning: 2025-03-31T22:20:41.849Z

Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75
Timestamp: 2025-03-31T22:20:41.849Z
Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.

Applied to files:

• lib/functions/artifacts/artifact-uboot.sh

📚 Learning: 2025-11-20T18:20:11.985Z

Learnt from: EvilOlaf
Repo: armbian/build PR: 8968
File: patch/u-boot/u-boot-sunxi/arm64-dts-sun50i-h6-orangepi.dtsi-Rollback-r_rsb-to-r_i2c.patch:36-36
Timestamp: 2025-11-20T18:20:11.985Z
Learning: The rewrite-patches tool (REWRITE_PATCHES=yes) in the Armbian build system can inadvertently introduce semantic changes when the u-boot/kernel git base revision differs from expected state. The tool applies patches, commits them, and re-exports them using git format-patch, which can cause the re-exported patch to reflect the base revision's state rather than preserving the original patch intent. This is particularly problematic for device tree changes like interrupt specifications. The tool currently lacks validation mechanisms to detect such semantic drift, and affected patches must be manually corrected after rewriting.

Applied to files:

• lib/functions/artifacts/artifact-uboot.sh

📚 Learning: 2025-03-31T22:20:48.475Z

Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.

Applied to files:

• lib/functions/artifacts/artifact-uboot.sh

📚 Learning: 2025-06-12T21:14:36.024Z

Learnt from: djurny
Repo: armbian/build PR: 8287
File: config/bootscripts/boot-sunxi.cmd:38-44
Timestamp: 2025-06-12T21:14:36.024Z
Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.

Applied to files:

• lib/functions/artifacts/artifact-uboot.sh

📚 Learning: 2025-05-21T06:04:48.711Z

Learnt from: EvilOlaf
Repo: armbian/build PR: 8211
File: extensions/linux-source-package.sh:7-10
Timestamp: 2025-05-21T06:04:48.711Z
Learning: In the Armbian build system's extension framework, function names follow the pattern `hook_point__unique_identifier()`. The part before `__` defines when in the build process the function is executed (the hook point), while the part after `__` is just a unique identifier and doesn't need to match the extension name. Functions with the same hook point are executed in alphanumerical order.

Applied to files:

• lib/functions/artifacts/artifact-uboot.sh

📚 Learning: 2025-04-28T08:27:26.890Z

Learnt from: leggewie
Repo: armbian/build PR: 8133
File: extensions/apa.sh:1-2
Timestamp: 2025-04-28T08:27:26.890Z
Learning: In the Armbian build system, extension scripts in the `extensions/` directory contain hook functions and are meant to be sourced, not executed directly. These scripts don't require a shebang or `set -euo pipefail`.

Applied to files:

• lib/functions/artifacts/artifact-uboot.sh

🧬 Code graph analysis (2)

lib/functions/compilation/atf.sh (1)

lib/functions/logging/display-alert.sh (1)

• display_alert (10-227)

lib/functions/artifacts/artifact-uboot.sh (1)

lib/functions/general/hash-files.sh (1)

• calculate_hash_for_function_bodies (85-101)

🔇 Additional comments (3)

lib/functions/compilation/atf.sh (2)

81-99: Feature detection logic looks correct.

The two-step verification (GCC first, then LD) with fallback paths and clear logging at each decision point is a solid approach. The handling of ATF_SKIP_LDFLAGS and ATF_SKIP_LDFLAGS_WL environment variables provides the necessary flexibility for different ATF/toolchain combinations.

---

100-100: Good cleanup of temporary helper functions.

lib/functions/artifacts/artifact-uboot.sh (1)

83-86: Good addition of compile_atf to artifact hashing.

Including compile_atf in the function hash ensures that changes to the ATF compilation logic (including the new feature detection code) will trigger artifact rebuilds. Per the calculate_hash_for_function_bodies implementation, non-existent functions are safely skipped, so this works correctly for builds that don't use ATF.

[EvilOlaf]

I guess a build train on all uboots will tell if something broke

[rpardini]

I guess a build train on all uboots will tell if something broke

I heard this before. This time, it really will. It's not about "something broke", it's about "it was already broken and not enough fixed by this".

[rpardini]

@igorpecovnik: espressobin / machiatto..: Sent a fix #9182 that goes together with this one. Both need to be merged together.