[sdkdimon]

This is an alternative to #438. It follows the review there.

The bug

imp_implementationWithBlock() allocates a trampoline set only when the list is empty. When every set is full, it reaches UNREACHABLE("Failed to allocate block"), so it never allocates a second set:

• Release builds: IMP 129 on AArch64 (IMP 257 on x86-64, with 4 KiB pages) is garbage, and calling it crashes.
• Builds with assertions: it aborts one IMP earlier, at IMP 128 or 256. assert(set->first_free < trampoline_header_per_page) compares -1 with a size_t, so it fails as soon as a set is full.

The fix

• New sets. When no set has a free trampoline, a new set is allocated and linked in at the head of the list, so the next allocations find a free trampoline at once. Older sets stay in the list, each with its inline free list. imp_getBlock() and imp_removeBlock() still find IMPs in every set, and trampolines freed in any set get reused.
• Allocation failure. If calloc() or mmap()/VirtualAlloc() fails, it returns a null IMP instead of crashing.
• Store the copy. The trampoline now stores the Block_copy() result, not the block that was passed in. For a stack block it used to keep a pointer into a dead stack frame, and imp_removeBlock() released the wrong block, leaking the copy.
• Removed IMPs. imp_getBlock() returns nil for a removed IMP instead of the free list link.
• Double removal. imp_removeBlock() returns NO for an IMP that was already removed. Before, it released the free list link and put the trampoline on the free list twice.
• Assertion. The first_free assertion now compares as int.

Tests

• New Test/ManyBlockImps.m sets and unsets 5000 block IMPs, first with blocks that return an int, then with blocks that return a struct (the sret trampolines on x86-64). Each block captures its index. The test:

  • calls every IMP, and the block that imp_getBlock() returns for it;
  • removes every other IMP, and checks that imp_removeBlock() returns YES, imp_getBlock() then returns nil, and a second remove returns NO;
  • makes as many new IMPs, and checks that they reuse the freed trampolines and that all live IMPs are distinct and still work;
  • removes them all, does it all again, and removes them all again.

  It's registered next to BlockImpTest.m, so it only runs with EMBEDDED_BLOCKS_RUNTIME.

• Test/BlockImpTest.m gets a stack block case: IMPs made from stack blocks in a function that has returned must still call the right block.

Results

Ubuntu 24.04 with clang 18, CMake and Ninja with -DTESTS=ON. aarch64 ran natively and x86-64 under qemu. "Block tests" means BlockImpTest and ManyBlockImps, each in its 4 variants (plain, _optimised, _legacy, _legacy_optimised).

	aarch64 Release	aarch64 Debug	x86-64 Release	x86-64 Debug
master	8 of 8 block tests fail	8 of 8 fail	8 of 8 fail	8 of 8 fail
#438	8 of 8 fail	not run	8 of 8 fail	not run
this PR	all 204 tests pass	all 204 pass	all 204 pass	8 of 8 block tests pass

How the tests fail:

• ManyBlockImps on master:
  • Release: IMP 129 (aarch64) or 257 (x86-64) is garbage. imp_getBlock() doesn't know it, and calling it crashes.
  • Debug: it aborts at IMP 128 or 256 on the first_free assertion.
• ManyBlockImps with Unlimited imp_implementationWithBlock() invocations #438: imp_getBlock() can't find IMP 127 (aarch64) or 255 (x86-64).
• BlockImpTest, on master and with Unlimited imp_implementationWithBlock() invocations #438: the trampoline stores the stack block instead of its copy.

With only the allocation part of this fix, ManyBlockImps passes and BlockImpTest still fails, so each test catches its own bug.

Not tested: Windows, macOS, 32-bit ARM, PowerPC, MIPS, and 16 KiB pages.

[davidchisnall]

This project does not accept LLM-generated code.

And I’m also getting fed up of the LLM-generated issues that spend a dozen paragraphs to report a bug where one sentence would do.

[sdkdimon]

Sorry for the LLM-generated PR and the noise it caused. I should have written the fix myself, and I won't submit generated code here again.