Bump bootgen to upstream Xilinx/bootgen xilinx_v2026.1, drop the jgmelber fork#3329
Queued
jgmelber wants to merge 15 commits into
Queued
Bump bootgen to upstream Xilinx/bootgen xilinx_v2026.1, drop the jgmelber fork#3329jgmelber wants to merge 15 commits into
jgmelber wants to merge 15 commits into
Conversation
jgmelber
requested review from
abisca,
fifield and
stephenneuendorffer
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the in-tree bootgen integration to match the upstream Xilinx/bootgen xilinx_v2026.1 source-tree restructure, including building the new vendored lms-hash-sigs dependency and updating the CMake build to discover sources per-architecture directory.
Changes:
- Reworks
tools/bootgen/CMakeLists.txtto glob sources from the new per-architecture*/srclayout and to use the relocated bison/flex outputs. - Adds a new static library target for the vendored
lms-hash-sigscode and wires it into the bootgen build. - Updates the existing configure-time include-stripping hacks (
malloc.h,applink.c) to the new upstream file locations, and pins PIC/PIE on the relevant targets.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Rebase the jgmelber/bootgen fork's two local commits (print-statement removal, C API for exception-safe PDI generation) onto upstream Xilinx/bootgen's xilinx_v2026.1 tag, 42 commits ahead of the fork's previous base. The old c-api-support branch tip is preserved at tag c-api-support-pre-2026.1-rebase in the fork for recovery. Upstream restructured its entire source tree between the fork's old base and 2026.1: 165 of 171 files moved from a single flat directory into per-architecture common/, spartanup/, versal/, versal_2ve_2vm/, zynq/, zynqmp/, and utils/ trees (each with src/ and include/ subdirs), and added a vendored lms-hash-sigs/ post-quantum hash-based signature library for newer secure-boot support. The C API commit (bootgen_c_api.cpp/.h) only adds new files and needed no source changes to rebase cleanly; it's now relocated into common/src and common/include to match the new layout. Rewrite tools/bootgen/CMakeLists.txt to match: - Source lists are now globbed per architecture directory (mirroring upstream's own Makefile) instead of a hand-maintained flat list. - Add all of upstream's new include directories. - Add a bootgen-lms-hash-sigs static library for the vendored LMS/HSS sources, linked into bootgen-lib, plus the new pthread dependency it introduces. - Update the malloc.h/applink.c configure-time strip hacks to the new file paths. Verified locally: the fork builds and runs standalone via its own Makefile (`bootgen -help` reports v2026.1). The updated CMakeLists.txt builds bootgen, bootgen-lib, and aiecc cleanly. Running an actual NPU2 (AIE2p) design (matrix_scalar_add) through aiecc's existing bootgen subprocess path produces a valid PDI. A standalone test calling bootgen_generate_pdi() directly against the same BIF produces a byte-identical PDI, confirming the rebased C API still works end-to-end. Note: AIECC_HAS_BOOTGEN_LIBRARY is never actually defined today because tools/CMakeLists.txt adds tools/aiecc before tools/bootgen, so `if(TARGET bootgen-lib)` in tools/aiecc/CMakeLists.txt is always false at configure time. This pre-existing ordering issue is unrelated to this bump and is left for a follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tools/aiecc/CMakeLists.txt gates the direct bootgen_generate_pdi() C API integration on `if(TARGET bootgen-lib)`, but tools/CMakeLists.txt added aiecc's subdirectory before bootgen's, so that target never existed yet at aiecc's configure time. AIECC_HAS_BOOTGEN_LIBRARY was therefore never defined and aiecc always fell back to shelling out to the bootgen executable, silently defeating the whole point of the C API added in the previous commit. Swap the two add_subdirectory calls so bootgen-lib exists first. Verified locally: aiecc now links against OpenSSL/bootgen-lib and AIECC_HAS_BOOTGEN_LIBRARY shows up in its compile flags. Running the matrix_scalar_add NPU2 example through aiecc now prints "Using bootgen library for PDI generation" / "Generated PDI via library" instead of falling back to the subprocess, and produces the same valid PDI as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Add CONFIGURE_DEPENDS to the per-architecture file(GLOB) calls, so CMake automatically reconfigures when upstream adds/removes source files instead of silently going stale until a manual reconfigure. - Make bootgen-lib's and bootgen-lms-hash-sigs' library dependencies PUBLIC rather than PRIVATE. Both are static libraries, so PRIVATE usage requirements don't propagate to consumers by CMake's own usage-requirement rules; PUBLIC is the correct way to express that bootgen/aiecc genuinely need OpenSSL::Crypto/Threads::Threads transitively. (CMake does already thread the raw .a files for static-lib chains onto the final link line regardless of this keyword, which is why this wasn't an observed link failure, but PUBLIC is the semantically correct choice and matters if either library ever becomes SHARED.) - Fix a real portability bug this exposed: the vendored lms-hash-sigs only ships a pthread-based implementation of its threading abstraction (hss_thread_pthread.c), which unconditionally #includes <pthread.h> and doesn't exist on MSVC. Add tools/bootgen/hss_thread_single.c, a small single-threaded implementation of the same documented API (hss_thread.h explicitly allows a 0/NULL thread collection as a valid non-threaded mode), and select it via CMAKE_USE_PTHREADS_INIT instead of assuming pthreads are always available. Verified locally: bootgen/bootgen-lib/aiecc still build and link (aiecc's link line now explicitly includes libbootgen-lms-hash-sigs.a and libcrypto/libssl), and re-running the matrix_scalar_add NPU2 example through aiecc still prints "Using bootgen library for PDI generation" and produces a valid PDI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
No functional change; tighten the comments added in this branch to be clear but concise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's clang-format-17 flagged the continuation-line indentation on hss_thread_issue_work's declaration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- ubuntu-24.04 gcc (all assert/rtti combos): GCC 14 makes -Wincompatible-pointer-types a hard error by default. Vendored bootgen code (utils/src/cdo-load.c) has a genuine char*/uint32_t* mismatch that only warned on older GCC. Add -Wno-incompatible- pointer-types to bootgen-lib's warning-ignore list, alongside the existing (Clang-only) -discards-qualifiers variant. - windows-2022 msvc: hss_thread_single.c (in tools/bootgen/) does #include "hss_thread.h", but bootgen-lms-hash-sigs never had third_party/bootgen/lms-hash-sigs on its include path -- it worked by accident for hss_thread_pthread.c only because that file lives in the same directory as the header. Add the missing include dir. Verified locally: reproduced the GCC 14 default-error behavior with gcc-13 -Werror=incompatible-pointer-types and confirmed -Wno-incompatible-pointer-types suppresses it; full bootgen/aiecc rebuild still succeeds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jgmelber
force-pushed
the
bump-bootgen-2026.1
branch
from
bc51660 to
acb0451
Compare
Contributor
Coverage ReportCreated: 2026-07-14 03:16Click here for information about interpreting this report.
Generated by llvm-cov -- llvm version 18.1.3 |
The fork existed only to carry two commits: a print-statement removal and the bootgen_c_api.cpp/.h C API. Neither actually needs to live in bootgen's own source: - bootgen_c_api.cpp/.h are pure new files that only consume bootgen's existing public headers (bootimage.h, options.h, bootgenexception.h) -- no upstream file is modified. Move them into tools/bootgen/ and compile/install them from there, the same way hss_thread_single.c already lives in mlir-aie rather than the submodule. - The print statement in cdo-driver/cdo_driver.c is stripped at configure time instead, via the same file(READ)/REPLACE/WRITE pattern already used for the malloc.h/applink.c hacks below it. (Confirmed real: cdo_driver_mlir_aie's startCDOFileStream is called directly by lib/Targets/AIETargetCDODirect.cpp once per emitted CDO file, not just from the bootgen executable, so this does affect aiecc's own output.) third_party/bootgen now points straight at Xilinx/bootgen's xilinx_v2026.1 tag commit (e576e5e), with no fork and nothing left to rebase going forward. The jgmelber/bootgen fork and its c-api-support/c-api-support-pre-2026.1-rebase refs are left in place but no longer used by anything. Verified locally: bootgen/bootgen-lib/aiecc still build; `bootgen -help` no longer prints "Generating: ..." per CDO file; the matrix_scalar_add NPU2 example still produces a valid PDI via aiecc's library path; a standalone call to bootgen_generate_pdi() against the same BIF (run in isolation, avoiding stale output files from earlier tests) is byte-identical to the CLI-generated PDI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jgmelber
changed the title
Now that these files live in mlir-aie rather than the bootgen submodule, use this repo's standard SPDX header (matching e.g. tools/aiecc/aiecc.cpp) instead of the full Apache License boilerplate carried over from upstream bootgen. Also fixes the clang-format findings from CI (include ordering, line wrapping). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AIECC_HAS_BOOTGEN_LIBRARY was never actually defined before this branch's tools/CMakeLists.txt fix (bootgen was added after aiecc, so `if(TARGET bootgen-lib)` was always false), so this test's PDI check had only ever run against the subprocess fallback in practice. Now that the library path is real, it prints "Generated PDI via library: ..." rather than "Generated PDI: ...", which doesn't literally contain the checked substring. Drop the trailing colon so the check matches both "Generated PDI:" (subprocess) and "Generated PDI via library:" (library) -- the test's own docstring already says it should cover both. Verified locally: FileCheck passes against real aiecc --verbose output through the library path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fifield
approved these changes
Jul 13, 2026
- bootgen_generate_pdi collapsed both bif_path and pdi_path NULL checks into BOOTGEN_ERROR_INVALID_BIF, leaving BOOTGEN_ERROR_INVALID_OUTPUT unused and giving callers no way to distinguish which argument was missing. Split into two checks returning the matching error code. - -Wno-incompatible-pointer-types(-discards-qualifiers) are C-only diagnostics; passing them while compiling bootgen-lib's .cpp sources is an unused command-line argument, which -Werror=unused (enabled at the top level) turns into a hard error. Gate both to C compilation only via $<$<COMPILE_LANGUAGE:C>:...>. Verified in an isolated CMake sandbox with -Werror=unused-command-line-argument: the flag reaches the .c compile line and is dropped from the .cpp one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e CMake property Same fix as #3333 (against main), carried forward here since this branch replaces bootgen's CMakeLists.txt with the 2026.1 upstream-source layout. The POSITION_INDEPENDENT_CODE property set on bootgen-lib/bootgen doesn't actually force PIE: this project never calls check_pie_supported(), which CMake's CMP0083 needs to turn that property into real -fPIE/-pie flags for an executable target. Confirmed via readelf + strace that the wheel built from #3327's own commit (main's earlier attempt at this) still ships a non-PIE `bootgen` and segfaults (SIGSEGV/SEGV_ACCERR jumping into its own non-executable DYNAMIC segment) once auditwheel repair patches in an RPATH for vendored OpenSSL -- manylinux_2_28's gcc-toolset doesn't default to PIE the way a distro's system gcc does. Pass -fPIC/-fPIE/-pie explicitly, last, so they win over any inherited -fno-pie/-fno-PIC via ordinary last-flag-wins compiler semantics. Verified locally under a forced -fno-pie -fno-PIC -no-pie global configuration that this reliably produces a true PIE (ET_DYN) bootgen, unlike the property-only approach. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Guard the new -fPIC/-fPIE blocks with NOT MSVC too: clang-cl reports CMAKE_CXX_COMPILER_ID "Clang" but uses the MSVC-style driver, which doesn't accept these GNU-style flags. - Drop -fPIE from target_link_options(bootgen ...); it's a compile-only flag, and passing it at link time can break toolchains where linking doesn't go through the compiler driver. Only -pie belongs there. Re-verified locally under the same forced -fno-pie -fno-PIC -no-pie global configuration as before: bootgen still links as a true PIE (ET_DYN) binary and runs correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Refactor comments for clarity and remove unnecessary lines.
andrej
approved these changes
Jul 13, 2026
Any commits made after this event will not be merged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
third_party/bootgennow tracks upstreamXilinx/bootgendirectly (pinned to thexilinx_v2026.1tag commit,e576e5e) instead of thejgmelber/bootgenfork. The fork existed only to carry two commits on top of upstream; both are reimplemented as mlir-aie-side build customizations instead, so there's nothing left to fork or rebase:bootgen_c_api.cpp/.h): these only ever added new files and never touched upstream source, so they now live intools/bootgen/and compile/install from there — the same treatment ashss_thread_single.cbelow.cdo-driver/cdo_driver.c's per-CDO-fileprintf("Generating: ...")(confirmed to matter:cdo_driver_mlir_aie'sstartCDOFileStreamis called directly bylib/Targets/AIETargetCDODirect.cpponce per emitted CDO file, polluting aiecc's own output) is now stripped at configure time via the samefile(READ)/REPLACE/WRITEpattern already used for the malloc.h/applink.c hacks.common/,spartanup/,versal/,versal_2ve_2vm/,zynq/,zynqmp/,utils/trees), and added a vendoredlms-hash-sigs/post-quantum signature library.tools/bootgen/CMakeLists.txtis rewritten to match: source lists are globbed per architecture directory (mirroring upstream's own Makefile), a newbootgen-lms-hash-sigsstatic library target is added, andtools/bootgen/hss_thread_single.cprovides a single-threaded fallback for lms-hash-sigs' threading abstraction on platforms without pthreads (MSVC).AIECC_HAS_BOOTGEN_LIBRARYnever being defined:tools/CMakeLists.txtaddedtools/aieccbeforetools/bootgen, soif(TARGET bootgen-lib)was always false at aiecc's configure time and thebootgen_generate_pdi()C API path was dead code. Swapped the order.main.jgmelber/bootgenfork (branchc-api-support, backup tagc-api-support-pre-2026.1-rebase) is left in place but no longer referenced by anything.Test plan
tools/bootgen/CMakeLists.txtrewrite buildsbootgen,bootgen-lib, andaiecccleanly against a real MLIR installmatrix_scalar_add) throughaieccend-to-end via the bootgen library path ("Using bootgen library for PDI generation" / "Generated PDI via library"), producing a valid PDIbootgen -help/ CDO generation no longer prints "Generating: ..." per filebootgen_generate_pdi()against the same BIF (in isolation) produces a byte-identical PDI to the CLI-generated one-Wincompatible-pointer-typeson vendoredcdo-load.c, and a missing include dir for MSVC's non-pthread fallback🤖 Generated with Claude Code