Use libBlocksRuntime from swift-corelibs-libdispatch as the blocks runtime

[ngrewe]

Fixes #199

This PR enables using libBlocksRuntime as the blocks runtime in libobjc2. The two important prerequisites are:

1. This only supports libBlocksRuntime from apple/swift-corelibs-libdispatch, and not the one from LLVM's compiler-rt. The reason being that the latter has not been updated to include the relevant Block_use_RR2() function that we need for weak reference integration. The older Block_use_RR hook would require implementing objc_destructInstance().
2. libBlocksRuntime must be installed with INSTALL_PRIVATE_HEADERS=ON, which distributions tend to do in their -dev packages.

The major differences between our internal blocks runtime and the external one are the following:

• libBlocksRuntime uses two additional classes for managing block behaviour, _NSConcreteAutoBlock , and _NSConcreteFinalizingBlock for blocks promoted from the stack to the heap (either without or with attached dtors), which the ARC machinery must be aware of to call out to the block retain/release function appropriately. In theory, this could be switched off in libBlocksRuntime if it is explicitly compiled with Objective-C support, but I don't entirely understand how their build process is supposed to achieve this and additionally, I wager, the probability of packagers willing to implement that is close to nil.
• Reference counting for blocks is different. While libobjc2 blocks use the reserved field in the block structure, libBlocksRuntime blocks use a part of the flags field.
• In order to correctly integrate with retain/release for objects contained in blocks and to get rid of weak references, we need to install the correct hooks using the Block_use_RR2() function mentioned above.

[ngrewe]

(still need to add variants to the test suite that exercises the integration)

[ngrewe]

Furthermore, while the test suite passes again, Valgrind shows an invalid read for the reference count in one additional place:

==488763== Memcheck, a memory error detector
==488763== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
==488763== Using Valgrind-3.23.0 and LibVEX; rerun with -h for copyright info
==488763== Command: ./Test/WeakBlock_arc
==488763== 
==488763== Invalid read of size 8
==488763==    at 0x48877BB: object_getRetainCount_np (arc.mm:254)
==488763==    by 0x4888E4C: objc_storeWeak (arc.mm:871)
==488763==    by 0x10A557: main (Test/WeakBlock_arc.m:10)
==488763==  Address 0x4fe48a8 is 8 bytes before a block of size 40 alloc'd
==488763==    at 0x4843859: malloc (in /var/home/linuxbrew/.linuxbrew/Cellar/valgrind/3.23.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==488763==    by 0x49A12B5: _Block_copy_internal (runtime.c:371)
==488763==    by 0x49A11A9: _Block_copy (runtime.c:504)
==488763==    by 0x4888A04: objc_retainBlock (arc.mm:635)
==488763==    by 0x10A517: main (Test/WeakBlock_arc.m:8)
==488763== 
==488763== 
==488763== HEAP SUMMARY:
==488763==     in use at exit: 1,212,805 bytes in 86 blocks
==488763==   total heap usage: 94 allocs, 8 frees, 1,814,093 bytes allocated
==488763== 
==488763== LEAK SUMMARY:
==488763==    definitely lost: 0 bytes in 0 blocks
==488763==    indirectly lost: 0 bytes in 0 blocks
==488763==      possibly lost: 0 bytes in 0 blocks
==488763==    still reachable: 1,212,805 bytes in 86 blocks
==488763==         suppressed: 0 bytes in 0 blocks
==488763== Rerun with --leak-check=full to see details of leaked memory
==488763== 
==488763== For lists of detected and suppressed errors, rerun with: -s
==488763== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

But this happens with the embedded blocks runtime as well, because objc_storeWeak() does not treat blocks specially when checking if the object is currently being deallocated.

[ngrewe]

Valgrind shows an invalid read for the reference count

Fixed in 628b291

[davidchisnall]

From the comment, it sounds as if things are broken if we don't have that symbol? Shouldn't we fail the build in that case?

[ngrewe]

Ah, yes absolutely. If forgot to make it fail hard when I remembered how, uhm, quirky the objc_constructInstance/objc_destructInstance API is that the legacy hook function would need.

[davidchisnall]

At some point, I am pondering doing the thing Apple does of embedding the refcount and weak flags in the isa pointer. I believe GNUstep should now all be clean to that.

It probably doesn’t make sense to do on 32-bit platforms so it would be 64-bit only, but would give us a nice memory saving there and would make it easier to support those APIs, at least on 64-bit platforms.

[davidchisnall]

This is starting to be a fairly expensive check. It's probably a good idea to use one of the spare bits in the info field of objc_class as an is-block check. Then we just set it once and do a single bitfield test and below, rather than a load of compares. Probably also good to mark this as unlikely as well.

[ngrewe]

Small complication here is that objc_create_block_classes_as_subclasses_of() only gets called when a Foundation library sets it up explicitly with its root class, so I had to pre-initialise just the info field when the runtime loads.

btw: I'm assuming that stack blocks are always promoted to the heap before they are assigned to weak references. Is that an accurate assumption? (we need a couple of checks for _NSConcreteStackBlock otherwise).

[davidchisnall]

I’m not sure. I vaguely remember being able to take a weak reference to a block and have it go away when the block scope ended, but I’m not completely sure.

[ngrewe]

So I've looked into this a bit more and did some experimentation and I think it's safe the way it is right now. The point I was worried about was what happens in objc_storeWeak(), where we don't do anything to tie the lifetime of a stack block to that of the weak reference. But since this is all happening under ARC, the compiler will always emit a call to objc_retainBlock() if there is any chance that the block will escape the current scope – at least that's my reading of CodeGenFunction::EmitARCRetainBlock() in LLVM.

[davidchisnall]

If we have the is-block info flag, we could move this up inside the is-it-a-block check.

[ngrewe]

Done in 7edfecc.

[davidchisnall]

Does this not need some atomic operation?

[ngrewe]

I've only moved this out from the main blocks_runtime.c and added a variant for libBlocksRuntime. Doing this without atomic operations should be fine because the function is only ever called when the runtime holds the weak reference lock.

[davidchisnall]

Would be nice to have a comment to that effect, since that's in a different file. Can we race something calling retain / release on the block, or can that never happen because we're in the dealloc path here and it's UB if someone releases / retains a block after the refcount becomes zero?

[ngrewe]

Can we race something calling retain / release on the block

Thanks for pointing that! As it turns out, we are indeed susceptible to race conditions in objc_loadWeakRetained() even with the current implementation.

Assume thread A holds a weak reference to a block and thread B the last strong reference to it. If thread A wants to promote the weak reference to a strong one, we can have the following sequence of events:

1. Thread A: Calls objc_loadWeakRetained() and obtains the weak reference lock. block_load_weak() returns the block because the reference count is still 1.
2. Thread B: Relinquishes the last strong reference, causing _Block_release() to run. The reference count drops to 0, so that destructors run and then the thread blocks again waiting to obtain the weak reference lock in order to delete the weak references.
3. Thread A: At the end of objc_loadWeakRetained() _Block_copy() increments the reference count back up to 1, and returns a strong reference to the block to the caller.
4. Thread B: Proceeds to remove weak references, and frees the memory allocated for the block.
5. Thread A: Still has the strong reference, which now points to freed memory 💥.

You've already documented the solution to that problem quite well in retain_fast(), and libBlocksRuntime exposes a function _Block_tryRetain() just for that purpose… I'm now using that and have implemented it for our embedded runtime as well.

Maybe you can have another look at the changeset in 97261ba to see whether that makes sense now?

[davidchisnall]

Looks good, thanks.

[davidchisnall]

The FreeBSD 13.2 error is probably due to 13.3 being released.

[ngrewe]

The FreeBSD 13.2 error is probably due to 13.3 being released.

That seems to be it. Bumping that to 13.3 makes the build pass again. I don't really understand why it now shows "Some checks haven't completed yet", where it seems to wait for the freebsd-13-2 CI run 🤔

[davidchisnall]

LGTM. Can you squash it down for merge?

[ngrewe]

LGTM. Can you squash it down for merge?

Happy to, but the "required statuses must pass before merging" thing with the freebsd-13-2 job is blocking the merge. That seems to be configured in the repository settings under Rules→Rulesets.

[davidchisnall]

I can override that if you force push a cleaned up history.

[qmfrederik]

@ngrewe Thank you for working on this. MSYS2 includes the apple/swift-corelibs-libdispatch package and libobjc2. In that scenario, would the recommendation be to build libobjc2 using the external blocks runtime?

[ngrewe]

@ngrewe Thank you for working on this. MSYS2 includes the apple/swift-corelibs-libdispatch package and libobjc2. In that scenario, would the recommendation be to build libobjc2 using the external blocks runtime?

Since this is brand new, I don't feel very confident about the term "recommendation" yet. But generally speaking, being able to use a platform-supplied libdispatch/libBlocksRuntime package without patches in conjunction with libobjc2 is the express purpose of this feature.