objc_retain() currently handles every Block by calling Block_copy() upon it:
if (UNLIKELY(objc_test_class_flag(cls, objc_class_flag_is_block)))
{
return Block_copy(obj);
}I believe this violates Clang's ARC runtime contract, with emphasis on that last line:
id objc_retain(id value);
Precondition: value is null or a pointer to a valid object.
If value is null, this call has no effect. Otherwise, it performs a retain operation exactly as if the object had been sent the retain message.
Always returns value.
From my reading, objc_retainBlock() is the (only?) routine permitted to copy a stack Block and return a different address if the Block transitions from the stack to the heap:
id objc_retainBlock(id value);
Precondition: value is null or a pointer to a valid block object.
If value is null, this call has no effect. Otherwise, if the block pointed to by value is still on the stack, it is copied to the heap and the address of the copy is returned. Otherwise a retain operation is performed on the block exactly as if it had been sent the retain message.
A simple example (a NSData-like class implementing a no-copy initializer with a custom deallocator Block) that illustrates the issue is as follows:
#import "Test.h"
#include <stdlib.h>
typedef void (^Deallocator)(void *, unsigned long);
typedef struct Buffer {
void *bytes;
unsigned long count;
Deallocator deallocator;
} Buffer;
@interface Data : Test {
Buffer *_buffer;
}
- (id)initWithBytesNoCopy:(void *)bytes count:(unsigned long)count deallocator:(Deallocator)deallocator;
@end
@implementation Data
- (id)initWithBytesNoCopy:(void *)bytes count:(unsigned long)count deallocator:(Deallocator)deallocator
{
_buffer = calloc(1, sizeof(*_buffer));
_buffer->bytes = bytes;
_buffer->count = count;
_buffer->deallocator = deallocator;
return self;
}
- (void)dealloc
{
_buffer->deallocator(_buffer->bytes, _buffer->count);
_buffer->deallocator = nil;
free(_buffer);
}
@end
static __weak id weakToken;
int
main(void)
{
@autoreleasepool {
Test *token = [Test new];
void *bytes = malloc(1);
Data *data;
weakToken = token;
assert(bytes != nullptr);
data = [[Data alloc] initWithBytesNoCopy:bytes count:1
deallocator:^(void *pointer, unsigned long count) {
assert(token != nil);
assert(count == 1);
free(pointer);
}];
}
assert(weakToken == nil);
return 0;
}
At -O0, Clang calls objc_storeStrong() on the Block parameter in -initWithBytesNoCopy:count:deallocator:. objc_storeStrong() calls objc_retain() on the Block, creating a heap Block which is subsequently assigned through the id *addr parameter. *addr is what gets assigned to deallocator in the buffer structure, so both the stack Block and the heap Block are properly released at the end of the @autoreleasepool's scope.
At -01 and above, Clang calls objc_retain() on the Block parameter, but it keeps using the original stack Block pointer (which is a valid optimization to make). Of course, this causes the newly allocated heap Block to leak, which triggers assert(weakToken == nil);, since at this point, there should be no Blocks in existence referencing token.
I propose making retain() check for stack Blocks and return them as-is:
if (UNLIKELY(objc_test_class_flag(cls, objc_class_flag_is_block)))
{
// objc_retain() must return its argument. Only objc_retainBlock() may
// promote a stack block to a heap block with a different address.
if (cls == static_cast<void*>(&_NSConcreteStackBlock))
{
return obj;
}
return Block_copy(obj);
}This aligns the behaviour of objc_retain() for Blocks with my reading of the ARC spec.
All libobjc2 tests continue to pass with this change. Even though I don't like to make statements carrying words of absolute certainty, I'm 100% certain that this change will not break GNUstep's libs-base, unlike my oopsie from last time :)
objc_retain()currently handles every Block by callingBlock_copy()upon it:I believe this violates Clang's ARC runtime contract, with emphasis on that last line:
From my reading,
objc_retainBlock()is the (only?) routine permitted to copy a stack Block and return a different address if the Block transitions from the stack to the heap:A simple example (a
NSData-like class implementing a no-copy initializer with a custom deallocator Block) that illustrates the issue is as follows:At
-O0, Clang callsobjc_storeStrong()on the Block parameter in-initWithBytesNoCopy:count:deallocator:.objc_storeStrong()callsobjc_retain()on the Block, creating a heap Block which is subsequently assigned through theid *addrparameter.*addris what gets assigned todeallocatorin the buffer structure, so both the stack Block and the heap Block are properly released at the end of the@autoreleasepool's scope.At
-01and above, Clang callsobjc_retain()on the Block parameter, but it keeps using the original stack Block pointer (which is a valid optimization to make). Of course, this causes the newly allocated heap Block to leak, which triggersassert(weakToken == nil);, since at this point, there should be no Blocks in existence referencingtoken.I propose making
retain()check for stack Blocks and return them as-is:This aligns the behaviour of
objc_retain()for Blocks with my reading of the ARC spec.All libobjc2 tests continue to pass with this change. Even though I don't like to make statements carrying words of absolute certainty, I'm 100% certain that this change will not break GNUstep's libs-base, unlike my oopsie from last time :)