Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-03 Thread Evgeniy Stepanov via cfe-commits 
eugenis closed this revision.
eugenis added a comment.

http://llvm.org/viewvc/llvm-project?rev=259716=rev


Repository:
  rL LLVM

http://reviews.llvm.org/D16823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-03 Thread Evgeniy Stepanov via cfe-commits 
eugenis added inline comments.


Comment at: lib/CodeGen/CGClass.cpp:2608
@@ -2607,3 +2607,3 @@
   auto TypeId = CGM.CreateCfiIdForTypeMetadata(MD);
   if (CGM.getCodeGenOpts().SanitizeCfiCrossDso && TypeId) {
 EmitCfiSlowPathCheck(M, BitSetTest, TypeId, CastedVTable, StaticData);

samsonov wrote:
> Can we rewrite this as if-elseif-else block now?
even better, with 2 early returns.


Repository:
  rL LLVM

http://reviews.llvm.org/D16823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-03 Thread Evgeniy Stepanov via cfe-commits 
eugenis updated this revision to Diff 46807.

Repository:
  rL LLVM

http://reviews.llvm.org/D16823

Files:
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  test/CodeGen/cfi-check-fail.c
  test/CodeGenCXX/cfi-cast.cpp
  test/CodeGenCXX/cfi-vcall.cpp

Index: test/CodeGenCXX/cfi-vcall.cpp
===
--- test/CodeGenCXX/cfi-vcall.cpp
+++ test/CodeGenCXX/cfi-vcall.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -fsanitize-trap=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=ITANIUM --check-prefix=NDIAG %s
-// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=ITANIUM --check-prefix=DIAG --check-prefix=DIAG-ABORT %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -fsanitize-trap=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=ITANIUM --check-prefix=ITANIUM-NDIAG --check-prefix=NDIAG %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=ITANIUM --check-prefix=ITANIUM-DIAG --check-prefix=DIAG --check-prefix=DIAG-ABORT %s
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -fsanitize-recover=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=ITANIUM --check-prefix=DIAG --check-prefix=DIAG-RECOVER %s
 // RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fsanitize=cfi-vcall -fsanitize-trap=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=MS --check-prefix=NDIAG %s
 
@@ -55,23 +55,25 @@
 
 // DIAG: @[[SRC:.*]] = private unnamed_addr constant [{{.*}} x i8] c"{{.*}}cfi-vcall.cpp\00", align 1
 // DIAG: @[[TYPE:.*]] = private unnamed_addr constant { i16, i16, [4 x i8] } { i16 -1, i16 0, [4 x i8] c"'A'\00" }
-// DIAG: @[[BADTYPESTATIC:.*]] = private unnamed_addr global { i8, { [{{.*}} x i8]*, i32, i32 }, { i16, i16, [4 x i8] }* } { i8 0, { [{{.*}} x i8]*, i32, i32 } { [{{.*}} x i8]* @[[SRC]], i32 [[@LINE+21]], i32 3 }, { i16, i16, [4 x i8] }* @[[TYPE]] }
+// DIAG: @[[BADTYPESTATIC:.*]] = private unnamed_addr global { i8, { [{{.*}} x i8]*, i32, i32 }, { i16, i16, [4 x i8] }* } { i8 0, { [{{.*}} x i8]*, i32, i32 } { [{{.*}} x i8]* @[[SRC]], i32 [[@LINE+23]], i32 3 }, { i16, i16, [4 x i8] }* @[[TYPE]] }
 
 // ITANIUM: define void @_Z2afP1A
 // MS: define void @"\01?af@@YAXPEAUA@@@Z"
 void af(A *a) {
   // ITANIUM: [[P:%[^ ]*]] = call i1 @llvm.bitset.test(i8* [[VT:%[^ ]*]], metadata !"_ZTS1A")
   // MS: [[P:%[^ ]*]] = call i1 @llvm.bitset.test(i8* [[VT:%[^ ]*]], metadata !"?AUA@@")
+  // DIAG-NEXT: [[VTVALID0:%[^ ]*]] = call i1 @llvm.bitset.test(i8* [[VT]], metadata !"all-vtables")
   // CHECK-NEXT: br i1 [[P]], label %[[CONTBB:[^ ,]*]], label %[[TRAPBB:[^ ,]*]]
   // CHECK-NEXT: {{^$}}
 
   // CHECK: [[TRAPBB]]
   // NDIAG-NEXT: call void @llvm.trap()
   // NDIAG-NEXT: unreachable
   // DIAG-NEXT: [[VTINT:%[^ ]*]] = ptrtoint i8* [[VT]] to i64
-  // DIAG-ABORT-NEXT: call void @__ubsan_handle_cfi_check_fail_abort(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]])
+  // DIAG-NEXT: [[VTVALID:%[^ ]*]] = zext i1 [[VTVALID0]] to i64
+  // DIAG-ABORT-NEXT: call void @__ubsan_handle_cfi_check_fail_abort(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]], i64 [[VTVALID]])
   // DIAG-ABORT-NEXT: unreachable
-  // DIAG-RECOVER-NEXT: call void @__ubsan_handle_cfi_check_fail(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]])
+  // DIAG-RECOVER-NEXT: call void @__ubsan_handle_cfi_check_fail(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]], i64 [[VTVALID]])
   // DIAG-RECOVER-NEXT: br label %[[CONTBB]]
 
   // CHECK: [[CONTBB]]
@@ -157,32 +159,47 @@
 
 }
 
-// Check for the expected number of elements (9 or 15 respectively).
-// MS: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){8}]]}
-// ITANIUM: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){14}]]}
+// Check for the expected number of elements (15 or 23 respectively).
+// MS-NDIAG: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){9}]]}
+// MS-DIAG: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){15}]]}
+// ITANIUM-NDIAG: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){14}]]}
+// ITANIUM-DIAG: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){23}]]}
 
 // ITANIUM-DAG: !{!"_ZTS1A", [3 x i8*]* @_ZTV1A, i64 16}
+// ITANIUM-DIAG-DAG: !{!"all-vtables", [3 x i8*]* @_ZTV1A, i64 16}
 // ITANIUM-DAG: !{!"_ZTS1A", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
+// ITANIUM-DIAG-DAG: !{!"all-vtables", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1B", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [9 x i8*]* @_ZTCN12_GLOBAL__N_11DE8_1C, i64 64}
+// ITANIUM-DIAG-DAG: !{!"all-vtables", [9 x i8*]*

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-03 Thread Alexey Samsonov via cfe-commits 
samsonov accepted this revision.
samsonov added a reviewer: samsonov.
samsonov added a comment.
This revision is now accepted and ready to land.

LGTM



Comment at: lib/CodeGen/CGClass.cpp:2608
@@ -2607,3 +2607,3 @@
   auto TypeId = CGM.CreateCfiIdForTypeMetadata(MD);
   if (CGM.getCodeGenOpts().SanitizeCfiCrossDso && TypeId) {
 EmitCfiSlowPathCheck(M, BitSetTest, TypeId, CastedVTable, StaticData);

Can we rewrite this as if-elseif-else block now?


Comment at: lib/CodeGen/CGExpr.cpp:2642
@@ +2641,3 @@
+  llvm::MDString::get(CGM.getLLVMContext(), "all-vtables"));
+  llvm::Value *ValidVtable = Builder.CreateZExt(
+  Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::bitset_test),

eugenis wrote:
> samsonov wrote:
> > This is almost the same as EmitVTablePtrCheck, but with ZExt? Is the 
> > difference intentional/important? Is it possible to extract this logic 
> > (getting "all-vtables" metadata and running bitset test) to a function?
> Not important. Zext makes the test a bit simpler.
> Extracting these two lines to a function is surely possible, but is it worth 
> it?
> 
Probably not, as long as we have the test coverage.


Repository:
  rL LLVM

http://reviews.llvm.org/D16823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-02 Thread Evgeniy Stepanov via cfe-commits 
eugenis created this revision.
eugenis added reviewers: pcc, kcc.
eugenis added a subscriber: cfe-commits.
eugenis set the repository for this revision to rL LLVM.

Avoid crashing when printing diagnostics for vtable-related CFI
errors. In diagnostic mode, the frontend does an additional check of
the vtable pointer against the set of all known vtable addresses and
lets the runtime handler know if it is safe to inspect the vtable.

Repository:
  rL LLVM

http://reviews.llvm.org/D16823

Files:
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenModule.cpp
  test/CodeGen/cfi-check-fail.c
  test/CodeGenCXX/cfi-cast.cpp
  test/CodeGenCXX/cfi-vcall.cpp

Index: test/CodeGenCXX/cfi-vcall.cpp
===
--- test/CodeGenCXX/cfi-vcall.cpp
+++ test/CodeGenCXX/cfi-vcall.cpp
@@ -55,7 +55,7 @@
 
 // DIAG: @[[SRC:.*]] = private unnamed_addr constant [{{.*}} x i8] c"{{.*}}cfi-vcall.cpp\00", align 1
 // DIAG: @[[TYPE:.*]] = private unnamed_addr constant { i16, i16, [4 x i8] } { i16 -1, i16 0, [4 x i8] c"'A'\00" }
-// DIAG: @[[BADTYPESTATIC:.*]] = private unnamed_addr global { i8, { [{{.*}} x i8]*, i32, i32 }, { i16, i16, [4 x i8] }* } { i8 0, { [{{.*}} x i8]*, i32, i32 } { [{{.*}} x i8]* @[[SRC]], i32 [[@LINE+21]], i32 3 }, { i16, i16, [4 x i8] }* @[[TYPE]] }
+// DIAG: @[[BADTYPESTATIC:.*]] = private unnamed_addr global { i8, { [{{.*}} x i8]*, i32, i32 }, { i16, i16, [4 x i8] }* } { i8 0, { [{{.*}} x i8]*, i32, i32 } { [{{.*}} x i8]* @[[SRC]], i32 [[@LINE+23]], i32 3 }, { i16, i16, [4 x i8] }* @[[TYPE]] }
 
 // ITANIUM: define void @_Z2afP1A
 // MS: define void @"\01?af@@YAXPEAUA@@@Z"
@@ -68,10 +68,12 @@
   // CHECK: [[TRAPBB]]
   // NDIAG-NEXT: call void @llvm.trap()
   // NDIAG-NEXT: unreachable
+  // DIAG-NEXT: [[VTVALID0:%[^ ]*]] = call i1 @llvm.bitset.test(i8* [[VT]], metadata !"all-vtables")
   // DIAG-NEXT: [[VTINT:%[^ ]*]] = ptrtoint i8* [[VT]] to i64
-  // DIAG-ABORT-NEXT: call void @__ubsan_handle_cfi_check_fail_abort(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]])
+  // DIAG-NEXT: [[VTVALID:%[^ ]*]] = zext i1 [[VTVALID0]] to i64
+  // DIAG-ABORT-NEXT: call void @__ubsan_handle_cfi_check_fail_abort(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]], i64 [[VTVALID]])
   // DIAG-ABORT-NEXT: unreachable
-  // DIAG-RECOVER-NEXT: call void @__ubsan_handle_cfi_check_fail(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]])
+  // DIAG-RECOVER-NEXT: call void @__ubsan_handle_cfi_check_fail(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]], i64 [[VTVALID]])
   // DIAG-RECOVER-NEXT: br label %[[CONTBB]]
 
   // CHECK: [[CONTBB]]
@@ -157,32 +159,45 @@
 
 }
 
-// Check for the expected number of elements (9 or 15 respectively).
-// MS: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){8}]]}
-// ITANIUM: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){14}]]}
+// Check for the expected number of elements (15 or 23 respectively).
+// MS: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){15}]]}
+// ITANIUM: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){23}]]}
 
 // ITANIUM-DAG: !{!"_ZTS1A", [3 x i8*]* @_ZTV1A, i64 16}
+// ITANIUM-DAG: !{!"all-vtables", [3 x i8*]* @_ZTV1A, i64 16}
 // ITANIUM-DAG: !{!"_ZTS1A", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
+// ITANIUM-DAG: !{!"all-vtables", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1B", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [9 x i8*]* @_ZTCN12_GLOBAL__N_11DE8_1C, i64 64}
+// ITANIUM-DAG: !{!"all-vtables", [9 x i8*]* @_ZTCN12_GLOBAL__N_11DE8_1C, i64 64}
 // ITANIUM-DAG: !{!"_ZTS1C", [9 x i8*]* @_ZTCN12_GLOBAL__N_11DE8_1C, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
+// ITANIUM-DAG: !{!"all-vtables", [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1B", [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1C", [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 88}
+// ITANIUM-DAG: !{!"all-vtables", [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 88}
 // ITANIUM-DAG: !{![[DTYPE]], [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [7 x i8*]* @_ZTV1B, i64 32}
+// ITANIUM-DAG: !{!"all-vtables", [7 x i8*]* @_ZTV1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1B", [7 x i8*]* @_ZTV1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [5 x i8*]* @_ZTV1C, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1C", [5 x i8*]* @_ZTV1C, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [3 x i8*]* @_ZTVZ3foovE2FA, i64 16}
 // ITANIUM-DAG: !{!{{[0-9]+}}, [3 x i8*]* @_ZTVZ3foovE2FA, i64 16}
 
 // MS-DAG: !{!"?AUA@@", [2 x i8*]* @[[VTA]], i64 8}
+// MS-DAG: !{!"all-vtables", [2 x i8*]* @[[VTA]], i64 8}
 // MS-DAG: !{!"?AUB@@", [3 x i8*]* @[[VTB]], i64 8}
+// MS-DAG: !{!"all-vtables", [3 x i8*]* @[[VTB]], i64 8}
 // MS-DAG: !{!"?AUA@@", [2 x i8*]* @[[VTAinB]], i64 8}
+// MS-DAG: !{!"all-vtables", [2 x i8*]*

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-02 Thread Evgeniy Stepanov via cfe-commits 
eugenis added inline comments.


Comment at: lib/CodeGen/CGExpr.cpp:2494
@@ +2493,3 @@
+  llvm::Value *ValidVtable = nullptr;
+  if (CheckAndAppendValidVtable) {
+llvm::Value *AllVtables = llvm::MetadataAsValue::get(

samsonov wrote:
> This is really ugly. Why are you not passing it down in DynamicArgs? Is it 
> performance penalty you don't want to pay if the check will not succeed? How 
> large will it be?
Yes, I want this code to be on the failing side of the check.
This would cost about the same as the check itself, so I suspect it could 
double the overhead.



Repository:
  rL LLVM

http://reviews.llvm.org/D16823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-02 Thread Alexey Samsonov via cfe-commits 
samsonov added a subscriber: samsonov.


Comment at: lib/CodeGen/CGExpr.cpp:2494
@@ +2493,3 @@
+  llvm::Value *ValidVtable = nullptr;
+  if (CheckAndAppendValidVtable) {
+llvm::Value *AllVtables = llvm::MetadataAsValue::get(

This is really ugly. Why are you not passing it down in DynamicArgs? Is it 
performance penalty you don't want to pay if the check will not succeed? How 
large will it be?


Repository:
  rL LLVM

http://reviews.llvm.org/D16823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-02 Thread Evgeniy Stepanov via cfe-commits 
eugenis added inline comments.


Comment at: lib/CodeGen/CGExpr.cpp:2494
@@ +2493,3 @@
+  llvm::Value *ValidVtable = nullptr;
+  if (CheckAndAppendValidVtable) {
+llvm::Value *AllVtables = llvm::MetadataAsValue::get(

pcc wrote:
> eugenis wrote:
> > samsonov wrote:
> > > This is really ugly. Why are you not passing it down in DynamicArgs? Is 
> > > it performance penalty you don't want to pay if the check will not 
> > > succeed? How large will it be?
> > Yes, I want this code to be on the failing side of the check.
> > This would cost about the same as the check itself, so I suspect it could 
> > double the overhead.
> > 
> I would just emit the call unconditionally. We don't care too much about the 
> performance in non-trapping mode, and if it becomes a problem in practice we 
> can see if we can have the optimizer move the call into the conditional block 
> (which I suspect it already knows how to do).
I care about performance in non-trapping mode.
Doing this change would not make the code any less ugly. For example, EmitCheck 
may not use the argument if the check has -fsanitize-trap behaviour, in which 
case we get an unused llvm.bitset.test call that affects some of the clang 
tests.



Repository:
  rL LLVM

http://reviews.llvm.org/D16823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-02 Thread Peter Collingbourne via cfe-commits 
pcc added inline comments.


Comment at: lib/CodeGen/CGExpr.cpp:2494
@@ +2493,3 @@
+  llvm::Value *ValidVtable = nullptr;
+  if (CheckAndAppendValidVtable) {
+llvm::Value *AllVtables = llvm::MetadataAsValue::get(

eugenis wrote:
> samsonov wrote:
> > This is really ugly. Why are you not passing it down in DynamicArgs? Is it 
> > performance penalty you don't want to pay if the check will not succeed? 
> > How large will it be?
> Yes, I want this code to be on the failing side of the check.
> This would cost about the same as the check itself, so I suspect it could 
> double the overhead.
> 
I would just emit the call unconditionally. We don't care too much about the 
performance in non-trapping mode, and if it becomes a problem in practice we 
can see if we can have the optimizer move the call into the conditional block 
(which I suspect it already knows how to do).


Comment at: lib/CodeGen/CodeGenModule.cpp:4053
@@ +4052,3 @@
+
+  if (!CodeGenOpts.SanitizeTrap.has(SanitizerKind::CFIVCall) ||
+  !CodeGenOpts.SanitizeTrap.has(SanitizerKind::CFINVCall) ||

This conditional doesn't look right. It should be something like 

```if (sanitize.has(this) && !sanitizetrap.has(this)) ||
   (sanitize.has(that) && !sanitizetrap.has(that)) ||
...
```
But that's sufficiently ugly that I wonder if we should just do this 
unconditionally. It shouldn't make a difference to the generated code either 
way.


Repository:
  rL LLVM

http://reviews.llvm.org/D16823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-02 Thread Alexey Samsonov via cfe-commits 
samsonov added inline comments.


Comment at: lib/CodeGen/CGExpr.cpp:2642
@@ +2641,3 @@
+  llvm::MDString::get(CGM.getLLVMContext(), "all-vtables"));
+  llvm::Value *ValidVtable = Builder.CreateZExt(
+  Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::bitset_test),

This is almost the same as EmitVTablePtrCheck, but with ZExt? Is the difference 
intentional/important? Is it possible to extract this logic (getting 
"all-vtables" metadata and running bitset test) to a function?


Comment at: lib/CodeGen/CodeGenModule.cpp:4034
@@ +4033,3 @@
+  // is not in the trapping mode.
+  return ((LangOpts.Sanitize.has(SanitizerKind::CFIVCall) &&
+   !CodeGenOpts.SanitizeTrap.has(SanitizerKind::CFIVCall)) ||

Hm, can you write this as a loop?


Repository:
  rL LLVM

http://reviews.llvm.org/D16823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-02 Thread Evgeniy Stepanov via cfe-commits 
eugenis updated this revision to Diff 46718.
eugenis added a comment.

Moved bitset.text call outside.
LLVM is smart enough to sink it along the cold branch, so performance should 
not suffer.


Repository:
  rL LLVM

http://reviews.llvm.org/D16823

Files:
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CodeGenModule.cpp
  test/CodeGen/cfi-check-fail.c
  test/CodeGenCXX/cfi-cast.cpp
  test/CodeGenCXX/cfi-vcall.cpp

Index: test/CodeGenCXX/cfi-vcall.cpp
===
--- test/CodeGenCXX/cfi-vcall.cpp
+++ test/CodeGenCXX/cfi-vcall.cpp
@@ -55,23 +55,25 @@
 
 // DIAG: @[[SRC:.*]] = private unnamed_addr constant [{{.*}} x i8] c"{{.*}}cfi-vcall.cpp\00", align 1
 // DIAG: @[[TYPE:.*]] = private unnamed_addr constant { i16, i16, [4 x i8] } { i16 -1, i16 0, [4 x i8] c"'A'\00" }
-// DIAG: @[[BADTYPESTATIC:.*]] = private unnamed_addr global { i8, { [{{.*}} x i8]*, i32, i32 }, { i16, i16, [4 x i8] }* } { i8 0, { [{{.*}} x i8]*, i32, i32 } { [{{.*}} x i8]* @[[SRC]], i32 [[@LINE+21]], i32 3 }, { i16, i16, [4 x i8] }* @[[TYPE]] }
+// DIAG: @[[BADTYPESTATIC:.*]] = private unnamed_addr global { i8, { [{{.*}} x i8]*, i32, i32 }, { i16, i16, [4 x i8] }* } { i8 0, { [{{.*}} x i8]*, i32, i32 } { [{{.*}} x i8]* @[[SRC]], i32 [[@LINE+23]], i32 3 }, { i16, i16, [4 x i8] }* @[[TYPE]] }
 
 // ITANIUM: define void @_Z2afP1A
 // MS: define void @"\01?af@@YAXPEAUA@@@Z"
 void af(A *a) {
   // ITANIUM: [[P:%[^ ]*]] = call i1 @llvm.bitset.test(i8* [[VT:%[^ ]*]], metadata !"_ZTS1A")
   // MS: [[P:%[^ ]*]] = call i1 @llvm.bitset.test(i8* [[VT:%[^ ]*]], metadata !"?AUA@@")
+  // DIAG-NEXT: [[VTVALID0:%[^ ]*]] = call i1 @llvm.bitset.test(i8* [[VT]], metadata !"all-vtables")
   // CHECK-NEXT: br i1 [[P]], label %[[CONTBB:[^ ,]*]], label %[[TRAPBB:[^ ,]*]]
   // CHECK-NEXT: {{^$}}
 
   // CHECK: [[TRAPBB]]
   // NDIAG-NEXT: call void @llvm.trap()
   // NDIAG-NEXT: unreachable
   // DIAG-NEXT: [[VTINT:%[^ ]*]] = ptrtoint i8* [[VT]] to i64
-  // DIAG-ABORT-NEXT: call void @__ubsan_handle_cfi_check_fail_abort(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]])
+  // DIAG-NEXT: [[VTVALID:%[^ ]*]] = zext i1 [[VTVALID0]] to i64
+  // DIAG-ABORT-NEXT: call void @__ubsan_handle_cfi_check_fail_abort(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]], i64 [[VTVALID]])
   // DIAG-ABORT-NEXT: unreachable
-  // DIAG-RECOVER-NEXT: call void @__ubsan_handle_cfi_check_fail(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]])
+  // DIAG-RECOVER-NEXT: call void @__ubsan_handle_cfi_check_fail(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]], i64 [[VTVALID]])
   // DIAG-RECOVER-NEXT: br label %[[CONTBB]]
 
   // CHECK: [[CONTBB]]
@@ -157,32 +159,45 @@
 
 }
 
-// Check for the expected number of elements (9 or 15 respectively).
-// MS: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){8}]]}
-// ITANIUM: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){14}]]}
+// Check for the expected number of elements (15 or 23 respectively).
+// MS: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){15}]]}
+// ITANIUM: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){23}]]}
 
 // ITANIUM-DAG: !{!"_ZTS1A", [3 x i8*]* @_ZTV1A, i64 16}
+// ITANIUM-DAG: !{!"all-vtables", [3 x i8*]* @_ZTV1A, i64 16}
 // ITANIUM-DAG: !{!"_ZTS1A", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
+// ITANIUM-DAG: !{!"all-vtables", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1B", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [9 x i8*]* @_ZTCN12_GLOBAL__N_11DE8_1C, i64 64}
+// ITANIUM-DAG: !{!"all-vtables", [9 x i8*]* @_ZTCN12_GLOBAL__N_11DE8_1C, i64 64}
 // ITANIUM-DAG: !{!"_ZTS1C", [9 x i8*]* @_ZTCN12_GLOBAL__N_11DE8_1C, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
+// ITANIUM-DAG: !{!"all-vtables", [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1B", [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1C", [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 88}
+// ITANIUM-DAG: !{!"all-vtables", [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 88}
 // ITANIUM-DAG: !{![[DTYPE]], [12 x i8*]* @_ZTVN12_GLOBAL__N_11DE, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [7 x i8*]* @_ZTV1B, i64 32}
+// ITANIUM-DAG: !{!"all-vtables", [7 x i8*]* @_ZTV1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1B", [7 x i8*]* @_ZTV1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [5 x i8*]* @_ZTV1C, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1C", [5 x i8*]* @_ZTV1C, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [3 x i8*]* @_ZTVZ3foovE2FA, i64 16}
 // ITANIUM-DAG: !{!{{[0-9]+}}, [3 x i8*]* @_ZTVZ3foovE2FA, i64 16}
 
 // MS-DAG: !{!"?AUA@@", [2 x i8*]* @[[VTA]], i64 8}
+// MS-DAG: !{!"all-vtables", [2 x i8*]* @[[VTA]], i64 8}
 // MS-DAG: !{!"?AUB@@", [3 x i8*]* @[[VTB]], i64 8}
+// MS-DAG: !{!"all-vtables", [3 x i8*]* @[[VTB]], i64 8}
 // MS-DAG: !{!"?AUA@@", [2 x i8*]* @[[VTAinB]], i64 8}
+// MS-DAG:

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-02 Thread Evgeniy Stepanov via cfe-commits 
eugenis updated this revision to Diff 46723.
eugenis marked an inline comment as done.

Repository:
  rL LLVM

http://reviews.llvm.org/D16823

Files:
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  test/CodeGen/cfi-check-fail.c
  test/CodeGenCXX/cfi-cast.cpp
  test/CodeGenCXX/cfi-vcall.cpp

Index: test/CodeGenCXX/cfi-vcall.cpp
===
--- test/CodeGenCXX/cfi-vcall.cpp
+++ test/CodeGenCXX/cfi-vcall.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -fsanitize-trap=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=ITANIUM --check-prefix=NDIAG %s
-// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=ITANIUM --check-prefix=DIAG --check-prefix=DIAG-ABORT %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -fsanitize-trap=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=ITANIUM --check-prefix=ITANIUM-NDIAG --check-prefix=NDIAG %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=ITANIUM --check-prefix=ITANIUM-DIAG --check-prefix=DIAG --check-prefix=DIAG-ABORT %s
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -fsanitize=cfi-vcall -fsanitize-recover=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=ITANIUM --check-prefix=DIAG --check-prefix=DIAG-RECOVER %s
 // RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fsanitize=cfi-vcall -fsanitize-trap=cfi-vcall -emit-llvm -o - %s | FileCheck --check-prefix=CHECK --check-prefix=MS --check-prefix=NDIAG %s
 
@@ -55,23 +55,25 @@
 
 // DIAG: @[[SRC:.*]] = private unnamed_addr constant [{{.*}} x i8] c"{{.*}}cfi-vcall.cpp\00", align 1
 // DIAG: @[[TYPE:.*]] = private unnamed_addr constant { i16, i16, [4 x i8] } { i16 -1, i16 0, [4 x i8] c"'A'\00" }
-// DIAG: @[[BADTYPESTATIC:.*]] = private unnamed_addr global { i8, { [{{.*}} x i8]*, i32, i32 }, { i16, i16, [4 x i8] }* } { i8 0, { [{{.*}} x i8]*, i32, i32 } { [{{.*}} x i8]* @[[SRC]], i32 [[@LINE+21]], i32 3 }, { i16, i16, [4 x i8] }* @[[TYPE]] }
+// DIAG: @[[BADTYPESTATIC:.*]] = private unnamed_addr global { i8, { [{{.*}} x i8]*, i32, i32 }, { i16, i16, [4 x i8] }* } { i8 0, { [{{.*}} x i8]*, i32, i32 } { [{{.*}} x i8]* @[[SRC]], i32 [[@LINE+23]], i32 3 }, { i16, i16, [4 x i8] }* @[[TYPE]] }
 
 // ITANIUM: define void @_Z2afP1A
 // MS: define void @"\01?af@@YAXPEAUA@@@Z"
 void af(A *a) {
   // ITANIUM: [[P:%[^ ]*]] = call i1 @llvm.bitset.test(i8* [[VT:%[^ ]*]], metadata !"_ZTS1A")
   // MS: [[P:%[^ ]*]] = call i1 @llvm.bitset.test(i8* [[VT:%[^ ]*]], metadata !"?AUA@@")
+  // DIAG-NEXT: [[VTVALID0:%[^ ]*]] = call i1 @llvm.bitset.test(i8* [[VT]], metadata !"all-vtables")
   // CHECK-NEXT: br i1 [[P]], label %[[CONTBB:[^ ,]*]], label %[[TRAPBB:[^ ,]*]]
   // CHECK-NEXT: {{^$}}
 
   // CHECK: [[TRAPBB]]
   // NDIAG-NEXT: call void @llvm.trap()
   // NDIAG-NEXT: unreachable
   // DIAG-NEXT: [[VTINT:%[^ ]*]] = ptrtoint i8* [[VT]] to i64
-  // DIAG-ABORT-NEXT: call void @__ubsan_handle_cfi_check_fail_abort(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]])
+  // DIAG-NEXT: [[VTVALID:%[^ ]*]] = zext i1 [[VTVALID0]] to i64
+  // DIAG-ABORT-NEXT: call void @__ubsan_handle_cfi_check_fail_abort(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]], i64 [[VTVALID]])
   // DIAG-ABORT-NEXT: unreachable
-  // DIAG-RECOVER-NEXT: call void @__ubsan_handle_cfi_check_fail(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]])
+  // DIAG-RECOVER-NEXT: call void @__ubsan_handle_cfi_check_fail(i8* getelementptr inbounds ({{.*}} @[[BADTYPESTATIC]], i32 0, i32 0), i64 [[VTINT]], i64 [[VTVALID]])
   // DIAG-RECOVER-NEXT: br label %[[CONTBB]]
 
   // CHECK: [[CONTBB]]
@@ -157,32 +159,47 @@
 
 }
 
-// Check for the expected number of elements (9 or 15 respectively).
-// MS: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){8}]]}
-// ITANIUM: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){14}]]}
+// Check for the expected number of elements (15 or 23 respectively).
+// MS-NDIAG: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){9}]]}
+// MS-DIAG: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){15}]]}
+// ITANIUM-NDIAG: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){14}]]}
+// ITANIUM-DIAG: !llvm.bitsets = !{[[X:[^,]*(,[^,]*){23}]]}
 
 // ITANIUM-DAG: !{!"_ZTS1A", [3 x i8*]* @_ZTV1A, i64 16}
+// ITANIUM-DIAG-DAG: !{!"all-vtables", [3 x i8*]* @_ZTV1A, i64 16}
 // ITANIUM-DAG: !{!"_ZTS1A", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
+// ITANIUM-DIAG-DAG: !{!"all-vtables", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1B", [7 x i8*]* @_ZTCN12_GLOBAL__N_11DE0_1B, i64 32}
 // ITANIUM-DAG: !{!"_ZTS1A", [9 x i8*]* @_ZTCN12_GLOBAL__N_11DE8_1C, i64 64}
+// ITANIUM-DIAG-DAG:

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-02 Thread Evgeniy Stepanov via cfe-commits 
eugenis added inline comments.


Comment at: lib/CodeGen/CodeGenModule.cpp:4053
@@ +4052,3 @@
+
+  if (!CodeGenOpts.SanitizeTrap.has(SanitizerKind::CFIVCall) ||
+  !CodeGenOpts.SanitizeTrap.has(SanitizerKind::CFINVCall) ||

I don't like emitting all these bitset entries if they are not needed.
Fixed.


Repository:
  rL LLVM

http://reviews.llvm.org/D16823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16823: [cfi] Safe handling of unaddressable vtable pointers (clang).

2016-02-02 Thread Evgeniy Stepanov via cfe-commits 
eugenis added inline comments.


Comment at: lib/CodeGen/CGExpr.cpp:2642
@@ +2641,3 @@
+  llvm::MDString::get(CGM.getLLVMContext(), "all-vtables"));
+  llvm::Value *ValidVtable = Builder.CreateZExt(
+  Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::bitset_test),

samsonov wrote:
> This is almost the same as EmitVTablePtrCheck, but with ZExt? Is the 
> difference intentional/important? Is it possible to extract this logic 
> (getting "all-vtables" metadata and running bitset test) to a function?
Not important. Zext makes the test a bit simpler.
Extracting these two lines to a function is surely possible, but is it worth it?



Repository:
  rL LLVM

http://reviews.llvm.org/D16823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits