Skip to content

cmd/compile: validate noescape function bodies - #2220

Closed
cpunion wants to merge 2 commits into
xgo-dev:mainfrom
cpunion:codex/xfail-noescape-pragma-20260730
Closed

cmd/compile: validate noescape function bodies#2220
cpunion wants to merge 2 commits into
xgo-dev:mainfrom
cpunion:codex/xfail-noescape-pragma-20260730

Conversation

@cpunion

@cpunion cpunion commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • validate the gc-only //go:noescape body restriction in the source loader when an earlier compiler diagnostic makes go list stop before reporting it
  • preserve cmd/compile pragma association across blank lines and ordinary comments, including method and //line position semantics
  • suppress the writer-only diagnostic when parsing, complete type checking, or imported packages have already failed
  • remove fixedbugs/issue18331.go from the Go 1.26 GOROOT xfail list

Tests and coverage

  • added focused pragma association, placement, deduplication, method-position, and phase-order unit tests
  • added end-to-end compile tests for the review counterexamples: blank/comment separation, preceding unknown pragma, type error, and parse error
  • Go 1.26.5: go test ./internal/packages ./internal/build ./cmd/internal/compile -count=1
  • Go 1.24.11, 1.25.0, and 1.26.0: go test ./internal/packages ./cmd/internal/compile -count=1
  • go test ./test/goroot -count=1
  • Go 1.24.11, 1.25.0, and 1.26.0: fixedbugs/issue18331.go and fixedbugs/issue48097.go
  • Go 1.26.0: target case plus all remaining 148 compile/errorcheck xfails

No functional dependency on another PR. #2201 only has a separate test-file overlap; #2218 and #2219 touch different xfail.yaml entries.

CI environment note: #2215 has landed on main with the isolated Ubuntu ESP QEMU package-install fix. This PR does not contain any CI changes.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: cmd/compile: validate noescape function bodies

Focused, well-tested change. The directive-scanning logic in internal/packages/pragma.go and the error-gating in loadPackageEx correctly mirror cmd/compile semantics, and the new unit + e2e tests cover the tricky cases (blank-line/comment separation, method///line positions, phase ordering). go test ./internal/packages passes.

A few things I verified that are not problems, for the record:

  • Tab-separated argument (//go:noescape\tignored → no error, per the test): this matches gc. cmd/compile splits the pragma verb on a literal space only (strings.Index(text, " ") in noder.go), so go:noescape\tignored is not recognized as go:noescape and a body is not flagged. The next == ' ' check is correct here — even though isEmbedDirectiveComment in the same package accepts \t (embed uses pragmaFields/isSpace, a different tokenizer). No change needed.
  • errs in the hasSourceErrors expression compiles and is in scope (the parseFiles result). It is slightly redundant with the later ParseError scan over lpkg.Errors, but harmless.
  • xfail.yaml removal of fixedbugs/issue18331.go: the errorcheck case also asserts three "is not allowed in the standard library" diagnostics for unknown pragmas that this PR does not implement directly. Those come through from the go list driver (which is exactly why it stops early before the body check — the scenario this PR targets), so the removal is justified.

Only two minor, low-severity notes inline. No blocking issues.

Comment thread internal/packages/pragma.go Outdated
Comment thread internal/packages/load.go
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cpunion

cpunion commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

CI triage: the Ubuntu Go 1.24 compatibility failure is in Install embedded dependencies → Verify ESP QEMU, before this PR’s tests run: qemu-system-riscv32 cannot load libslirp.so.0 (job 90736223056). This is the environment issue fixed by #2215, whose install-only head is fully green. I am keeping that CI package change out of this functional branch; the failed job should be rerun after #2215 lands.

@cpunion

cpunion commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Closing because fixedbugs/issue18331.go is now explicitly classified on main as not applicable: it validates a gc-only standard-library pragma pipeline that LLGo does not intend to implement.

@cpunion cpunion closed this Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go-test-compat Go standard-library and GOROOT test compatibility

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant