Repository navigation
build: fix function value lookup without prebuilt PCLN - #3044
Merged
cpunion merged 3 commits intoNov 11, 2026
Merged
Conversation
Collaborator
Author
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
This is a clean, well-scoped fix for #2857. Broadening the exact-entry inline-asm record from the Win32-only gate (siteObjectCOFF && PointerSize()==4 && functionAddressTaken) to if functionAddressTaken(fn) correctly makes reflect.ValueOf(fn).Pointer() → FuncForPC(...).Entry() resolve to the true entry on every target when the prebuilt PCLN rewrite is disabled, instead of the post-prologue anchor.
Correctness / safety — verified clean:
- The
${0:c}/"X"operand is a body-local inline-asm use, notllvm.used/llvm.compiler.used. So it does not pin otherwise-dead functions: if the containing body is DCE'd, the record goes with it. GlobalDCE behavior is unchanged, matching the existing"X"pattern inssa/globaldce.go. - The per-target object emission is actively validated: the generalized
TestFuncInfoUsesExactEntryForAddressTakenFunctioncross-emits a real object (EmitToMemoryBuffer) for all 8 goos/goarch combos, so a malformed/non-relocatable record would surface as an assembler error rather than silent FuncForPC corruption. Nice. functionAddressTakenis conservative (any non-direct-call use ⇒ address-taken), which is the safe direction here.
Test changes — well targeted: adding LLGO_PCLNPOST=0 is what actually exercises the no-prebuilt-PCLN path, and the new once / closure (same-entry=true) assertions check exactly the contract the PR fixes. t.Setenv is auto-restored, no state leak. No secret/token exposure.
The notes below are all minor (comment accuracy); the code change itself looks correct.
Scope note on the "Known limitation" comment (lines ~894-904, unchanged by this diff): that comment documents the LTO-inlining section bloat (~4x on multipkg) and dead-PC-under-inlinee-symbol-ID effects for the in-body exact-entry record. Previously the exact-entry variant of this record only existed on the Win32/386 path; this PR now emits it for every address-taken function on every target. The per-record size is unchanged (the "X" operand is a relocation, not extra bytes), so this is bounded, but the comment now reads as more narrowly scoped than the behavior. Worth a follow-up to update that comment to state the limitation is now target-general, and to confirm the link-phase dedup mitigation it references actually covers the newly-enabled non-Win32 targets.
Contributor
|
Confirmed — the workspace is at head Review: PR #3044 — build: fix function value lookup without prebuilt PCLNThis is a clean, well-scoped fix for #2857. Broadening the exact-entry inline-asm record from the Win32-only gate ( Verified clean (no findings)
Minor notes (comment accuracy only)
Verdict: Nothing blocking. The fix is correct and the test coverage targets the exact bug; the only items are the three minor comment-accuracy notes above. |
Collaborator
Author
Contributor
|
Confirmed: the update since my last review is comment-only in Re-review: PR #3044 @
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
@fennoai Applied the shared CI fix for the repeated Ubuntu archive TLS failures: prioritized official HTTPS mirrors in the existing setup-deps helper. The helper and workflow-contract tests pass; no jobs, source suites or signing keys changed. Please re-review this CI-only update. |
Contributor
|
I have a complete picture. The CI workflow runs the test ( Assessment of the CI change (
The trigger comment's claims all check out: CI-only, no jobs/source-suites/signing-keys changed, helper + contract tests pass. The compiler fix and its tests are byte-identical to the revision I already fully reviewed and approved. In managed mode I return the result as Markdown in my final message. Re-review: PR #3044 @
|
LLGo baseline benchmarks
String equality benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
zhouguangyuan0718
approved these changes
Nov 11, 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
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.
Fixes #2857.
Record true entry addresses for address-taken native functions, reusing the existing Win32 mechanism. With
LLGO_PCLNPOST=0, unaligned amd64 function-value PCs previously resolved to the preceding function because entry anchors follow the prologue.The existing Linux fallback test now explicitly disables prebuilding and checks cold, warmed and function-value lookups. Cross-target object tests cover ELF, Mach-O and COFF; macOS PCLN packaging and native/full-LTO pruning checks pass.
go vet ./...matches the 12 baseline diagnostics.CI: retain official HTTPS mirror fallback in the existing Ubuntu setup helper after repeated archive TLS failures; no new jobs.