From 2372ce0fc1b113e8c3ae74cf7028f324c461acd7 Mon Sep 17 00:00:00 2001 From: Michael Layzell Date: Thu, 27 Apr 2017 12:44:55 -0400 Subject: [PATCH] Bug 1339537 - Part 5: 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. MozReview-Commit-ID: 4KIbzEKnmNU --- build/clang-plugin/CustomTypeAnnotation.cpp | 47 +++++++++++-------- build/clang-plugin/CustomTypeAnnotation.h | 9 ++-- build/clang-plugin/MemMoveAnnotation.h | 8 ++-- .../NonParamInsideFunctionDeclChecker.cpp | 13 +++-- .../tests/TestNonMemMovableStd.cpp | 4 +- .../tests/TestNonParameterChecker.cpp | 10 ++-- 6 files changed, 50 insertions(+), 41 deletions(-) diff --git a/build/clang-plugin/CustomTypeAnnotation.cpp b/build/clang-plugin/CustomTypeAnnotation.cpp index 8a6d85e3b914..8c2b8110ca70 100644 --- a/build/clang-plugin/CustomTypeAnnotation.cpp +++ b/build/clang-plugin/CustomTypeAnnotation.cpp @@ -27,6 +27,8 @@ void CustomTypeAnnotation::dumpAnnotationReason(BaseCheck &Check, "%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 (;;) { @@ -54,6 +56,14 @@ void CustomTypeAnnotation::dumpAnnotationReason(BaseCheck &Check, << 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; @@ -64,22 +74,19 @@ void CustomTypeAnnotation::dumpAnnotationReason(BaseCheck &Check, } } -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 @@ -93,7 +100,7 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) { if (const clang::ArrayType *Array = T->getAsArrayTypeUnsafe()) { if (hasEffectiveAnnotation(Array->getElementType())) { AnnotationReason Reason = {Array->getElementType(), RK_ArrayElement, - nullptr}; + nullptr, ""}; Cache[Key] = Reason; return Reason; } @@ -106,7 +113,7 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) { 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; } @@ -115,7 +122,7 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) { // 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; } @@ -140,7 +147,7 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) { } } - AnnotationReason Reason = {QualType(), RK_None, nullptr}; + AnnotationReason Reason = {QualType(), RK_None, nullptr, ""}; Cache[Key] = Reason; return Reason; } @@ -151,7 +158,7 @@ CustomTypeAnnotation::tmplArgAnnotationReason(ArrayRef Args) { if (Arg.getKind() == TemplateArgument::Type) { QualType Type = Arg.getAsType(); if (hasEffectiveAnnotation(Type)) { - AnnotationReason Reason = {Type, RK_TemplateInherited, nullptr}; + AnnotationReason Reason = {Type, RK_TemplateInherited, nullptr, ""}; return Reason; } } else if (Arg.getKind() == TemplateArgument::Pack) { @@ -162,6 +169,6 @@ CustomTypeAnnotation::tmplArgAnnotationReason(ArrayRef Args) { } } - AnnotationReason Reason = {QualType(), RK_None, nullptr}; + AnnotationReason Reason = {QualType(), RK_None, nullptr, ""}; return Reason; } diff --git a/build/clang-plugin/CustomTypeAnnotation.h b/build/clang-plugin/CustomTypeAnnotation.h index 1069a30fe3bc..8ce55f4b89c2 100644 --- a/build/clang-plugin/CustomTypeAnnotation.h +++ b/build/clang-plugin/CustomTypeAnnotation.h @@ -15,11 +15,13 @@ class CustomTypeAnnotation { 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; } }; @@ -53,13 +55,14 @@ public: } private: - bool hasLiteralAnnotation(QualType T) const; AnnotationReason directAnnotationReason(QualType T); AnnotationReason tmplArgAnnotationReason(ArrayRef 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; diff --git a/build/clang-plugin/MemMoveAnnotation.h b/build/clang-plugin/MemMoveAnnotation.h index 498ef9d9396c..1c0122f73e9e 100644 --- a/build/clang-plugin/MemMoveAnnotation.h +++ b/build/clang-plugin/MemMoveAnnotation.h @@ -17,7 +17,7 @@ public: 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") { @@ -48,11 +48,11 @@ protected: 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 ""; } }; diff --git a/build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp b/build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp index 4a70bc5f8259..214ff154206b 100644 --- a/build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp +++ b/build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp @@ -15,12 +15,11 @@ protected: // 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(A)) { - D->dump(); - return true; + return "it has an alignas(_) annotation"; } } @@ -29,9 +28,7 @@ protected: for (auto F : RD->fields()) { for (auto A : F->attrs()) { if (isa(A)) { - D->dump(); - - return true; + return ("member '" + F->getName() + "' has an alignas(_) annotation").str(); } } } @@ -39,7 +36,7 @@ protected: // We don't need to check the types of fields, as the CustomTypeAnnotation // infrastructure will handle that for us. - return false; + return ""; } }; NonParamAnnotation NonParam; @@ -111,6 +108,8 @@ void NonParamInsideFunctionDeclChecker::check( DiagnosticIDs::Note) << Spec->getSpecializedTemplate(); } + + NonParam.dumpAnnotationReason(*this, T, p->getLocation()); } } } diff --git a/build/clang-plugin/tests/TestNonMemMovableStd.cpp b/build/clang-plugin/tests/TestNonMemMovableStd.cpp index 9fce52496e41..3a1e67fe01c5 100644 --- a/build/clang-plugin/tests/TestNonMemMovableStd.cpp +++ b/build/clang-plugin/tests/TestNonMemMovableStd.cpp @@ -6,10 +6,10 @@ class MOZ_NEEDS_MEMMOVABLE_TYPE Mover { T mForceInst; }; // expected-error-re 4 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 basic_string { }; +template class basic_string { }; // expected-note 2 {{'std::basic_string' 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') is a non-memmove()able type because it is an stl-provided type not guaranteed to be memmove-able}} typedef basic_string string; template class pair { T mT; U mU; }; // expected-note {{std::pair >' is a non-memmove()able type because member 'mU' is a non-memmove()able type 'std::basic_string'}} -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')}} diff --git a/build/clang-plugin/tests/TestNonParameterChecker.cpp b/build/clang-plugin/tests/TestNonParameterChecker.cpp index 340cfd814231..d3c2b9b379a1 100644 --- a/build/clang-plugin/tests/TestNonParameterChecker.cpp +++ b/build/clang-plugin/tests/TestNonParameterChecker.cpp @@ -7,9 +7,9 @@ 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" @@ -180,10 +180,10 @@ void testLambda() // 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) { }