diff options
Diffstat (limited to 'objectivec')
-rw-r--r-- | objectivec/GPBCodedInputStream.m | 2 | ||||
-rw-r--r-- | objectivec/GPBCodedInputStream_PackagePrivate.h | 2 | ||||
-rw-r--r-- | objectivec/GPBDescriptor.m | 6 | ||||
-rw-r--r-- | objectivec/GPBMessage.m | 53 | ||||
-rw-r--r-- | objectivec/GPBMessage_PackagePrivate.h | 18 | ||||
-rw-r--r-- | objectivec/GPBUtilities.m | 32 | ||||
-rw-r--r-- | objectivec/GPBUtilities_PackagePrivate.h | 4 | ||||
-rw-r--r-- | objectivec/README.md | 10 | ||||
-rw-r--r-- | objectivec/Tests/GPBCodedInputStreamTests.m | 2 | ||||
-rw-r--r-- | objectivec/Tests/GPBCodedOuputStreamTests.m | 2 | ||||
-rw-r--r-- | objectivec/Tests/GPBUtilitiesTests.m | 8 | ||||
-rw-r--r-- | objectivec/google/protobuf/Struct.pbobjc.m | 7 | ||||
-rw-r--r-- | objectivec/google/protobuf/Type.pbobjc.m | 17 |
13 files changed, 92 insertions, 71 deletions
diff --git a/objectivec/GPBCodedInputStream.m b/objectivec/GPBCodedInputStream.m index 0759640d..a8262e5d 100644 --- a/objectivec/GPBCodedInputStream.m +++ b/objectivec/GPBCodedInputStream.m @@ -110,7 +110,7 @@ static int64_t ReadRawVarint64(GPBCodedInputStreamState *state) { int64_t result = 0; while (shift < 64) { int8_t b = ReadRawByte(state); - result |= (int64_t)(b & 0x7F) << shift; + result |= (int64_t)((uint64_t)(b & 0x7F) << shift); if ((b & 0x80) == 0) { return result; } diff --git a/objectivec/GPBCodedInputStream_PackagePrivate.h b/objectivec/GPBCodedInputStream_PackagePrivate.h index 90bd0c92..43ec6e79 100644 --- a/objectivec/GPBCodedInputStream_PackagePrivate.h +++ b/objectivec/GPBCodedInputStream_PackagePrivate.h @@ -34,8 +34,6 @@ #import "GPBCodedInputStream.h" -#import <libkern/OSAtomic.h> - @class GPBUnknownFieldSet; @class GPBFieldDescriptor; diff --git a/objectivec/GPBDescriptor.m b/objectivec/GPBDescriptor.m index 3c3844da..615d2234 100644 --- a/objectivec/GPBDescriptor.m +++ b/objectivec/GPBDescriptor.m @@ -548,7 +548,8 @@ uint32_t GPBFieldAlternateTag(GPBFieldDescriptor *self) { // descriptor structure. const uint8_t *bytes = (const uint8_t *)defaultValue_.valueData; if (bytes) { - uint32_t length = *((uint32_t *)bytes); + uint32_t length; + memcpy(&length, bytes, sizeof(length)); length = ntohl(length); bytes += sizeof(length); defaultValue_.valueData = @@ -963,7 +964,8 @@ uint32_t GPBFieldAlternateTag(GPBFieldDescriptor *self) { const uint8_t *bytes = (const uint8_t *)description->defaultValue.valueData; if (bytes) { - uint32_t length = *((uint32_t *)bytes); + uint32_t length; + memcpy(&length, bytes, sizeof(length)); // The length is stored in network byte order. length = ntohl(length); bytes += sizeof(length); diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m index cc1a650a..db5d3b60 100644 --- a/objectivec/GPBMessage.m +++ b/objectivec/GPBMessage.m @@ -32,6 +32,7 @@ #import <objc/runtime.h> #import <objc/message.h> +#import <stdatomic.h> #import "GPBArray_PackagePrivate.h" #import "GPBCodedInputStream_PackagePrivate.h" @@ -77,6 +78,20 @@ static NSString *const kGPBDataCoderKey = @"GPBData"; GPBMessage *autocreator_; GPBFieldDescriptor *autocreatorField_; GPBExtensionDescriptor *autocreatorExtension_; + + // A lock to provide mutual exclusion from internal data that can be modified + // by *read* operations such as getters (autocreation of message fields and + // message extensions, not setting of values). Used to guarantee thread safety + // for concurrent reads on the message. + // NOTE: OSSpinLock may seem like a good fit here but Apple engineers have + // pointed out that they are vulnerable to live locking on iOS in cases of + // priority inversion: + // http://mjtsai.com/blog/2015/12/16/osspinlock-is-unsafe/ + // https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000372.html + // Use of readOnlySemaphore_ must be prefaced by a call to + // GPBPrepareReadOnlySemaphore to ensure it has been created. This allows + // readOnlySemaphore_ to be only created when actually needed. + _Atomic(dispatch_semaphore_t) readOnlySemaphore_; } @end @@ -742,16 +757,22 @@ void GPBClearMessageAutocreator(GPBMessage *self) { void GPBPrepareReadOnlySemaphore(GPBMessage *self) { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdirect-ivar-access" -#pragma clang diagnostic ignored "-Wdeprecated-declarations" // Create the semaphore on demand (rather than init) as developers might not cause them // to be needed, and the heap usage can add up. The atomic swap is used to avoid needing // another lock around creating it. if (self->readOnlySemaphore_ == nil) { dispatch_semaphore_t worker = dispatch_semaphore_create(1); - if (!OSAtomicCompareAndSwapPtrBarrier(NULL, worker, (void * volatile *)&(self->readOnlySemaphore_))) { + dispatch_semaphore_t expected = nil; + if (!atomic_compare_exchange_strong(&self->readOnlySemaphore_, &expected, worker)) { dispatch_release(worker); } +#if defined(__clang_analyzer__) + // The Xcode 9.2 (and 9.3 beta) static analyzer thinks worker is leaked + // (doesn't seem to know about atomic_compare_exchange_strong); so just + // for the analyzer, let it think worker is also released in this case. + else { dispatch_release(worker); } +#endif } #pragma clang diagnostic pop @@ -3265,4 +3286,32 @@ id GPBGetMessageMapField(GPBMessage *self, GPBFieldDescriptor *field) { return GetOrCreateMapIvarWithField(self, field, syntax); } +id GPBGetObjectIvarWithField(GPBMessage *self, GPBFieldDescriptor *field) { + NSCAssert(!GPBFieldIsMapOrArray(field), @"Shouldn't get here"); + if (GPBGetHasIvarField(self, field)) { + uint8_t *storage = (uint8_t *)self->messageStorage_; + id *typePtr = (id *)&storage[field->description_->offset]; + return *typePtr; + } + // Not set... + + // Non messages (string/data), get their default. + if (!GPBFieldDataTypeIsMessage(field)) { + return field.defaultValue.valueMessage; + } + + GPBPrepareReadOnlySemaphore(self); + dispatch_semaphore_wait(self->readOnlySemaphore_, DISPATCH_TIME_FOREVER); + GPBMessage *result = GPBGetObjectIvarWithFieldNoAutocreate(self, field); + if (!result) { + // For non repeated messages, create the object, set it and return it. + // This object will not initially be visible via GPBGetHasIvar, so + // we save its creator so it can become visible if it's mutated later. + result = GPBCreateMessageWithAutocreator(field.msgClass, self, field); + GPBSetAutocreatedRetainedObjectIvarWithField(self, field, result); + } + dispatch_semaphore_signal(self->readOnlySemaphore_); + return result; +} + #pragma clang diagnostic pop diff --git a/objectivec/GPBMessage_PackagePrivate.h b/objectivec/GPBMessage_PackagePrivate.h index 90834d40..ca10983b 100644 --- a/objectivec/GPBMessage_PackagePrivate.h +++ b/objectivec/GPBMessage_PackagePrivate.h @@ -34,6 +34,10 @@ #import "GPBMessage.h" +// TODO: Remove this import. Older generated code use the OSAtomic* apis, +// so anyone that hasn't regenerated says building by having this. After +// enough time has passed, this likely can be removed as folks should have +// regenerated. #import <libkern/OSAtomic.h> #import "GPBBootstrap.h" @@ -57,20 +61,6 @@ typedef struct GPBMessage_Storage *GPBMessage_StoragePtr; // GPBMessage_Storage with _has_storage__ as the first field. // Kept public because static functions need to access it. GPBMessage_StoragePtr messageStorage_; - - // A lock to provide mutual exclusion from internal data that can be modified - // by *read* operations such as getters (autocreation of message fields and - // message extensions, not setting of values). Used to guarantee thread safety - // for concurrent reads on the message. - // NOTE: OSSpinLock may seem like a good fit here but Apple engineers have - // pointed out that they are vulnerable to live locking on iOS in cases of - // priority inversion: - // http://mjtsai.com/blog/2015/12/16/osspinlock-is-unsafe/ - // https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000372.html - // Use of readOnlySemaphore_ must be prefaced by a call to - // GPBPrepareReadOnlySemaphore to ensure it has been created. This allows - // readOnlySemaphore_ to be only created when actually needed. - dispatch_semaphore_t readOnlySemaphore_; } // Gets an extension value without autocreating the result if not found. (i.e. diff --git a/objectivec/GPBUtilities.m b/objectivec/GPBUtilities.m index 77ea9577..e2a12ca4 100644 --- a/objectivec/GPBUtilities.m +++ b/objectivec/GPBUtilities.m @@ -291,7 +291,7 @@ BOOL GPBGetHasIvar(GPBMessage *self, int32_t idx, uint32_t fieldNumber) { } else { NSCAssert(idx != GPBNoHasBit, @"Invalid has bit."); uint32_t byteIndex = idx / 32; - uint32_t bitMask = (1 << (idx % 32)); + uint32_t bitMask = (1U << (idx % 32)); BOOL hasIvar = (self->messageStorage_->_has_storage_[byteIndex] & bitMask) ? YES : NO; return hasIvar; @@ -315,7 +315,7 @@ void GPBSetHasIvar(GPBMessage *self, int32_t idx, uint32_t fieldNumber, NSCAssert(idx != GPBNoHasBit, @"Invalid has bit."); uint32_t *has_storage = self->messageStorage_->_has_storage_; uint32_t byte = idx / 32; - uint32_t bitMask = (1 << (idx % 32)); + uint32_t bitMask = (1U << (idx % 32)); if (value) { has_storage[byte] |= bitMask; } else { @@ -628,34 +628,6 @@ id GPBGetObjectIvarWithFieldNoAutocreate(GPBMessage *self, return *typePtr; } -id GPBGetObjectIvarWithField(GPBMessage *self, GPBFieldDescriptor *field) { - NSCAssert(!GPBFieldIsMapOrArray(field), @"Shouldn't get here"); - if (GPBGetHasIvarField(self, field)) { - uint8_t *storage = (uint8_t *)self->messageStorage_; - id *typePtr = (id *)&storage[field->description_->offset]; - return *typePtr; - } - // Not set... - - // Non messages (string/data), get their default. - if (!GPBFieldDataTypeIsMessage(field)) { - return field.defaultValue.valueMessage; - } - - GPBPrepareReadOnlySemaphore(self); - dispatch_semaphore_wait(self->readOnlySemaphore_, DISPATCH_TIME_FOREVER); - GPBMessage *result = GPBGetObjectIvarWithFieldNoAutocreate(self, field); - if (!result) { - // For non repeated messages, create the object, set it and return it. - // This object will not initially be visible via GPBGetHasIvar, so - // we save its creator so it can become visible if it's mutated later. - result = GPBCreateMessageWithAutocreator(field.msgClass, self, field); - GPBSetAutocreatedRetainedObjectIvarWithField(self, field, result); - } - dispatch_semaphore_signal(self->readOnlySemaphore_); - return result; -} - // Only exists for public api, no core code should use this. int32_t GPBGetMessageEnumField(GPBMessage *self, GPBFieldDescriptor *field) { GPBFileSyntax syntax = [self descriptor].file.syntax; diff --git a/objectivec/GPBUtilities_PackagePrivate.h b/objectivec/GPBUtilities_PackagePrivate.h index c8b21ed7..ed424ce3 100644 --- a/objectivec/GPBUtilities_PackagePrivate.h +++ b/objectivec/GPBUtilities_PackagePrivate.h @@ -124,7 +124,7 @@ GPB_INLINE int64_t GPBDecodeZigZag64(uint64_t n) { // thus always taking 10 bytes on the wire.) GPB_INLINE uint32_t GPBEncodeZigZag32(int32_t n) { // Note: the right-shift must be arithmetic - return (uint32_t)((n << 1) ^ (n >> 31)); + return ((uint32_t)n << 1) ^ (uint32_t)(n >> 31); } // Encode a ZigZag-encoded 64-bit value. ZigZag encodes signed integers @@ -133,7 +133,7 @@ GPB_INLINE uint32_t GPBEncodeZigZag32(int32_t n) { // thus always taking 10 bytes on the wire.) GPB_INLINE uint64_t GPBEncodeZigZag64(int64_t n) { // Note: the right-shift must be arithmetic - return (uint64_t)((n << 1) ^ (n >> 63)); + return ((uint64_t)n << 1) ^ (uint64_t)(n >> 63); } #pragma clang diagnostic push diff --git a/objectivec/README.md b/objectivec/README.md index 7226f0b9..469a61fb 100644 --- a/objectivec/README.md +++ b/objectivec/README.md @@ -33,19 +33,21 @@ Building There are two ways to include the Runtime sources in your project: -Add `objectivec/\*.h` & `objectivec/GPBProtocolBuffers.m` to your project. +Add `objectivec/*.h`, `objectivec/google/protobuf/*.pbobjc.h`, and +`objectivec/GPBProtocolBuffers.m` to your project. *or* -Add `objectivec/\*.h` & `objectivec/\*.m` except for +Add `objectivec/*.h`, `objectivec/google/protobuf/*.pbobjc.h`, +`objectivec/google/protobuf/*.pbobjc.m`, and `objectivec/*.m` except for `objectivec/GPBProtocolBuffers.m` to your project. If the target is using ARC, remember to turn off ARC (`-fno-objc-arc`) for the `.m` files. -The files generated by `protoc` for the `*.proto` files (`\*.pbobjc.h` and -`\*.pbobjc.m`) are then also added to the target. +The files generated by `protoc` for the `*.proto` files (`*.pbobjc.h` and +`*.pbobjc.m`) are then also added to the target. Usage ----- diff --git a/objectivec/Tests/GPBCodedInputStreamTests.m b/objectivec/Tests/GPBCodedInputStreamTests.m index 11f7ceaf..f5aa6903 100644 --- a/objectivec/Tests/GPBCodedInputStreamTests.m +++ b/objectivec/Tests/GPBCodedInputStreamTests.m @@ -220,7 +220,7 @@ 0xa6, 0x01) value:(0x1b << 0) | (0x28 << 7) | (0x79 << 14) | (0x42 << 21) | (0x3bLL << 28) | (0x56LL << 35) | (0x00LL << 42) | - (0x05LL << 49) | (0x26LL << 56) | (0x01LL << 63)]; + (0x05LL << 49) | (0x26LL << 56) | (0x01ULL << 63)]; // Failures [self assertReadVarintFailure:bytes(0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, diff --git a/objectivec/Tests/GPBCodedOuputStreamTests.m b/objectivec/Tests/GPBCodedOuputStreamTests.m index 2ad326be..878e7aa9 100644 --- a/objectivec/Tests/GPBCodedOuputStreamTests.m +++ b/objectivec/Tests/GPBCodedOuputStreamTests.m @@ -266,7 +266,7 @@ value:(0x1b << 0) | (0x28 << 7) | (0x79 << 14) | (0x42 << 21) | (0x3bLL << 28) | (0x56LL << 35) | (0x00LL << 42) | (0x05LL << 49) | (0x26LL << 56) | - (0x01LL << 63)]; + (0x01ULL << 63)]; } - (void)testWriteLittleEndian { diff --git a/objectivec/Tests/GPBUtilitiesTests.m b/objectivec/Tests/GPBUtilitiesTests.m index 2e206a54..8a8ba93e 100644 --- a/objectivec/Tests/GPBUtilitiesTests.m +++ b/objectivec/Tests/GPBUtilitiesTests.m @@ -52,12 +52,12 @@ - (void)testRightShiftFunctions { XCTAssertEqual((1UL << 31) >> 31, 1UL); - XCTAssertEqual((1 << 31) >> 31, -1); + XCTAssertEqual((int32_t)(1U << 31) >> 31, -1); XCTAssertEqual((1ULL << 63) >> 63, 1ULL); - XCTAssertEqual((1LL << 63) >> 63, -1LL); + XCTAssertEqual((int64_t)(1ULL << 63) >> 63, -1LL); - XCTAssertEqual(GPBLogicalRightShift32((1 << 31), 31), 1); - XCTAssertEqual(GPBLogicalRightShift64((1LL << 63), 63), 1LL); + XCTAssertEqual(GPBLogicalRightShift32((1U << 31), 31), 1); + XCTAssertEqual(GPBLogicalRightShift64((1ULL << 63), 63), 1LL); } - (void)testGPBDecodeTextFormatName { diff --git a/objectivec/google/protobuf/Struct.pbobjc.m b/objectivec/google/protobuf/Struct.pbobjc.m index f36ec582..816fd6df 100644 --- a/objectivec/google/protobuf/Struct.pbobjc.m +++ b/objectivec/google/protobuf/Struct.pbobjc.m @@ -13,6 +13,8 @@ #import "GPBProtocolBuffers_RuntimeSupport.h" #endif +#import <stdatomic.h> + #if GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS #import <Protobuf/Struct.pbobjc.h> #else @@ -51,7 +53,7 @@ static GPBFileDescriptor *GPBStructRoot_FileDescriptor(void) { #pragma mark - Enum GPBNullValue GPBEnumDescriptor *GPBNullValue_EnumDescriptor(void) { - static GPBEnumDescriptor *descriptor = NULL; + static _Atomic(GPBEnumDescriptor*) descriptor = nil; if (!descriptor) { static const char *valueNames = "NullValue\000"; @@ -64,7 +66,8 @@ GPBEnumDescriptor *GPBNullValue_EnumDescriptor(void) { values:values count:(uint32_t)(sizeof(values) / sizeof(int32_t)) enumVerifier:GPBNullValue_IsValidValue]; - if (!OSAtomicCompareAndSwapPtrBarrier(nil, worker, (void * volatile *)&descriptor)) { + GPBEnumDescriptor *expected = nil; + if (!atomic_compare_exchange_strong(&descriptor, &expected, worker)) { [worker release]; } } diff --git a/objectivec/google/protobuf/Type.pbobjc.m b/objectivec/google/protobuf/Type.pbobjc.m index 7a949388..bb64d876 100644 --- a/objectivec/google/protobuf/Type.pbobjc.m +++ b/objectivec/google/protobuf/Type.pbobjc.m @@ -13,6 +13,8 @@ #import "GPBProtocolBuffers_RuntimeSupport.h" #endif +#import <stdatomic.h> + #if GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS #import <Protobuf/Type.pbobjc.h> #import <Protobuf/Any.pbobjc.h> @@ -54,7 +56,7 @@ static GPBFileDescriptor *GPBTypeRoot_FileDescriptor(void) { #pragma mark - Enum GPBSyntax GPBEnumDescriptor *GPBSyntax_EnumDescriptor(void) { - static GPBEnumDescriptor *descriptor = NULL; + static _Atomic(GPBEnumDescriptor*) descriptor = nil; if (!descriptor) { static const char *valueNames = "SyntaxProto2\000SyntaxProto3\000"; @@ -68,7 +70,8 @@ GPBEnumDescriptor *GPBSyntax_EnumDescriptor(void) { values:values count:(uint32_t)(sizeof(values) / sizeof(int32_t)) enumVerifier:GPBSyntax_IsValidValue]; - if (!OSAtomicCompareAndSwapPtrBarrier(nil, worker, (void * volatile *)&descriptor)) { + GPBEnumDescriptor *expected = nil; + if (!atomic_compare_exchange_strong(&descriptor, &expected, worker)) { [worker release]; } } @@ -368,7 +371,7 @@ void SetGPBField_Cardinality_RawValue(GPBField *message, int32_t value) { #pragma mark - Enum GPBField_Kind GPBEnumDescriptor *GPBField_Kind_EnumDescriptor(void) { - static GPBEnumDescriptor *descriptor = NULL; + static _Atomic(GPBEnumDescriptor*) descriptor = nil; if (!descriptor) { static const char *valueNames = "TypeUnknown\000TypeDouble\000TypeFloat\000TypeInt" @@ -404,7 +407,8 @@ GPBEnumDescriptor *GPBField_Kind_EnumDescriptor(void) { values:values count:(uint32_t)(sizeof(values) / sizeof(int32_t)) enumVerifier:GPBField_Kind_IsValidValue]; - if (!OSAtomicCompareAndSwapPtrBarrier(nil, worker, (void * volatile *)&descriptor)) { + GPBEnumDescriptor *expected = nil; + if (!atomic_compare_exchange_strong(&descriptor, &expected, worker)) { [worker release]; } } @@ -441,7 +445,7 @@ BOOL GPBField_Kind_IsValidValue(int32_t value__) { #pragma mark - Enum GPBField_Cardinality GPBEnumDescriptor *GPBField_Cardinality_EnumDescriptor(void) { - static GPBEnumDescriptor *descriptor = NULL; + static _Atomic(GPBEnumDescriptor*) descriptor = nil; if (!descriptor) { static const char *valueNames = "CardinalityUnknown\000CardinalityOptional\000C" @@ -458,7 +462,8 @@ GPBEnumDescriptor *GPBField_Cardinality_EnumDescriptor(void) { values:values count:(uint32_t)(sizeof(values) / sizeof(int32_t)) enumVerifier:GPBField_Cardinality_IsValidValue]; - if (!OSAtomicCompareAndSwapPtrBarrier(nil, worker, (void * volatile *)&descriptor)) { + GPBEnumDescriptor *expected = nil; + if (!atomic_compare_exchange_strong(&descriptor, &expected, worker)) { [worker release]; } } |