Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialization bug on 64 bit architecture for "repeated uint32" fields #8

Closed
daj opened this issue Jun 5, 2014 · 8 comments
Closed

Comments

@daj
Copy link

daj commented Jun 5, 2014

We've written an automated test (included below) that you can use to reproduce the problem. Please let us know if we've made a mistake with how we're using the field!

In the short term, the workaround I'm planning is to change to "repeated uint64" fields, which work fine.

Here's an example "test.proto" file:

package test;

message Example
{
    repeated uint32 myfield = 1;
}

Here's the test that reproduces the problem:

#import <XCTest/XCTest.h>

#import "Test.pb.h"

@interface ProtobufTests : XCTestCase

@end

@implementation ProtobufTests

- (void)testSerializationAndDeserialization
{
    NSArray *testValuesAsNumbers = @[[NSNumber numberWithUnsignedInteger:26234], [NSNumber numberWithUnsignedInteger:26235]];

    ExampleBuilder *builder = [ExampleBuilder new];
    [builder setMyfieldArray:testValuesAsNumbers];

    Example *pbMessage = [builder build];

    for (int i = 0; i < testValuesAsNumbers.count; i++) {
        XCTAssertEqual([pbMessage.myfield uint32AtIndex:i], [testValuesAsNumbers[i] unsignedIntegerValue],
                       @"Repeated primitive value at index %d is not the value it was set to", i);
    }

    NSData *serializedProtobuf = [pbMessage data];

    Example *unserializedMessage = [Example parseFromData:serializedProtobuf];

    for (int i = 0; i < testValuesAsNumbers.count; i++) {
        XCTAssertEqual([unserializedMessage.myfield uint32AtIndex:i], [testValuesAsNumbers[i] unsignedIntegerValue],
                       @"After serializing/deserializing, repeated primitive value at index %d is not the value it was set to", i);
    }
}

@end

The test passes fine when run with a 32 bit target (e.g. "iPhone Retina (4-inch)"), but fails when run on a 64 bit target (e.g. "iPhone Retina (4-inch 64-bit)") the final line with this error message:

error: -[ProtobufTests testSerializationAndDeserialization] : (([unserializedMessage.myfield uint32AtIndex:i]) equal to ([testValuesAsNumbers[i] unsignedIntegerValue])) failed: ("278373775") is not equal to ("26235") - After serializing/deserializing, repeated primitive value at index 1 is not the value it was set to
@alexeyxo
Copy link
Owner

alexeyxo commented Jun 5, 2014

Thank You. I see it today or tomorrow.

@AtomicCactus
Copy link

We've ran into a similar issue when running on 64-bit architectures. Sorry, I don't have any test code to reproduce, but we have fixed the issue by "merging" in some of the code from Synapse's fork:

https://gitlab.synapse.com/rachelbl/protobuf-objc/tree/master

Specifically, long and long long (as well as their unsigned brethren) are used throughout the code. We maneuvered around the issue by doing the following replacements:

long replaced with int32_t
unsigned long replaced with uint32_t
long long replaced with int64_t
unsigned long long replaced with uint64_t

That fixed the problem for us. Hope that helps! Let me know if you'd like more details.

@AtomicCactus
Copy link

Some more information. Take a look at PBArray.m and you will see this:

static PBArrayValueTypeInfo PBValueTypes[] =
{
    { sizeof(id),           NULL},
    { sizeof(BOOL),         PBArraySetBoolValue },
    { sizeof(long),         PBArraySetInt32Value    },
    { sizeof(unsigned long),    PBArraySetUInt32Value   },
    { sizeof(long long),        PBArraySetInt64Value    },
    { sizeof(unsigned long long),   PBArraySetUInt64Value   },
    { sizeof(Float32),      PBArraySetFloatValue    },
    { sizeof(Float64),      PBArraySetDoubleValue   },
};

On 32-bit and 64-bit architectures, the values of long and unsigned long are different. You can test this by running this code on both 32 and 64 bit simulators:

- (void)testPrimitiveSizes {
    NSLog(@"sizeof(id): %lu", sizeof(id));
    NSLog(@"sizeof(BOOL): %lu", sizeof(BOOL));
    NSLog(@"sizeof(long): %lu", sizeof(long));
    NSLog(@"sizeof(unsigned long): %lu", sizeof(unsigned long));
    NSLog(@"sizeof(long long): %lu", sizeof(long long));
    NSLog(@"sizeof(unsigned long long): %lu", sizeof(unsigned long long));
    NSLog(@"sizeof(Float32): %lu", sizeof(Float32));
    NSLog(@"sizeof(Float64): %lu", sizeof(Float64));
}

Output on 32 bit looks like this:

sizeof(id): 4
sizeof(BOOL): 1
sizeof(long): 4
sizeof(unsigned long): 4
sizeof(long long): 8
sizeof(unsigned long long): 8
sizeof(Float32): 4
sizeof(Float64): 8

And on 64 bit output looks like this:

sizeof(id): 8
sizeof(BOOL): 1
sizeof(long): 8
sizeof(unsigned long): 8
sizeof(long long): 8
sizeof(unsigned long long): 8
sizeof(Float32): 4
sizeof(Float64): 8

You can see now that this becomes a problem because the PBArrayValueTypeSize(type) and PBArrayValueTypeSetter(type) functions won't work right.

The fix is to use the explicit integer types in place of longs everywhere in the code, because this error isn't limited to the PBArray class.

@alexeyxo
Copy link
Owner

alexeyxo commented Jun 5, 2014

No, this test all okay
In the 64-bit runtime, the alignment of all 64-bit integer types changes from 4 bytes to 8 bytes.

32 bit:
long = 4

64 bit:
long = 8

This is my example.


message Person {
 repeated uint32 myfield = 1;
}

   PersonBuilder *builder = [Person builder];   

  NSArray *arr = @[[NSNumber numberWithUnsignedInteger:23],[NSNumber numberWithUnsignedInteger:25]];

    [builder setMyfieldArray:arr];

    Person *person = [builder build];

    NSAssert(([person.myfield uint32AtIndex:0] == arr[0]), @"NOT EQUALS");

    NSLog(@"%lu",[person.myfield uint32AtIndex:0]);

    Person *person2 = [Person parseFromData:person.data];

    NSLog(@"%lu",[person2.myfield uint32AtIndex:1]);



    if ([person.myfield uint32AtIndex:1] == [person2.myfield uint32AtIndex:1]) {

        NSLog(@"OK");

    }

Log for 32 and 64 bit system(iPhone5S, iPhone Simulator Retina (4-inch 64-bit),iPhone Retina (4-inch 32-bit)):

2014-06-06 03:31:02.282 Proto[4464:60b] 23
2014-06-06 03:31:06.045 Proto[4464:60b] 25
2014-06-06 03:31:08.338 Proto[4464:60b] OK

I can not repeat the problem.
Any ideas?

@daj
Copy link
Author

daj commented Jun 6, 2014

Damn, I suspect this is because I am running a slightly older release. The last commit I have from you is Apr 19th 2014 (link).

I was a bit scared to update to your latest code, because a colleague of mine had pulled it, and he said there were compile errors. He plans to retry tomorrow and update this existing issue with the information: #4

@daj
Copy link
Author

daj commented Jun 6, 2014

I confirmed that your test exhibits the broken behavior on my system with a "iPhone Retina (4-inch 64-bit)" target:

2014-06-05 17:33:47.729 Test[44188:60b] 23
2014-06-05 17:33:47.731 Test[44188:60b] 285174134

This probably confirms that the issue is the version of protobuf-objc that I'm using.

BTW, I had a warning on your NSAssert so I commented it out (I'm on Xcode 5.1.1).

alexeyxo added a commit that referenced this issue Jun 6, 2014
@alexeyxo
Copy link
Owner

alexeyxo commented Jun 6, 2014

oups...
At first(was 3am):

NSAssert(([person.myfield uint32AtIndex:0] == [arr[0] unsignedIntegerValue]), @"NOT EQUALS");

And now, when this task created(#4) i am using two branches (master and arm64beta). Arm64beta branch contained fixes for arm64 architecture. One month ago I am merged two branches in master. You need to update your protobuf compiller:

git clone git@github.com:alexeyxo/protobuf-objc.git
./build.sh

and update runtime library

/src/runtime/ProtocolBuffers.xcodeproj

If the problem persists after the update let me know. we will continue to deal. I use this version every day at work and at home, on all possible devices and I haven't any problems.

@adamawolf
Copy link

Hi @alexeyxo, I'm the aforementioned colleague of @daj

I can confirm that the 64-bit issues we were seeing are resolved in the latest from the master branch.

As @daj mentioned, there were other problems I ran in to when trying the latest code which caused me to balk at using it. I've now worked around both of them. All of our tests are passing. I've opened two new issues to track these problems:

#9
#10

I believe this issue can be closed. Thanks!

@daj daj closed this as completed Jun 6, 2014
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

No branches or pull requests

4 participants