Add support for atomic type qualifiers.

[triplef]

Fixes #101.

[davidchisnall]

Looks good (no idea why Linux CI is unhappy - it appears to be an issue with the Azure hosted Linux agents). Please can you add a test?

[triplef]

I’ve added a test for atomic ivars (passing), and also one for properties with atomic types which is currently failing like this:

Assertion failed: (attrsCount == size), function testPropertyForProperty_alt, file PropertyIntrospectionTest2_arc.m, line 295.

It seems to me that property_copyAttributeList() requires modifications to support atomic types, but I can’t quite figure how it’s supposed to work (e.g. why does this loop start at strlen(types)+1?).

property_getAttributes() returns "TAB" for the "atomicBoolDefault" property I added, but property_copyAttributeList() returns two properties "T":"B", and "D":"" which seems incorrect.

I’d appreciate your help to fix this.

[davidchisnall]

That really needs a comment (and not the one that's there already, because that stopped being true as part of the 2.0 ABI refactoring). The property encoding string is T{Objective-C type encoding of the property value}{other stuff}. The strlen(types)+1 is skipping the T and the encoding of the type.

With the 2.0 (and 1.8ish or later version of the 1.x) ABI, the full attribute encoding is generated by the compiler, so this should be tproperty_getAttributesreated as canonical. The "TAB" comes from there (T[ype] is A[tomic] B[oolean]). I'd expect property_copyAttributeList for this to just return { "T" : "AB" }.

I suspect the problem is that, for atomic types, the type of the property is _Atomic(T), the type of the accessor is T and is therefore one character smaller. I think this may be a clang bug, because the types field isn't intended to be the type of the accessor. We can work around this in the runtime by seeing if the character at strlen(types)+1 is a comma and, if not, fix up types by copying the substring from the attributes string.

The "D" value feels like it's a different bug - can to single-step through that function and see where count is being incremented to give us the nonsense value?

[triplef]

I think I found the issue in property_copyAttributeList: the default case in the switch statement was (I think) incorrectly incrementing the counter in addition to it being incremented by the for loop. This caused it to skip over the string end in this case where types = "B" and attributes = "TAB", as the start of the loop was not the end of the attributes string but its last character.

However property_copyAttributeList will still return {"T":"B"} instead of {"T":"AB"}, I think due to A being skipped by objc_skip_type_qualifiers. I can’t say if this is correct/expected behavior.

[triplef]

Hm the PropertyIntrospectionTest2_arc test passes for me on macOS using clang from the latest Xcode 10.2. Any idea why it might fail on the CI?

[davidchisnall]

The test also fails for me locally. It looks as if it's because you fixed the bug and not the test (maybe you didn't commit the test?). The test is looking for { "T" : "B" } but it's finding { "T" : "AB"} (which is correct).

[triplef]

I’ve updated the test to expect { "T" : "AB" } in both cases, although this is not what it finds for me locally (it finds { "T" : "B" }). I’m curious though if this will make them pass on the CI.

[triplef]

Looks like only the legacy variants of the test failed. Is this expected and what would be a way to handle that?

[davidchisnall]

You can use #if __OBJC_GNUSTEP_RUNTIME_ABI__ >= 20 in the tests to check for the new ABI. It looks as if there's enough metadata generated by clang for this to work though, so I'd take a look at what's going on in legacy.c to upgrade property structures to the new ABI to see if there's a bug there. It looks as if it's getting the type from the getter and not from the attribute string.

[triplef]

Thanks, I’ve disabled the tests on legacy ABIs and the tests are passing on the CI (and also on my machine now, I guess I was using the old ABI).

TBH I’m not sure I have the context to really dig into this further, but please let me know if there are any further changes you’d like to see.

[triplef]

If it makes sense I could also do the following:

• Reduce this PR to just fix atomic ivars (i.e. this change in objc_skip_type_qualifiers + the test)
• Optionally create a new draft PR for the other changes for atomic property and method parameter support so it can be looked at in more detail

This should unblock the fix for the probably more common use case of atomic ivars (which is also what we are running into) from the changes for atomic types in properties and method signatures (which I’m guessing are very rarely used).

[davidchisnall]

I'd like to fix the legacy ABI issue as well. I'll try to take a look in the next day or two. I suspect that there's an almost identical bug to the one that you've fixed in the legacy upgrade path.

[triplef]

Ok great, thanks!

[davidchisnall]

I've merged this and fixed the legacy path. The test now passes with both the old and new ABIs. See: 375018a933d197d12bed7b937480cf912280fd13

[triplef]

Great, thank you David!