Sema: Add a flag for rejecting member pointers with incomplete base types.

[pcc]

Summary

Codebases that need to be compatible with the Microsoft ABI can pass
this flag to avoid issues caused by the lack of a fixed ABI for
incomplete member pointers.

[pcc]

• Add some negative tests

[pcc]

• One more negative test

[rnk]

Should we do this in exactly the places we would lock in an inheritance model in the MS ABI, i.e. when the member pointer type is required to be complete? I think we could take this code:

bool Sema::RequireCompleteTypeImpl(SourceLocation Loc, QualType T,
                                   TypeDiagnoser *Diagnoser) {
  // FIXME: Add this assertion to make sure we always get instantiation points.
  //  assert(!Loc.isInvalid() && "Invalid location in RequireCompleteType");
  // FIXME: Add this assertion to help us flush out problems with
  // checking for dependent types and type-dependent expressions.
  //
  //  assert(!T->isDependentType() &&
  //         "Can't ask whether a dependent type is complete");

  // We lock in the inheritance model once somebody has asked us to ensure
  // that a pointer-to-member type is complete.
  if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
    if (const MemberPointerType *MPTy = T->getAs<MemberPointerType>()) {
      if (!MPTy->getClass()->isDependentType()) {
        (void)isCompleteType(Loc, QualType(MPTy->getClass(), 0));
        assignInheritanceModel(*this, MPTy->getMostRecentCXXRecordDecl());
      }
    }
  }

And rewrite the side-effecting isCompleteType call to use RequireCompleteType with the warning diagnostic. I guess we would change the isMicrosoft check to alternatively check if the diagnostic is enabled. It still has the problem that enabling a warning can cause template instantiation errors, but oh well.

[rsmith]

In D47503#1115505, @rnk wrote:

[...] And rewrite the side-effecting isCompleteType call to use RequireCompleteType with the warning diagnostic.

We should keep the isCompleteType call and diagnose if (!isCompleteType(...)). RequireCompleteType needs an error diagnostic. (In particular, RequireCompleteType will produce an error suggesting a module import if a module containing the definition is known but not imported; isCompleteType will just treat the type as incomplete in that case. Don't RequireCompleteType unless you require a complete type :) )

clang/lib/Sema/SemaType.cpp

2451–2455

This doesn't seem right. Calling RequireCompleteType will trigger instantiation of the type if it's templated, which can affect the validity (and rarely, the meaning) of the program. Also, passing a warning diagnostic into RequireCompleteType doesn't actually work -- there are cases where it will disregard your diagnostic and substitute one of its own, which will be an error.

[pcc]

clang/lib/Sema/SemaType.cpp

2451–2455

You're right, but I couldn't see a way of testing whether a type is complete without triggering those side effects. It does look like we can at least avoid some of them with isCompleteType, though. I guess the best we can do is to move the diagnostic into RequireCompleteTypeImpl and make it conditional on isCompleteType as @rnk suggested.

[pcc]

• Move the warning to RequireCompleteTypeImpl

[rsmith]

clang/lib/Sema/SemaType.cpp

7599–7604

It's not reasonable to call isCompleteType here; this is non-conforming behavior, and it's not reasonable for a warning flag to change the semantics or interpretation of the program. (We actually rely on this in several ways -- -Weverything is not supposed to change the validity of programs, and reuse of implicit modules with different warning flags relies on the property that the compiler behavior is as if we compile with -Weverything and then filter the warnings after the fact.)

It seems fine to do this in the MS ABI case, since we will attempt to complete the type anyway in that mode. If you want to apply this more generally, I think there are two options: either add a -f flag to carry this semantic change, or figure out whether the type is completable without actually completing it (for example, check whether it's a templated class whose pattern is a definition).

[pcc]

clang/lib/Sema/SemaType.cpp

7599–7604

That all seems reasonable. I think the right thing to do here is to add a -fcomplete-member-pointers flag then -- couple of reasons:

• a user of this feature would probably expect to see diagnostics in the case where RequireCompleteType would fail in MS mode;
• I'm planning to use the extra semantic information that would result from turning this on in IRgen when a new -fsanitize=cfi-* flag is enabled, which would mean that we would actually need to call RequireCompleteType at Sema time. I was thinking about tying the extra RequireCompleteType calls to the new -fsanitize=cfi-* flag, but that seems somewhat questionable as well (one of the main reasons is that it would be part of the -fsanitize=cfi group, and I don't want to make -fsanitize=cfi non-conforming), so we can probably get away with just a recommendation that -fcomplete-member-pointers is used with -fsanitize=cfi.

[pcc]

• Implement as -f flag

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_qualified_catch_declarator : Error<
   "exception declarator cannot be qualified">;
 def err_early_catch_all : Error<"catch-all handler must come last">;
 def err_bad_memptr_rhs : Error<
   "right hand operand to %0 has non-pointer-to-member type %1">;
 def err_bad_memptr_lhs : Error<
   "left hand operand to %0 must be a %select{|pointer to }1class "
   "compatible with the right hand operand, but is %2">;
+def warn_memptr_incomplete : Warning<
+  "member pointer has incomplete base type %0">,
+  InGroup<DiagGroup<"incomplete-member-pointers">>, DefaultIgnore;
 def warn_exception_caught_by_earlier_handler : Warning<
   "exception of type %0 will be caught by earlier handler">,
   InGroup<Exceptions>;
 def note_previous_exception_handler : Note<"for type %0">;
 def err_exceptions_disabled : Error<
   "cannot use '%0' with exceptions disabled">;
 def err_objc_exceptions_disabled : Error<
   "cannot use '%0' with Objective-C exceptions disabled">;

clang/lib/Sema/SemaType.cpp

if (T->isVoidType()) {
     return QualType();
   }
 
   if (!Class->isDependentType() && !Class->isRecordType()) {
     Diag(Loc, diag::err_mempointer_in_nonclass_type) << Class;
     return QualType();
   }
 
   // Adjust the default free function calling convention to the default method
   // calling convention.
   bool IsCtorOrDtor =
       (Entity.getNameKind() == DeclarationName::CXXConstructorName) ||
       (Entity.getNameKind() == DeclarationName::CXXDestructorName);
   if (T->isFunctionType())
     adjustMemberFunctionCC(T, /*IsStatic=*/false, IsCtorOrDtor, Loc);
 
   return Context.getMemberPointerType(T, Class.getTypePtr());
 }
 
 /// Build a block pointer type.
 ///
bool Sema::RequireCompleteTypeImpl(SourceLocation Loc, QualType T,
   // FIXME: Add this assertion to make sure we always get instantiation points.
   //  assert(!Loc.isInvalid() && "Invalid location in RequireCompleteType");
   // FIXME: Add this assertion to help us flush out problems with
   // checking for dependent types and type-dependent expressions.
   //
   //  assert(!T->isDependentType() &&
   //         "Can't ask whether a dependent type is complete");
 
+  if (const MemberPointerType *MPTy = T->getAs<MemberPointerType>()) {
+    if (!MPTy->getClass()->isDependentType()) {
       // We lock in the inheritance model once somebody has asked us to ensure
       // that a pointer-to-member type is complete.
       if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
-    if (const MemberPointerType *MPTy = T->getAs<MemberPointerType>()) {
-      if (!MPTy->getClass()->isDependentType()) {
         (void)isCompleteType(Loc, QualType(MPTy->getClass(), 0));
         assignInheritanceModel(*this, MPTy->getMostRecentCXXRecordDecl());
       }
+
+      // We only test for this warning if enabled because testing whether a type
+      // is complete has side effects.
+      if (!Context.getDiagnostics().isIgnored(diag::warn_memptr_incomplete,
+                                              Loc) &&
+          !MPTy->getClass()->getAsCXXRecordDecl()->isBeingDefined() &&
+          !isCompleteType(Loc, QualType(MPTy->getClass(), 0)))
+        Diag(Loc, diag::warn_memptr_incomplete)
+            << QualType(MPTy->getClass(), 0);
     }
   }
 
   NamedDecl *Def = nullptr;
   bool Incomplete = T->isIncompleteType(&Def);
 
   // Check that any necessary explicit specializations are visible. For an
   // enum, we just need the declaration, so don't check this.

clang/test/SemaCXX/warn-incomplete-member-pointers.cpp

+// RUN: %clang_cc1 -verify -fsyntax-only -Wincomplete-member-pointers %s
+
+struct S;
+typedef int S::*t;
+t foo; // expected-warning {{member pointer has incomplete base type 'S'}}
+
+struct S2 {
+  int S2::*foo;
+};
+int S2::*bar;
+
+template <typename T>
+struct S3 {
+  int T::*foo;
+};