Improve "global-destructor" filtering rule: multiple fields, arrays, ...

This CL improves the "global-destructor" filtering rule, that excludes
from the CheckedPtr rewrite fields that would have resulted in running
a custom destructor for a global-scoped variable.  Before this CL,
this rule would incorrectly handle the following scenarios:
*) Only the first matched field in a struct would be filtered
   (subsequent fields would not be covered)
*) Nesting the affected struct in an array would hide the struct from
   the rule
*) Nesting an affected field in a template would hide the field from
   the rule.

After this CL, the scenarios listed above should work correctly.  The
CL adds extra test cases to cover these scenarios.

Bug: 1069567
Change-Id: I1a14e44f49023335554c1c46f10739b4c210538c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2492464Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820873}

base/memory/checked_ptr.md

@@ -164,6 +164,24 @@ and with `CheckedPtr`, then both overloads might be needed:
     }
 ```
 
+#### Global scope
+
+`-Wexit-time-destructors` disallows triggering custom destructors
+when global variables are destroyed.
+Since `CheckedPtr` has a custom destructor,
+it cannot be used as a field of structs that are used as global variables.
+If a pointer needs to be used in a global variable
+(directly or indirectly - e.g. embedded in an array or struct),
+then the only solution is avoiding `CheckedPtr`.
+
+Build error:
+
+```build
+error: declaration requires an exit-time destructor
+[-Werror,-Wexit-time-destructors]
+```
+
+
 #### No `constexpr` for non-null values
 
 `constexpr` raw pointers can be initialized with pointers to string literals
@@ -181,6 +199,14 @@ will not have a destructor.  Because of this `CheckedPtr<T>` usually cannot be
 used to replace the type of union members, because `CheckedPtr<T>` has
 a non-trivial destructor.
 
+Build error:
+
+```build
+error: attempt to use a deleted function
+note: destructor of 'SomeUnion' is implicitly deleted because variant
+field 'checked_ptr' has a non-trivial destructor
+```
+
 
 ### Runtime errors
 

tools/clang/rewrite_raw_ptr_fields/RewriteRawPtrFields.cpp

@@ -673,18 +673,35 @@ AST_MATCHER(clang::FieldDecl, overlapsOtherDeclsWithinRecordDecl) {
   return has_sibling_with_overlapping_location;
 }
 
-// Matches RecordDecl if
-// 1) it has a FieldDecl that matches the InnerMatcher
+// Matches clang::Type if
+// 1) it represents a RecordDecl with a FieldDecl that matches the InnerMatcher
+//    (*all* such FieldDecls will be matched)
 // or
-// 2) it has a FieldDecl that hasType of a RecordDecl that matches the
-//    InnerMatcher (this recurses to any depth).
-AST_MATCHER_P(clang::RecordDecl,
-              hasNestedFieldDecl,
+// 2) it represents an array or a RecordDecl that nests the case #1
+//    (this recurses to any depth).
+AST_MATCHER_P(clang::QualType,
+              typeWithEmbeddedFieldDecl,
               clang::ast_matchers::internal::Matcher<clang::FieldDecl>,
               InnerMatcher) {
-  auto matcher = recordDecl(has(fieldDecl(anyOf(
-      InnerMatcher, hasType(recordDecl(hasNestedFieldDecl(InnerMatcher)))))));
-  return matcher.matches(Node, Finder, Builder);
+  const clang::Type* type =
+      Node.getDesugaredType(Finder->getASTContext()).getTypePtrOrNull();
+  if (!type)
+    return false;
+
+  if (const clang::CXXRecordDecl* record_decl = type->getAsCXXRecordDecl()) {
+    auto matcher = recordDecl(forEach(fieldDecl(hasExplicitFieldDecl(anyOf(
+        InnerMatcher, hasType(typeWithEmbeddedFieldDecl(InnerMatcher)))))));
+    return matcher.matches(*record_decl, Finder, Builder);
+  }
+
+  if (type->isArrayType()) {
+    const clang::ArrayType* array_type =
+        Finder->getASTContext().getAsArrayType(Node);
+    auto matcher = typeWithEmbeddedFieldDecl(InnerMatcher);
+    return matcher.matches(array_type->getElementType(), Finder, Builder);
+  }
+
+  return false;
 }
 
 // Rewrites |SomeClass* field| (matched as "affectedFieldDecl") into
@@ -1080,11 +1097,10 @@ int main(int argc, const char* argv[]) {
                           &char_ptr_field_decl_writer);
 
   // See the testcases in tests/gen-global-destructor-test.cc.
-  auto global_destructor_matcher = varDecl(
-      allOf(hasGlobalStorage(),
-            hasType(recordDecl(hasNestedFieldDecl(field_decl_matcher)))));
-  FilteredExprWriter global_destructor_writer(&output_helper,
-                                              "global-destructor");
+  auto global_destructor_matcher =
+      varDecl(allOf(hasGlobalStorage(),
+                    hasType(typeWithEmbeddedFieldDecl(field_decl_matcher))));
+  FilteredExprWriter global_destructor_writer(&output_helper, "global-scope");
   match_finder.addMatcher(global_destructor_matcher, &global_destructor_writer);
 
   // Matches rewritable fields in generated code - see the testcase in

tools/clang/rewrite_raw_ptr_fields/tests/gen-global-destructor-expected.txt

-==== BEGIN EDITS ====
-include-user-header:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-destructor-actual.cc:::-1:::-1:::base/memory/checked_ptr.h
-r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-destructor-actual.cc:::1715:::5:::CheckedPtr<int> 
-r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-destructor-actual.cc:::1940:::5:::CheckedPtr<int> 
-r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-destructor-actual.cc:::2192:::5:::CheckedPtr<int> 
-r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-destructor-actual.cc:::2603:::5:::CheckedPtr<int> 
-r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-destructor-actual.cc:::2745:::10:::CheckedPtr<MyStruct> 
-==== END EDITS ====
-==== BEGIN FIELD FILTERS ====
-global_variables_test::MyStruct::ptr  # global-destructor
-nested_struct_test::MyStruct::ptr  # global-destructor
-pointer_nesting_test::MyOuterStruct::inner_struct  # global-destructor
-static_variables_test::MyStruct::ptr  # global-destructor
-==== END FIELD FILTERS ====

tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-expected.txt

+==== BEGIN EDITS ====
+include-user-header:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-actual.cc:::-1:::-1:::base/memory/checked_ptr.h
+r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-actual.cc:::1715:::5:::CheckedPtr<int> 
+r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-actual.cc:::1865:::5:::CheckedPtr<int> 
+r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-actual.cc:::2091:::5:::CheckedPtr<int> 
+r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-actual.cc:::2343:::5:::CheckedPtr<int> 
+r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-actual.cc:::2639:::5:::CheckedPtr<int> 
+r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-actual.cc:::2935:::3:::CheckedPtr<T> 
+r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-actual.cc:::3351:::5:::CheckedPtr<int> 
+r:::/usr/local/google/home/lukasza/src/chromium4/src/tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-actual.cc:::3493:::10:::CheckedPtr<MyStruct> 
+==== END EDITS ====
+==== BEGIN FIELD FILTERS ====
+global_variables_test::MyStruct::ptr  # global-scope
+global_variables_test::MyStruct::ptr2  # global-scope
+nested_in_array_test::MyStruct::ptr  # global-scope
+nested_struct_test::MyStruct::ptr  # global-scope
+nested_template_test::MyStruct::ptr  # global-scope
+pointer_nesting_test::MyOuterStruct::inner_struct  # global-scope
+static_variables_test::MyStruct::ptr  # global-scope
+==== END FIELD FILTERS ====

tools/clang/rewrite_raw_ptr_fields/tests/gen-global-destructor-test.cc → tools/clang/rewrite_raw_ptr_fields/tests/gen-global-scope-test.cc

@@ -34,6 +34,10 @@ struct MyStruct {
   // Expected to be emitted in automated-fields-to-ignore.txt, because
   // of |g_struct| below.
   int* ptr;
+
+  // Verification that *all* fields of a struct are covered (e.g. that the
+  // |forEach| matcher is used instead of the |has| matcher).
+  int* ptr2;
 };
 
 MyStruct g_struct;
@@ -70,6 +74,35 @@ static MyOuterStruct g_outer_struct;
 
 }  // namespace nested_struct_test
 
+namespace nested_in_array_test {
+
+struct MyStruct {
+  // Expected to be emitted in automated-fields-to-ignore.txt, because
+  // of |g_outer_array| below.
+  int* ptr;
+};
+
+static MyStruct g_outer_struct[] = {nullptr, nullptr, nullptr};
+
+}  // namespace nested_in_array_test
+
+namespace nested_template_test {
+
+template <typename T>
+struct MyStruct {
+  // Expected to be emitted in automated-fields-to-ignore.txt, because
+  // of |g_outer_struct| below.
+  T* ptr;
+};
+
+struct MyOuterStruct {
+  MyStruct<int> inner_struct;
+};
+
+static MyOuterStruct g_outer_struct;
+
+}  // namespace nested_template_test
+
 namespace pointer_nesting_test {
 
 struct MyStruct {