Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

ObjCGNU: Fix empty v3 protocols being emitted two fields short

Authored by DHowett-MSFT on Apr 4 2018, 5:23 PM.



Protocols that were being referenced but could not be fully realized were being emitted without properties/optional_properties. Since all v3 protocols must be 9 processor words wide, the lack of these fields is catastrophic for the runtime.

As an example, the runtime cannot know here that properties and optional_properties are invalid.

Diff Detail

rC Clang

Event Timeline

DHowett-MSFT created this revision.Apr 4 2018, 5:23 PM

Looks correct to me. Protocols are only referenced by pointer, so it doesn't matter for old versions of the runtime if these fields are present.

theraven accepted this revision.Apr 5 2018, 1:49 AM
This revision is now accepted and ready to land.Apr 5 2018, 1:49 AM

The change LGTM, but please add a test.

Added a test per @rjmccall's suggestion. I chose to test this by emitting assembly because the llvm ir is significantly harder to pick apart for the true _size_ of the protocol.

rjmccall accepted this revision.Apr 7 2018, 10:40 AM

Hmm. Alright, I guess.

Isn't it better to test for the correct structure existing in the IR?

It seems more fragile to check the contents of the protocol rather than the invariant we care most about (its size).
I'm willing to do that, and am updating the patch accordingly, but I don't want to commit to a more fragile test than is necessary.
Illustratively, if the GNUstep runtime one day chooses to emit empty v3 protocols as {3, null, null, null, null, null, null, null} (which it may want to do to avoid emitting empty method lists, and which seems to work at runtime), a test capturing the body of the protocol will fail.

I think that we emit empty method lists so that the GCC runtime doesn't choke on them, though there's no reason why we couldn't with the GNUstep ABI. It would therefore be nice if the test failed if we did change to emitting null so that we could update the test at that point and check that it's actually doing the right thing.

The ELF version of the v2 ABI is mostly finished now, modulo writing a load of clang tests, so I'll upload that for review soon.

It would also help to have a gnu- prefix on the test name so that it's easy to identify the relevant tests, though not essential.

DHowett-MSFT edited the summary of this revision. (Show Details)

I've fixed the test to check the LLVM IR; I chose to use CHECK-SAME and use indentation to clarify what was going on. We're now checking that an empty protocol is emitted in full, and that all of its members match our expectations.

Hi! Is there anything else holding up this patch? Thanks!

This revision was automatically updated to reflect the committed changes.