[Sema] Don't emit "pure virtual" warning for fully qualified calls.

[thakis]

Qualified calls to virtual methods still go through the vtable in
-fapple-kext mode, so you probably don't want to change that there. (Look
for AppleKext in CodeGen and Sema.)

[davide]

Interesting, thanks. I didn't know about that. I'll update the patch accordingly tomorrow.

[davide]

Addressed Nico's comments. While at it added a test.
Nico, I hope this is the right way of checking.

[thakis]

The kext bits look good to me. I'm not sure if not warning on this at all is a good idea (it looks pretty weird), but the existing warning isn't correct. Maybe the "; overrides of 'f' in subclasses are not available in the constructor of 'A'" should just be omitted in this case? (This is probably up to Richard.)

[rsmith]

In D10935#199903, @thakis wrote:

Maybe the "; overrides of 'f' in subclasses are not available in the constructor of 'A'" should just be omitted in this case? (This is probably up to Richard.)

Maybe add a note "qualified call to 'A::f' is treated as a virtual call to 'f' due to -fapple-kext"?

(Also, how about adding "has undefined behavior" before the semicolon in the warning?)

I also wonder if we should add an performsVirtualDispatch(const LangOptions &) to MemberExpr, instead of the somewhat more opaque check for a qualifier here. (There are other places where we perform a similar check, such as MarkExprReferenced and Sema::MarkMemberReferenced, which get the kext case wrong.)

lib/Sema/SemaOverload.cpp

11796 ↗

(On Diff #29118)

Use hasQualifier instead of getQualifier.

[davide]

Addressed Richards' comments.

[rsmith]

LGTM

include/clang/AST/Expr.h

2584 ↗

(On Diff #29631)

Would it make sense for this to also check whether the named member is a virtual function?

include/clang/Basic/DiagnosticSemaKinds.td

1316 ↗

(On Diff #29631)

Remove the 'u' from behaviour; our diagnostics are in US English.

cfe/trunk/include/clang/AST/Expr.h

 #include "clang/AST/ASTVector.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclAccessPair.h"
 #include "clang/AST/OperationKinds.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
+#include "clang/Basic/LangOptions.h"
 #include "clang/Basic/TypeTraits.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Compiler.h"
 
 namespace clang {
public:
   }
   /// \brief Sets the flag telling whether this expression refers to
   /// a method that was resolved from an overloaded set having size
   /// greater than 1.
   void setHadMultipleCandidates(bool V = true) {
     HadMultipleCandidates = V;
   }
 
+  /// \brief Returns true if virtual dispatch is performed.
+  /// If the member access is fully qualified, (i.e. X::f()), virtual
+  /// dispatching is not performed. In -fapple-kext mode qualified
+  /// calls to virtual method will still go through the vtable.
+  bool performsVirtualDispatch(const LangOptions &LO) const {
+    return LO.AppleKext || !hasQualifier();
+  }
+
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == MemberExprClass;
   }
 
   // Iterators
   child_range children() { return child_range(&Base, &Base+1); }
 
   friend class ASTReader;

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def err_type_defined_in_param_type : Error<
   "%0 cannot be defined in a parameter type">;
 def err_type_defined_in_alias_template : Error<
   "%0 cannot be defined in a type alias template">;
 
 def note_pure_virtual_function : Note<
   "unimplemented pure virtual method %0 in %1">;
 
+def note_pure_qualified_call_kext : Note<
+  "qualified call to %0::%1 is treated as a virtual call to %1 due to -fapple-kext">;
+
 def err_deleted_decl_not_first : Error<
   "deleted definition must be first declaration">;
 
 def err_deleted_override : Error<
   "deleted function %0 cannot override a non-deleted function">;
 
 def err_non_deleted_override : Error<
   "non-deleted function %0 cannot override a deleted function">;
def ext_pure_function_definition : ExtWarn<
   InGroup<Microsoft>;
 def err_implicit_object_parameter_init : Error<
   "cannot initialize object parameter of type %0 with an expression "
   "of type %1">;
 def err_qualified_member_of_unrelated : Error<
   "%q0 is not a member of class %1">;
 
 def warn_call_to_pure_virtual_member_function_from_ctor_dtor : Warning<
-  "call to pure virtual member function %0; overrides of %0 in subclasses are "
-  "not available in the %select{constructor|destructor}1 of %2">;
+  "call to pure virtual member function %0 has undefined behavior; "
+  "overrides of %0 in subclasses are not available in the "
+  "%select{constructor|destructor}1 of %2">;
 
 def note_member_declared_at : Note<"member is declared here">;
 def note_ivar_decl : Note<"instance variable is declared here">;
 def note_bitfield_decl : Note<"bit-field is declared here">;
 def note_implicit_param_decl : Note<"%0 is an implicit parameter">;
 def note_member_synthesized_at : Note<
   "implicit %select{default constructor|copy constructor|move constructor|copy "
   "assignment operator|move assignment operator|destructor}0 for %1 first "

cfe/trunk/lib/Sema/SemaExpr.cpp

static void MarkExprReferenced(Sema &SemaRef, SourceLocation Loc,
   // derived class used in case codegen can devirtualize the call.
   const MemberExpr *ME = dyn_cast<MemberExpr>(E);
   if (!ME)
     return;
   CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(ME->getMemberDecl());
   if (!MD)
     return;
   // Only attempt to devirtualize if this is truly a virtual call.
-  bool IsVirtualCall = MD->isVirtual() && !ME->hasQualifier();
+  bool IsVirtualCall = MD->isVirtual() &&
+                          ME->performsVirtualDispatch(SemaRef.getLangOpts());
   if (!IsVirtualCall)
     return;
   const Expr *Base = ME->getBase();
   const CXXRecordDecl *MostDerivedClassDecl = Base->getBestDynamicClassType();
   if (!MostDerivedClassDecl)
     return;
   CXXMethodDecl *DM = MD->getCorrespondingMethodInClass(MostDerivedClassDecl);
   if (!DM || DM->isPure())

 void Sema::MarkMemberReferenced(MemberExpr *E) {
   // C++11 [basic.def.odr]p2:
   //   A non-overloaded function whose name appears as a potentially-evaluated
   //   expression or a member of a set of candidate functions, if selected by
   //   overload resolution when referred to from a potentially-evaluated
   //   expression, is odr-used, unless it is a pure virtual function and its
   //   name is not explicitly qualified.
   bool OdrUse = true;
-  if (!E->hasQualifier()) {
+  if (E->performsVirtualDispatch(getLangOpts())) {
     if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(E->getMemberDecl()))
       if (Method->isPure())
         OdrUse = false;
   }
   SourceLocation Loc = E->getMemberLoc().isValid() ?
                             E->getMemberLoc() : E->getLocStart();
   MarkExprReferenced(*this, Loc, E->getMemberDecl(), E, OdrUse);
 }

cfe/trunk/lib/Sema/SemaOverload.cpp

Sema::BuildCallToMemberFunction(Scope *S, Expr *MemExprE,
   if (CheckFunctionCall(Method, TheCall, Proto))
     return ExprError();
 
   if ((isa<CXXConstructorDecl>(CurContext) || 
        isa<CXXDestructorDecl>(CurContext)) && 
       TheCall->getMethodDecl()->isPure()) {
     const CXXMethodDecl *MD = TheCall->getMethodDecl();
 
-    if (isa<CXXThisExpr>(MemExpr->getBase()->IgnoreParenCasts())) {
-      Diag(MemExpr->getLocStart(), 
+    if (isa<CXXThisExpr>(MemExpr->getBase()->IgnoreParenCasts()) &&
+        MemExpr->performsVirtualDispatch(getLangOpts())) {
+      Diag(MemExpr->getLocStart(),
            diag::warn_call_to_pure_virtual_member_function_from_ctor_dtor)
         << MD->getDeclName() << isa<CXXDestructorDecl>(CurContext)
         << MD->getParent()->getDeclName();
 
       Diag(MD->getLocStart(), diag::note_previous_decl) << MD->getDeclName();
+      if (getLangOpts().AppleKext)
+        Diag(MemExpr->getLocStart(),
+             diag::note_pure_qualified_call_kext)
+             << MD->getParent()->getDeclName()
+             << MD->getDeclName();
     }
   }
   return MaybeBindToTemporary(TheCall);
 }
 
 /// BuildCallToObjectOfClassType - Build a call to an object of class
 /// type (C++ [over.call.object]), which can end up invoking an
 /// overloaded function call operator (@c operator()) or performing a

cfe/trunk/test/SemaCXX/warn-pure-virtual-call-from-ctor-dtor.cpp

 // RUN: %clang_cc1 %s -fsyntax-only -verify
 struct A {
-  A() { f(); } // expected-warning {{call to pure virtual member function 'f'; overrides of 'f' in subclasses are not available in the constructor of 'A'}}
-  ~A() { f(); } // expected-warning {{call to pure virtual member function 'f'; overrides of 'f' in subclasses are not available in the destructor of 'A'}}
+  A() { f(); } // expected-warning {{call to pure virtual member function 'f' has undefined behavior; overrides of 'f' in subclasses are not available in the constructor of 'A'}}
+  ~A() { f(); } // expected-warning {{call to pure virtual member function 'f' has undefined behavior; overrides of 'f' in subclasses are not available in the destructor of 'A'}}
 
   virtual void f() = 0; // expected-note 2 {{'f' declared here}}
 };
 
 // Don't warn (or note) when calling the function on a pointer. (PR10195)
 struct B {
   A *a;
   B() { a->f(); };
   ~B() { a->f(); };
 };
+
+// Don't warn if the call is fully qualified. (PR23215)
+struct C {
+    virtual void f() = 0;
+    C() {
+        C::f();
+    }
+};

cfe/trunk/test/SemaCXX/warn-pure-virtual-kext.cpp

+// RUN: %clang_cc1 %s -fapple-kext -fsyntax-only -verify
+
+struct A {
+    virtual void f() = 0; // expected-note {{'f' declared here}}
+    A() {
+        A::f(); // expected-warning {{call to pure virtual member function 'f' has undefined behavior; overrides of 'f' in subclasses are not available in the constructor of 'A'}} // expected-note {{qualified call to 'A'::'f' is treated as a virtual call to 'f' due to -fapple-kext}}
+    }
+};