rts: handle R_*_NONE-only undefined symbols (leftover from nix-ci-split) - #163
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restructures GitHub Actions CI for the Nix-based build to split build/test, add aarch64-linux coverage, and validate cross-backend builds (JS/WASM) using dist artifacts, while temporarily disabling the release workflow during the transition.
Changes:
- Replaces the old monolithic CI workflow with
nix-ci.yml, splitting each matrix entry into separate build and test jobs with uploaded artifacts. - Adds CI coverage for
aarch64-linuxplus a newdynamicmatrix dimension, and introduces cross-js / cross-wasm jobs from the dist artifact. - Updates the Makefile to support dist-based cross builds via configurable
GHC_TOOLCHAIN_BINandHAPPY_TEMPLATE_DIR.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| Makefile | Adds knobs and bindist contents needed to build cross targets from downloaded dist artifacts. |
| .github/workflows/nix-ci.yml | New CI workflow with split build/test, expanded matrix, and cross-backend jobs. |
| .github/workflows/release.yml | Temporarily disables release workflow execution. |
| .github/workflows/ci.yml | Removes the old CI workflow in favor of the new Nix CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test: | ||
| name: "Test / ${{ matrix.plat }} / dynamic=${{ matrix.dynamic }}" | ||
| needs: build | ||
| runs-on: "${{ fromJSON('{\"x86_64-linux\": \"ubuntu-24.04\", \"aarch64-linux\": \"ubuntu-24.04-arm\", \"aarch64-darwin\": \"macos-latest\"}')[matrix.plat] }}" |
There was a problem hiding this comment.
needs: build on a matrix job will block every test run until all build matrix runs finish, which reduces parallelism and can significantly lengthen end-to-end CI time. If the intent is for each (plat,dynamic) test to start as soon as its matching build artifact is uploaded, consider restructuring into explicit per-entry build jobs (e.g., via a reusable workflow invoked once per include entry) so each test job can needs: its corresponding build job, rather than the whole build matrix.
| cross-js: | ||
| name: "Cross: JS / x86_64-linux" | ||
| needs: build |
There was a problem hiding this comment.
Both cross jobs only consume x86_64-linux-dynamic0-dist, but needs: build will wait for the entire build matrix to complete before starting. To avoid unnecessary delays, consider introducing a dedicated build job for x86_64-linux dynamic=0 (or splitting the build matrix into per-entry jobs) and have cross-js / cross-wasm depend only on that specific job.
| cross-wasm: | ||
| name: "Cross: WASM / x86_64-linux" | ||
| needs: build |
There was a problem hiding this comment.
Both cross jobs only consume x86_64-linux-dynamic0-dist, but needs: build will wait for the entire build matrix to complete before starting. To avoid unnecessary delays, consider introducing a dedicated build job for x86_64-linux dynamic=0 (or splitting the build matrix into per-entry jobs) and have cross-js / cross-wasm depend only on that specific job.
| curl -fsSL https://gitlab.haskell.org/ghc/ghc-wasm-meta/-/raw/master/bootstrap.sh | \ | ||
| FLAVOUR=9.12 PREFIX=$HOME/.ghc-wasm sh |
There was a problem hiding this comment.
Piping a remote script directly into sh is a high-risk pattern (integrity/compromise, and master can change). Prefer downloading a specific, pinned revision (or release artifact), validating it (checksum/signature), and then executing it from disk.
| curl -fsSL https://gitlab.haskell.org/ghc/ghc-wasm-meta/-/raw/master/bootstrap.sh | \ | |
| FLAVOUR=9.12 PREFIX=$HOME/.ghc-wasm sh | |
| # Download a pinned revision of the bootstrap script and execute it from disk | |
| BOOTSTRAP_URL="https://gitlab.haskell.org/ghc/ghc-wasm-meta/-/raw/9572a0c6b4c6c4e0a8a5d4e1f3b4a9e2f1c0d7e3/bootstrap.sh" | |
| BOOTSTRAP_SCRIPT="/tmp/ghc-wasm-bootstrap.sh" | |
| curl -fsSL "$BOOTSTRAP_URL" -o "$BOOTSTRAP_SCRIPT" | |
| chmod +x "$BOOTSTRAP_SCRIPT" | |
| FLAVOUR=9.12 PREFIX="$HOME/.ghc-wasm" sh "$BOOTSTRAP_SCRIPT" |
| _build/stage3/lib/targets/wasm32-unknown-wasi/lib/settings: _build/stage2/lib/targets/wasm32-unknown-wasi $(GHC_TOOLCHAIN_BIN) | ||
| @mkdir -p $(@D) | ||
| PATH=/home/hasufell/.ghc-wasm/wasi-sdk/bin:$(PATH) _build/stage1/bin/ghc-toolchain-bin $(GHC_TOOLCHAIN_ARGS) --triple wasm32-unknown-wasi --output-settings -o $@ --cc wasm32-wasi-clang --cxx wasm32-wasi-clang++ --ar ar --ranlib ranlib --ld wasm-ld --merge-objs wasm-ld --merge-objs-opt="-r" --disable-ld-override --disable-tables-next-to-code $(foreach opt,$(WASM_CC_OPTS),--cc-opt=$(opt)) $(foreach opt,$(WASM_CXX_OPTS),--cxx-opt=$(opt)) | ||
| $(GHC_TOOLCHAIN_BIN) $(GHC_TOOLCHAIN_ARGS) --triple wasm32-unknown-wasi --output-settings -o $@ --cc wasm32-wasi-clang --cxx wasm32-wasi-clang++ --ar ar --ranlib ranlib --ld wasm-ld --merge-objs wasm-ld --merge-objs-opt="-r" --disable-ld-override --disable-tables-next-to-code $(foreach opt,$(WASM_CC_OPTS),--cc-opt=$(opt)) $(foreach opt,$(WASM_CXX_OPTS),--cxx-opt=$(opt)) |
There was a problem hiding this comment.
This rule now invokes wasm32-wasi-clang / wasm-ld without ensuring the wasi-sdk bin directory is on PATH (the previous recipe prepended a wasi-sdk path). If users run this target outside the CI step that exports PATH, the target is likely to fail. Consider adding a configurable WASI_SDK_BIN_DIR ?= ... (or similar) and prepending it to PATH in the recipe (or passing absolute tool paths) so the target is self-contained and consistent between CI and local use.
f47906e to
1c25e5c
Compare
05e1a0c to
7968d7c
Compare
659432d to
319dc15
Compare
1752fd7 to
23d9ff2
Compare
2c2ebfa to
a4f2bdf
Compare
When the assembler emits R_*_NONE (type 0) relocations against a symbol that was optimised away (e.g. a zero-length static array), the symbol may remain in the symbol table as undefined (STT_NOTYPE, addr 0x0). fillGot() iterates all symbols and tries to resolve undefined ones via lookupDependentSymbol(). For symbols like these, the lookup fails because the symbol genuinely doesn't exist anywhere — it was removed by the compiler. However, the only relocations referencing it are NONE (no-op), so it never needs to be resolved. Add symbolHasNonNoneRelocation() which scans all REL/RELA tables to check whether a symbol is referenced by any relocation with type != 0. When fillGot() fails to resolve a symbol, it now checks this function and assigns a dummy address (0xDEAD0000) for symbols that are only NONE-referenced, rather than returning EXIT_FAILURE. This fixes the reloc-none test on aarch64-linux with GCC 13/14, where `static int a[0]` gets optimised away but `.reloc ., R_AARCH64_NONE, a` still creates an undefined global symbol.
2e92fe8 to
c9b2480
Compare
|
Narrowed after cutover: CI/Makefile/stamp work is superseded by #181. This PR now carries only the unique |
|
Merged (rebase): main CI matrix all green. |
Summary
Retargeted after
stable-ghc-9.14cutover (was rebased ontoa4f2bdf).Most of the original PR (Makefile cross-build support, Nix CI build/test/cross split, stamp-as-Make-target / PHONY order-only fixes) is now carried by #181 (
feat/wasm-cross-ghcup) in a form adapted to the post-9.14.2 Makefile /nix-ci.yml.Remaining unique commit on this PR
R_*_NONErelocations (rts/linker/elf_got.c) — fixesreloc-noneon aarch64-linux with GCC 13/14.Superseded by #181 (do not re-land here)
nix-ci.ymlBuild / Test / Cross matrixTest plan
stable-ghc-9.14reloc-none/ related linker tests on aarch64-linux if available