Avoiding rewriting pointers to non-free-standing structs.

A tag (struct, class, union or enum) is free-standing if its declaration
is not immediately used as part of a type of a field or variable.  See
also clang::TagDecl::isFreeStanding method.  Example of a
non-free-standing struct:
  struct NonFreeStandingStructCanHaveAName {
    int some_field;
  }* ptr_to_the_non_free_standing_struct;

After this CL, the rewriter will avoid rewriting pointers to
non-free-standing structs, because 1) such structs are relatively rare
and 2) such structs are difficult to rewrite correctly (essentially the
definition of the struct has to be used as a template argument of
CheckedPtr).

Bug: 1069567
Change-Id: Ib571344594795334be90bef5350a467d5841d96d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2173575
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#768963}

tools/clang/rewrite_raw_ptr_fields/RewriteRawPtrFields.cpp

@@ -79,6 +79,11 @@ class ReplacementsPrinter {
   std::set<std::string> files_with_already_added_includes_;
 };
 
+AST_MATCHER(clang::TagDecl, isNotFreeStandingTagDecl) {
+  const clang::TagDecl* tag_decl = Node.getCanonicalDecl();
+  return !tag_decl->isFreeStanding();
+}
+
 AST_MATCHER(clang::ClassTemplateSpecializationDecl, isImplicitSpecialization) {
   return !Node.isExplicitSpecialization();
 }
@@ -190,15 +195,19 @@ int main(int argc, const char* argv[]) {
   //     int (MyStruct::* member_func_ptr)(char);
   //     int (*ptr_to_array_of_ints)[123]
   //     StructOrClassWithDeletedOperatorNew* stack_or_gc_ptr;
+  //     struct { int i }* ptr_to_non_free_standing_record_or_union_or_class;
   //   };
   // matches |int*|, but not the other types.
   auto record_with_deleted_allocation_operator_type_matcher =
       recordType(hasDeclaration(cxxRecordDecl(
           hasMethod(allOf(hasOverloadedOperatorName("new"), isDeleted())))));
+  auto non_free_standing_tag_type =
+      tagType(hasDeclaration(tagDecl(isNotFreeStandingTagDecl())));
   auto supported_pointer_types_matcher =
-      pointerType(unless(pointee(hasUnqualifiedDesugaredType(anyOf(
-          record_with_deleted_allocation_operator_type_matcher, functionType(),
-          memberPointerType(), anyCharType(), arrayType())))));
+      pointerType(unless(pointee(hasUnqualifiedDesugaredType(
+          anyOf(record_with_deleted_allocation_operator_type_matcher,
+                non_free_standing_tag_type, functionType(), memberPointerType(),
+                anyCharType(), arrayType())))));
 
   // Implicit field declarations =========
   // Matches field declarations that do not explicitly appear in the source

tools/clang/rewrite_raw_ptr_fields/tests/various-types-expected.cc

@@ -129,6 +129,37 @@ struct MyStruct {
   // replacement is tricky, because the |replacement_range| needs to cover
   // "[123]" that comes *after* the field name).
   const SomeClass (*ptr_to_array)[123];
+
+  // Definition of the non-freestanding struct should not disappear - i.e.
+  // we do not want the rewrite to be: CheckedPtr<struct NonFreestandingStruct>.
+  //
+  // Expected rewrite: ??? (as long as the struct definition doesn't disappear).
+  struct NonFreeStandingStruct {
+    int non_ptr;
+  } * ptr_to_non_free_standing_struct;
+
+  // Pointer to an inline definition of a struct.  There is a risk of generating
+  // an overlapping replacement (wrt the pointer field within the inline
+  // struct).
+  //
+  // Note that before a fix, the rewriter would generate an overlapping
+  // replacement under
+  // //sandbox/linux/integration_tests/bpf_dsl_seccomp_unittest.cc
+  // (see the ArgValue struct and the non-free-standing Tests struct inside).
+  //
+  // Expected rewrite: ??? (as long as there are no overlapping replacements).
+  struct NonFreeStandingStruct2 {
+    CheckedPtr<SomeClass> inner_ptr;
+  } * ptr_to_non_free_standing_struct2;
+
+  // Despite avoiding the problems in NonFreeStandingStruct and
+  // NonFreeStandingStruct2 above, we should still rewrite the example below.
+  struct FreeStandingStruct {
+    // Expected rewrite: CheckedPtr<SomeClass> inner_ptr;
+    CheckedPtr<SomeClass> inner_ptr;
+  };
+  // Expected rewrite: CheckedPtr<InnerStruct2> ...
+  CheckedPtr<FreeStandingStruct> ptr_to_free_standing_struct;
 };
 
 }  // namespace my_namespace

tools/clang/rewrite_raw_ptr_fields/tests/various-types-original.cc

@@ -127,6 +127,37 @@ struct MyStruct {
   // replacement is tricky, because the |replacement_range| needs to cover
   // "[123]" that comes *after* the field name).
   const SomeClass (*ptr_to_array)[123];
+
+  // Definition of the non-freestanding struct should not disappear - i.e.
+  // we do not want the rewrite to be: CheckedPtr<struct NonFreestandingStruct>.
+  //
+  // Expected rewrite: ??? (as long as the struct definition doesn't disappear).
+  struct NonFreeStandingStruct {
+    int non_ptr;
+  } * ptr_to_non_free_standing_struct;
+
+  // Pointer to an inline definition of a struct.  There is a risk of generating
+  // an overlapping replacement (wrt the pointer field within the inline
+  // struct).
+  //
+  // Note that before a fix, the rewriter would generate an overlapping
+  // replacement under
+  // //sandbox/linux/integration_tests/bpf_dsl_seccomp_unittest.cc
+  // (see the ArgValue struct and the non-free-standing Tests struct inside).
+  //
+  // Expected rewrite: ??? (as long as there are no overlapping replacements).
+  struct NonFreeStandingStruct2 {
+    SomeClass* inner_ptr;
+  } * ptr_to_non_free_standing_struct2;
+
+  // Despite avoiding the problems in NonFreeStandingStruct and
+  // NonFreeStandingStruct2 above, we should still rewrite the example below.
+  struct FreeStandingStruct {
+    // Expected rewrite: CheckedPtr<SomeClass> inner_ptr;
+    SomeClass* inner_ptr;
+  };
+  // Expected rewrite: CheckedPtr<InnerStruct2> ...
+  FreeStandingStruct* ptr_to_free_standing_struct;
 };
 
 }  // namespace my_namespace