[cfe-commits] r159212 - in /cfe/trunk: include/clang/AST/DeclCXX.h include/clang/AST/Expr.h lib/AST/DeclCXX.cpp lib/AST/Expr.cpp lib/CodeGen/CGExprCXX.cpp lib/Sema/SemaExpr.cpp test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp

[John McCall]

On Jun 26, 2012, at 10:45 AM, Rafael Espindola wrote:

> Author: rafael
> Date: Tue Jun 26 12:45:31 2012
> New Revision: 159212
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=159212&view=rev
> Log:
> During codegen of a virtual call we would extract any casts in the expression
> to see if we had an underlying final class or method, but we would then
> use the cast type to do the call, resulting in a direct call to the wrong
> method.
> 
> Modified:
>    cfe/trunk/include/clang/AST/DeclCXX.h
>    cfe/trunk/include/clang/AST/Expr.h
>    cfe/trunk/lib/AST/DeclCXX.cpp
>    cfe/trunk/lib/AST/Expr.cpp
>    cfe/trunk/lib/CodeGen/CGExprCXX.cpp
>    cfe/trunk/lib/Sema/SemaExpr.cpp
>    cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp
> 
> Modified: cfe/trunk/include/clang/AST/DeclCXX.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=159212&r1=159211&r2=159212&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/DeclCXX.h (original)
> +++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Jun 26 12:45:31 2012
> @@ -1630,7 +1630,20 @@
>   /// supplied by IR generation to either forward to the function call operator
>   /// or clone the function call operator.
>   bool isLambdaStaticInvoker() const;
> -  
> +
> +  /// \brief Find the method in RD that corresponds to this one.
> +  ///
> +  /// Find if RD or one of the classes it inherits from override this method.
> +  /// If so, return it. RD is assumed to be a base class of the class defining
> +  /// this method (or be the class itself).
> +  CXXMethodDecl *
> +  getCorrespondingMethodInClass(const CXXRecordDecl *RD);
> +
> +  const CXXMethodDecl *
> +  getCorrespondingMethodInClass(const CXXRecordDecl *RD) const {
> +    return const_cast<CXXMethodDecl*>(this)->getCorrespondingMethodInClass(RD);
> +  }
> +
>   // Implement isa/cast/dyncast/etc.
>   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
>   static bool classof(const CXXMethodDecl *D) { return true; }
> 
> Modified: cfe/trunk/include/clang/AST/Expr.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=159212&r1=159211&r2=159212&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/Expr.h (original)
> +++ cfe/trunk/include/clang/AST/Expr.h Tue Jun 26 12:45:31 2012
> @@ -665,6 +665,13 @@
> 
>   static bool hasAnyTypeDependentArguments(llvm::ArrayRef<Expr *> Exprs);
> 
> +  /// \brief If we have class type (or pointer to class type), return the
> +  /// class decl. Return NULL otherwise.
> +  ///
> +  /// If this expression is a cast, this method looks through it to find the
> +  /// most derived decl that can be infered from the expression.
> +  const CXXRecordDecl *getMostDerivedClassDeclForType() const;
> +

This is not a great name for this.  We want this method to return the best
available information about the dynamic type of the object.  I would call it
getBestDynamicClassType().

Also, it would be nice to see some documentation of why this is a legal
analysis.  In particular, it's legal because derived-to-base conversions
have undefined behavior if the object isn't dynamically of the derived type,
so we can look through derived-to-base conversions with full confidence
that the dynamic type must be at least the derived type.

Also, I see no reason for this method to be valid on an expression that
isn't of class or pointer-to-class type.  It should just assert.

> Modified: cfe/trunk/lib/AST/Expr.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Expr.cpp?rev=159212&r1=159211&r2=159212&view=diff
> ==============================================================================
> --- cfe/trunk/lib/AST/Expr.cpp (original)
> +++ cfe/trunk/lib/AST/Expr.cpp Tue Jun 26 12:45:31 2012
> @@ -33,6 +33,37 @@
> #include <cstring>
> using namespace clang;
> 
> +const CXXRecordDecl *Expr::getMostDerivedClassDeclForType() const {
> +  const Expr *E = this;
> +
> +  while (true) {
> +    E = E->IgnoreParens();
> +    if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
> +      if (CE->getCastKind() == CK_DerivedToBase ||
> +          CE->getCastKind() == CK_UncheckedDerivedToBase ||
> +          CE->getCastKind() == CK_NoOp) {
> +        E = CE->getSubExpr();
> +        continue;
> +      }
> +    }
> +
> +    break;
> +  }
> +
> +  QualType DerivedType = E->getType();
> +  if (DerivedType->isDependentType())
> +    return NULL;

It's not clear to me that this is necessary.

> +  if (const PointerType *PTy = DerivedType->getAs<PointerType>())
> +    DerivedType = PTy->getPointeeType();
> +
> +  const RecordType *Ty = DerivedType->castAs<RecordType>();
> +  if (!Ty)
> +    return NULL;

castAs<> can never return null;  it's an asserting cast, like cast<>.

> Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=159212&r1=159211&r2=159212&view=diff
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Tue Jun 26 12:45:31 2012
> @@ -56,30 +56,6 @@
>                   Callee, ReturnValue, Args, MD);
> }
> 
> -static const CXXRecordDecl *getMostDerivedClassDecl(const Expr *Base) {
> -  const Expr *E = Base;
> -  
> -  while (true) {
> -    E = E->IgnoreParens();
> -    if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
> -      if (CE->getCastKind() == CK_DerivedToBase || 
> -          CE->getCastKind() == CK_UncheckedDerivedToBase ||
> -          CE->getCastKind() == CK_NoOp) {
> -        E = CE->getSubExpr();
> -        continue;
> -      }
> -    }
> -
> -    break;
> -  }
> -
> -  QualType DerivedType = E->getType();
> -  if (const PointerType *PTy = DerivedType->getAs<PointerType>())
> -    DerivedType = PTy->getPointeeType();
> -
> -  return cast<CXXRecordDecl>(DerivedType->castAs<RecordType>()->getDecl());
> -}
> -
> // FIXME: Ideally Expr::IgnoreParenNoopCasts should do this, but it doesn't do
> // quite what we want.
> static const Expr *skipNoOpCastsAndParens(const Expr *E) {
> @@ -126,7 +102,8 @@
>   //   b->f();
>   // }
>   //
> -  const CXXRecordDecl *MostDerivedClassDecl = getMostDerivedClassDecl(Base);
> +  const CXXRecordDecl *MostDerivedClassDecl =
> +    Base->getMostDerivedClassDeclForType();
>   if (MostDerivedClassDecl->hasAttr<FinalAttr>())
>     return true;
> 
> @@ -247,10 +224,13 @@
>   //
>   // We also don't emit a virtual call if the base expression has a record type
>   // because then we know what the type is.
> -  bool UseVirtualCall;
> -  UseVirtualCall = MD->isVirtual() && !ME->hasQualifier()
> -                   && !canDevirtualizeMemberFunctionCalls(getContext(),
> -                                                          ME->getBase(), MD);
> +  const Expr *Base = ME->getBase();
> +  bool UseVirtualCall = MD->isVirtual() && !ME->hasQualifier()
> +                        && !canDevirtualizeMemberFunctionCalls(getContext(),
> +                                                               Base, MD);
> +  const CXXRecordDecl *MostDerivedClassDecl =
> +    Base->getMostDerivedClassDeclForType();
> +
>   llvm::Value *Callee;
>   if (const CXXDestructorDecl *Dtor = dyn_cast<CXXDestructorDecl>(MD)) {
>     if (UseVirtualCall) {
> @@ -260,8 +240,13 @@
>           MD->isVirtual() &&
>           ME->hasQualifier())
>         Callee = BuildAppleKextVirtualCall(MD, ME->getQualifier(), Ty);
> -      else
> -        Callee = CGM.GetAddrOfFunction(GlobalDecl(Dtor, Dtor_Complete), Ty);
> +      else {
> +        const CXXMethodDecl *DM =
> +          Dtor->getCorrespondingMethodInClass(MostDerivedClassDecl);
> +        assert(DM);
> +        const CXXDestructorDecl *DDtor = cast<CXXDestructorDecl>(DM);
> +        Callee = CGM.GetAddrOfFunction(GlobalDecl(DDtor, Dtor_Complete), Ty);
> +      }
>     }
>   } else if (const CXXConstructorDecl *Ctor =
>                dyn_cast<CXXConstructorDecl>(MD)) {
> @@ -273,8 +258,12 @@
>         MD->isVirtual() &&
>         ME->hasQualifier())
>       Callee = BuildAppleKextVirtualCall(MD, ME->getQualifier(), Ty);
> -    else 
> -      Callee = CGM.GetAddrOfFunction(MD, Ty);
> +    else {
> +      const CXXMethodDecl *DerivedMethod =
> +        MD->getCorrespondingMethodInClass(MostDerivedClassDecl);
> +      assert(DerivedMethod);
> +      Callee = CGM.GetAddrOfFunction(DerivedMethod, Ty);
> +    }
>   }

There is another latent bug in this optimization:  'this' will have been
converted to the base type, but we may be calling a method on a
derived type, which will then be passed an invalid 'this' pointer.

Why don't we just constrain the optimization for now to only apply
when the static method has not been overridden in the most-derived
type?  That will catch the common case of derived->foo().

John.