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

17

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

           ;
       }
 
+- ``-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
 -------------------------------------------------

include/clang/Basic/DiagnosticGroups.td

 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">;

include/clang/Basic/DiagnosticSemaKinds.td

   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 - "

include/clang/Basic/LangOptions.def

                "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")

include/clang/Driver/Options.td

 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]>;

include/clang/Sema/Sema.h

   DeclGroupPtrTy ConvertDeclToDeclGroup(Decl *Ptr, Decl *OwnedType = nullptr);
 
   void DiagnoseUseOfUnimplementedSelectors();
+  void DiagnoseLargeIvarEncodings(ObjCImplementationDecl *ID);
 
   bool isSimpleTypeSpecifier(tok::TokenKind Kind) const;
 

lib/Driver/ToolChains/Clang.cpp

     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()) {

lib/Frontend/CompilerInvocation.cpp

     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);

lib/Sema/SemaDeclObjC.cpp

   }
 }
 
+// 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 (IDecl->hasDesignatedInitializers())
         DiagnoseMissingDesignatedInitOverrides(IC, IDecl);
       DiagnoseWeakIvars(*this, IC);
+      DiagnoseLargeIvarEncodings(IC);
       DiagnoseRetainableFlexibleArrayMember(*this, IDecl);
 
       bool HasRootClassAttr = IDecl->hasAttr<ObjCRootClassAttr>();

     }
   }
 
+  /// 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;

lib/Sema/SemaExpr.cpp

   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;
 }
 

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)() = ^() {};
+}