CodeGen: Properly null-check typeid expressions

[majnemer]

Summary

Thoroughly check for a pointer dereference which yields a glvalue. Look
through casts, comma operators, conditional operators, paren
expressions, etc.

[rsmith]

lib/CodeGen/CGExprCXX.cpp

1618 ↗

(On Diff #11143)

This doesn't handle

((T*)nullptr)[0]

Its semantics are defined in terms of a desugaring to

*((T*)nullptr + 0)

... so I suppose we should probably handle it here, but it's not entirely clear.

1619 ↗

(On Diff #11143)

I think this will insert a null check for

T *p;
typeid( (const T&)(T)*p )

... which doesn't seem strictly necessary (though superfluous null checks here seem pretty harmless).

[majnemer]

• Address Richard's comments.

[majnemer]

lib/CodeGen/CGExprCXX.cpp

1618 ↗

(On Diff #11143)

I've updated isGLValueFromPointerDeref to handle this.

1619 ↗

(On Diff #11143)

I can live with inserting an extra null check here.

[majnemer]

• Address Richard's comments.

[rsmith]

lib/CodeGen/CGExprCXX.cpp

1622 ↗

(On Diff #11471)

Maybe add a test for the xvalue case; something like typeid((T&&)*(T*)nullptr) ?

1636 ↗

(On Diff #11471)

How could this not be the case? Also, you have no test coverage for binary conditionals.

1660 ↗

(On Diff #11471)

Maybe extend this to say that it's not clear what this means, but we use the most generous interpretation.

[majnemer]

Closed by commit rL213401 (authored by @majnemer).

lib/CodeGen/CGExprCXX.cpp

1622 ↗

(On Diff #11471)

Done.

1636 ↗

(On Diff #11471)

A test has been added, this code has been merged with the ConditionalOperator case above.

1660 ↗

(On Diff #11471)

Done.

cfe/trunk/lib/CodeGen/CGExprCXX.cpp

void CodeGenFunction::EmitCXXDeleteExpr(const CXXDeleteExpr *E) {
   } else {
     EmitObjectDelete(*this, E->getOperatorDelete(), Ptr, DeleteTy,
                      E->isGlobalDelete());
   }
 
   EmitBlock(DeleteEnd);
 }
 
+static bool isGLValueFromPointerDeref(const Expr *E) {
+  E = E->IgnoreParenCasts();
+
+  if (const auto *UO = dyn_cast<UnaryOperator>(E))
+    if (UO->getOpcode() == UO_Deref)
+      return true;
+
+  if (const auto *BO = dyn_cast<BinaryOperator>(E))
+    if (BO->getOpcode() == BO_Comma)
+      return isGLValueFromPointerDeref(BO->getRHS());
+
+  if (const auto *CO = dyn_cast<ConditionalOperator>(E))
+    return isGLValueFromPointerDeref(CO->getTrueExpr()) ||
+           isGLValueFromPointerDeref(CO->getFalseExpr());
+
+  if (const auto *BCO = dyn_cast<BinaryConditionalOperator>(E))
+    if (const auto *OVE = dyn_cast<OpaqueValueExpr>(BCO->getTrueExpr()))
+      return isGLValueFromPointerDeref(OVE->getSourceExpr()) ||
+             isGLValueFromPointerDeref(BCO->getFalseExpr());
+
+  return false;
+}
+
 static llvm::Value *EmitTypeidFromVTable(CodeGenFunction &CGF, const Expr *E,
                                          llvm::Type *StdTypeInfoPtrTy) {
   // Get the vtable pointer.
   llvm::Value *ThisPtr = CGF.EmitLValue(E).getAddress();
 
   // C++ [expr.typeid]p2:
   //   If the glvalue expression is obtained by applying the unary * operator to
   //   a pointer and the pointer is a null pointer value, the typeid expression
   //   throws the std::bad_typeid exception.
-  bool IsDeref = false;
-  if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(E->IgnoreParens()))
-    if (UO->getOpcode() == UO_Deref)
-      IsDeref = true;
-
   QualType SrcRecordTy = E->getType();
-  if (CGF.CGM.getCXXABI().shouldTypeidBeNullChecked(IsDeref, SrcRecordTy)) {
+  if (CGF.CGM.getCXXABI().shouldTypeidBeNullChecked(
+          isGLValueFromPointerDeref(E), SrcRecordTy)) {
     llvm::BasicBlock *BadTypeidBlock =
         CGF.createBasicBlock("typeid.bad_typeid");
     llvm::BasicBlock *EndBlock = CGF.createBasicBlock("typeid.end");
 
     llvm::Value *IsNull = CGF.Builder.CreateIsNull(ThisPtr);
     CGF.Builder.CreateCondBr(IsNull, BadTypeidBlock, EndBlock);
 
     CGF.EmitBlock(BadTypeidBlock);

cfe/trunk/test/CodeGenCXX/typeid-should-throw.cpp

+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -Wno-unused-value -emit-llvm -o - | FileCheck %s
+namespace std {
+struct type_info;
+}
+
+struct A {
+  virtual ~A();
+};
+struct B : A {};
+
+void f1(A *x) { typeid(false, *x); }
+// CHECK-LABEL: define void @_Z2f1P1A
+// CHECK:       icmp eq {{.*}}, null
+// CHECK-NEXT:  br i1
+
+void f2(bool b, A *x, A *y) { typeid(b ? *x : *y); }
+// CHECK-LABEL: define void @_Z2f2bP1AS0_
+// CHECK:       icmp eq {{.*}}, null
+// CHECK-NEXT:  br i1
+
+void f3(bool b, A *x, A &y) { typeid(b ? *x : y); }
+// CHECK-LABEL: define void @_Z2f3bP1ARS_
+// CHECK:       icmp eq {{.*}}, null
+// CHECK-NEXT:  br i1
+
+void f4(bool b, A &x, A *y) { typeid(b ? x : *y); }
+// CHECK-LABEL: define void @_Z2f4bR1APS_
+// CHECK:       icmp eq {{.*}}, null
+// CHECK-NEXT:  br i1
+
+void f5(volatile A *x) { typeid(*x); }
+// CHECK-LABEL: define void @_Z2f5PV1A
+// CHECK:       icmp eq {{.*}}, null
+// CHECK-NEXT:  br i1
+
+void f6(A *x) { typeid((B &)*(B *)x); }
+// CHECK-LABEL: define void @_Z2f6P1A
+// CHECK:       icmp eq {{.*}}, null
+// CHECK-NEXT:  br i1
+
+void f7(A *x) { typeid((*x)); }
+// CHECK-LABEL: define void @_Z2f7P1A
+// CHECK:       icmp eq {{.*}}, null
+// CHECK-NEXT:  br i1