Repository navigation
fix(lto): reject malformed interface metadata without aborting - #2562
Merged
cpunion merged 1 commit intoSep 14, 2026
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: fix LTO invalid-metadata exit behavior
The change is clean, well-scoped, and correct. Swapping report_fatal_error for reportFatalUsageError in the malformed-input rejection path is the right hardening: it takes the exit(1) path (GenCrashDiag=false) instead of abort(), so invalid IR metadata is rejected as a usage error without invoking crash reporters/symbolizers. The API is valid on the required LLVM 22 toolchain, and the inline comment accurately describes the new behavior.
The test rewrite is a solid improvement — exec.CommandContext with a 30s timeout + cmd.WaitDelay guards against a hang, and the assertions now verify three distinct things (process terminated, exit code exactly 1, expected diagnostic present) rather than merely "not accepted".
Verified:
- Imports (
context,time) are added;[[noreturn]]oninvalidMetadatais preserved, so callers relying on non-return control flow stay sound. - The two remaining
report_fatal_errorcalls inLLGOReflectMethodByNamePass.cppwere correctly left unchanged — they fire on internal invariant violations (eraseGenericCheckedLoadfailing), not external input, where a crash report is appropriate. - No security or documentation concerns; there is no doc referencing the prior crash behavior.
Only one minor, optional note inline. No blocking issues.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
cpunion
force-pushed
the
codex/fix-lto-invalid-metadata-exit-20260911
branch
from
September 14, 2026 05:32
a87bb23 to
7312875
Compare
Collaborator
Author
|
@fennoai Review again after rebasing onto the latest main. |
Contributor
|
FennoAI is processing your request, please wait a moment... |
Contributor
There was a problem hiding this comment.
Review — post-rebase
Re-reviewed against the rebased range 5c58743...7312875. The diff is now just the two intended files; the descendant-probe prerequisite commits from #2563 are no longer in the range, confirming the clean rebase onto latest main.
Conclusion: no blocking issues. The change is correct, minimal, and well-scoped.
ltoplugin/LLGOInterfaceMethodTypeIDPass.cpp— SwitchinginvalidMetadata()fromreport_fatal_errortoreportFatalUsageErroris the right call. Malformed interface type-id metadata is invalid input, not an internal invariant violation, so a cleanexit(1)diagnostic (no crash reporter / symbolizer /abort()) is the correct classification. The helper stays[[noreturn]], theconst Twine&overload matches, andllvm/Support/ErrorHandling.his already included — no include churn. The plugin is pinned to LLVM 22.x inCMakeLists.txt, andreportFatalUsageErrorexists there, so this is API-safe.cl/compile_test.go— The strengthened negative test is a solid improvement over the previous "any failure" check. The assertion order is correct:ctx.Err()is inspected first (distinguishes a hang/timeout from a wrong exit), then*exec.ExitErrorwithExitCode() == 1(rejects both success and signal-induced crashes), then the diagnostic substring. The 30s context timeout + 1sWaitDelaybound a stuckoptso it fails fast locally instead of consuming the package timeout.
Non-blocking (out of scope): ltoplugin/LLGOReflectMethodByNamePass.cpp:1951,2121 still use report_fatal_error for what also look like input-validation failures. Not part of this diff; a future follow-up could align them with the convention this PR establishes.
Collaborator
Author
|
Final validation on 7312875: all 64 checks passed, with no pending or failed checks. The latest fennoai review is on this head and all review threads are resolved. The rebase leaves only the intended two-file LTO fix. In addition to this PR CI, cherry-picking this fix into #2530 made its previously hanging malformed-metadata LTO job pass: https://github.com/xgo-dev/llgo/actions/runs/34812965081/job/103877868707 . Locally, all three rejection cases were executed with the built LLVM 22 plugin and -tags=dev (no skips), verifying exit status 1 and the expected diagnostic. |
zhouguangyuan0718
approved these changes
Sep 14, 2026
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
2 participants
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
Reject malformed interface metadata with LLVM 22's
reportFatalUsageError, retaining the fatal diagnostic and exit status 1 without invoking abort-time crash reporting or symbolization. Valid IR and method reachability are unchanged.The existing negative test now requires normal exit 1, not just any failure containing the expected message. Bound each
optinvocation to 30 seconds and pipe cleanup to one second, so a stuck subprocess fails locally instead of consuming the entire package's 30-minute timeout. No test is skipped and no existing timeout is increased.Rebased onto current main
5c5874359. Main now contains the Linux descendant-probe corrections, so Git dropped both obsolete prerequisite commits and this PR again contains only the core malformed-LTO-metadata fix. The focusedTestLTOPluginRejectsMalformedInterfaceAttributestest passes locally against the rebased head; fresh CI is running.Evidence and validation
opt; this PR does not claim that missing detail is proven.exit(1)) from fatal internal errors (abort()). Use the former for rejected input instead of depending on crash handling for an expected test outcome.d4791e94e, all six jobs in the fork Go workflow and Format Check pass, including Linux/macOS LTO and Linux/macOS/Windows MSVC/MinGW coverage suites. Unrelated fork workflows were cancelled to conserve runner capacity. The production Wasm/runtime code is untouched.