Skip to content

compiler: bound aggregate call signatures - #5625

Merged
deadprogram merged 1 commit into
tinygo-org:devfrom
jakebailey:fix/aggregate-lowering
Sep 16, 2026
Merged

deadprogram merged 1 commit into
tinygo-org:devfrom
jakebailey:fix/aggregate-lowering

Conversation

@jakebailey

Copy link
Copy Markdown
Member

Fixes #5615

This is a sort-of more involved alternative to #5618; the gist is that we ensure we limit everything to up to 1000 scalar elements across the board. Back when I did #5526, I had something like this, but scaled it back because I didn't think exactness would matter too much so long as we didn't overload LLVM. But, I guess it does matter.

Comment thread builder/build.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR limits scalarized aggregate function signatures to 1,000 parameters.

Changes:

  • Selects aggregate parameters for indirect passing.
  • Handles calls, interfaces, defers, goroutines, and exported ABI validation.
  • Adds ABI and global-reference tests.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
builder/build.go Validates WebAssembly signatures.
compiler/calls.go Centralizes ABI argument handling.
compiler/compiler.go Applies ABI choices to calls and functions.
compiler/compiler_test.go Tests aggregate ABI limits.
compiler/defer.go Applies the ABI to deferred calls.
compiler/func.go Calculates and validates function ABIs.
compiler/goroutine.go Applies the ABI to goroutine arguments.
compiler/interface.go Handles interface invoke wrappers.
compiler/llvmutil/llvm.go Removes temporary global references.
compiler/llvmutil/llvm_test.go Tests global-reference removal.
compiler/symbol.go Protects indirect signatures during optimization.
compiler/testdata/aggregate-abi.go Adds internal ABI fixtures.
compiler/testdata/aggregate-export-abi.go Adds exported ABI fixtures.
transform/interface-lowering.go Reports incompatible interface ABIs.
transform/optimizer.go Removes temporary ABI roots.
Suppressed comments (2)

compiler/compiler.go:2326

  • This ABI calculation omits the synthetic typecode parameter that is appended below. At the 1000 parameter boundary, the invoke function spills an aggregate but this call passes it directly, which gives the callee a different ABI.
		abi = b.getInterfaceFunctionABI(instr.Signature())

compiler/interface.go:1350

  • This uses the concrete receiver when it selects indirect parameters. The interface invoke ABI replaces that receiver with a pointer, so it can select different nonreceiver parameters and the wrapper then forwards values with the wrong ABI.
	receiverType := abi.params[0].llvmType
	var expandedReceiverType []llvm.Type
	receiverIndirect := abi.params[0].indirect
	var receiverInfos []paramInfo
	if receiverIndirect {
		receiverInfos = []paramInfo{{llvmType: c.dataPtrType}}
	} else {
		receiverInfos = c.expandDirectFormalParamType(receiverType, "", nil)

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread compiler/interface.go
Comment thread compiler/compiler.go Outdated
Comment thread transform/optimizer.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

compiler/compiler_test.go:249

  • This test checks only that the compiler emits the marker. It does not run LowerInterfaces, so the new diagnostic path is not tested. Add a transform or build test that checks the expected error and a compatible exported interface call.
	if markedWrappers != 1 {

@deadprogram

Copy link
Copy Markdown
Member

Thank you for this work @jakebailey. Here are the edited results from an automated review.

Findings

Dead code and stale text

  • compiler/func.go:198 - aggregateValueCount now has only one caller, compiler_test.go:505. No production code uses it.
  • compiler/calls.go:175 - the comment refers to expandFormalParamType, which this PR removes.

Unrelated behavior change

  • transform/optimizer.go - the second commit moves the config.Scheduler() == "none" goroutine check from before the ThinLTO pre-link pipeline to after it. Only RemoveGlobalReferences and the new globaldce must move. The check now runs after dead code elimination, so a goroutine start that the optimizer removes no longer reports attempted to start a goroutine without a scheduler. The commit message does not mention this. Is the move necessary?

Polish

  • transform/optimizer.go - the added globaldce runs on every build, also when no tinygo.indirect-abi global exists. A guard prevents the unnecessary pass.
  • transform/interface-lowering.go:279 - fmt.Errorf("%s", ...) gives an error with no source position. The other code in this pass uses errorAt. The loop also returns at the first error, so the user sees one diagnostic at a time.
  • compiler/compiler.go:1446 - SetInsertPointBefore in the phi loop leaves the insert point in a predecessor block after the loop ends. This is correct now, but it can break later changes.
  • The validation message function bigExport has more than 1000 WebAssembly parameters after ABI lowering does not tell the user that //export prevented the lowering, or what to change.

Process

  • The branch is 36 commits behind dev. Please rebase before merge.

@jakebailey
jakebailey force-pushed the fix/aggregate-lowering branch from 8af9cfd to 04564c7 Compare September 12, 2026 13:42
@jakebailey

Copy link
Copy Markdown
Member Author

Debased and updated, PTAL

@jakebailey
jakebailey force-pushed the fix/aggregate-lowering branch from 04564c7 to 4637038 Compare September 15, 2026 14:53
@dgryski

dgryski commented Sep 15, 2026

Copy link
Copy Markdown
Member

Passes my test corpus.

@deadprogram

Copy link
Copy Markdown
Member

Thank you for the update @jakebailey. Here are the edited results from an automated review. All findings from the previous review are addressed.

Findings

  1. compiler/interface.go:1345 - an exported method that an interface can reach stops the build with an LLVM verifier error, not with the new diagnostic:
function declaration may only have a unique !dbg attachment
ptr @"tcall$invoke"
verification error after compiling package t

attachDebugInfoRaw makes a DISubprogram with IsDefinition: true, but the ABI-error wrapper stays a declaration. LLVM permits only a unique, non-definition subprogram on a declaration (LLVM Language Reference, "Source Level Debugging", DISubprogram). The verifier stops the build before LowerInterfaces reports the error. A declaration-style DISubprogram corrects this and keeps the source position.

  1. compiler/compiler_test.go:236 - TestAggregateExportedInterfaceABI does not call llvm.VerifyModule, so it does not find finding 1. Add a verification step for both debug settings.

  2. compiler/defer.go:632 - the first getFunctionABI result is discarded for the invoke case. Move it into the else branch. Also, params = abi.params and params = params[1:] can be one line.

@jakebailey
jakebailey force-pushed the fix/aggregate-lowering branch from 4637038 to 9490789 Compare September 16, 2026 15:21
Plan internal aggregate parameter lowering from each complete function
signature. Count scalar leaves, the context parameter, and any hidden
aggregate-result pointer, then pass the largest aggregates indirectly
until the signature fits within the 1,000-parameter limit.

Keep the ABI policy target-independent and leave fitting signatures
unchanged. Include the interface typecode when budgeting invoke
signatures and keep parameter lowering consistent between concrete and
interface calls.

Preserve exported ABIs and diagnose exported methods that cannot use the
internal interface ABI. Attach valid debug metadata to these diagnostic
wrappers so that errors keep their source positions.

Materialize indirect aggregate phi inputs in their predecessor blocks.
Retain temporary argument-promotion guards through the ThinLTO pre-link
pipeline, then remove the guards before final dead-code elimination.
Validate final WebAssembly signatures that cannot be rewritten.
@jakebailey
jakebailey force-pushed the fix/aggregate-lowering branch from 9490789 to b7d1990 Compare September 16, 2026 15:33

@deadprogram deadprogram left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All looking good now @jakebailey thank you very much. Also thank you @dgryski for review. Now merging.

@deadprogram
deadprogram merged commit 3a84b1a into tinygo-org:dev Sep 16, 2026
33 checks passed
@jakebailey
jakebailey deleted the fix/aggregate-lowering branch September 16, 2026 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wasm: struct-by-value parameters scalarize into >1000-param function types, rejected by browsers and wasm-tools

4 participants