[PATCH] Implement function type checker for the undefined behavior sanitizer.

[Richard Smith]

  Clever! The approach only makes me feel a //little// uneasy =)

  It would be polite to issue a diagnostic if someone explicitly asks for this (not via `-fsanitize=undefined`) and we don't support it for their target.


================
Comment at: lib/CodeGen/CGExpr.cpp:3129
@@ +3128,3 @@
+      !dyn_cast_or_null<FunctionDecl>(TargetDecl)) {
+    if (llvm::Constant* PDSig =
+            CGM.getTargetCodeGenInfo().getUBSanFunctionSignature(CGM)) {
----------------
`*` on the right.

================
Comment at: lib/CodeGen/CGExpr.cpp:3128
@@ +3127,3 @@
+  if (getLangOpts().CPlusPlus && SanOpts->Function &&
+      !dyn_cast_or_null<FunctionDecl>(TargetDecl)) {
+    if (llvm::Constant* PDSig =
----------------
This is a bit hard to parse. `(!TargetDecl || !isa<FunctionDecl>(TargetDecl))` maybe?

================
Comment at: lib/CodeGen/CGExpr.cpp:3133-3134
@@ +3132,4 @@
+          CGM.GetAddrOfRTTIDescriptor(QualType(FnType, 0), /*ForEH=*/true);
+      llvm::Type *PDStructTyElems[] = {PDSig->getType(),
+                                       FTRTTIConst->getType()};
+      llvm::StructType *PDStructTy = llvm::StructType::get(
----------------
We usually format this as

    llvm::Type *PDStructTyElems[] = {
      PDSig->getType(),
      FTRTTIConst->getType()
    };

or similar.

================
Comment at: lib/CodeGen/CGExpr.cpp:3155-3156
@@ +3154,4 @@
+          Builder.CreateICmpEQ(CalleeRTTI, FTRTTIConst);
+      llvm::Constant *StaticData[] = {EmitCheckSourceLocation(CallLoc),
+                                      EmitCheckTypeDescriptor(CalleeType)};
+      EmitCheck(CalleeRTTIMatch,
----------------
Likewise.

================
Comment at: lib/CodeGen/CGExpr.cpp:3133
@@ +3132,3 @@
+          CGM.GetAddrOfRTTIDescriptor(QualType(FnType, 0), /*ForEH=*/true);
+      llvm::Type *PDStructTyElems[] = {PDSig->getType(),
+                                       FTRTTIConst->getType()};
----------------
Richard Smith wrote:
> We usually format this as
> 
>     llvm::Type *PDStructTyElems[] = {
>       PDSig->getType(),
>       FTRTTIConst->getType()
>     };
> 
> or similar.
Some indication of what "PD" stands for here would be useful.

================
Comment at: lib/CodeGen/CGExpr.cpp:3138-3142
@@ +3137,7 @@
+
+      llvm::Value *CalleePDStruct = Builder.CreateBitCast(
+          Callee, llvm::PointerType::getUnqual(PDStructTy));
+      llvm::Value *CalleeSigPtr =
+          Builder.CreateConstGEP2_32(CalleePDStruct, 0, 0);
+      llvm::Value *CalleeSig = Builder.CreateLoad(CalleeSigPtr);
+      llvm::Value *CalleeSigMatch = Builder.CreateICmpEQ(CalleeSig, PDSig);
----------------
What happens if the callee address is at the end of a page and doesn't have the extra data? Could this load trigger a segfault?

================
Comment at: lib/CodeGen/CodeGenFunction.cpp:531
@@ +530,3 @@
+            CGM.GetAddrOfRTTIDescriptor(FD->getType(), /*ForEH=*/true);
+        llvm::Constant *PDStructElems[] = {PDSig, FTRTTIConst};
+        llvm::Constant *PDStructConst =
----------------
Spaces around braces.

================
Comment at: lib/CodeGen/CodeGenFunction.cpp:523-524
@@ -521,1 +522,4 @@
 
+  // If we are checking function types, emit a function type signature as
+  // prefix data.
+  if (getLangOpts().CPlusPlus && SanOpts->Function) {
----------------
It'd be nice if we could only do this for address-taken functions. When we take the address of a function, we could emit `linkonce_odr` thunk with the extra data, and use that instead of the original address... except that would break address-of-function comparisons between instrumented and uninstrumented code. Can we provide an option to enable that behavior for the case where we're OK with an ABI change?

================
Comment at: lib/CodeGen/TargetInfo.cpp:607-608
@@ +606,4 @@
+                   (0x06 << 8) |  //           .+0x08
+                   ('F' << 16) |
+                   ('T' << 24);
+    return llvm::ConstantInt::get(CGM.Int32Ty, Sig);
----------------
What do 'F' and 'T' demangle as? An invalid instruction encoding would give me more confidence here.

================
Comment at: lib/CodeGen/CodeGenFunction.cpp:529-530
@@ +528,4 @@
+              CGM.getTargetCodeGenInfo().getUBSanFunctionSignature(CGM)) {
+        llvm::Constant *FTRTTIConst =
+            CGM.GetAddrOfRTTIDescriptor(FD->getType(), /*ForEH=*/true);
+        llvm::Constant *PDStructElems[] = {PDSig, FTRTTIConst};
----------------
Does this include the calling convention attributes?


http://llvm-reviews.chandlerc.com/D1338