This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by pcc on May 29 2018, 3:56 PM.

Details

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.

Event Timeline

pcc created this revision.May 29 2018, 3:56 PM
pcc updated this revision to Diff 149000.May 29 2018, 3:59 PM
  • Add some negative tests
pcc updated this revision to Diff 149001.May 29 2018, 4:00 PM
  • One more negative test
rnk added a comment.May 29 2018, 4:19 PM

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.

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 added inline comments.May 29 2018, 5:58 PM
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 updated this revision to Diff 149017.May 29 2018, 6:26 PM
  • Move the warning to RequireCompleteTypeImpl
rsmith added inline comments.May 29 2018, 6:44 PM
clang/lib/Sema/SemaType.cpp
7605–7610

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 added inline comments.May 29 2018, 7:40 PM
clang/lib/Sema/SemaType.cpp
7605–7610

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 updated this revision to Diff 149029.May 29 2018, 8:32 PM
  • Implement as -f flag
pcc retitled this revision from Sema: Add a warning for member pointers with incomplete base types. to Sema: Add a flag for rejecting member pointers with incomplete base types..May 29 2018, 8:35 PM
pcc edited the summary of this revision. (Show Details)
rsmith accepted this revision.May 29 2018, 8:35 PM
This revision is now accepted and ready to land.May 29 2018, 8:35 PM
This revision was automatically updated to reflect the committed changes.