Page MenuHomePhabricator

[clang-tidy] Implement a check for large Objective-C type encodings 🔍
Changes PlannedPublic

Authored by stephanemoore on Dec 12 2018, 7:25 PM.

Details

Summary

Objective-C type encodings are normally pretty small but they it is
pretty easy for them to bloat to undesirable levels. Bloated Objective-C
type encodings are particularly common for Objective-C methods with
templated C++ types in their interface in Objective-C++. For example,
in Objective-C type encodings std::string expands to 277 bytes and
std::map<std::string, std::string> expands to 1219 bytes. The bloat
isn't particularly important for larger binaries but SDKs sometimes
optimize their binary size to ease adoption. This check aims to provide
some level of visibility into the size of generated Objective-C type
encodings so that developers can address them if they want.

Related article:
https://medium.com/@dmaclach/objective-c-encoding-and-you-866624cc02de

Test Notes:
Verified clang-tidy tests pass successfully.

Diff Detail

Event Timeline

stephanemoore created this revision.Dec 12 2018, 7:25 PM

I wonder if we want to have an option to elide ObjC type info for all non-POD C++ types. Nothing that you do with the type encoding is likely to be correct (for example, you can see the pointer field in a std::shared_ptr, but you can't see that changes to it need to update reference counts) so it probably does more harm than good.

Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added inline comments.
docs/clang-tidy/checks/objc-type-encoding-size.rst
7

Please synchronize with Release Notes.

Update check description to match release notes.

dmaclach added inline comments.
clang-tidy/objc/TypeEncodingSizeCheck.cpp
54

I found it very useful in my diagnostics to know how big it actually was. Can you add that?

102

Can you add what the actual size was in the output?

hokein added inline comments.
clang-tidy/objc/TypeEncodingSizeCheck.cpp
37

hasAncestor is an expensive matcher, does hasDeclContext meet your use cases?

61

nit: add an assertion assert(EncodedDecl).

64

Do you forget to register the matcher for ObjCIvarDecl? In the matcher you register it for ObjCPropertyDecl, and ObjCInterfaceDecl, so this branch will never be executed.

docs/clang-tidy/checks/objc-type-encoding-size.rst
14

nit: mention the default value.

stephanemoore marked 6 inline comments as done.

Changes:
• Assert on EncodedDecl.
• Mention default value in objc-type-encoding-size check notes.

Outstanding action items:
• Evaluate using hasDeclContext instead of hasAncestor.
• Include type encoding size information in diagnostic messages.

I wonder if we want to have an option to elide ObjC type info for all non-POD C++ types. Nothing that you do with the type encoding is likely to be correct (for example, you can see the pointer field in a std::shared_ptr, but you can't see that changes to it need to update reference counts) so it probably does more harm than good.

I think it's worth investigating some kind of option for eliding Objective-C type information though I think that investigation is likely beyond the scope of this proposal. Eliding Objective-C type information could change runtime behavior of a variety of systems that rely on this information. This could include systems embedded in Apple's libraries as well as systems implemented in third party or open source libraries. I imagine that an investigation into such a feature would probably need to be quite thorough.

clang-tidy/objc/TypeEncodingSizeCheck.cpp
37

I started looking into using hasDeclContext but encountered some unexpected behavior when adding relevant test cases—which should have been added from the beginning—to verify. I am going to continue investigating whether hasDeclContext is satisfactory here and try to resolve the unexpected behavior that I encountered.

64

On line 32 in this diff is where I register the matcher for ObjCIvarDecl. The matching is functioning as I expect it should. Let me know if you want me to reorganize the matching logic.

docs/clang-tidy/checks/objc-type-encoding-size.rst
7

I changed the check description to match the release notes. Let me know if I misunderstood the requested change.

alexfh removed a reviewer: alexfh.Jan 23 2019, 4:59 AM
stephanemoore planned changes to this revision.Jan 28 2019, 8:22 PM

Reiterating outstanding action items:
• Evaluate using hasDeclContext instead of hasAncestor.
• Include type encoding size information in diagnostic messages.

I've been observing some unexpected behaviors with matching in the proposed test sample that I am still investigating (as I recall the issue pertained to interfaces/protocols without interfaces).

Including the type encoding size information in diagnostic messages shouldn't be too hard (assuming we can get precise type encoding size information by analyzing the AST).