Page MenuHomePhabricator

Warning: objc-encodings-larger-than=
Needs ReviewPublic

Authored by dmaclach on Dec 10 2018, 7:58 PM.

Details

Summary

Issue a warning when the Objective C runtime encoding generated for an iVar, method, or block exceeds a user configurable value.

Off by default.

Note that handling iVars and methods will also get us properties.

Objective C runtime information can get very large (several K) and most of it is unused. Having the warning allows us to diagnose where the problem exists in our code.

Diff Detail

Repository
rC Clang

Event Timeline

dmaclach created this revision.Dec 10 2018, 7:58 PM

FYI:
I have a clang-tidy check almost ready for review that aims to flag large Objective-C type encodings.

include/clang/Basic/LangOptions.def
103

Objective-C

It would probably be a good idea to have a similar check on properties, as property encoding strings contain the type encoding (plus extra stuff).

I wonder if we also want an option in code generation to replace very long type encodings (or encodings of specifically annotated ivars?) with "?" ('unknown type')?

lib/Sema/SemaDeclObjC.cpp
3881

I missed this early exit on the first pass, please can you add some spacing to make it more obvious?

dmaclach updated this revision to Diff 177732.Dec 11 2018, 9:18 AM

Added some spacing around early exit as requested by theraven.

It would probably be a good idea to have a similar check on properties, as property encoding strings contain the type encoding (plus extra stuff).

Properties are already picked up based on the ivars and methods that they generate.

I wonder if we also want an option in code generation to replace very long type encodings (or encodings of specifically annotated ivars?) with "?" ('unknown type')?

Yeah, this is the next step. I'm trying to decide how best to do this. For a large cross platform code base I don't necessarily want to have folks having to annotate every C++ class they use with a somewhat non-portable annotation. At the same time you can't make all structs/classes encode as ? because I think it will mess up some existing Objective C patterns such as NSCoding.

arphaman added a subscriber: arphaman.

Thanks for working on this! Could you please post a patch with full context (git diff -U9999)?

dmaclach updated this revision to Diff 177744.Dec 11 2018, 11:08 AM
dmaclach marked an inline comment as done.

Full Diffs as requested.

dmaclach updated this revision to Diff 177746.Dec 11 2018, 11:11 AM

Updated to fix Stephane's good catch of Objective C vs Objective-C

dmaclach marked an inline comment as done.Dec 11 2018, 11:11 AM

FYI:
I have a clang-tidy check almost ready for review that aims to flag large Objective-C type encodings.

https://reviews.llvm.org/D55640

Akira/John/Erik - any thoughts?

The patch looks largely fine to me.

include/clang/Basic/DiagnosticSemaKinds.td
6207

You can use '%select' to choose between "class" and "instance" for %0.

6209

You can merge warn_objc_block_encoding_too_large and warn_objc_ivar_encoding_too_large using '%select'.

test/SemaObjC/objc-large-encoding-warn.m
16

Can you add tests for properties and categories just to make sure clang prints the expected diagnostics?