Warning: objc-encodings-larger-than=

[dmaclach]

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.

[stephanemoore]

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

[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).

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]

Added some spacing around early exit as requested by theraven.

[dmaclach]

In D55544#1326956, @theraven wrote:

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]

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

[dmaclach]

Full Diffs as requested.

[dmaclach]

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

[stephanemoore]

In D55544#1326606, @stephanemoore wrote:

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

[dmaclach]

Akira/John/Erik - any thoughts?

[ahatanak]

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?

[thakis]

FYI D96816 made clang emit way smaller encodings by default

docs/ReleaseNotes.rst

.. code-block:: c++
           ...
         }
 
         for (; // <- warning: init-statement of 'range-based for' is a null statement
              int y : S())
           ;
       }
 
+- ``-Wobjc-encodings-larger-than=`` is a new diagnostic that diagnoses when the Objective
+  C runtime is generating large Objective C runtime information. This often happens
+  when passing C++ classes in Objective C messages/blocks. This information can get very
+  large. A good starting value is 1024 (1K).
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - The experimental feature Pretokenized Headers (PTH) was removed in its
   entirely from Clang. The feature did not properly work with about 1/3 of the
   possible tokens available and was unmaintained.
 

include/clang/Basic/DiagnosticGroups.td

 def ObjCRetainBlockProperty : DiagGroup<"objc-noncopy-retain-block-property">;
 def ObjCReadonlyPropertyHasSetter : DiagGroup<"objc-readonly-with-setter-property">;
 def ObjCInvalidIBOutletProperty : DiagGroup<"invalid-iboutlet">;
 def ObjCRootClass : DiagGroup<"objc-root-class">;
 def ObjCPointerIntrospectPerformSelector : DiagGroup<"deprecated-objc-pointer-introspection-performSelector">;
 def ObjCPointerIntrospect : DiagGroup<"deprecated-objc-pointer-introspection", [ObjCPointerIntrospectPerformSelector]>;
 def ObjCMultipleMethodNames : DiagGroup<"objc-multiple-method-names">;
 def ObjCFlexibleArray : DiagGroup<"objc-flexible-array">;
+def ObjCEncodingLargerThan : DiagGroup<"objc-encoding-larger-than=">;
+
 def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">;
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def Packed : DiagGroup<"packed">;
 def Padded : DiagGroup<"padded">;
 
 def PessimizingMove : DiagGroup<"pessimizing-move">;
 def ReturnStdMoveInCXX11 : DiagGroup<"return-std-move-in-c++11">;

include/clang/Basic/DiagnosticSemaKinds.td

def warn_objc_circular_container : Warning<
   InGroup<DiagGroup<"objc-circular-container">>;
 def note_objc_circular_container_declared_here : Note<"%0 declared here">;
 def warn_objc_unsafe_perform_selector : Warning<
   "%0 is incompatible with selectors that return a "
   "%select{struct|union|vector}1 type">,
   InGroup<DiagGroup<"objc-unsafe-perform-selector">>;
 def note_objc_unsafe_perform_selector_method_declared_here :  Note<
   "method %0 that returns %1 declared here">;
+def warn_objc_method_encoding_too_large : Warning<
+  "%0 method %1 encoding of size %2 is larger than %3 bytes">,
+  InGroup<ObjCEncodingLargerThan>;
+def warn_objc_block_encoding_too_large : Warning<
+  "block argument encoding of size %0 is larger than %1 bytes">,
+  InGroup<ObjCEncodingLargerThan>;
+def warn_objc_ivar_encoding_too_large : Warning<
+  "instance variable encoding of size %0 is larger than %1 bytes">,
+  InGroup<ObjCEncodingLargerThan>;
 
 def warn_setter_getter_impl_required : Warning<
   "property %0 requires method %1 to be defined - "
   "use @synthesize, @dynamic or provide a method implementation "
   "in this class implementation">,
   InGroup<ObjCPropertyImpl>;
 def warn_setter_getter_impl_required_in_category : Warning<
   "property %0 requires method %1 to be defined - "

include/clang/Basic/LangOptions.def

 LANGOPT(CPlusPlus2a       , 1, 0, "C++2a")
 LANGOPT(ObjC              , 1, 0, "Objective-C")
 BENIGN_LANGOPT(ObjCDefaultSynthProperties , 1, 0,
                "Objective-C auto-synthesized properties")
 BENIGN_LANGOPT(EncodeExtendedBlockSig , 1, 0,
                "Encoding extended block type signature")
 BENIGN_LANGOPT(ObjCInferRelatedResultType , 1, 1,
                "Objective-C related result type inference")
+BENIGN_LANGOPT(ObjCLargeEncodingSize, 32, 0,
+              "if non-zero, warn about Objective-C encodings larger in bytes than this setting. 0 is no check.")
 LANGOPT(AppExt , 1, 0, "Objective-C App Extension")
 LANGOPT(Trigraphs         , 1, 0,"trigraphs")
 LANGOPT(LineComment       , 1, 0, "'//' comments")
 LANGOPT(Bool              , 1, 0, "bool, true, and false keywords")
 LANGOPT(Half              , 1, 0, "half keyword")
 LANGOPT(WChar             , 1, CPlusPlus, "wchar_t keyword")
 LANGOPT(Char8             , 1, 0, "char8_t keyword")
 LANGOPT(DeclSpecKeyword   , 1, 0, "__declspec keyword")

include/clang/Driver/Options.td

 def Wlarge_by_value_copy_EQ : Joined<["-"], "Wlarge-by-value-copy=">, Flags<[CC1Option]>;
 
 // These "special" warning flags are effectively processed as f_Group flags by the driver:
 // Just silence warnings about -Wlarger-than for now.
 def Wlarger_than_EQ : Joined<["-"], "Wlarger-than=">, Group<clang_ignored_f_Group>;
 def Wlarger_than_ : Joined<["-"], "Wlarger-than-">, Alias<Wlarger_than_EQ>;
 def Wframe_larger_than_EQ : Joined<["-"], "Wframe-larger-than=">, Group<f_Group>, Flags<[DriverOption]>;
 
+def Wobjc_encoding_larger_than_EQ : Joined<["-"], "Wobjc-encoding-larger-than=">, Flags<[CC1Option]>;
+
 def : Flag<["-"], "fterminated-vtables">, Alias<fapple_kext>;
 def fthreadsafe_statics : Flag<["-"], "fthreadsafe-statics">, Group<f_Group>;
 def ftime_report : Flag<["-"], "ftime-report">, Group<f_Group>, Flags<[CC1Option]>;
 def ftlsmodel_EQ : Joined<["-"], "ftls-model=">, Group<f_Group>, Flags<[CC1Option]>;
 def ftrapv : Flag<["-"], "ftrapv">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Trap on integer overflow">;
 def ftrapv_handler_EQ : Joined<["-"], "ftrapv-handler=">, Group<f_Group>,
   MetaVarName<"<function name>">,

include/clang/Sema/Sema.h

struct SkipBodyInfo {
     bool CheckSameAsPrevious;
     NamedDecl *Previous;
     NamedDecl *New;
   };
 
   DeclGroupPtrTy ConvertDeclToDeclGroup(Decl *Ptr, Decl *OwnedType = nullptr);
 
   void DiagnoseUseOfUnimplementedSelectors();
+  void DiagnoseLargeIvarEncodings(ObjCImplementationDecl *ID);
 
   bool isSimpleTypeSpecifier(tok::TokenKind Kind) const;
 
   ParsedType getTypeName(const IdentifierInfo &II, SourceLocation NameLoc,
                          Scope *S, CXXScopeSpec *SS = nullptr,
                          bool isClassName = false, bool HasTrailingDot = false,
                          ParsedType ObjectType = nullptr,
                          bool IsCtorOrDtorName = false,

lib/Driver/ToolChains/Clang.cpp

if (Arg *A = Args.getLastArg(options::OPT_fconstexpr_steps_EQ)) {
     CmdArgs.push_back(A->getValue());
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_fbracket_depth_EQ)) {
     CmdArgs.push_back("-fbracket-depth");
     CmdArgs.push_back(A->getValue());
   }
 
+  if (Arg *A = Args.getLastArg(options::OPT_Wobjc_encoding_larger_than_EQ)) {
+    if (A->getNumValues()) {
+      StringRef bytes = A->getValue();
+      CmdArgs.push_back(
+          Args.MakeArgString("-Wobjc-encoding-larger-than=" + bytes));
+    }
+  }
+  
   if (Arg *A = Args.getLastArg(options::OPT_Wlarge_by_value_copy_EQ,
                                options::OPT_Wlarge_by_value_copy_def)) {
     if (A->getNumValues()) {
       StringRef bytes = A->getValue();
       CmdArgs.push_back(Args.MakeArgString("-Wlarge-by-value-copy=" + bytes));
     } else
       CmdArgs.push_back("-Wlarge-by-value-copy=64"); // default value
   }

lib/Frontend/CompilerInvocation.cpp

#include "clang/Frontend/LangStandards.def"
   Opts.DelayedTemplateParsing = Args.hasArg(OPT_fdelayed_template_parsing);
   Opts.NumLargeByValueCopy =
       getLastArgIntValue(Args, OPT_Wlarge_by_value_copy_EQ, 0, Diags);
   Opts.MSBitfields = Args.hasArg(OPT_mms_bitfields);
   Opts.ObjCConstantStringClass =
     Args.getLastArgValue(OPT_fconstant_string_class);
   Opts.ObjCDefaultSynthProperties =
     !Args.hasArg(OPT_disable_objc_default_synthesize_properties);
+  Opts.ObjCLargeEncodingSize =
+    getLastArgIntValue(Args, OPT_Wobjc_encoding_larger_than_EQ, 0, Diags);
   Opts.EncodeExtendedBlockSig =
     Args.hasArg(OPT_fencode_extended_block_signature);
   Opts.EmitAllDecls = Args.hasArg(OPT_femit_all_decls);
   Opts.PackStruct = getLastArgIntValue(Args, OPT_fpack_struct_EQ, 0, Diags);
   Opts.MaxTypeAlign = getLastArgIntValue(Args, OPT_fmax_type_align_EQ, 0, Diags);
   Opts.AlignDouble = Args.hasArg(OPT_malign_double);
   Opts.PICLevel = getLastArgIntValue(Args, OPT_pic_level, 0, Diags);
   Opts.PIE = Args.hasArg(OPT_pic_is_pie);

lib/Sema/SemaDeclObjC.cpp

if (SuperClass) {
             << LastIvar->getType() << SuperClass->getDeclName();
         S.Diag(LastIvar->getLocation(), diag::note_entity_declared_at)
             << LastIvar->getDeclName();
       }
     }
   }
 }
 
+// Run through the ivars and see if any of their encodings are larger than
+// ObjCLargeEncodingSize.
+void Sema::DiagnoseLargeIvarEncodings(ObjCImplementationDecl *ID) {
+  unsigned long encodingSize = LangOpts.ObjCLargeEncodingSize;
+  if (encodingSize == 0) return;
+
+  for (ObjCIvarDecl *ivar = ID->getClassInterface()->all_declared_ivar_begin();
+       ivar; ivar = ivar->getNextIvar()) {
+    QualType QT = Context.getBaseElementType(ivar->getType());
+    std::string encoding;
+    QualType NotEncodedT;
+    Context.getObjCEncodingForType(QT, encoding, nullptr, &NotEncodedT);
+    if (encoding.length() > encodingSize) {
+      Diag(ivar->getLocation(), diag::warn_objc_ivar_encoding_too_large)
+        << (unsigned)encoding.length() << (unsigned)encodingSize;
+    }
+  }
+}
+
 // Note: For class/category implementations, allMethods is always null.
 Decl *Sema::ActOnAtEnd(Scope *S, SourceRange AtEnd, ArrayRef<Decl *> allMethods,
                        ArrayRef<DeclGroupPtrTy> allTUVars) {
   if (getObjCContainerKind() == Sema::OCK_None)
     return nullptr;
 
   assert(AtEnd.isValid() && "Invalid location for '@end'");
 
if (ObjCInterfaceDecl* IDecl = IC->getClassInterface()) {
       }
       ImplMethodsVsClassMethods(S, IC, IDecl);
       AtomicPropertySetterGetterRules(IC, IDecl);
       DiagnoseOwningPropertyGetterSynthesis(IC);
       DiagnoseUnusedBackingIvarInAccessor(S, IC);
       if (IDecl->hasDesignatedInitializers())
         DiagnoseMissingDesignatedInitOverrides(IC, IDecl);
       DiagnoseWeakIvars(*this, IC);
+      DiagnoseLargeIvarEncodings(IC);
       DiagnoseRetainableFlexibleArrayMember(*this, IDecl);
 
       bool HasRootClassAttr = IDecl->hasAttr<ObjCRootClassAttr>();
       if (IDecl->getSuperClass() == nullptr) {
         // This class has no superclass, so check that it has been marked with
         // __attribute((objc_root_class)).
         if (!HasRootClassAttr) {
           SourceLocation DeclLoc(IDecl->getLocation());
if (const auto *attr = ObjCMethod->getAttr<AvailabilityAttr>()) {
     if (ObjCMethod->isClassMethod() &&
         ObjCMethod->getSelector().getAsString() == "load") {
       Diag(attr->getLocation(), diag::warn_availability_on_static_initializer)
           << 0;
       ObjCMethod->dropAttr<AvailabilityAttr>();
     }
   }
 
+  /// Check for method encoding size.
+  std::string encoding = Context.getObjCEncodingForMethodDecl(ObjCMethod);
+  unsigned long encodingSize = LangOpts.ObjCLargeEncodingSize;
+  if (encodingSize && encoding.length() > encodingSize) {
+    Diag(ObjCMethod->getLocation(), diag::warn_objc_method_encoding_too_large)
+      << (ObjCMethod->isInstanceMethod() ? "instance" : "class")
+      << ObjCMethod->getDeclName() << (unsigned)encoding.length()
+      << (unsigned)encodingSize;
+  }
   ActOnDocumentableDecl(ObjCMethod);
 
   return ObjCMethod;
 }
 
 bool Sema::CheckObjCDeclScope(Decl *D) {
   // Following is also an error. But it is caused by a missing @end
   // and diagnostic is issued elsewhere.

lib/Sema/SemaExpr.cpp

         break;
       }
     }
   }
 
   if (getCurFunction())
     getCurFunction()->addBlock(BD);
 
+  /// Check for block objective C encoding size
+  if (getLangOpts().ObjC) {
+    std::string encoding = Context.getObjCEncodingForBlock(Result);
+    unsigned long encodingSize = LangOpts.ObjCLargeEncodingSize;
+    if (encodingSize && encoding.length() > encodingSize) {
+      Diag(BD->getLocation(), diag::warn_objc_block_encoding_too_large)
+        << (unsigned)encoding.length() << (unsigned)encodingSize;
+    }
+  }
+
   return Result;
 }
 
 ExprResult Sema::ActOnVAArg(SourceLocation BuiltinLoc, Expr *E, ParsedType Ty,
                             SourceLocation RPLoc) {
   TypeSourceInfo *TInfo;
   GetTypeFromParser(Ty, &TInfo);
   return BuildVAArgExpr(BuiltinLoc, E, TInfo, RPLoc);

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

+// RUN: %clang_cc1 -fsyntax-only -fblocks -verify -Wobjc-encoding-larger-than=10 -Wno-objc-root-class %s
+
+struct MyStruct {
+  int a;
+  int b;
+  struct MyStruct *c;
+};
+
+@interface MyClass {
+  struct MyStruct iVarThatWarns; // expected-warning {{instance variable encoding of size 24 is larger than 10 bytes}}
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wobjc-encoding-larger-than="
+  struct MyStruct iVarThatDoesntWarn;
+#pragma clang diagnostic pop
+  id idThatDoesntWarn;
+}
+@end
+
+@implementation MyClass
+- (void)methodThatWarns:(struct MyStruct)aStruct {} // expected-warning {{instance method 'methodThatWarns:' encoding of size 33 is larger than 10 bytes}}
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wobjc-encoding-larger-than="
+- (void)methodThatDoesntWarn:(struct MyStruct)aStruct {}
+#pragma clang diagnostic pop
+- (void)methodThatAlsoDoesntWarn {}
+@end
+
+void BlockFunc() {
+  void(^aBlockThatWarns)(struct MyStruct) = ^(struct MyStruct a) {}; // expected-warning {{block argument encoding of size 31 is larger than 10 bytes}}
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wobjc-encoding-larger-than="
+  void(^aBlockThatDoesntWarn)(struct MyStruct) = ^(struct MyStruct a) {};
+#pragma clang diagnostic pop
+  void(^aBlockThatAlsoDoesntWarn)() = ^() {};
+}