Backed out 3 changesets (bug 1744604) for causing build bustages in TestNonParameterChecker. CLOSED TREE

Backed out changeset 31d7633b2826 (bug 1744604)
Backed out changeset 3fc47ff6a295 (bug 1744604)
Backed out changeset 051767551298 (bug 1744604)

build/clang-plugin/CustomTypeAnnotation.cpp

@@ -75,22 +75,13 @@ void CustomTypeAnnotation::dumpAnnotationReason(BaseCheck &Check, QualType T,
 
 CustomTypeAnnotation::AnnotationReason
 CustomTypeAnnotation::directAnnotationReason(QualType T) {
-  VisitFlags ToVisit = VISIT_FIELDS | VISIT_BASES;
-
   if (const TagDecl *D = T->getAsTagDecl()) {
-    // Recurse into template arguments if the annotation
-    // MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS is present
-    if (hasCustomAttribute<moz_inherit_type_annotations_from_template_args>(
-            D)) {
-      ToVisit |= VISIT_TMPL_ARGS;
-    }
-
     if (hasCustomAttribute(D, Attribute)) {
       AnnotationReason Reason = {T, RK_Direct, nullptr, ""};
       return Reason;
     }
 
-    std::string ImplAnnotReason = getImplicitReason(D, ToVisit);
+    std::string ImplAnnotReason = getImplicitReason(D);
     if (!ImplAnnotReason.empty()) {
       AnnotationReason Reason = {T, RK_Implicit, nullptr, ImplAnnotReason};
       return Reason;
@@ -107,8 +98,8 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) {
   // Check if we have a type which we can recurse into
   if (const clang::ArrayType *Array = T->getAsArrayTypeUnsafe()) {
     if (hasEffectiveAnnotation(Array->getElementType())) {
-      AnnotationReason Reason{Array->getElementType(), RK_ArrayElement, nullptr,
-                              ""};
+      AnnotationReason Reason = {Array->getElementType(), RK_ArrayElement,
+                                 nullptr, ""};
       Cache[Key] = Reason;
       return Reason;
     }
@@ -119,27 +110,27 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) {
     if (Declaration->hasDefinition()) {
       Declaration = Declaration->getDefinition();
 
-      if (ToVisit & VISIT_BASES) {
-        for (const CXXBaseSpecifier &Base : Declaration->bases()) {
-          if (hasEffectiveAnnotation(Base.getType())) {
-            AnnotationReason Reason{Base.getType(), RK_BaseClass, nullptr, ""};
-            Cache[Key] = Reason;
-            return Reason;
-          }
+      for (const CXXBaseSpecifier &Base : Declaration->bases()) {
+        if (hasEffectiveAnnotation(Base.getType())) {
+          AnnotationReason Reason = {Base.getType(), RK_BaseClass, nullptr, ""};
+          Cache[Key] = Reason;
+          return Reason;
         }
       }
 
-      if (ToVisit & VISIT_FIELDS) {
-        for (const FieldDecl *Field : Declaration->fields()) {
-          if (hasEffectiveAnnotation(Field->getType())) {
-            AnnotationReason Reason{Field->getType(), RK_Field, Field, ""};
-            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, ""};
+          Cache[Key] = Reason;
+          return Reason;
         }
       }
 
-      if (ToVisit & VISIT_TMPL_ARGS) {
+      // Recurse into template arguments if the annotation
+      // MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS is present
+      if (hasCustomAttribute<moz_inherit_type_annotations_from_template_args>(
+              Declaration)) {
         const ClassTemplateSpecializationDecl *Spec =
             dyn_cast<ClassTemplateSpecializationDecl>(Declaration);
         if (Spec) {
@@ -155,7 +146,7 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) {
     }
   }
 
-  AnnotationReason Reason{QualType(), RK_None, nullptr, ""};
+  AnnotationReason Reason = {QualType(), RK_None, nullptr, ""};
   Cache[Key] = Reason;
   return Reason;
 }

build/clang-plugin/CustomTypeAnnotation.h

@@ -7,7 +7,6 @@
 
 #include "CustomAttributes.h"
 #include "plugin.h"
-#include "llvm/ADT/BitmaskEnum.h"
 
 class CustomTypeAnnotation {
   enum ReasonKind {
@@ -59,26 +58,10 @@ private:
   AnnotationReason tmplArgAnnotationReason(ArrayRef<TemplateArgument> Args);
 
 protected:
-  // Flags specifying which properties of the underlying type we want to visit.
-  enum VisitFlags {
-    VISIT_NONE = 0,
-    VISIT_FIELDS = 1,
-    VISIT_TMPL_ARGS = 2,
-    VISIT_BASES = 4,
-    LLVM_MARK_AS_BITMASK_ENUM(VISIT_BASES)
-  };
-
   // 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.
-  //
-  // The subclass may also modify `VisitFlags` to change what properties of the
-  // type will be inspected to skip inspecting fields, force template arguments
-  // to be inspected, etc.
-  virtual std::string getImplicitReason(const TagDecl *D,
-                                        VisitFlags &Flags) const {
-    return "";
-  }
+  virtual std::string getImplicitReason(const TagDecl *D) const { return ""; }
 };
 
 extern CustomTypeAnnotation StackClass;

build/clang-plugin/MemMoveAnnotation.h

@@ -19,41 +19,59 @@ public:
   virtual ~MemMoveAnnotation() {}
 
 protected:
-  std::string getImplicitReason(const TagDecl *D,
-                                VisitFlags &ToVisit) 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") {
-      return "";
+    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 (isNameExcepted(Name.data())) {
+        return "";
+      }
+      return "it is an stl-provided type not guaranteed to be memmove-able";
     }
-
-    StringRef Name = getNameChecked(D);
-
-    // If the type has a trivial move constructor and destructor, it is safe to
-    // memmove, and we don't need to visit any fields.
-    auto RD = dyn_cast<CXXRecordDecl>(D);
-    if (RD && RD->isCompleteDefinition() &&
-        (RD->hasTrivialMoveConstructor() ||
-         (!RD->hasMoveConstructor() && RD->hasTrivialCopyConstructor())) &&
-        RD->hasTrivialDestructor()) {
-      ToVisit = VISIT_NONE;
-      return "";
-    }
-
-    // This doesn't check that it's really ::std::pair and not
-    // ::std::something_else::pair, but should be good enough.
-    if (isNameExcepted(Name.data())) {
-      // If we're an excepted name, stop traversing within the type further,
-      // and only check template arguments for foreign types.
-      ToVisit = VISIT_TMPL_ARGS;
-      return "";
-    }
-    return "it is an stl-provided type not guaranteed to be memmove-able";
+    return "";
   }
 
 private:
-  bool isNameExcepted(StringRef Name) const {
-    return Name == "pair" || Name == "atomic";
+  bool isNameExcepted(const char *Name) const {
+    static std::unordered_set<std::string> NamesSet = {
+        {"pair"},
+        {"atomic"},
+        // libstdc++ specific names
+        {"__atomic_base"},
+        {"atomic_bool"},
+        {"__cxx_atomic_impl"},
+        {"__cxx_atomic_base_impl"},
+        {"__pair_base"},
+        // MSVCRT specific names
+        {"_Atomic_impl"},
+        {"_Atomic_base"},
+        {"_Atomic_bool"},
+        {"_Atomic_char"},
+        {"_Atomic_schar"},
+        {"_Atomic_uchar"},
+        {"_Atomic_char16_t"},
+        {"_Atomic_char32_t"},
+        {"_Atomic_wchar_t"},
+        {"_Atomic_short"},
+        {"_Atomic_ushort"},
+        {"_Atomic_int"},
+        {"_Atomic_uint"},
+        {"_Atomic_long"},
+        {"_Atomic_ulong"},
+        {"_Atomic_llong"},
+        {"_Atomic_ullong"},
+        {"_Atomic_address"},
+        // MSVCRT 2019
+        {"_Atomic_integral"},
+        {"_Atomic_integral_facade"},
+        {"_Atomic_padded"},
+        {"_Atomic_pointer"},
+        {"_Atomic_storage"}};
+
+    return NamesSet.find(Name) != NamesSet.end();
   }
 };
 

build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp

@@ -4,70 +4,33 @@
 
 #include "NonParamInsideFunctionDeclChecker.h"
 #include "CustomMatchers.h"
-#include "clang/Basic/TargetInfo.h"
 
 class NonParamAnnotation : public CustomTypeAnnotation {
 public:
   NonParamAnnotation() : CustomTypeAnnotation(moz_non_param, "non-param"){};
 
 protected:
-  // Helper for checking if a Decl has an explicitly specified alignment.
-  // Returns the alignment, in char units, of the largest alignment attribute,
-  // if it exceeds pointer alignment, and 0 otherwise.
-  unsigned checkExplicitAlignment(const Decl *D) const {
-    ASTContext &Context = D->getASTContext();
-    unsigned PointerAlign = Context.getTargetInfo().getPointerAlign(0);
-
-    // getMaxAlignment gets the largest alignment, in bits, specified by an
-    // alignment attribute directly on the declaration. If no alignment
-    // attribute is specified, it will return `0`.
-    unsigned MaxAlign = D->getMaxAlignment();
-    if (MaxAlign > PointerAlign) {
-      return Context.toCharUnitsFromBits(MaxAlign).getQuantity();
-    }
-    return 0;
-  }
-
   // 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.
-  std::string getImplicitReason(const TagDecl *D,
-                                VisitFlags &ToVisit) const override {
-    // Some stdlib types are known to have alignments over the pointer size on
-    // non-win32 platforms, but should not be linted against. Clear any
-    // annotations on those types.
-    if (!D->getASTContext().getTargetInfo().getCXXABI().isMicrosoft() &&
-        getDeclarationNamespace(D) == "std") {
-      StringRef Name = getNameChecked(D);
-      if (Name == "function") {
-        ToVisit = VISIT_NONE;
-        return "";
+  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)) {
+        return "it has an alignas(_) annotation";
       }
     }
 
-    // If the type doesn't have a destructor and can be passed in registers,
-    // clang will handle re-aligning it for us automatically, and we don't need
-    // to worry about the passed alignment.
-    auto RD = dyn_cast<CXXRecordDecl>(D);
-    if (RD && RD->canPassInRegisters()) {
-      return "";
-    }
-
-    // Check if the decl itself has an explicit alignment on it.
-    if (unsigned ExplicitAlign = checkExplicitAlignment(D)) {
-      return "it has an explicit alignment of '" +
-             std::to_string(ExplicitAlign) + "'";
-    }
-
-    // Check if any of the decl's fields have an explicit alignment on them.
+    // 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()) {
-        if (unsigned ExplicitAlign = checkExplicitAlignment(F)) {
-          return ("member '" + F->getName() +
-                  "' has an explicit alignment of '" +
-                  std::to_string(ExplicitAlign) + "'")
-              .str();
+        for (auto A : F->attrs()) {
+          if (isa<AlignedAttr>(A)) {
+            return ("member '" + F->getName() +
+                    "' has an alignas(_) annotation")
+                .str();
+          }
         }
       }
     }

build/clang-plugin/tests/TestNonMemMovableStd.cpp

@@ -1,70 +1,21 @@
 #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 9 {{Cannot instantiate 'Mover<{{.*}}>' with non-memmovable template argument '{{.*}}'}}
+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 { // 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}}
- public:
-  basic_string();
-  basic_string(const basic_string&);
-  basic_string(basic_string&&);
-  basic_string& operator=(const basic_string&);
-  basic_string& operator=(basic_string&&);
-  ~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-re 4 {{'std::pair<bool, {{.*}}>' is a non-memmove()able type because it has a template argument non-memmove()able type '{{.*}}'}}
-
-struct has_nontrivial_dtor {  // expected-note 2 {{'std::has_nontrivial_dtor' is a non-memmove()able type because it is an stl-provided type not guaranteed to be memmove-able}}
-  has_nontrivial_dtor() = default;
-  ~has_nontrivial_dtor();
-};
-struct has_nontrivial_copy {  // expected-note 2 {{'std::has_nontrivial_copy' is a non-memmove()able type because it is an stl-provided type not guaranteed to be memmove-able}}
-  has_nontrivial_copy() = default;
-  has_nontrivial_copy(const has_nontrivial_copy&);
-  has_nontrivial_copy& operator=(const has_nontrivial_copy&);
-};
-struct has_nontrivial_move {  // expected-note 2 {{'std::has_nontrivial_move' is a non-memmove()able type because it is an stl-provided type not guaranteed to be memmove-able}}
-  has_nontrivial_move() = default;
-  has_nontrivial_move(const has_nontrivial_move&);
-  has_nontrivial_move& operator=(const has_nontrivial_move&);
-};
-struct has_trivial_dtor {
-  has_trivial_dtor() = default;
-  ~has_trivial_dtor() = default;
-};
-struct has_trivial_copy {
-  has_trivial_copy() = default;
-  has_trivial_copy(const has_trivial_copy&) = default;
-  has_trivial_copy& operator=(const has_trivial_copy&) = default;
-};
-struct has_trivial_move {
-  has_trivial_move() = default;
-  has_trivial_move(const has_trivial_move&) = default;
-  has_trivial_move& operator=(const has_trivial_move&) = default;
-};
+template<class T, class U> class pair { T mT; U mU; }; // expected-note-re {{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 { }; // 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-re {{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;
 static Mover<std::pair<bool, std::string>> not_good; // expected-note-re {{instantiation of 'Mover<std::pair<bool, std::basic_string<char>{{ ?}}>{{ ?}}>' requested here}}
-
-static Mover<std::has_nontrivial_dtor> nontrivial_dtor; // expected-note {{instantiation of 'Mover<std::has_nontrivial_dtor>' requested here}}
-static Mover<std::has_nontrivial_copy> nontrivial_copy; // expected-note {{instantiation of 'Mover<std::has_nontrivial_copy>' requested here}}
-static Mover<std::has_nontrivial_move> nontrivial_move; // expected-note {{instantiation of 'Mover<std::has_nontrivial_move>' requested here}}
-static Mover<std::has_trivial_dtor> trivial_dtor;
-static Mover<std::has_trivial_copy> trivial_copy;
-static Mover<std::has_trivial_move> trivial_move;
-
-static Mover<std::pair<bool, std::has_nontrivial_dtor>> pair_nontrivial_dtor; // expected-note {{instantiation of 'Mover<std::pair<bool, std::has_nontrivial_dtor>>' requested here}}
-static Mover<std::pair<bool, std::has_nontrivial_copy>> pair_nontrivial_copy; // expected-note {{instantiation of 'Mover<std::pair<bool, std::has_nontrivial_copy>>' requested here}}
-static Mover<std::pair<bool, std::has_nontrivial_move>> pair_nontrivial_move; // expected-note {{instantiation of 'Mover<std::pair<bool, std::has_nontrivial_move>>' requested here}}
-static Mover<std::pair<bool, std::has_trivial_dtor>> pair_trivial_dtor;
-static Mover<std::pair<bool, std::has_trivial_copy>> pair_trivial_copy;
-static Mover<std::pair<bool, std::has_trivial_move>> pair_trivial_move;

build/clang-plugin/tests/TestNonParameterChecker.cpp

@@ -178,10 +178,12 @@ void testLambda()
     (void)[](HasNonParamStructUnion x) -> void {}; //expected-error {{Type 'HasNonParamStructUnion' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}}
 }
 
-struct alignas(16) AlignasStruct { char a; ~AlignasStruct(); };  // expected-note {{'AlignasStruct' is a non-param type because it has an explicit alignment of '16'}}
+// Check that alignas() implies the MOZ_NON_PARAM attribute.
+
+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(16) char a; ~AlignasMember(); }; // expected-note {{'AlignasMember' is a non-param type because member 'a' has an explicit alignment of '16'}}
+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) { }