Bug 1339537 - Part 4: Produce better annotation reason diagnostics for implicit annotations, r=ehsan
☠☠ backed out by 609ed9f634a4 ☠ ☠
authorMichael Layzell <michael@thelayzells.com>
Fri, 17 Feb 2017 17:31:04 -0500
changeset 349643 ca8fc58d0cef79d38a801e659444c504230cc9b2
parent 349642 828eba714030d8ec34130b6c4c6eeef391de61fe
child 349644 314479dc240362389222571ba959d6bde8ebc9be
push id31556
push userphilringnalda@gmail.com
push dateSun, 26 Mar 2017 01:40:08 +0000
treeherdermozilla-central@f5e214144799 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1339537
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1339537 - Part 4: Produce better annotation reason diagnostics for implicit annotations, r=ehsan This allows for the alignas(_) case to be distinguished from the MOZ_NON_PARAM case through notes. The following is what the IntInterval bug fixed in Part 2 of this bug looks like with the new diagnostics: /home/mlayzell/Code/moz/working/dom/media/gtest/TestIntervalSet.cpp:196:47: error: Type 'IntIntervals' (aka 'IntervalSet<int>') must not be used as parameter static void GeneratePermutations(IntIntervals aI1, ^ /home/mlayzell/Code/moz/working/dom/media/gtest/TestIntervalSet.cpp:196:47: note: Please consider passing a const reference instead /home/mlayzell/Code/moz/working/dom/media/Intervals.h:695:17: note: 'IntIntervals' (aka 'IntervalSet<int>') is a non-param type because member 'mIntervals' is a non-param type 'ContainerType' (aka 'AutoTArray<Interval<int>, 4>') ContainerType mIntervals; ^ /home/mlayzell/Code/moz/working/obj-clang-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2472:3: note: 'ContainerType' (aka 'AutoTArray<Interval<int>, 4>') is a non-param type because member '' is a non-param type 'union (anonymous union at /home/mlayzell/Code/moz/working/obj-clang-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2472:3)' union ^ /home/mlayzell/Code/moz/working/obj-clang-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2477:72: note: 'union (anonymous union at /home/mlayzell/Code/moz/working/obj-clang-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2472:3)' is a non-param type because member 'mAlign' is a non-param type 'mozilla::AlignedElem<(mozilla::AlignmentFinder<Header>::alignment > mozilla::AlignmentFinder<elem_type>::alignment) ? mozilla::AlignmentFinder<Header>::alignment : mozilla::AlignmentFinder<elem_type>::alignment>' MOZ_ALIGNOF(Header) : MOZ_ALIGNOF(elem_type)> mAlign; ^ /home/mlayzell/Code/moz/working/obj-clang-x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h:85:8: note: 'mozilla::AlignedElem<(mozilla::AlignmentFinder<Header>::alignment > mozilla::AlignmentFinder<elem_type>::alignment) ? mozilla::AlignmentFinder<Header>::alignment : mozilla::AlignmentFinder<elem_type>::alignment>' is a non-param type because member 'elem' has an alignas(_) annotation struct AlignedElem<4> ^ MozReview-Commit-ID: 4KIbzEKnmNU
build/clang-plugin/CustomTypeAnnotation.cpp
build/clang-plugin/CustomTypeAnnotation.h
build/clang-plugin/MemMoveAnnotation.h
build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp
build/clang-plugin/tests/TestNonMemMovableStd.cpp
build/clang-plugin/tests/TestNonParameterChecker.cpp
--- a/build/clang-plugin/CustomTypeAnnotation.cpp
+++ b/build/clang-plugin/CustomTypeAnnotation.cpp
@@ -22,16 +22,18 @@ void CustomTypeAnnotation::dumpAnnotatio
   const char* Inherits =
       "%1 is a %0 type because it inherits from a %0 type %2";
   const char* Member =
       "%1 is a %0 type because member %2 is a %0 type %3";
   const char* Array =
       "%1 is a %0 type because it is an array of %0 type %2";
   const char* Templ =
       "%1 is a %0 type because it has a template argument %0 type %2";
+  const char* Implicit =
+      "%1 is a %0 type because %2";
 
   AnnotationReason Reason = directAnnotationReason(T);
   for (;;) {
     switch (Reason.Kind) {
     case RK_ArrayElement:
       Check.diag(Loc, Array, DiagnosticIDs::Note) << Pretty << T << Reason.Type;
       break;
     case RK_BaseClass: {
@@ -49,42 +51,47 @@ void CustomTypeAnnotation::dumpAnnotatio
     case RK_TemplateInherited: {
       const CXXRecordDecl *Declaration = T->getAsCXXRecordDecl();
       assert(Declaration && "This type should be a C++ class");
 
       Check.diag(Declaration->getLocation(), Templ, DiagnosticIDs::Note)
         << Pretty << T << Reason.Type;
       break;
     }
+    case RK_Implicit: {
+      const TagDecl *Declaration = T->getAsTagDecl();
+      assert(Declaration && "This type should be a TagDecl");
+
+      Check.diag(Declaration->getLocation(), Implicit, DiagnosticIDs::Note)
+        << Pretty << T << Reason.ImplicitReason;
+      return;
+    }
     default:
       // FIXME (bug 1203263): note the original annotation.
       return;
     }
 
     T = Reason.Type;
     Reason = directAnnotationReason(T);
   }
 }
 
-bool CustomTypeAnnotation::hasLiteralAnnotation(QualType T) const {
-#if CLANG_VERSION_FULL >= 306
-  if (const TagDecl *D = T->getAsTagDecl()) {
-#else
-  if (const CXXRecordDecl *D = T->getAsCXXRecordDecl()) {
-#endif
-    return hasFakeAnnotation(D) || hasCustomAnnotation(D, Spelling);
-  }
-  return false;
-}
-
 CustomTypeAnnotation::AnnotationReason
 CustomTypeAnnotation::directAnnotationReason(QualType T) {
-  if (hasLiteralAnnotation(T)) {
-    AnnotationReason Reason = {T, RK_Direct, nullptr};
-    return Reason;
+  if (const TagDecl *D = T->getAsTagDecl()) {
+    if (hasCustomAnnotation(D, Spelling)) {
+      AnnotationReason Reason = {T, RK_Direct, nullptr, ""};
+      return Reason;
+    }
+
+    std::string ImplAnnotReason = getImplicitReason(D);
+    if (!ImplAnnotReason.empty()) {
+      AnnotationReason Reason = {T, RK_Implicit, nullptr, ImplAnnotReason};
+      return Reason;
+    }
   }
 
   // Check if we have a cached answer
   void *Key = T.getAsOpaquePtr();
   ReasonCache::iterator Cached = Cache.find(T.getAsOpaquePtr());
   if (Cached != Cache.end()) {
     return Cached->second;
   }
@@ -101,26 +108,26 @@ CustomTypeAnnotation::directAnnotationRe
 
   // Recurse into Base classes
   if (const CXXRecordDecl *Declaration = T->getAsCXXRecordDecl()) {
     if (Declaration->hasDefinition()) {
       Declaration = Declaration->getDefinition();
 
       for (const CXXBaseSpecifier &Base : Declaration->bases()) {
         if (hasEffectiveAnnotation(Base.getType())) {
-          AnnotationReason Reason = {Base.getType(), RK_BaseClass, nullptr};
+          AnnotationReason Reason = {Base.getType(), RK_BaseClass, nullptr, ""};
           Cache[Key] = Reason;
           return Reason;
         }
       }
 
       // Recurse into members
       for (const FieldDecl *Field : Declaration->fields()) {
         if (hasEffectiveAnnotation(Field->getType())) {
-          AnnotationReason Reason = {Field->getType(), RK_Field, Field};
+          AnnotationReason Reason = {Field->getType(), RK_Field, Field, ""};
           Cache[Key] = Reason;
           return Reason;
         }
       }
 
       // Recurse into template arguments if the annotation
       // MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS is present
       if (hasCustomAnnotation(
@@ -135,17 +142,17 @@ CustomTypeAnnotation::directAnnotationRe
             Cache[Key] = Reason;
             return Reason;
           }
         }
       }
     }
   }
 
-  AnnotationReason Reason = {QualType(), RK_None, nullptr};
+  AnnotationReason Reason = {QualType(), RK_None, nullptr, ""};
   Cache[Key] = Reason;
   return Reason;
 }
 
 CustomTypeAnnotation::AnnotationReason
 CustomTypeAnnotation::tmplArgAnnotationReason(ArrayRef<TemplateArgument> Args) {
   for (const TemplateArgument &Arg : Args) {
     if (Arg.getKind() == TemplateArgument::Type) {
@@ -157,11 +164,11 @@ CustomTypeAnnotation::tmplArgAnnotationR
     } else if (Arg.getKind() == TemplateArgument::Pack) {
       AnnotationReason Reason = tmplArgAnnotationReason(Arg.getPackAsArray());
       if (Reason.Kind != RK_None) {
         return Reason;
       }
     }
   }
 
-  AnnotationReason Reason = {QualType(), RK_None, nullptr};
+  AnnotationReason Reason = {QualType(), RK_None, nullptr, ""};
   return Reason;
 }
--- a/build/clang-plugin/CustomTypeAnnotation.h
+++ b/build/clang-plugin/CustomTypeAnnotation.h
@@ -10,21 +10,23 @@
 class CustomTypeAnnotation {
   enum ReasonKind {
     RK_None,
     RK_Direct,
     RK_ArrayElement,
     RK_BaseClass,
     RK_Field,
     RK_TemplateInherited,
+    RK_Implicit,
   };
   struct AnnotationReason {
     QualType Type;
     ReasonKind Kind;
     const FieldDecl *Field;
+    std::string ImplicitReason;
 
     bool valid() const { return Kind != RK_None; }
   };
   typedef DenseMap<void *, AnnotationReason> ReasonCache;
 
   const char *Spelling;
   const char *Pretty;
   ReasonCache Cache;
@@ -48,23 +50,24 @@ public:
     if (hasEffectiveAnnotation(T)) {
       Check.diag(Loc, Error, DiagnosticIDs::Error) << T;
       Check.diag(Loc, Note, DiagnosticIDs::Note);
       dumpAnnotationReason(Check, T, Loc);
     }
   }
 
 private:
-  bool hasLiteralAnnotation(QualType T) const;
   AnnotationReason directAnnotationReason(QualType T);
   AnnotationReason tmplArgAnnotationReason(ArrayRef<TemplateArgument> Args);
 
 protected:
-  // Allow subclasses to apply annotations to external code:
-  virtual bool hasFakeAnnotation(const TagDecl *D) const { return false; }
+  // Allow subclasses to apply annotations for reasons other than a direct
+  // annotation. A non-empty string return value means that the object D is
+  // annotated, and should contain the reason why.
+  virtual std::string getImplicitReason(const TagDecl *D) const { return ""; }
 };
 
 extern CustomTypeAnnotation StackClass;
 extern CustomTypeAnnotation GlobalClass;
 extern CustomTypeAnnotation NonHeapClass;
 extern CustomTypeAnnotation HeapClass;
 extern CustomTypeAnnotation NonTemporaryClass;
 
--- a/build/clang-plugin/MemMoveAnnotation.h
+++ b/build/clang-plugin/MemMoveAnnotation.h
@@ -12,17 +12,17 @@
 class MemMoveAnnotation final : public CustomTypeAnnotation {
 public:
   MemMoveAnnotation()
       : CustomTypeAnnotation("moz_non_memmovable", "non-memmove()able") {}
 
   virtual ~MemMoveAnnotation() {}
 
 protected:
-  bool hasFakeAnnotation(const TagDecl *D) const override {
+  std::string getImplicitReason(const TagDecl *D) const override {
     // Annotate everything in ::std, with a few exceptions; see bug
     // 1201314 for discussion.
     if (getDeclarationNamespace(D) == "std") {
       // This doesn't check that it's really ::std::pair and not
       // ::std::something_else::pair, but should be good enough.
       StringRef Name = getNameChecked(D);
       if (Name == "pair" ||
           Name == "atomic" ||
@@ -43,19 +43,19 @@ protected:
           Name == "_Atomic_ushort" ||
           Name == "_Atomic_int" ||
           Name == "_Atomic_uint" ||
           Name == "_Atomic_long" ||
           Name == "_Atomic_ulong" ||
           Name == "_Atomic_llong" ||
           Name == "_Atomic_ullong" ||
           Name == "_Atomic_address") {
-        return false;
+        return "";
       }
-      return true;
+      return "it is an stl-provided type not guaranteed to be memmove-able";
     }
-    return false;
+    return "";
   }
 };
 
 extern MemMoveAnnotation NonMemMovable;
 
 #endif
--- a/build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp
+++ b/build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp
@@ -10,41 +10,38 @@ class NonParamAnnotation : public Custom
 public:
   NonParamAnnotation() : CustomTypeAnnotation("moz_non_param", "non-param") {};
 
 protected:
   // Adding alignas(_) on a struct implicitly marks it as MOZ_NON_PARAM, due to
   // MSVC limitations which prevent passing explcitly aligned types by value as
   // parameters. This overload of hasFakeAnnotation injects fake MOZ_NON_PARAM
   // annotations onto these types.
-  bool hasFakeAnnotation(const TagDecl *D) const override {
+  std::string getImplicitReason(const TagDecl *D) const override {
     // Check if the decl itself has an AlignedAttr on it.
     for (const Attr *A : D->attrs()) {
       if (isa<AlignedAttr>(A)) {
-        D->dump();
-        return true;
+        return "it has an alignas(_) annotation";
       }
     }
 
     // Check if any of the decl's fields have an AlignedAttr on them.
     if (auto RD = dyn_cast<RecordDecl>(D)) {
       for (auto F : RD->fields()) {
         for (auto A : F->attrs()) {
           if (isa<AlignedAttr>(A)) {
-            D->dump();
-
-            return true;
+            return ("member '" + F->getName() + "' has an alignas(_) annotation").str();
           }
         }
       }
     }
 
     // We don't need to check the types of fields, as the CustomTypeAnnotation
     // infrastructure will handle that for us.
-    return false;
+    return "";
   }
 };
 NonParamAnnotation NonParam;
 
 void NonParamInsideFunctionDeclChecker::registerMatchers(MatchFinder* AstMatcher) {
   AstMatcher->addMatcher(
       functionDecl(anyOf(allOf(isDefinition(),
                                hasAncestor(classTemplateSpecializationDecl()
@@ -95,11 +92,13 @@ void NonParamInsideFunctionDeclChecker::
            DiagnosticIDs::Note);
 
       if (Spec) {
         diag(Spec->getPointOfInstantiation(),
              "The bad argument was passed to %0 here",
              DiagnosticIDs::Note)
           << Spec->getSpecializedTemplate();
       }
+
+      NonParam.dumpAnnotationReason(*this, T, p->getLocation());
     }
   }
 }
--- a/build/clang-plugin/tests/TestNonMemMovableStd.cpp
+++ b/build/clang-plugin/tests/TestNonMemMovableStd.cpp
@@ -1,20 +1,20 @@
 #define MOZ_NEEDS_MEMMOVABLE_TYPE __attribute__((annotate("moz_needs_memmovable_type")))
 
 template<class T>
 class MOZ_NEEDS_MEMMOVABLE_TYPE Mover { T mForceInst; }; // expected-error-re 4 {{Cannot instantiate 'Mover<{{.*}}>' with non-memmovable template argument '{{.*}}'}}
 
 namespace std {
 // In theory defining things in std:: like this invokes undefined
 // behavior, but in practice it's good enough for this test case.
-template<class C> class basic_string { };
+template<class C> class basic_string { }; // expected-note 2 {{'std::basic_string<char>' is a non-memmove()able type because it is an stl-provided type not guaranteed to be memmove-able}} expected-note {{'std::string' (aka 'basic_string<char>') is a non-memmove()able type because it is an stl-provided type not guaranteed to be memmove-able}}
 typedef basic_string<char> string;
 template<class T, class U> class pair { T mT; U mU; }; // expected-note {{std::pair<bool, std::basic_string<char> >' is a non-memmove()able type because member 'mU' is a non-memmove()able type 'std::basic_string<char>'}}
-class arbitrary_name { };
+class arbitrary_name { }; // expected-note {{'std::arbitrary_name' is a non-memmove()able type because it is an stl-provided type not guaranteed to be memmove-able}}
 }
 
 class HasString { std::string m; }; // expected-note {{'HasString' is a non-memmove()able type because member 'm' is a non-memmove()able type 'std::string' (aka 'basic_string<char>')}}
 
 static Mover<std::string> bad; // expected-note {{instantiation of 'Mover<std::basic_string<char> >' requested here}}
 static Mover<HasString> bad_mem; // expected-note {{instantiation of 'Mover<HasString>' requested here}}
 static Mover<std::arbitrary_name> assumed_bad; // expected-note {{instantiation of 'Mover<std::arbitrary_name>' requested here}}
 static Mover<std::pair<bool, int>> good;
--- a/build/clang-plugin/tests/TestNonParameterChecker.cpp
+++ b/build/clang-plugin/tests/TestNonParameterChecker.cpp
@@ -2,19 +2,19 @@
 
 struct Param {};
 struct MOZ_NON_PARAM NonParam {};
 union MOZ_NON_PARAM NonParamUnion {};
 class MOZ_NON_PARAM NonParamClass {};
 enum MOZ_NON_PARAM NonParamEnum { X, Y, Z };
 enum class MOZ_NON_PARAM NonParamEnumClass { X, Y, Z };
 
-struct HasNonParamStruct { NonParam x; int y; };
-union HasNonParamUnion { NonParam x; int y; };
-struct HasNonParamStructUnion { HasNonParamUnion z; };
+struct HasNonParamStruct { NonParam x; int y; }; // expected-note 14 {{'HasNonParamStruct' is a non-param type because member 'x' is a non-param type 'NonParam'}}
+union HasNonParamUnion { NonParam x; int y; }; // expected-note 18 {{'HasNonParamUnion' is a non-param type because member 'x' is a non-param type 'NonParam'}}
+struct HasNonParamStructUnion { HasNonParamUnion z; }; // expected-note 9 {{'HasNonParamStructUnion' is a non-param type because member 'z' is a non-param type 'HasNonParamUnion'}}
 
 #define MAYBE_STATIC
 #include "NonParameterTestCases.h"
 #undef MAYBE_STATIC
 
 // Do not check typedef and using.
 typedef void (*funcTypeParam)(Param x);
 typedef void (*funcTypeNonParam)(NonParam x);
@@ -175,15 +175,15 @@ void testLambda()
     (void)[](NonParam x) -> void {}; //expected-error {{Type 'NonParam' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}}
     (void)[](HasNonParamStruct x) -> void {}; //expected-error {{Type 'HasNonParamStruct' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}}
     (void)[](HasNonParamUnion x) -> void {}; //expected-error {{Type 'HasNonParamUnion' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}}
     (void)[](HasNonParamStructUnion x) -> void {}; //expected-error {{Type 'HasNonParamStructUnion' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}}
 }
 
 // Check that alignas() implies the MOZ_NON_PARAM attribute.
 
-struct alignas(8) AlignasStruct { char a; };
+struct alignas(8) AlignasStruct { char a; }; // expected-note {{'AlignasStruct' is a non-param type because it has an alignas(_) annotation}}
 void takesAlignasStruct(AlignasStruct x) { } // expected-error {{Type 'AlignasStruct' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}}
 void takesAlignasStructByRef(const AlignasStruct& x) { }
 
-struct AlignasMember { alignas(8) char a; };
+struct AlignasMember { alignas(8) char a; }; // expected-note {{'AlignasMember' is a non-param type because member 'a' has an alignas(_) annotation}}
 void takesAlignasMember(AlignasMember x) { } // expected-error {{Type 'AlignasMember' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}}
 void takesAlignasMemberByRef(const AlignasMember& x) { }