Page MenuHomePhabricator

[objc_direct] Tigthen checks for direct methods
ClosedPublic

Authored by MadCoder on Dec 19 2019, 2:00 AM.

Details

Summary

Because the name of a direct method must be agreed upon by the caller and the implementation, certain bad practices that one can get away with when using dynamism are fatal with direct methods.

To avoid really weird and unscruttable linker error, tighten the front-end error reporting.

Rule 1:

Direct methods can only have at most one declaration in an @interface container. Any redeclaration is strictly forbidden.
Today some amount of redeclaration is tolerated between the main interface and categories for dynamic methods, but we can't have that.

Rule 2:

Direct method implementations can only be declared in a matching @interface container: when implemented in the primary @implementation then the declaration must be in the primary @interface or an extension, and when implemented in a category, the declaration must be in the @interface for the same category.

Also fix another issue with ObjCMethod::getCanonicalDecl(): when an implementation lives in the primary @interface, then its canonical declaration can be in any extension, even when it's not an accessor.

Add Sema tests to cover the new errors, and CG tests to beef up testing around function names for categories and extensions.

Radar-Id: rdar://problem/58054563

Diff Detail

Event Timeline

MadCoder created this revision.Dec 19 2019, 2:00 AM
MadCoder updated this revision to Diff 234684.Dec 19 2019, 2:34 AM

Small update because I uploaded before the full completion of the test-suite and somehow it's sentient and punished me... (some diagnostics were too verbose and stuttering things with Sema::CheckObjCMethodOverrides).

MadCoder added inline comments.
clang/test/CodeGenObjC/direct-method.m
196

this may be questionable as a chosen mangling. it's exactly what dynamic dispatch does, but because a direct method cannot have clashes on the same class, this extra namespacing is not useful and is likely making the life of debuggers harder for expression evaluation.

@aprantl should I generate @"\01-[Foo directMethodInCategory]" instead? if yes I'd rather do it in this review

Added superficial LLVM coding style comments.

clang/lib/Sema/SemaDeclObjC.cpp
4735

missing . at end of sentence.

"Upfront" as opposed to? It would be helpful to add a ", because ..." here

4759

// If

Is there a better way to express Class (not super)?

4782

how about factoring this into a diagContainerMismatch lambda?

4841

.

4857

full sentence please

aprantl added inline comments.Dec 19 2019, 12:40 PM
clang/lib/AST/DeclObjC.cpp
963–969

What does "principal" mean here?

966

Does principal mean the ObjCInterfaceDecl of ImplD? Should we say that instead?

MadCoder marked an inline comment as done.Dec 19 2019, 12:53 PM
MadCoder added inline comments.
clang/lib/AST/DeclObjC.cpp
963–969

primary sorry, will fix

MadCoder updated this revision to Diff 234777.Dec 19 2019, 1:27 PM
MadCoder marked 7 inline comments as done.

Addressed @aprantl review feedback.

MadCoder updated this revision to Diff 234778.Dec 19 2019, 1:30 PM

I had forgotten to clang format and left some tabs here... some day I'll fix my vim config

aprantl added inline comments.
clang/test/CodeGenObjC/direct-method.m
196

@teemperor implemented the LLDB support. I would assume that changing it to Foo without the category will be necessary for LLDB to find it. @teemperor Can you add an LLDB test for this scenario?

MadCoder marked an inline comment as done.Dec 19 2019, 2:06 PM
MadCoder added inline comments.
clang/test/CodeGenObjC/direct-method.m
196

that was my expectation as well, let me update the code to do so then :)

MadCoder updated this revision to Diff 234810.Dec 19 2019, 5:03 PM

Removed the category from the direct method name mangling as discussed with @aprantl

MadCoder marked 2 inline comments as done.Dec 19 2019, 5:03 PM
teemperor added inline comments.Dec 20 2019, 2:09 AM
clang/test/CodeGenObjC/direct-method.m
196
aprantl accepted this revision.Dec 20 2019, 8:17 AM
This revision is now accepted and ready to land.Dec 20 2019, 8:17 AM
This revision was automatically updated to reflect the committed changes.