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

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

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

I've updated isGLValueFromPointerDeref to handle this.

1619

I can live with inserting an extra null check here.

[majnemer]

• Address Richard's comments.

[rsmith]

lib/CodeGen/CGExprCXX.cpp

1622

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

1636

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

1660

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

Done.

1636

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

1660

Done.

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->IgnoreParens();
+
+  if (const auto *CE = dyn_cast<CastExpr>(E)) {
+    if (!CE->getSubExpr()->isGLValue())
+      return false;
+    return isGLValueFromPointerDeref(CE->getSubExpr());
+  }
+
+  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());
+
+  // C++11 [expr.sub]p1:
+  //   The expression E1[E2] is identical (by definition) to *((E1)+(E2))
+  if (isa<ArraySubscriptExpr>(E))
+    return true;
+
+  if (const auto *UO = dyn_cast<UnaryOperator>(E))
+    if (UO->getOpcode() == UO_Deref)
+      return true;
+
+  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);

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
+
+void f8(A *x) { typeid(x[0]); }
+// CHECK-LABEL: define void @_Z2f8P1A
+// CHECK:       icmp eq {{.*}}, null
+// CHECK-NEXT:  br i1
+
+void f9(A *x) { typeid(0[x]); }
+// CHECK-LABEL: define void @_Z2f9P1A
+// CHECK:       icmp eq {{.*}}, null
+// CHECK-NEXT:  br i1
+
+void f10(A *x) { typeid((const A &)(A)*x); }
+// CHECK-LABEL: define void @_Z3f10P1A
+// CHECK-NOT:   icmp eq {{.*}}, null