This is an archive of the discontinued LLVM Phabricator instance.

[ObjC] Error out when using forward-declared protocol in a @protocol expression
ClosedPublic

Authored by arphaman on Jul 17 2018, 5:51 PM.

Details

Summary

Clang emits invalid protocol metadata when a @protocol expression is used with a forward-declared protocol. The protocol metadata is missing protocol conformance list of the protocol since we don't have access to the definition of it in the compiled translation unit. The linker then might end up picking the invalid metadata when linking which will lead to incorrect runtime protocol conformance checks.

This patch makes sure that Clang fails to compile code that uses a @protocol expression with a forward-declared protocol. This ensures that Clang does not emit invalid protocol metadata. I added an extra assert in CodeGen to ensure that this kind of issue won't happen in other places.

Diff Detail

Repository
rC Clang

Event Timeline

arphaman created this revision.Jul 17 2018, 5:51 PM

Hmm. I think this is a reasonable change to make to the language. Have you investigated to see if this causes source-compatibility problems?

include/clang/Basic/DiagnosticSemaKinds.td
871

I think that's the only warning in -Wat-protocol; we can at least anonymize it and leave a comment saying it's now empty.

lib/Sema/SemaExprObjC.cpp
1234–1235

Please brace the second clause here. I think even the people who don't generally use braces still advise being consistent within a single if-else chain.

test/CodeGenObjC/forward-declare-protocol-gnu.m
6–11

Does this really not require a definition of P? Ugh. I wonder if that's reasonable to fix, too.

test/Parser/objc-cxx-keyword-identifiers.mm
22

Why did this test need to change?

arphaman updated this revision to Diff 161142.Aug 16 2018, 4:30 PM
arphaman marked 3 inline comments as done.

address review comments.

Sorry for the delay.

Hmm. I think this is a reasonable change to make to the language. Have you investigated to see if this causes source-compatibility problems?

Yes, I tested this change on internal code base. There's an impact, but the impact is fairly minimal. The biggest issue is actually in some Swift overlay: https://github.com/apple/swift/blob/c275826a41aca8268719061636efc12717b8dae1/stdlib/public/SDK/Dispatch/Dispatch.mm#L36

test/CodeGenObjC/forward-declare-protocol-gnu.m
6–11

Nope, we don't emit the protocol metadata for it. It might make sense to require the definition with the implementation?

test/Parser/objc-cxx-keyword-identifiers.mm
22

We need to declare delete because it's used in a @protocol expression on line 63.

rjmccall added inline comments.Aug 16 2018, 11:05 PM
test/CodeGenObjC/forward-declare-protocol-gnu.m
6–11

Yeah, I think so. I would argue that there no places where we should be emitting metadata for an incomplete protocol.

test/Parser/objc-cxx-keyword-identifiers.mm
22

Ah, gotcha.

arphaman added inline comments.Aug 17 2018, 11:19 AM
test/CodeGenObjC/forward-declare-protocol-gnu.m
6–11

Ok, makes sense.
I will fix it in a follow-up patch then. I don't want to block this change as this patch fixes some nasty runtime issues. I will run a separate source compatibility assessment for the follow-up.

rjmccall added inline comments.Aug 17 2018, 12:22 PM
lib/CodeGen/CGObjCMac.cpp
6788

What happens in the @implementation case (the one that we're not diagnosing yet) when the protocol is a forward declaration?

arphaman added inline comments.Aug 17 2018, 12:52 PM
lib/CodeGen/CGObjCMac.cpp
6788

We emit an external global reference to the protocol metadata using GetOrEmitProtocolRef, so this assertion won't be triggered until we force the emission of protocol metadata from implementation as planned in a follow-up patch.

rjmccall accepted this revision.Aug 17 2018, 12:59 PM

LGTM.

lib/CodeGen/CGObjCMac.cpp
6788

Okay. I mean, that's also unfortunate behavior, since protocol descriptors are basically linkonce and should be emitted in every translation unit that uses them, but I agree it's less damaging than the behavior for @protocol, and it means this assertion is safe.

This revision is now accepted and ready to land.Aug 17 2018, 12:59 PM
This revision was automatically updated to reflect the committed changes.