[PATCH] D50927: [Sema] Remove location from implicit capture init expr
vsk created this revision.
vsk added reviewers: rsmith, rtrieu, erichkeane.
A lambda's closure is initialized when the lambda is declared. For
implicit captures, the initialization code emitted from EmitLambdaExpr
references source locations *within the lambda body* in the function
containing the lambda. This results in a poor debugging experience: we
step to the line containing the lambda, then into lambda, out again,
over and over, until every capture's field is initialized.
To improve stepping behavior, assign an empty location to expressions
which initialize an implicit capture within a closure. This prevents the
debugger from stepping into a lambda when single-stepping in its parent
function.
rdar://39807527
https://reviews.llvm.org/D50927
Files:
clang/include/clang/AST/Expr.h
clang/lib/AST/Expr.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaLambda.cpp
clang/test/CodeGenCXX/debug-info-lambda.cpp
clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
===
--- clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
@@ -75,11 +75,14 @@
TEST(RecursiveASTVisitor, VisitsImplicitLambdaCaptureInit) {
DeclRefExprVisitor Visitor;
Visitor.setShouldVisitImplicitCode(true);
- // We're expecting the "i" in the lambda to be visited twice:
- // - Once for the DeclRefExpr in the lambda capture initialization (whose
- // source code location is set to the first use of the variable).
- // - Once for the DeclRefExpr for the use of "i" inside the lambda.
- Visitor.ExpectMatch("i", 1, 24, /*Times=*/2);
+ // We're expecting the "i" in the lambda to be visited just once (for the
+ // DeclRefExpr for the use of "i" inside the lambda).
+ //
+ // Previously, the DeclRefExpr in the implicit lambda capture initialization
+ // (whose source code location is set to the first use of the variable) was
+ // also matched. This behavior was removed because it resulted in poor debug
+ // info.
+ Visitor.ExpectMatch("i", 1, 24, /*Times=*/1);
EXPECT_TRUE(Visitor.runOver(
"void f() { int i; [=]{ i; }; }",
DeclRefExprVisitor::Lang_CXX11));
Index: clang/test/CodeGenCXX/debug-info-lambda.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-lambda.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm \
+// RUN: -debug-info-kind=line-tables-only -std=c++11 %s -o - | FileCheck %s
+
+// CHECK-LABEL: define{{.*}}lambda_in_func
+void lambda_in_func(int &ref) {
+ // CHECK: [[ref_slot:%.*]] = getelementptr inbounds %class.anon, %class.anon* {{.*}}, i32 0, i32 0, !dbg [[lambda_decl_loc:![0-9]+]]
+ // CHECK-NEXT: %1 = load i32*, i32** %ref.addr, align 8, !dbg [[init_sequence_loc:![0-9]+]]
+ // CHECK-NEXT: store i32* %1, i32** %0, align 8, !dbg [[lambda_decl_loc]]
+ // CHECK-NEXT: call void {{.*}}, !dbg [[lambda_call_loc:![0-9]+]]
+
+ auto helper = [&]() { // CHECK: [[lambda_decl_loc]] = !DILocation(line: [[@LINE]]
+++ref; // CHECK: [[init_sequence_loc]] = !DILocation(line: 0
+ };
+ helper(); // CHECK: [[lambda_call_loc]] = !DILocation(line: [[@LINE]]
+}
Index: clang/lib/Sema/SemaLambda.cpp
===
--- clang/lib/Sema/SemaLambda.cpp
+++ clang/lib/Sema/SemaLambda.cpp
@@ -1392,13 +1392,13 @@
Class->addDecl(Conversion);
}
-static ExprResult performLambdaVarCaptureInitialization(Sema &S,
-const Capture &Capture,
-FieldDecl *Field) {
+static ExprResult performLambdaVarCaptureInitialization(
+Sema &S, const Capture &Capture, FieldDecl *Field, bool IsImplicitCapture) {
assert(Capture.isVariableCapture() && "not a variable capture");
auto *Var = Capture.getVariable();
SourceLocation Loc = Capture.getLocation();
+ SourceLocation InitLoc = IsImplicitCapture ? SourceLocation() : Loc;
// C++11 [expr.prim.lambda]p21:
// When the lambda-expression is evaluated, the entities that
@@ -1413,7 +1413,7 @@
// An entity captured by a lambda-expression is odr-used (3.2) in
// the scope containing the lambda-expression.
ExprResult RefResult = S.BuildDeclarationNameExpr(
- CXXScopeSpec(), DeclarationNameInfo(Var->getDeclName(), Loc), Var);
+ CXXScopeSpec(), DeclarationNameInfo(Var->getDeclName(), InitLoc), Var);
if (RefResult.isInvalid())
return ExprError();
Expr *Ref = RefResult.get();
@@ -1607,8 +1607,8 @@
Var, From.getEllipsisLoc()));
Expr *Init = From.getInitExpr();
if (!Init) {
-auto InitResult =
-performLambdaVarCaptureInitialization(*this, From, *CurField);
+
[PATCH] D42043: c-index: CXString: fix MSAN read-past-end bug
vsk added subscribers: jkorous, vsapsai. vsk added a comment. + Jan and Volodymyr. This seemed to be in good shape the last time I looked at it. Not having touched libclang for a while I don't think I can give an official lgtm. Repository: rC Clang https://reviews.llvm.org/D42043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50927: [Sema] Remove location from implicit capture init expr
vsk updated this revision to Diff 161536.
vsk added a comment.
- Here is a GIF that might help illustrate the bug:
http://net.vedantk.com/static/llvm/lambda-implicit-capture-bug.gif
- Update test/SemaCXX/uninitialized.cpp to highlight the behavior change which
comes from using getBeginOrDeclLoc().
https://reviews.llvm.org/D50927
Files:
clang/include/clang/AST/Expr.h
clang/lib/AST/Expr.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaLambda.cpp
clang/test/CodeGenCXX/debug-info-lambda.cpp
clang/test/SemaCXX/uninitialized.cpp
clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
===
--- clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
@@ -75,11 +75,14 @@
TEST(RecursiveASTVisitor, VisitsImplicitLambdaCaptureInit) {
DeclRefExprVisitor Visitor;
Visitor.setShouldVisitImplicitCode(true);
- // We're expecting the "i" in the lambda to be visited twice:
- // - Once for the DeclRefExpr in the lambda capture initialization (whose
- // source code location is set to the first use of the variable).
- // - Once for the DeclRefExpr for the use of "i" inside the lambda.
- Visitor.ExpectMatch("i", 1, 24, /*Times=*/2);
+ // We're expecting the "i" in the lambda to be visited just once (for the
+ // DeclRefExpr for the use of "i" inside the lambda).
+ //
+ // Previously, the DeclRefExpr in the implicit lambda capture initialization
+ // (whose source code location is set to the first use of the variable) was
+ // also matched. This behavior was removed because it resulted in poor debug
+ // info.
+ Visitor.ExpectMatch("i", 1, 24, /*Times=*/1);
EXPECT_TRUE(Visitor.runOver(
"void f() { int i; [=]{ i; }; }",
DeclRefExprVisitor::Lang_CXX11));
Index: clang/test/SemaCXX/uninitialized.cpp
===
--- clang/test/SemaCXX/uninitialized.cpp
+++ clang/test/SemaCXX/uninitialized.cpp
@@ -884,8 +884,10 @@
int x;
};
A a0([] { return a0.x; }); // ok
- void f() {
-A a1([=] { return a1.x; }); // expected-warning{{variable 'a1' is uninitialized when used within its own initialization}}
+ void f() {
+A a1([=] { // expected-warning{{variable 'a1' is uninitialized when used within its own initialization}}
+ return a1.x;
+});
A a2([&] { return a2.x; }); // ok
}
}
Index: clang/test/CodeGenCXX/debug-info-lambda.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-lambda.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm \
+// RUN: -debug-info-kind=line-tables-only -std=c++11 %s -o - | FileCheck %s
+
+// CHECK-LABEL: define{{.*}}lambda_in_func
+void lambda_in_func(int &ref) {
+ // CHECK: [[ref_slot:%.*]] = getelementptr inbounds %class.anon, %class.anon* {{.*}}, i32 0, i32 0, !dbg [[lambda_decl_loc:![0-9]+]]
+ // CHECK-NEXT: %1 = load i32*, i32** %ref.addr, align 8, !dbg [[init_sequence_loc:![0-9]+]]
+ // CHECK-NEXT: store i32* %1, i32** %0, align 8, !dbg [[lambda_decl_loc]]
+ // CHECK-NEXT: call void {{.*}}, !dbg [[lambda_call_loc:![0-9]+]]
+
+ auto helper = [&]() { // CHECK: [[lambda_decl_loc]] = !DILocation(line: [[@LINE]]
+++ref; // CHECK: [[init_sequence_loc]] = !DILocation(line: 0
+ };
+ helper(); // CHECK: [[lambda_call_loc]] = !DILocation(line: [[@LINE]]
+}
Index: clang/lib/Sema/SemaLambda.cpp
===
--- clang/lib/Sema/SemaLambda.cpp
+++ clang/lib/Sema/SemaLambda.cpp
@@ -1392,13 +1392,13 @@
Class->addDecl(Conversion);
}
-static ExprResult performLambdaVarCaptureInitialization(Sema &S,
-const Capture &Capture,
-FieldDecl *Field) {
+static ExprResult performLambdaVarCaptureInitialization(
+Sema &S, const Capture &Capture, FieldDecl *Field, bool IsImplicitCapture) {
assert(Capture.isVariableCapture() && "not a variable capture");
auto *Var = Capture.getVariable();
SourceLocation Loc = Capture.getLocation();
+ SourceLocation InitLoc = IsImplicitCapture ? SourceLocation() : Loc;
// C++11 [expr.prim.lambda]p21:
// When the lambda-expression is evaluated, the entities that
@@ -1413,7 +1413,7 @@
// An entity captured by a lambda-expression is odr-used (3.2) in
// the scope containing the lambda-expression.
ExprResult RefResult = S.BuildDeclarationNameExpr(
- CXXScopeSpec(), DeclarationNameInfo(Var->getDeclName(), Loc), Var);
+ CXXScopeSpec(), DeclarationNameInfo(Var->getDeclName(), InitLoc), Var);
if (RefResult.isInvalid())
return ExprError();
Expr *Ref = RefResult.get();
@@ -1607,8 +1607,8 @@
[PATCH] D50901: [clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks
vsk added inline comments.
Comment at:
test/CodeGen/catch-implicit-integer-truncations-basics-negatives.c:23
+__attribute__((no_sanitize("integer"))) unsigned char
blacklist_1_convert_unsigned_int_to_unsigned_char(unsigned int x) {
+ // CHECK: }
+ return x;
It looks like 'CHECK: }' is meant to check that the end of the function is
reached without any diagnostic handlers being emitted. If so, you'll need
something stricter to actually check that.
Considering removing all of the 'CHECK: }' lines and adding
`-implicit-check-not=__ubsan_handle_implicit` as a FileCheck option.
Comment at:
test/CodeGen/catch-implicit-signed-integer-truncations-basics-negatives.c:17
+__attribute__((no_sanitize("integer"))) signed char
blacklist_1_convert_signed_int_to_signed_char(signed int x) {
+ // CHECK: }
+ return x;
Ditto, I think this applies to the rest of the tests.
Repository:
rC Clang
https://reviews.llvm.org/D50901
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46694: [diagtool] Install diagtool
vsk added a comment. Sgtm. I think it would be worthwhile to release-note this. Repository: rC Clang https://reviews.llvm.org/D46694 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46918: [Coverage] Discard the last uncompleted deferred region in a decl
vsk created this revision.
vsk added a reviewer: arphaman.
Discard the last uncompleted deferred region in a decl, if one exists.
This prevents lines at the end of a function containing only whitespace
or closing braces from being marked as uncovered, if they follow a
region terminator (return/break/etc).
The previous behavior was to heuristically complete deferred regions at
the end of a decl. In practice this ended up being too brittle for too
little gain. Users would complain that there was no way to reach full
code coverage because whitespace at the end of a function would be
marked uncovered.
rdar://40238228
https://reviews.llvm.org/D46918
Files:
lib/CodeGen/CoverageMappingGen.cpp
test/CoverageMapping/deferred-region.cpp
test/CoverageMapping/label.cpp
test/CoverageMapping/moremacros.c
test/CoverageMapping/trycatch.cpp
Index: test/CoverageMapping/trycatch.cpp
===
--- test/CoverageMapping/trycatch.cpp
+++ test/CoverageMapping/trycatch.cpp
@@ -18,7 +18,7 @@
// CHECK: File 0, [[@LINE+1]]:10 -> [[@LINE+2]]:27 = (#0 - #1)
} else if(i == 8) // CHECK-NEXT: File 0, [[@LINE]]:13 -> [[@LINE]]:19 = (#0 - #1)
throw ImportantError(); // CHECK: File 0, [[@LINE]]:5 -> [[@LINE]]:27 = #2
-} // CHECK-NEXT: File 0, [[@LINE-1]]:27 -> [[@LINE]]:2 = ((#0 - #1) - #2)
+}
// CHECK-NEXT: main
int main() { // CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+13]]:2 = #0
Index: test/CoverageMapping/moremacros.c
===
--- test/CoverageMapping/moremacros.c
+++ test/CoverageMapping/moremacros.c
@@ -31,7 +31,7 @@
if (!argc) {
return 0;
// CHECK-NEXT: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:8 = #4
- RBRAC // CHECK-NEXT: [[@LINE]]:8 -> [[@LINE+1]]:2 = (((#0 - #2) - #3) - #4)
+ RBRAC
}
// CHECK-NEXT: File 1, 3:15 -> 3:16 = #2
Index: test/CoverageMapping/label.cpp
===
--- test/CoverageMapping/label.cpp
+++ test/CoverageMapping/label.cpp
@@ -16,19 +16,18 @@
goto x;// CHECK: File 0, [[@LINE]]:7 -> [[@LINE]]:13 = (#1 - #2)
int k = 3; // CHECK-NEXT: File 0, [[@LINE-1]]:13 -> [[@LINE]]:5 = #3
} // CHECK-NEXT: File 0, [[@LINE-1]]:5 -> [[@LINE]]:4 = #3
- static int j = 0; // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+5]]:2 = ((#0 + #3) - #1)
+ static int j = 0; // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+4]]:2 = ((#0 + #3) - #1)
++j;
if(j == 1) // CHECK-NEXT: File 0, [[@LINE]]:6 -> [[@LINE]]:12 = ((#0 + #3) - #1)
goto x; // CHECK: File 0, [[@LINE]]:5 -> [[@LINE]]:11 = #4
- // CHECK-NEXT: File 0, [[@LINE-1]]:11 -> [[@LINE+1]]:2 = (((#0 + #3) - #1) - #4)
}
// CHECK-NEXT: test1
void test1(int x) { // CHECK-NEXT: File 0, [[@LINE]]:19 -> {{[0-9]+}}:2 = #0
if(x == 0) // CHECK-NEXT: File 0, [[@LINE]]:6 -> [[@LINE]]:12 = #0
goto a; // CHECK: File 0, [[@LINE]]:5 -> [[@LINE]]:11 = #1
// CHECK-NEXT: File 0, [[@LINE-1]]:11 -> [[@LINE+1]]:3 = (#0 - #1)
- goto b;// CHECK: Gap,File 0, [[@LINE]]:3 -> [[@LINE+5]]:2 = #3
+ goto b;// CHECK: File 0, [[@LINE]]:3 -> [[@LINE+5]]:2 = (#0 - #1)
// CHECK-NEXT: Gap,File 0, [[@LINE-1]]:9 -> [[@LINE+1]]:1 = #2
a: // CHECK-NEXT: File 0, [[@LINE]]:1 -> [[@LINE+3]]:2 = #2
b: // CHECK-NEXT: File 0, [[@LINE]]:1 -> [[@LINE+2]]:2 = #3
Index: test/CoverageMapping/deferred-region.cpp
===
--- test/CoverageMapping/deferred-region.cpp
+++ test/CoverageMapping/deferred-region.cpp
@@ -7,19 +7,20 @@
void foo(int x) {
if (x == 0) {
return;
- } // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+2]]:2 = (#0 - #1)
-
+ } // CHECK-NOT: Gap,File 0, [[@LINE]]:4
+//< Don't complete the last deferred region in a decl, even though it may
+//< leave some whitespace marked with the same counter as the final return.
}
-// CHECK-NEXT: _Z4foooi:
+// CHECK-LABEL: _Z4foooi:
void fooo(int x) {
if (x == 0) {
return;
} // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+2]]:3 = (#0 - #1)
if (x == 1) {
return;
- } // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+2]]:2 = ((#0 - #1) - #2)
+ } // CHECK-NOT: Gap,File 0, [[@LINE]]:4
}
@@ -108,7 +109,7 @@
}
if (false)
-return; // CHECK: Gap,File 0, [[@LINE]]:11 -> [[@LINE+1]]:2
+return; // CHECK-NOT: Gap,File 0, [[@LINE]]:11
}
// CHECK-LABEL: _Z8for_loopv:
@@ -167,7 +168,7 @@
return; // CHECK: [[@LINE]]:3
[PATCH] D46918: [Coverage] Discard the last uncompleted deferred region in a decl
vsk updated this revision to Diff 147360.
vsk added a comment.
- Add a regression test for switches.
https://reviews.llvm.org/D46918
Files:
lib/CodeGen/CoverageMappingGen.cpp
test/CoverageMapping/deferred-region.cpp
test/CoverageMapping/label.cpp
test/CoverageMapping/moremacros.c
test/CoverageMapping/trycatch.cpp
Index: test/CoverageMapping/trycatch.cpp
===
--- test/CoverageMapping/trycatch.cpp
+++ test/CoverageMapping/trycatch.cpp
@@ -18,7 +18,7 @@
// CHECK: File 0, [[@LINE+1]]:10 -> [[@LINE+2]]:27 = (#0 - #1)
} else if(i == 8) // CHECK-NEXT: File 0, [[@LINE]]:13 -> [[@LINE]]:19 = (#0 - #1)
throw ImportantError(); // CHECK: File 0, [[@LINE]]:5 -> [[@LINE]]:27 = #2
-} // CHECK-NEXT: File 0, [[@LINE-1]]:27 -> [[@LINE]]:2 = ((#0 - #1) - #2)
+}
// CHECK-NEXT: main
int main() { // CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+13]]:2 = #0
Index: test/CoverageMapping/moremacros.c
===
--- test/CoverageMapping/moremacros.c
+++ test/CoverageMapping/moremacros.c
@@ -31,7 +31,7 @@
if (!argc) {
return 0;
// CHECK-NEXT: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:8 = #4
- RBRAC // CHECK-NEXT: [[@LINE]]:8 -> [[@LINE+1]]:2 = (((#0 - #2) - #3) - #4)
+ RBRAC
}
// CHECK-NEXT: File 1, 3:15 -> 3:16 = #2
Index: test/CoverageMapping/label.cpp
===
--- test/CoverageMapping/label.cpp
+++ test/CoverageMapping/label.cpp
@@ -16,19 +16,18 @@
goto x;// CHECK: File 0, [[@LINE]]:7 -> [[@LINE]]:13 = (#1 - #2)
int k = 3; // CHECK-NEXT: File 0, [[@LINE-1]]:13 -> [[@LINE]]:5 = #3
} // CHECK-NEXT: File 0, [[@LINE-1]]:5 -> [[@LINE]]:4 = #3
- static int j = 0; // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+5]]:2 = ((#0 + #3) - #1)
+ static int j = 0; // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+4]]:2 = ((#0 + #3) - #1)
++j;
if(j == 1) // CHECK-NEXT: File 0, [[@LINE]]:6 -> [[@LINE]]:12 = ((#0 + #3) - #1)
goto x; // CHECK: File 0, [[@LINE]]:5 -> [[@LINE]]:11 = #4
- // CHECK-NEXT: File 0, [[@LINE-1]]:11 -> [[@LINE+1]]:2 = (((#0 + #3) - #1) - #4)
}
// CHECK-NEXT: test1
void test1(int x) { // CHECK-NEXT: File 0, [[@LINE]]:19 -> {{[0-9]+}}:2 = #0
if(x == 0) // CHECK-NEXT: File 0, [[@LINE]]:6 -> [[@LINE]]:12 = #0
goto a; // CHECK: File 0, [[@LINE]]:5 -> [[@LINE]]:11 = #1
// CHECK-NEXT: File 0, [[@LINE-1]]:11 -> [[@LINE+1]]:3 = (#0 - #1)
- goto b;// CHECK: Gap,File 0, [[@LINE]]:3 -> [[@LINE+5]]:2 = #3
+ goto b;// CHECK: File 0, [[@LINE]]:3 -> [[@LINE+5]]:2 = (#0 - #1)
// CHECK-NEXT: Gap,File 0, [[@LINE-1]]:9 -> [[@LINE+1]]:1 = #2
a: // CHECK-NEXT: File 0, [[@LINE]]:1 -> [[@LINE+3]]:2 = #2
b: // CHECK-NEXT: File 0, [[@LINE]]:1 -> [[@LINE+2]]:2 = #3
Index: test/CoverageMapping/deferred-region.cpp
===
--- test/CoverageMapping/deferred-region.cpp
+++ test/CoverageMapping/deferred-region.cpp
@@ -7,19 +7,20 @@
void foo(int x) {
if (x == 0) {
return;
- } // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+2]]:2 = (#0 - #1)
-
+ } // CHECK-NOT: Gap,File 0, [[@LINE]]:4
+//< Don't complete the last deferred region in a decl, even though it may
+//< leave some whitespace marked with the same counter as the final return.
}
-// CHECK-NEXT: _Z4foooi:
+// CHECK-LABEL: _Z4foooi:
void fooo(int x) {
if (x == 0) {
return;
} // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+2]]:3 = (#0 - #1)
if (x == 1) {
return;
- } // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+2]]:2 = ((#0 - #1) - #2)
+ } // CHECK-NOT: Gap,File 0, [[@LINE]]:4
}
@@ -108,7 +109,7 @@
}
if (false)
-return; // CHECK: Gap,File 0, [[@LINE]]:11 -> [[@LINE+1]]:2
+return; // CHECK-NOT: Gap,File 0, [[@LINE]]:11
}
// CHECK-LABEL: _Z8for_loopv:
@@ -167,7 +168,18 @@
return; // CHECK: [[@LINE]]:3 -> [[@LINE+4]]:2 = (#0 - #1)
out:
- return; // CHECK: Gap,File 0, [[@LINE]]:8 -> [[@LINE+1]]:2 = 0
+ return; // CHECK-NOT: Gap,File 0, [[@LINE]]:8
+}
+
+// CHECK-LABEL: _Z8switchesv:
+void switches() {
+ int x;
+ switch (x) {
+case 0:
+ return;
+default:
+ return; // CHECK-NOT: Gap,File 0, [[@LINE]]
+ }
}
#include "deferred-region-helper.h"
Index: lib/CodeGen/CoverageMappingGen.cpp
===
--- lib/CodeGen/CoverageMappingGen.cpp
[PATCH] D47097: [WIP][DebugInfo] Preserve scope in auto generated StoreInst
vsk added a comment. I think CodeGenFunction::EmitParmDecl is the right place to set up an ApplyDebugLocation instance. You can look at CodeGenFunction::EmitAutoVarInit for an example of how to use ApplyDebugLocation. Comment at: test/CodeGen/debug-info-preserve-scope.c:1 +// RUN: %clang_cc1 -O0 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s + The -O0 flag isn't needed here. Comment at: test/CodeGen/debug-info-preserve-scope.c:11 +// CHECK: store i32 %b, i32* %b.addr, align 4, !dbg ![[dbgLocForStore:[0-9]+]] + To check that we set the right location on the store, you might add: `; CHECK: ![[dbgLocForStore]] = !DILocation(line: 0` Repository: rC Clang https://reviews.llvm.org/D47097 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [WIP][DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:2062 EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true); +ApplyDebugLocation::CreateArtificial(*this); + } There are two issues here: 1) ApplyDebugLocation is a RAII helper, which means that it installs the proper debug location in its constructor, and restores the old debug location in its destructor. Since you are not assigning the result of the static constructor (CreateArtificial) to anything, the ApplyDebugLocation instance is immediately destroyed, so it's a no-op. 2) This is being applied in the wrong place, because it sets a debug location *after* the store is emitted. The right location needs to be applied before the store or alloca are emitted. Repository: rC Clang https://reviews.llvm.org/D47097 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:2074
+ if (DoStore) {
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+ EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
Ideally this would precede the calls to CreateMemTemp which set up the allocas.
Comment at: test/CodeGen/debug-info-preserve-scope.c:10
+
+// CHECK: store i32 %b, i32* %b.addr, align 4, !dbg ![[dbgLocForStore:[0-9]+]]
+
Can you check that the alloca gets the same location as well? You can do this
with:
```
CHECK: alloca {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
CHECK: store i32 {{.*}} !dbg ![[dbgLocForStore]]
```
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments.
Comment at: test/CodeGen/debug-info-preserve-scope.c:10
+
+// CHECK: alloca i32 {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
+// CHECK: store i32 {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
In these two check lines, you're capturing the variable dbgLocForStore twice.
That means that if the !dbg location on the alloca were different from the
location on the store, this test would still pass.
To fix that, just capture the dbgLocForStore variable once, the first time you
see it on the alloca. In the second check line, you can simply refer to the
captured variable with `[[dbgLocForStore]]`.
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments.
Comment at: lib/CodeGen/CGExpr.cpp:72
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
I think we need to be a bit more careful here. The current debug location
stored in the builder may not be an artificial 0-location. This may cause
non-linear single-stepping behavior. Consider this example:
```
void foo() {
bar();
if (...) {
int var = ...; //< Clang emits an alloca for "var".
}
...
```
The current debug location at the line "int var = ..." would be at line 4. But
the alloca is emitted in the entry block of the function. In the debugger, this
may result in strange single-stepping behavior when stepping into foo(). You
could step to line 4, then line 2, then line 3, then line 4 again.
I think we can avoid that by setting an artificial location on allocas.
Comment at: lib/CodeGen/CGExpr.cpp:105
return new llvm::AllocaInst(Ty, CGM.getDataLayout().getAllocaAddrSpace(),
ArraySize, Name, AllocaInsertPt);
}
Why not apply the location here to cover more cases?
Comment at: test/CodeGen/debug-info-preserve-scope.c:11
+
+// CHECK: [[B:%.*]] = alloca i32 {{.*}} !dbg ![[artificialDbgLoc:[0-9]+]]
+// CHECK: store i32 {{.*}} !dbg ![[artificialDbgLoc]]
Why is "B" captured?
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments.
Comment at: lib/CodeGen/CGExpr.cpp:72
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
aprantl wrote:
> vsk wrote:
> > I think we need to be a bit more careful here. The current debug location
> > stored in the builder may not be an artificial 0-location. This may cause
> > non-linear single-stepping behavior. Consider this example:
> >
> > ```
> > void foo() {
> > bar();
> > if (...) {
> > int var = ...; //< Clang emits an alloca for "var".
> > }
> > ...
> > ```
> >
> > The current debug location at the line "int var = ..." would be at line 4.
> > But the alloca is emitted in the entry block of the function. In the
> > debugger, this may result in strange single-stepping behavior when stepping
> > into foo(). You could step to line 4, then line 2, then line 3, then line 4
> > again.
> >
> > I think we can avoid that by setting an artificial location on allocas.
> > I think we can avoid that by setting an artificial location on allocas.
> An alloca doesn't really generate any code (or rather.. the code it generates
> is in the function prologue). In what situation would the debug location on
> an alloca influence stepping? Are you thinking about the alloca() function?
An alloca instruction can lower to a subtraction (off the stack pointer)
though: https://godbolt.org/g/U4nGzJ.
`dwarfdump` shows that this subtraction instruction is actually assigned a
location -- it currently happens to be the first location in the body of the
function. I thought that assigning an artificial location to the alloca would
be a first step towards fixing this.
Also, using an artificial location could mitigate possible bad interactions
between code motion passes and IRBuilder. If, say, we were to set the insertion
point right after an alloca, we might infer some arbitrary debug location. So
long as this inference happens, it seems safer to infer an artificial location.
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments.
Comment at: lib/CodeGen/CGExpr.cpp:72
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
aprantl wrote:
> vsk wrote:
> > aprantl wrote:
> > > vsk wrote:
> > > > I think we need to be a bit more careful here. The current debug
> > > > location stored in the builder may not be an artificial 0-location.
> > > > This may cause non-linear single-stepping behavior. Consider this
> > > > example:
> > > >
> > > > ```
> > > > void foo() {
> > > > bar();
> > > > if (...) {
> > > > int var = ...; //< Clang emits an alloca for "var".
> > > > }
> > > > ...
> > > > ```
> > > >
> > > > The current debug location at the line "int var = ..." would be at line
> > > > 4. But the alloca is emitted in the entry block of the function. In the
> > > > debugger, this may result in strange single-stepping behavior when
> > > > stepping into foo(). You could step to line 4, then line 2, then line
> > > > 3, then line 4 again.
> > > >
> > > > I think we can avoid that by setting an artificial location on allocas.
> > > > I think we can avoid that by setting an artificial location on allocas.
> > > An alloca doesn't really generate any code (or rather.. the code it
> > > generates is in the function prologue). In what situation would the debug
> > > location on an alloca influence stepping? Are you thinking about the
> > > alloca() function?
> > An alloca instruction can lower to a subtraction (off the stack pointer)
> > though: https://godbolt.org/g/U4nGzJ.
> >
> > `dwarfdump` shows that this subtraction instruction is actually assigned a
> > location -- it currently happens to be the first location in the body of
> > the function. I thought that assigning an artificial location to the alloca
> > would be a first step towards fixing this.
> >
> > Also, using an artificial location could mitigate possible bad interactions
> > between code motion passes and IRBuilder. If, say, we were to set the
> > insertion point right after an alloca, we might infer some arbitrary debug
> > location. So long as this inference happens, it seems safer to infer an
> > artificial location.
> >
> >
> This may have unintended side-effects: By assigning a debug location to an
> alloca you are moving the end of the function prolog to before the alloca
> instructions, since LLVM computes the end of the function prologue as the
> first instruction with a non-empty debug location. Moving the end of the
> function prologue to before that stack pointer is adjusted is wrong, since
> that's the whole point of the prologue_end marker.
>
> To me it looks more like a bug in a much later stage. With the exception of
> shrink-wrapped code, the prologue_end should always be after the stack
> pointer adjustment, I think.
Thanks for explaining, I didn't realize that's how the end of the function
prologue is computed! Should we leave out the any debug location changes for
allocas in this patch, then?
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added a comment.
In https://reviews.llvm.org/D47097#248, @aprantl wrote:
> In https://reviews.llvm.org/D47097#223, @gramanas wrote:
>
> > In https://reviews.llvm.org/D47097#149, @probinson wrote:
> >
> > > Can we step back a second and better explain what the problem is? With
> > > current Clang the debug info for this function looks okay to me.
> > > The store that is "missing" a debug location is homing the formal
> > > parameter to its local stack location; this is part of prolog setup, not
> > > "real" code.
> >
>
The problem @gramanas is trying to address appears after SROA. SROA invokes
mem2reg, which tries to assign scope information to the phis it creates (see
https://reviews.llvm.org/D45397). Subsequent passes which touch these phis can
use these debug locations. This makes it easier for the debugger to find the
right scope when handling a machine exception.
Store instructions in the prolog without scope information cause mem2reg to
create phis without scope info. E.g:
// foo.c
extern int map[];
void f(int a, int n) {
for (int i = 0; i < n; ++i)
a = map[a];
}
$ clang foo.c -S -emit-llvm -o - -g -O1 -mllvm -opt-bisect-limit=2
..
%a.addr.0 = phi i32 [ %a, %entry ], [ %0, %for.body ]
(Side note: @gramanas, it would help to have the full context/motivation for
changes in the patch summary.)
>> Isn't this the reason the artificial debug loc exists? The store in the
>> prolog might not be real code but it should still have the scope that the
>> rest of the function has.
>
> Instructions that are part of the function prologue are supposed to have no
> debug location, not an artificial one. The funciton prologue ends at the
> first instruction with a nonempty location.
I've been reading through PEI but I'm having a hard time verifying this. How
does llvm determine where the function prologue ends when there isn't any debug
info? What would go wrong if llvm started to behave as if the prologue ended
sooner than it should? Also, why isn't this a problem for the swift compiler,
which appears to always assign debug locations to each instruction?
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46918: [Coverage] Discard the last uncompleted deferred region in a decl
vsk added a comment. Ping. https://reviews.llvm.org/D46918 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38441: [compiler-rt] [cmake] Add a separate CMake var to control profile runtime
vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. Repository: rL LLVM https://reviews.llvm.org/D38441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37542: [ubsan] Save a ptrtoint when emitting alignment checks
vsk closed this revision. vsk added a comment. Landed as r314749 https://reviews.llvm.org/D37542 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38210: [ubsan] Port the function sanitizer to C
vsk added a reviewer: arphaman. vsk added a comment. Ping. https://reviews.llvm.org/D38210 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38210: [ubsan] Port the function sanitizer to C
vsk planned changes to this revision. vsk added a comment. In https://reviews.llvm.org/D38210#887635, @pcc wrote: > Wouldn't we get false positives if there is an indirect call in C++ code that > calls into C code (or vice versa)? Ah, right, I'm surprised I didn't hit that while testing. > I think I'd prefer it if we came up with a precise encoding of function types > that was independent of RTTI, and use it in all languages. One possibility > would be to represent each function type with an object of size 1 whose name > contains the mangled function type, and use its address as the identity of > the function type. That makes sense. Like the RTTI object it could be made linkonce_odr. https://reviews.llvm.org/D38210 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38567: [config] Warn when POSIX_C_SOURCE breaks threading support on Darwin
vsk created this revision.
Prior to macOS 10.13 and iOS 11, defining POSIX_C_SOURCE before
including resulted in hard-to-understand errors. That
definition causes a bunch of important definitions from the system
headers to be skipped, so users see failures like "can't find
mach_port_t".
This patch adds a friendly warning message about the issue.
rdar://problem/31263056
https://reviews.llvm.org/D38567
Files:
include/__config
include/__threading_support
test/libcxx/posix_source.sh.cpp
Index: test/libcxx/posix_source.sh.cpp
===
--- /dev/null
+++ test/libcxx/posix_source.sh.cpp
@@ -0,0 +1,29 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// Test that we get a warning when using the threading support header without
+// the right defines present.
+
+// REQUIRES: apple-darwin
+// UNSUPPORTED: libcpp-has-no-threads
+
+// RUN: %cxx -c %s -o /dev/null %compile_flags -arch x86_64
-mmacosx-version-min=10.12 -D_LIBCPP_DISABLE_AVAILABILITY 2>&1 | not grep
"warning"
+// RUN: %cxx -c %s -o /dev/null %compile_flags -arch x86_64
-mmacosx-version-min=10.12 -D_LIBCPP_DISABLE_AVAILABILITY
-D_POSIX_C_SOURCE=200112L 2>&1 | grep "warning" | grep "Define _DARWIN_C_SOURCE"
+// RUN: %cxx -c %s -o /dev/null %compile_flags -arch x86_64
-mmacosx-version-min=10.12 -D_LIBCPP_DISABLE_AVAILABILITY
-D_POSIX_C_SOURCE=200112L -D_DARWIN_C_SOURCE 2>&1 | not grep "warning"
+
+// RUN: %cxx -c %s -o /dev/null %compile_flags -arch x86_64
-mmacosx-version-min=10.13 -D_LIBCPP_DISABLE_AVAILABILITY 2>&1 | not grep
"warning"
+// RUN: %cxx -c %s -o /dev/null %compile_flags -arch x86_64
-mmacosx-version-min=10.13 -D_LIBCPP_DISABLE_AVAILABILITY
-D_POSIX_C_SOURCE=200112L 2>&1 | not grep "warning"
+// RUN: %cxx -c %s -o /dev/null %compile_flags -arch x86_64
-mmacosx-version-min=10.13 -D_LIBCPP_DISABLE_AVAILABILITY
-D_POSIX_C_SOURCE=200112L -D_DARWIN_C_SOURCE 2>&1 | not grep "warning"
+
+#include
+
+int main() {
+ return 0;
+}
Index: include/__threading_support
===
--- include/__threading_support
+++ include/__threading_support
@@ -23,6 +23,15 @@
# include <__external_threading>
#elif !defined(_LIBCPP_HAS_NO_THREADS)
+#if (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) &&
(__MAC_OS_X_VERSION_MIN_REQUIRED < 101300)) \
+|| (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) &&
(__IPHONE_OS_VERSION_MIN_REQUIRED < 11)) \
+|| (defined(__WATCH_OS_VERSION_MIN_REQUIRED) &&
(__WATCH_OS_VERSION_MIN_REQUIRED < 4)) \
+|| (defined(__TV_OS_VERSION_MIN_REQUIRED) && (__TV_OS_VERSION_MIN_REQUIRED
< 11))
+# if defined(_POSIX_C_SOURCE) && !defined(_DARWIN_C_SOURCE)
+# warning "Define _DARWIN_C_SOURCE (or undefine _POSIX_C_SOURCE) for
threading support."
+# endif
+#endif
+
#if defined(_LIBCPP_HAS_THREAD_API_PTHREAD)
# include
# include
Index: include/__config
===
--- include/__config
+++ include/__config
@@ -901,6 +901,18 @@
defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__)
# define __MAC_OS_X_VERSION_MIN_REQUIRED
__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
# endif
+# if !defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && \
+ defined(__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__)
+# define __IPHONE_OS_VERSION_MIN_REQUIRED
__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__
+# endif
+# if !defined(__WATCH_OS_VERSION_MIN_REQUIRED) && \
+ defined(__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__)
+# define __WATCH_OS_VERSION_MIN_REQUIRED
__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__
+# endif
+# if !defined(__TV_OS_VERSION_MIN_REQUIRED) && \
+ defined(__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__)
+# define __TV_OS_VERSION_MIN_REQUIRED
__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__
+# endif
# if defined(__MAC_OS_X_VERSION_MIN_REQUIRED)
# if __MAC_OS_X_VERSION_MIN_REQUIRED < 1060
# define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
Index: test/libcxx/posix_source.sh.cpp
===
--- /dev/null
+++ test/libcxx/posix_source.sh.cpp
@@ -0,0 +1,29 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// Test that we get a warning when using the threading support header without
+// the right defines present.
+
+// R
[PATCH] D38567: [config] Warn when POSIX_C_SOURCE breaks threading support on Darwin
vsk added a comment. I'm not sure how to test the warning against anything but the macOS SDK. When I tried, I hit a -Wincompatible-sysroot issue. I can leave those changes out of this patch if we want to be more conservative. https://reviews.llvm.org/D38567 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38567: [config] Warn when POSIX_C_SOURCE breaks threading support on Darwin
vsk abandoned this revision. vsk added a comment. For those following along, Alex worked out that this doesn't affect apple-clang 802. We took a closer look and found that the build break just affects clang-900, and was introduced in this r290889. The fix (r293167) didn't make it into clang-900. Adding a warning here wouldn't be the right solution, since it would be better to just cherry pick r293167. https://reviews.llvm.org/D38567 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38861: [CodeGen] Error on unsupported checked multiplies early
vsk created this revision.
LLVM's smul.with.overflow intrinsic isn't supported on X86 for bit
widths larger than 64, or on X86-64 for bit widths larger than 128.
The failure mode is either a linker error ("the __muloti4 builtin isn't
available for this target") or an assertion failure ("SelectionDAG
doesn't know what builtin to call").
Until we actually add builtin support for 128-bit multiply-with-overflow
on X86, we should error-out on unsupported calls as early as possible.
https://bugs.llvm.org/show_bug.cgi?id=34920
https://reviews.llvm.org/D38861
Files:
lib/CodeGen/CGBuiltin.cpp
test/CodeGen/builtins-overflow-unsupported.c
test/CodeGen/builtins-overflow.c
Index: test/CodeGen/builtins-overflow.c
===
--- test/CodeGen/builtins-overflow.c
+++ test/CodeGen/builtins-overflow.c
@@ -2,7 +2,7 @@
// rdar://13421498
// RUN: %clang_cc1 -triple "i686-unknown-unknown" -emit-llvm -x c %s -o - |
FileCheck %s
-// RUN: %clang_cc1 -triple "x86_64-unknown-unknown" -emit-llvm -x c %s -o - |
FileCheck %s
+// RUN: %clang_cc1 -triple "x86_64-unknown-unknown" -emit-llvm -x c %s -o - |
FileCheck %s --check-prefixes=CHECK,M64
// RUN: %clang_cc1 -triple "x86_64-mingw32" -emit-llvm -x c %s -o - |
FileCheck %s
extern unsigned UnsignedErrorCode;
@@ -338,3 +338,20 @@
return LongLongErrorCode;
return result;
}
+
+#if defined(__LP64__)
+signed long long test_mixed_sign_mul_i64(signed long long a, unsigned long
long b) {
+ // M64-LABEL: define i64 @test_mixed_sign_mul_i64
+ // M64: sext i64 {{.*}} to i65
+ // M64-NEXT: zext i64 {{.*}} to i65
+ // M64-NEXT: call { i65, i1 } @llvm.smul.with.overflow.i65
+ // M64-NEXT: [[OFLOW_1:%.*]] = extractvalue { i65, i1 } {{.*}}, 1
+ // M64-NEXT: [[RES:%.*]] = extractvalue { i65, i1 } {{.*}}, 0
+ // M64-NEXT: [[RES_TRUNC:%.*]] = trunc i65 {{.*}} to i64
+ // M64-NEXT: [[RES_EXT:%.*]] = zext i64 {{.*}} to i65
+ // M64-NEXT: [[OFLOW_2:%.*]] = icmp ne i65 [[RES]], [[RES_EXT]]
+ // M64-NEXT: or i1 [[OFLOW_1]], [[OFLOW_2]]
+ // M64-NEXT: store i64 [[RES_TRUNC]]
+ return __builtin_mul_overflow(a, b, &b);
+}
+#endif
Index: test/CodeGen/builtins-overflow-unsupported.c
===
--- /dev/null
+++ test/CodeGen/builtins-overflow-unsupported.c
@@ -0,0 +1,14 @@
+// RUN: not %clang_cc1 -triple "i686-unknown-unknown" -emit-llvm -x c %s -o
- 2>&1 | FileCheck %s --check-prefix=M32
+// RUN: not %clang_cc1 -triple "x86_64-unknown-unknown" -emit-llvm -x c %s -o
- 2>&1 | FileCheck %s --check-prefix=M64
+
+signed long long try_smul_i65(signed long long a, unsigned long long b) {
+ // M32: [[@LINE+1]]:10: error: cannot compile this __builtin_mul_overflow
with mixed-sign operands yet
+ return __builtin_mul_overflow(a, b, &b);
+}
+
+#if defined(__LP64__)
+__int128_t try_smul_i29(__int128_t a, __uint128_t b) {
+ // M64: [[@LINE+1]]:10: error: cannot compile this __builtin_mul_overflow
with mixed-sign operands yet
+ return __builtin_mul_overflow(a, b, &b);
+}
+#endif
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -2248,11 +2248,23 @@
WidthAndSignedness EncompassingInfo =
EncompassingIntegerType({LeftInfo, RightInfo, ResultInfo});
+llvm::Type *ResultLLVMTy = CGM.getTypes().ConvertType(ResultQTy);
+
+const auto &Triple = getTarget().getTriple();
+if (BuiltinID == Builtin::BI__builtin_mul_overflow) {
+ if ((EncompassingInfo.Width > 64 &&
+ Triple.getArch() == llvm::Triple::ArchType::x86) ||
+ (EncompassingInfo.Width > 128 &&
+ Triple.getArch() == llvm::Triple::ArchType::x86_64)) {
+CGM.ErrorUnsupported(E,
+ "__builtin_mul_overflow with mixed-sign
operands");
+return RValue::get(llvm::UndefValue::get(ResultLLVMTy));
+ }
+}
+
llvm::Type *EncompassingLLVMTy =
llvm::IntegerType::get(CGM.getLLVMContext(), EncompassingInfo.Width);
-llvm::Type *ResultLLVMTy = CGM.getTypes().ConvertType(ResultQTy);
-
llvm::Intrinsic::ID IntrinsicId;
switch (BuiltinID) {
default:
Index: test/CodeGen/builtins-overflow.c
===
--- test/CodeGen/builtins-overflow.c
+++ test/CodeGen/builtins-overflow.c
@@ -2,7 +2,7 @@
// rdar://13421498
// RUN: %clang_cc1 -triple "i686-unknown-unknown" -emit-llvm -x c %s -o - | FileCheck %s
-// RUN: %clang_cc1 -triple "x86_64-unknown-unknown" -emit-llvm -x c %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple "x86_64-unknown-unknown" -emit-llvm -x c %s -o - | FileCheck %s --check-prefixes=CHECK,M64
// RUN: %clang_cc1 -triple "x86_64-mingw32" -emit-llvm -x c %s -o - | FileCheck %s
extern unsigned UnsignedErrorCode;
@@ -338,3 +338,20 @@
return LongLongErrorCode;
return result;
}
+
+#if defi
[PATCH] D38861: [CodeGen] Error on unsupported checked multiplies early
vsk updated this revision to Diff 118857.
vsk added a comment.
Herald added a subscriber: aheejin.
- Update to check against a whitelist of supported targets.
https://reviews.llvm.org/D38861
Files:
lib/CodeGen/CGBuiltin.cpp
test/CodeGen/builtins-overflow-unsupported.c
test/CodeGen/builtins-overflow.c
Index: test/CodeGen/builtins-overflow.c
===
--- test/CodeGen/builtins-overflow.c
+++ test/CodeGen/builtins-overflow.c
@@ -2,7 +2,9 @@
// rdar://13421498
// RUN: %clang_cc1 -triple "i686-unknown-unknown" -emit-llvm -x c %s -o - |
FileCheck %s
-// RUN: %clang_cc1 -triple "x86_64-unknown-unknown" -emit-llvm -x c %s -o - |
FileCheck %s
+// RUN: %clang_cc1 -triple "x86_64-unknown-unknown" -emit-llvm -x c %s -o - |
FileCheck %s --check-prefixes=CHECK,M64
+// RUN: %clang_cc1 -triple "wasm64-unknown-unknown" -emit-llvm -x c %s -o - |
FileCheck %s --check-prefixes=M64
+// RUN: %clang_cc1 -triple "mips64-unknown-unknown" -emit-llvm -x c %s -o - |
FileCheck %s --check-prefixes=M64
// RUN: %clang_cc1 -triple "x86_64-mingw32" -emit-llvm -x c %s -o - |
FileCheck %s
extern unsigned UnsignedErrorCode;
@@ -338,3 +340,20 @@
return LongLongErrorCode;
return result;
}
+
+#if defined(__LP64__)
+signed long long test_mixed_sign_mul_i64(signed long long a, unsigned long
long b) {
+ // M64-LABEL: define i64 @test_mixed_sign_mul_i64
+ // M64: sext i64 {{.*}} to i65
+ // M64-NEXT: zext i64 {{.*}} to i65
+ // M64-NEXT: call { i65, i1 } @llvm.smul.with.overflow.i65
+ // M64-NEXT: [[OFLOW_1:%.*]] = extractvalue { i65, i1 } {{.*}}, 1
+ // M64-NEXT: [[RES:%.*]] = extractvalue { i65, i1 } {{.*}}, 0
+ // M64-NEXT: [[RES_TRUNC:%.*]] = trunc i65 {{.*}} to i64
+ // M64-NEXT: [[RES_EXT:%.*]] = zext i64 {{.*}} to i65
+ // M64-NEXT: [[OFLOW_2:%.*]] = icmp ne i65 [[RES]], [[RES_EXT]]
+ // M64-NEXT: or i1 [[OFLOW_1]], [[OFLOW_2]]
+ // M64-NEXT: store i64 [[RES_TRUNC]]
+ return __builtin_mul_overflow(a, b, &b);
+}
+#endif
Index: test/CodeGen/builtins-overflow-unsupported.c
===
--- /dev/null
+++ test/CodeGen/builtins-overflow-unsupported.c
@@ -0,0 +1,14 @@
+// RUN: not %clang_cc1 -triple "i686-unknown-unknown" -emit-llvm -x c %s -o
- 2>&1 | FileCheck %s --check-prefix=M32
+// RUN: not %clang_cc1 -triple "x86_64-unknown-unknown" -emit-llvm -x c %s -o
- 2>&1 | FileCheck %s --check-prefix=M64
+
+signed long long try_smul_i65(signed long long a, unsigned long long b) {
+ // M32: [[@LINE+1]]:10: error: cannot compile this __builtin_mul_overflow
with mixed-sign operands yet
+ return __builtin_mul_overflow(a, b, &b);
+}
+
+#if defined(__LP64__)
+__int128_t try_smul_i29(__int128_t a, __uint128_t b) {
+ // M64: [[@LINE+1]]:10: error: cannot compile this __builtin_mul_overflow
with mixed-sign operands yet
+ return __builtin_mul_overflow(a, b, &b);
+}
+#endif
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -397,6 +397,15 @@
return {Width, Signed};
}
+// Check if the target supports checked multiplication with 128-bit operands.
+static bool has128BitMulOverflowSupport(const llvm::Triple &Triple) {
+ if (!Triple.isArch64Bit())
+return false;
+ return StringSwitch(llvm::Triple::getArchTypePrefix(Triple.getArch()))
+ .Cases("x86", "wasm", "mips", true)
+ .Default(false);
+}
+
Value *CodeGenFunction::EmitVAStartEnd(Value *ArgValue, bool IsStart) {
llvm::Type *DestType = Int8PtrTy;
if (ArgValue->getType() != DestType)
@@ -2248,11 +2257,21 @@
WidthAndSignedness EncompassingInfo =
EncompassingIntegerType({LeftInfo, RightInfo, ResultInfo});
+llvm::Type *ResultLLVMTy = CGM.getTypes().ConvertType(ResultQTy);
+
+if (BuiltinID == Builtin::BI__builtin_mul_overflow) {
+ if ((EncompassingInfo.Width > 64 &&
+ !has128BitMulOverflowSupport(getTarget().getTriple())) ||
+ (EncompassingInfo.Width > 128)) {
+CGM.ErrorUnsupported(E,
+ "__builtin_mul_overflow with mixed-sign
operands");
+return RValue::get(llvm::UndefValue::get(ResultLLVMTy));
+ }
+}
+
llvm::Type *EncompassingLLVMTy =
llvm::IntegerType::get(CGM.getLLVMContext(), EncompassingInfo.Width);
-llvm::Type *ResultLLVMTy = CGM.getTypes().ConvertType(ResultQTy);
-
llvm::Intrinsic::ID IntrinsicId;
switch (BuiltinID) {
default:
Index: test/CodeGen/builtins-overflow.c
===
--- test/CodeGen/builtins-overflow.c
+++ test/CodeGen/builtins-overflow.c
@@ -2,7 +2,9 @@
// rdar://13421498
// RUN: %clang_cc1 -triple "i686-unknown-unknown" -emit-llvm -x c %s -o - | FileCheck %s
-// RUN: %clang_cc1 -triple "x86_64-unknown-unknown" -emit-llvm -x c %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple "x86_64-un
[PATCH] D38861: [CodeGen] Error on unsupported checked multiplies early
vsk added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:2263
+ }
+}
+
rjmccall wrote:
> Is there a reason this only fails on x86? If LLVM doesn't have generic
> wide-operation lowering, this probably needs to be a target whitelist rather
> than a blacklist.
That makes sense. For the 128-bit operation, the whitelist is {x86-64, wasm64,
mips64}. We don't support this operation for bit widths larger than 128 bits on
any target. I'll update the patch accordingly.
https://reviews.llvm.org/D38861
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38861: [CodeGen] Error on unsupported checked multiplies early
vsk added a comment. In https://reviews.llvm.org/D38861#896435, @rjmccall wrote: > Okay. Sounds good to me. > > We intend to actually implement the generic lowering, I hope? Yes, I'll make a note of that in PR34920, and am tracking the bug internally in rdar://problem/34963321. https://reviews.llvm.org/D38861 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38859: [clang] Enable clang build with LLVM_BUILD_INSTRUMENTED without setting LLVM_PROFTDATA
vsk added a comment. llvm-profdata is tightly coupled with the host compiler: while this setup may work if you get lucky, I don't think it's resilient to changes in libProfData. Also, using the instrumented llvm-profdata will be slow and create extra profiles. As an alternative, have you considered building an llvm-profdata compatible with your host compiler in a separate step? Repository: rL LLVM https://reviews.llvm.org/D38859 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38859: [clang] Enable clang build with LLVM_BUILD_INSTRUMENTED without setting LLVM_PROFTDATA
vsk added a comment. In https://reviews.llvm.org/D38859#896517, @alexshap wrote: > yeah, i agree, this is not a good idea. My thoughts were different - right > now it's not particularly convenient that one has to specify LLVM_PROFDATA > when it's not actually used by the build. > Maybe we can create the target "generate-profdata" only if LLVM_PROFDATA is > set (but don't fail otherwise) ? + 1, sgtm Repository: rL LLVM https://reviews.llvm.org/D38859 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38859: [clang] Enable clang build with LLVM_BUILD_INSTRUMENTED without setting LLVM_PROFTDATA
vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks! Repository: rL LLVM https://reviews.llvm.org/D38859 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38861: [CodeGen] Error on unsupported checked multiplies early
vsk added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:2263
+ }
+}
+
joerg wrote:
> vsk wrote:
> > rjmccall wrote:
> > > Is there a reason this only fails on x86? If LLVM doesn't have generic
> > > wide-operation lowering, this probably needs to be a target whitelist
> > > rather than a blacklist.
> > That makes sense. For the 128-bit operation, the whitelist is {x86-64,
> > wasm64, mips64}. We don't support this operation for bit widths larger than
> > 128 bits on any target. I'll update the patch accordingly.
> That sounds wrong. __int128_t should be supported by all 64bit architectures,
> not just those three.
I didn't mean to imply generic int128_t operations aren't supported. This patch
just focuses on muloti4 (signed multiplication with overflow checking).
https://reviews.llvm.org/D38861
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38903: [ubsan] Only use indirect RTTI in prologues on Darwin
vsk created this revision.
Using a layer of indirection to point to RTTI through function prologues
is not supported on some setups. One reported error message is:
error: Cannot represent a difference across sections
This is a regression. This patch limits the indirect RTTI behavior to
Darwin, where we know it works. We can add more configurations to the
whitelist once we know it won't be a regression.
For context, see the mailing list discussion re:
r313096 - [ubsan] Function Sanitizer: Don't require writable text segments
Testing: check-clang, check-ubsan
https://reviews.llvm.org/D38903
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/TargetInfo.cpp
test/CodeGenCXX/catch-undef-behavior.cpp
Index: test/CodeGenCXX/catch-undef-behavior.cpp
===
--- test/CodeGenCXX/catch-undef-behavior.cpp
+++ test/CodeGenCXX/catch-undef-behavior.cpp
@@ -3,6 +3,7 @@
// RUN: %clang_cc1 -std=c++11 -fsanitize=vptr -fsanitize-recover=vptr -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=DOWNCAST-NULL
// RUN: %clang_cc1 -std=c++11 -fsanitize=function -emit-llvm %s -o - -triple x86_64-linux-gnux32 | FileCheck %s --check-prefix=CHECK-X32
// RUN: %clang_cc1 -std=c++11 -fsanitize=function -emit-llvm %s -o - -triple i386-linux-gnu | FileCheck %s --check-prefix=CHECK-X86
+// RUN: %clang_cc1 -std=c++11 -fsanitize=function -emit-llvm %s -o - -triple x86_64-apple-darwin | FileCheck %s --check-prefix=DARWIN64
struct S {
double d;
@@ -16,9 +17,7 @@
// Check that type mismatch handler is not modified by ASan.
// CHECK-ASAN: private unnamed_addr global { { [{{.*}} x i8]*, i32, i32 }, { i16, i16, [4 x i8] }*, i8*, i8 } { {{.*}}, { i16, i16, [4 x i8] }* [[TYPE_DESCR]], {{.*}} }
-// CHECK: [[IndirectRTTI_ZTIFvPFviEE:@.+]] = private constant i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*)
-// CHECK-X86: [[IndirectRTTI_ZTIFvPFviEE:@.+]] = private constant i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*)
-// CHECK-X32: [[IndirectRTTI_ZTIFvPFviEE:@.+]] = private constant i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*)
+// DARWIN64: [[IndirectRTTI_ZTIFvPFviEE:@.+]] = private constant i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*)
struct T : S {};
@@ -399,29 +398,42 @@
// CHECK-NEXT: br i1 [[AND]]
}
-//
-// CHECK-LABEL: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i32 }> <{ i32 846595819, i32 trunc (i64 sub (i64 ptrtoint (i8** {{.*}} to i64), i64 ptrtoint (void (void (i32)*)* @_Z22indirect_function_callPFviE to i64)) to i32) }>
-// CHECK-X32: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i32 }> <{ i32 846595819, i32 sub (i32 ptrtoint (i8** [[IndirectRTTI_ZTIFvPFviEE]] to i32), i32 ptrtoint (void (void (i32)*)* @_Z22indirect_function_callPFviE to i32)) }>
-// CHECK-X86: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i32 }> <{ i32 846595819, i32 sub (i32 ptrtoint (i8** [[IndirectRTTI_ZTIFvPFviEE]] to i32), i32 ptrtoint (void (void (i32)*)* @_Z22indirect_function_callPFviE to i32)) }>
+// CHECK-LABEL: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i8* }> <{ i32 863374059, i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*) }>
+// CHECK-X32: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i8* }> <{ i32 863373035, i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*) }>
+// CHECK-X86: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i8* }> <{ i32 863373035, i8* bitcast ({ i8*, i8* }* @_ZTIFvPFviEE to i8*) }>
+// DARWIN64: @_Z22indirect_function_callPFviE({{.*}} prologue <{ i32, i32 }> <{ i32 863373035, i32 trunc (i64 sub (i64 ptrtoint (i8** [[IndirectRTTI_ZTIFvPFviEE]] to i64), i64 ptrtoint (void (void (i32)*)* @_Z22indirect_function_callPFviE to i64)) to i32) }>
void indirect_function_call(void (*p)(int)) {
- // CHECK: [[PTR:%.+]] = bitcast void (i32)* {{.*}} to <{ i32, i32 }>*
+ // CHECK: [[PTR:%.+]] = bitcast void (i32)* {{.*}} to <{ i32, i8* }>*
+ // DARWIN64: [[PTR:%.+]] = bitcast void (i32)* {{.*}} to <{ i32, i32 }>*
// Signature check
- // CHECK-NEXT: [[SIGPTR:%.+]] = getelementptr <{ i32, i32 }>, <{ i32, i32 }>* [[PTR]], i32 0, i32 0
+ //
+ // CHECK-NEXT: [[SIGPTR:%.+]] = getelementptr <{ i32, i8* }>, <{ i32, i8* }>* [[PTR]], i32 0, i32 0
// CHECK-NEXT: [[SIG:%.+]] = load i32, i32* [[SIGPTR]]
- // CHECK-NEXT: [[SIGCMP:%.+]] = icmp eq i32 [[SIG]], 846595819
+ // CHECK-NEXT: [[SIGCMP:%.+]] = icmp eq i32 [[SIG]], 863374059
// CHECK-NEXT: br i1 [[SIGCMP]]
+ //
+ // DARWIN64-NEXT: [[SIGPTR:%.+]] = getelementptr <{ i32, i32 }>, <{ i32, i32 }>* [[PTR]], i32 0, i32 0
+ // DARWIN64-NEXT: [[SIG:%.+]] = load i32, i32* [[SIGPTR]]
+ // DARWIN64-NEXT: [[SIGCMP:%.+]] = icmp eq i32 [[SIG]], 863373035
+ // DARWIN64-NEXT: br i1 [[SIGCMP]]
// RTTI pointer check
- // CHECK: [[RTTIPTR:%.+]] = getelementptr <{ i32, i32 }>, <{ i32, i32 }>* [[PTR]], i32 0, i32 1
- // CHECK-NEXT: [[RTTIEncIntTrunc:%.+]] = load i32, i32* [[RTTIPTR]]
-
[PATCH] D38903: [ubsan] Only use indirect RTTI in prologues on Darwin
vsk added a comment. Sounds good. This doesn't seem too controversial, since it just takes us back to the old behavior on all platforms except Darwin. I'll wait an hour or so before committing in case there are any more comments. https://reviews.llvm.org/D38903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38913: [ubsan] Don't emit function signatures for virtual methods
vsk created this revision.
The function sanitizer only checks indirect calls through function
pointers. This excludes all non-static member functions (constructor
calls, calls through thunks, etc all use a separate code path). Don't
emit function signatures for functions that won't be checked.
Apart from cutting down on code size, this should fix a regression on
Linux caused by r313096. For context, see the mailing list discussion:
r313096 - [ubsan] Function Sanitizer: Don't require writable text segments
Testing: check-clang, check-ubsan
Supersedes https://reviews.llvm.org/D38903.
https://reviews.llvm.org/D38913
Files:
lib/CodeGen/CodeGenFunction.cpp
test/CodeGenCXX/catch-undef-behavior.cpp
Index: test/CodeGenCXX/catch-undef-behavior.cpp
===
--- test/CodeGenCXX/catch-undef-behavior.cpp
+++ test/CodeGenCXX/catch-undef-behavior.cpp
@@ -426,6 +426,66 @@
p(42);
}
+namespace FunctionSanitizerVirtualCalls {
+struct A {
+ virtual void f() {}
+ virtual void g() {}
+ void h() {}
+};
+
+struct B : virtual A {
+ virtual void b() {}
+ virtual void f();
+ void g() final {}
+ static void q() {}
+};
+
+void B::f() {}
+
+void force_irgen() {
+ A a;
+ a.g();
+ a.h();
+
+ B b;
+ b.f();
+ b.b();
+ b.g();
+ B::q();
+}
+
+// CHECK-LABEL: define void @_ZN29FunctionSanitizerVirtualCalls1B1fEv
+// CHECK-NOT: prologue
+//
+// CHECK-LABEL: define void @_ZTv0_n24_N29FunctionSanitizerVirtualCalls1B1fEv
+// CHECK-NOT: prologue
+//
+// CHECK-LABEL: define void @_ZN29FunctionSanitizerVirtualCalls11force_irgenEv()
+// CHECK: prologue
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN29FunctionSanitizerVirtualCalls1AC1Ev
+// CHECK-NOT: prologue
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN29FunctionSanitizerVirtualCalls1A1gEv
+// CHECK-NOT: prologue
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN29FunctionSanitizerVirtualCalls1A1hEv
+// CHECK-NOT: prologue
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN29FunctionSanitizerVirtualCalls1BC1Ev
+// CHECK-NOT: prologue
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN29FunctionSanitizerVirtualCalls1B1bEv
+// CHECK-NOT: prologue
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN29FunctionSanitizerVirtualCalls1B1gEv
+// CHECK-NOT: prologue
+//
+// CHECK-LABEL: define linkonce_odr void @_ZN29FunctionSanitizerVirtualCalls1B1qEv
+// CHECK: prologue
+
+}
+
namespace UpcastPointerTest {
struct S {};
struct T : S { double d; };
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -789,6 +789,15 @@
return true;
}
+/// Return the UBSan prologue signature for \p FD if one is available.
+static llvm::Constant *getPrologueSignature(CodeGenModule &CGM,
+const FunctionDecl *FD) {
+ if (const auto *MD = dyn_cast(FD))
+if (!MD->isStatic())
+ return nullptr;
+ return CGM.getTargetCodeGenInfo().getUBSanFunctionSignature(CGM);
+}
+
void CodeGenFunction::StartFunction(GlobalDecl GD,
QualType RetTy,
llvm::Function *Fn,
@@ -908,8 +917,7 @@
// prologue data.
if (getLangOpts().CPlusPlus && SanOpts.has(SanitizerKind::Function)) {
if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
- if (llvm::Constant *PrologueSig =
- CGM.getTargetCodeGenInfo().getUBSanFunctionSignature(CGM)) {
+ if (llvm::Constant *PrologueSig = getPrologueSignature(CGM, FD)) {
llvm::Constant *FTRTTIConst =
CGM.GetAddrOfRTTIDescriptor(FD->getType(), /*ForEH=*/true);
llvm::Constant *FTRTTIConstEncoded =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38903: [ubsan] Only use indirect RTTI in prologues on Darwin
vsk added a comment. @pcc made an alternate suggestion which led to https://reviews.llvm.org/D38913. We're still discussing whether the new patch is a sufficient fix. https://reviews.llvm.org/D38903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38903: [ubsan] Only use indirect RTTI in prologues on Darwin
vsk abandoned this revision. vsk added a comment. https://reviews.llvm.org/D38913 should make this unnecessary. https://reviews.llvm.org/D38903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
vsk added a comment.
In https://reviews.llvm.org/D27607#901882, @fjricci wrote:
> On platforms where `BOOL` == `signed char`, is it actually undefined behavior
> (or is it just bad programming practice) to store a value other than 0 or 1
> in your `BOOL`? I can't find any language specs suggesting that it is, and
> given that it's just a typedef for a `signed char`, I don't see why it would
> be.
Treating BOOL as a regular 'signed char' creates bad interactions with
bitfields. For example, this code calls panic:
struct S {
BOOL b1 : 1;
};
S s;
s.b1 = YES;
if (s.b1 != YES)
panic();
> If it's not actually undefined behavior, could we make it controllable via a
> separate fsanitize switch (like we have for unsigned integer overflow, which
> is also potentially bad practice but not actually undefined behavior).
The only defined values for BOOL are 'YES' and 'NO' {1, 0}. We've documented
that it's UB to load values outside of this range from a BOOL here:
https://developer.apple.com/documentation/code_diagnostics/undefined_behavior_sanitizer/invalid_boolean
Repository:
rL LLVM
https://reviews.llvm.org/D27607
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51239: [ubsan] Enable -fsanitize=vptr on Apple devices and simulators
vsk created this revision.
vsk added a reviewer: delcypher.
It seems like an oversight that this check was not always enabled for
on-device or device simulator targets.
https://reviews.llvm.org/D51239
Files:
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/Driver/fsanitize.c
Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -423,6 +423,12 @@
// RUN: %clang -target x86_64-apple-darwin10 -mmacosx-version-min=10.8
-fsanitize=vptr %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-DARWIN-OLD
// CHECK-VPTR-DARWIN-OLD: unsupported option '-fsanitize=vptr' for target
'x86_64-apple-darwin10'
+// RUN: %clang -target aarch64-apple-darwin15.0.0 -fsanitize=vptr %s -### 2>&1
+// OK
+
+// RUN: %clang -target x86_64-apple-darwin15.0.0-simulator -fsanitize=vptr %s
-### 2>&1
+// OK
+
// RUN: %clang -target x86_64-apple-darwin10 -mmacosx-version-min=10.9
-fsanitize=alignment,vptr %s -### 2>&1 | FileCheck %s
--check-prefix=CHECK-VPTR-DARWIN-NEW
// CHECK-VPTR-DARWIN-NEW: -fsanitize=alignment,vptr
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -2251,9 +2251,9 @@
Res |= SanitizerKind::Fuzzer;
Res |= SanitizerKind::FuzzerNoLink;
Res |= SanitizerKind::Function;
+ if (!isTargetMacOS() || !isMacosxVersionLT(10, 9))
+Res |= SanitizerKind::Vptr;
if (isTargetMacOS()) {
-if (!isMacosxVersionLT(10, 9))
- Res |= SanitizerKind::Vptr;
if (IsX86_64)
Res |= SanitizerKind::Thread;
} else if (isTargetIOSSimulator() || isTargetTvOSSimulator()) {
Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -423,6 +423,12 @@
// RUN: %clang -target x86_64-apple-darwin10 -mmacosx-version-min=10.8 -fsanitize=vptr %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-DARWIN-OLD
// CHECK-VPTR-DARWIN-OLD: unsupported option '-fsanitize=vptr' for target 'x86_64-apple-darwin10'
+// RUN: %clang -target aarch64-apple-darwin15.0.0 -fsanitize=vptr %s -### 2>&1
+// OK
+
+// RUN: %clang -target x86_64-apple-darwin15.0.0-simulator -fsanitize=vptr %s -### 2>&1
+// OK
+
// RUN: %clang -target x86_64-apple-darwin10 -mmacosx-version-min=10.9 -fsanitize=alignment,vptr %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-DARWIN-NEW
// CHECK-VPTR-DARWIN-NEW: -fsanitize=alignment,vptr
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -2251,9 +2251,9 @@
Res |= SanitizerKind::Fuzzer;
Res |= SanitizerKind::FuzzerNoLink;
Res |= SanitizerKind::Function;
+ if (!isTargetMacOS() || !isMacosxVersionLT(10, 9))
+Res |= SanitizerKind::Vptr;
if (isTargetMacOS()) {
-if (!isMacosxVersionLT(10, 9))
- Res |= SanitizerKind::Vptr;
if (IsX86_64)
Res |= SanitizerKind::Thread;
} else if (isTargetIOSSimulator() || isTargetTvOSSimulator()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51239: [ubsan] Enable -fsanitize=vptr on Apple devices and simulators
vsk added inline comments.
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2254
Res |= SanitizerKind::Function;
+ if (!isTargetMacOS() || !isMacosxVersionLT(10, 9))
+Res |= SanitizerKind::Vptr;
delcypher wrote:
> Could we apply De'Morgan's rule here and write that as
>
> ```
> if (!(isTargetMacOS() && isMacosxVersionLT(10, 9)) {
> Res |= SanitizerKind::Vptr
> }
> ```
>
> I find that a bit easier to read.
>
> Is there any particular reason why vptr isn't supported for old macOS
> versions? There's no mention of ios here which suggests that it's supported
> on all ios versions which seems like an odd disparity. Perhaps a comment
> briefly explaining why this is the case would be helpful?
Sure.
MacOS versions older than 10.8 shipped a version of the C++ standard library
which is incompatible with the vptr check's implementation (see:
https://trac.macports.org/wiki/LibcxxOnOlderSystems). I'll add a comment to
that effect.
As far as I know, all currently-supported versions of iOS ship libc++. I'll ask
around and double-check, just to be safe.
https://reviews.llvm.org/D51239
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51239: [ubsan] Enable -fsanitize=vptr on Apple devices and simulators
vsk updated this revision to Diff 162732.
vsk added a comment.
Address some review feedback.
I'm not sure whether iOS 4 is really supported anymore. There are a handful of
code paths in the driver which handle that target, so I've added in a version
check for it.
https://reviews.llvm.org/D51239
Files:
clang/lib/Driver/ToolChains/Darwin.cpp
clang/test/Driver/fsanitize.c
Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -423,6 +423,15 @@
// RUN: %clang -target x86_64-apple-darwin10 -mmacosx-version-min=10.8
-fsanitize=vptr %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-DARWIN-OLD
// CHECK-VPTR-DARWIN-OLD: unsupported option '-fsanitize=vptr' for target
'x86_64-apple-darwin10'
+// RUN: %clang -target arm-apple-ios4 -fsanitize=vptr %s -### 2>&1 | FileCheck
%s --check-prefix=CHECK-VPTR-IOS-OLD
+// CHECK-VPTR-IOS-OLD: unsupported option '-fsanitize=vptr' for target
'arm-apple-ios4'
+
+// RUN: %clang -target aarch64-apple-darwin15.0.0 -fsanitize=vptr %s -### 2>&1
+// OK
+
+// RUN: %clang -target x86_64-apple-darwin15.0.0-simulator -fsanitize=vptr %s
-### 2>&1
+// OK
+
// RUN: %clang -target x86_64-apple-darwin10 -mmacosx-version-min=10.9
-fsanitize=alignment,vptr %s -### 2>&1 | FileCheck %s
--check-prefix=CHECK-VPTR-DARWIN-NEW
// CHECK-VPTR-DARWIN-NEW: -fsanitize=alignment,vptr
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -2258,9 +2258,15 @@
Res |= SanitizerKind::Fuzzer;
Res |= SanitizerKind::FuzzerNoLink;
Res |= SanitizerKind::Function;
+
+ // Prior to 10.9, macOS shipped a version of the C++ standard library without
+ // C++11 support. The same is true of iOS prior to version 5. These OS'es are
+ // incompatible with -fsanitize=vptr.
+ if (!(isTargetMacOS() && isMacosxVersionLT(10, 9))
+ && !(isTargetIPhoneOS() && isIPhoneOSVersionLT(5, 0)))
+Res |= SanitizerKind::Vptr;
+
if (isTargetMacOS()) {
-if (!isMacosxVersionLT(10, 9))
- Res |= SanitizerKind::Vptr;
if (IsX86_64)
Res |= SanitizerKind::Thread;
} else if (isTargetIOSSimulator() || isTargetTvOSSimulator()) {
Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -423,6 +423,15 @@
// RUN: %clang -target x86_64-apple-darwin10 -mmacosx-version-min=10.8 -fsanitize=vptr %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-DARWIN-OLD
// CHECK-VPTR-DARWIN-OLD: unsupported option '-fsanitize=vptr' for target 'x86_64-apple-darwin10'
+// RUN: %clang -target arm-apple-ios4 -fsanitize=vptr %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-IOS-OLD
+// CHECK-VPTR-IOS-OLD: unsupported option '-fsanitize=vptr' for target 'arm-apple-ios4'
+
+// RUN: %clang -target aarch64-apple-darwin15.0.0 -fsanitize=vptr %s -### 2>&1
+// OK
+
+// RUN: %clang -target x86_64-apple-darwin15.0.0-simulator -fsanitize=vptr %s -### 2>&1
+// OK
+
// RUN: %clang -target x86_64-apple-darwin10 -mmacosx-version-min=10.9 -fsanitize=alignment,vptr %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-DARWIN-NEW
// CHECK-VPTR-DARWIN-NEW: -fsanitize=alignment,vptr
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -2258,9 +2258,15 @@
Res |= SanitizerKind::Fuzzer;
Res |= SanitizerKind::FuzzerNoLink;
Res |= SanitizerKind::Function;
+
+ // Prior to 10.9, macOS shipped a version of the C++ standard library without
+ // C++11 support. The same is true of iOS prior to version 5. These OS'es are
+ // incompatible with -fsanitize=vptr.
+ if (!(isTargetMacOS() && isMacosxVersionLT(10, 9))
+ && !(isTargetIPhoneOS() && isIPhoneOSVersionLT(5, 0)))
+Res |= SanitizerKind::Vptr;
+
if (isTargetMacOS()) {
-if (!isMacosxVersionLT(10, 9))
- Res |= SanitizerKind::Vptr;
if (IsX86_64)
Res |= SanitizerKind::Thread;
} else if (isTargetIOSSimulator() || isTargetTvOSSimulator()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50927: [Sema] Remove location from implicit capture init expr
vsk added a comment. Ping. https://reviews.llvm.org/D50927 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50927: [Sema] Remove location from implicit capture init expr
vsk added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:10689 - S.DiagRuntimeBehavior(DRE->getBeginLoc(), DRE, + S.DiagRuntimeBehavior(DRE->getBeginOrDeclLoc(), DRE, S.PDiag(diag) rsmith wrote: > I'm a bit surprised you updated this call to `DeclRefExpr::getBeginLoc()` > (and no others). I think this should be unreachable for a `DeclRefExpr` that > refers to an implicit lambda capture, because such a lambda-capture cannot > refer to itself from its own (implicit) initializer. IIUC, that's exactly what this visitor (SelfReferenceChecker) diagnoses. The only test I found which exercises this code path is SemaCXX/uninitialized.cpp. The relevant portion is updated in this diff, but to summarize, there's a DeclRefExpr in the CXXConstructExpr initializer generated for `a1` which self-refers to `a1`. https://reviews.llvm.org/D50927 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50927: [Sema] Remove location from implicit capture init expr
vsk updated this revision to Diff 162764.
vsk added a comment.
- Partially address some of @rsmith's feedback. Instead of using the capture
default location, I used the beginning location of the capture list. This
results in smoother single-stepping when those two locations are on different
lines.
https://reviews.llvm.org/D50927
Files:
clang/lib/Sema/SemaLambda.cpp
clang/test/CodeGenCXX/debug-info-lambda.cpp
clang/test/SemaCXX/uninitialized.cpp
clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
===
--- clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
@@ -75,11 +75,15 @@
TEST(RecursiveASTVisitor, VisitsImplicitLambdaCaptureInit) {
DeclRefExprVisitor Visitor;
Visitor.setShouldVisitImplicitCode(true);
- // We're expecting the "i" in the lambda to be visited twice:
- // - Once for the DeclRefExpr in the lambda capture initialization (whose
- // source code location is set to the first use of the variable).
- // - Once for the DeclRefExpr for the use of "i" inside the lambda.
- Visitor.ExpectMatch("i", 1, 24, /*Times=*/2);
+ // We're expecting the "i" in the lambda to be visited just once (for the
+ // DeclRefExpr for the use of "i" inside the lambda).
+ //
+ // Previously, the DeclRefExpr in the implicit lambda capture initialization
+ // (whose source code location was set to the first use of the variable) was
+ // also matched. This location was changed to the starting site of the
+ // capture list to improve stepping behavior.
+ Visitor.ExpectMatch("i", 1, 19, /*Times=*/1);
+ Visitor.ExpectMatch("i", 1, 24, /*Times=*/1);
EXPECT_TRUE(Visitor.runOver(
"void f() { int i; [=]{ i; }; }",
DeclRefExprVisitor::Lang_CXX11));
Index: clang/test/SemaCXX/uninitialized.cpp
===
--- clang/test/SemaCXX/uninitialized.cpp
+++ clang/test/SemaCXX/uninitialized.cpp
@@ -884,8 +884,10 @@
int x;
};
A a0([] { return a0.x; }); // ok
- void f() {
-A a1([=] { return a1.x; }); // expected-warning{{variable 'a1' is uninitialized when used within its own initialization}}
+ void f() {
+A a1([=] { // expected-warning{{variable 'a1' is uninitialized when used within its own initialization}}
+ return a1.x;
+});
A a2([&] { return a2.x; }); // ok
}
}
Index: clang/test/CodeGenCXX/debug-info-lambda.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-lambda.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm \
+// RUN: -debug-info-kind=line-tables-only -dwarf-column-info -std=c++11 %s -o - | FileCheck %s
+
+// CHECK-LABEL: define{{.*}}lambda_in_func
+void lambda_in_func(int &ref) {
+ // CHECK: [[ref_slot:%.*]] = getelementptr inbounds %class.anon, %class.anon* {{.*}}, i32 0, i32 0, !dbg [[lambda_decl_loc:![0-9]+]]
+ // CHECK-NEXT: %1 = load i32*, i32** %ref.addr, align 8, !dbg [[lambda_decl_loc]]
+ // CHECK-NEXT: store i32* %1, i32** %0, align 8, !dbg [[lambda_decl_loc]]
+ // CHECK-NEXT: call void {{.*}}, !dbg [[lambda_call_loc:![0-9]+]]
+
+ auto helper = [ // CHECK: [[lambda_decl_loc]] = !DILocation(line: [[@LINE]], column: 17
+ &]() {
+++ref;
+ };
+ helper(); // CHECK: [[lambda_call_loc]] = !DILocation(line: [[@LINE]]
+}
Index: clang/lib/Sema/SemaLambda.cpp
===
--- clang/lib/Sema/SemaLambda.cpp
+++ clang/lib/Sema/SemaLambda.cpp
@@ -1392,13 +1392,14 @@
Class->addDecl(Conversion);
}
-static ExprResult performLambdaVarCaptureInitialization(Sema &S,
-const Capture &Capture,
-FieldDecl *Field) {
+static ExprResult performLambdaVarCaptureInitialization(
+Sema &S, const Capture &Capture, FieldDecl *Field,
+SourceLocation ImplicitCaptureLoc, bool IsImplicitCapture) {
assert(Capture.isVariableCapture() && "not a variable capture");
auto *Var = Capture.getVariable();
SourceLocation Loc = Capture.getLocation();
+ SourceLocation InitLoc = IsImplicitCapture ? ImplicitCaptureLoc : Loc;
// C++11 [expr.prim.lambda]p21:
// When the lambda-expression is evaluated, the entities that
@@ -1413,7 +1414,7 @@
// An entity captured by a lambda-expression is odr-used (3.2) in
// the scope containing the lambda-expression.
ExprResult RefResult = S.BuildDeclarationNameExpr(
- CXXScopeSpec(), DeclarationNameInfo(Var->getDeclName(), Loc), Var);
+ CXXScopeSpec(), DeclarationNameInfo(Var->getDeclName(), InitLoc), Var);
if (RefResult.isInvalid())
return ExprError();
Expr *Ref = RefResult.get();
@@ -1607,8 +16
[PATCH] D50927: [Sema] Remove location from implicit capture init expr
vsk updated this revision to Diff 162918.
vsk marked 2 inline comments as done.
vsk added a comment.
Address the latest round of review feedback.
https://reviews.llvm.org/D50927
Files:
clang/lib/Sema/SemaLambda.cpp
clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
clang/test/CXX/expr/expr.prim/expr.prim.lambda/templates.cpp
clang/test/CodeGenCXX/debug-info-lambda.cpp
clang/test/SemaCXX/uninitialized.cpp
clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
===
--- clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp
@@ -75,11 +75,11 @@
TEST(RecursiveASTVisitor, VisitsImplicitLambdaCaptureInit) {
DeclRefExprVisitor Visitor;
Visitor.setShouldVisitImplicitCode(true);
- // We're expecting the "i" in the lambda to be visited twice:
- // - Once for the DeclRefExpr in the lambda capture initialization (whose
- // source code location is set to the first use of the variable).
- // - Once for the DeclRefExpr for the use of "i" inside the lambda.
- Visitor.ExpectMatch("i", 1, 24, /*Times=*/2);
+ // We're expecting "i" to be visited twice: once for the initialization expr
+ // for the captured variable "i" outside of the lambda body, and again for
+ // the use of "i" inside the lambda.
+ Visitor.ExpectMatch("i", 1, 20, /*Times=*/1);
+ Visitor.ExpectMatch("i", 1, 24, /*Times=*/1);
EXPECT_TRUE(Visitor.runOver(
"void f() { int i; [=]{ i; }; }",
DeclRefExprVisitor::Lang_CXX11));
Index: clang/test/SemaCXX/uninitialized.cpp
===
--- clang/test/SemaCXX/uninitialized.cpp
+++ clang/test/SemaCXX/uninitialized.cpp
@@ -884,8 +884,10 @@
int x;
};
A a0([] { return a0.x; }); // ok
- void f() {
-A a1([=] { return a1.x; }); // expected-warning{{variable 'a1' is uninitialized when used within its own initialization}}
+ void f() {
+A a1([=] { // expected-warning{{variable 'a1' is uninitialized when used within its own initialization}}
+ return a1.x;
+});
A a2([&] { return a2.x; }); // ok
}
}
Index: clang/test/CodeGenCXX/debug-info-lambda.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-lambda.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm \
+// RUN: -debug-info-kind=line-tables-only -dwarf-column-info -std=c++11 %s -o - | FileCheck %s
+
+// CHECK-LABEL: define{{.*}}lambda_in_func
+void lambda_in_func(int &ref) {
+ // CHECK: [[ref_slot:%.*]] = getelementptr inbounds %class.anon, %class.anon* {{.*}}, i32 0, i32 0, !dbg [[lambda_decl_loc:![0-9]+]]
+ // CHECK-NEXT: %1 = load i32*, i32** %ref.addr, align 8, !dbg [[capture_init_loc:![0-9]+]]
+ // CHECK-NEXT: store i32* %1, i32** %0, align 8, !dbg [[lambda_decl_loc]]
+ // CHECK-NEXT: call void {{.*}}, !dbg [[lambda_call_loc:![0-9]+]]
+
+ auto helper = [ // CHECK: [[lambda_decl_loc]] = !DILocation(line: [[@LINE]], column: 17
+ &]() { // CHECK: [[capture_init_loc]] = !DILocation(line: [[@LINE]], column: 18
+++ref;
+ };
+ helper(); // CHECK: [[lambda_call_loc]] = !DILocation(line: [[@LINE]]
+}
Index: clang/test/CXX/expr/expr.prim/expr.prim.lambda/templates.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.lambda/templates.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.lambda/templates.cpp
@@ -88,8 +88,8 @@
template
void odr_used2(R &r, Boom boom) {
const std::type_info &ti
- = typeid([=,&r] () -> R& {
- boom.tickle(); // expected-note{{in instantiation of member function}}
+ = typeid([=,&r] () -> R& { // expected-note{{in instantiation of member function 'p2::Boom::Boom' requested here}}
+ boom.tickle();
return r;
}());
}
Index: clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
@@ -15,8 +15,8 @@
void capture_by_copy(NonCopyable nc, NonCopyable &ncr, const NonConstCopy nco) {
(void)[nc] { }; // expected-error{{capture of variable 'nc' as type 'NonCopyable' calls private copy constructor}}
- (void)[=] {
-ncr.foo(); // expected-error{{capture of variable 'ncr' as type 'NonCopyable' calls private copy constructor}}
+ (void)[=] { // expected-error{{capture of variable 'ncr' as type 'NonCopyable' calls private copy constructor}}
+ncr.foo();
}();
[nco] {}(); // expected-error{{no matching constructor for initialization of 'const NonConstCopy'}}
Index: clang/lib/Sema/SemaLambda.cpp
=
[PATCH] D50927: [Sema] Remove location from implicit capture init expr
vsk added inline comments. Comment at: clang/lib/Sema/SemaLambda.cpp:1422-1424 auto Entity = InitializedEntity::InitializeLambdaCapture( Var->getIdentifier(), Field->getType(), Loc); InitializationKind InitKind = InitializationKind::CreateDirect(Loc, Loc, Loc); rsmith wrote: > Should these locations also be updated to `InitLoc`? If we're modeling the > initialization as happening at the capture-default, we should do that > consistently. I've revised the patch to use one location consistently here. The tradeoff is that a few diagnostics now point to CaptureDefaultLoc instead of within the lambda body, but I'm happy to defer to more experienced Sema hands. Comment at: clang/lib/Sema/SemaLambda.cpp:1612 +auto InitResult = performLambdaVarCaptureInitialization( +*this, From, *CurField, IntroducerRange.getBegin(), IsImplicit); if (InitResult.isInvalid()) rsmith wrote: > Use `CaptureDefaultLoc` here instead of the start of the introducer range. Sure. This does seem better for diagnostic reporting purposes. I'll just note that it may make the line table look awkward in the (admittedly unlikely) event that 'IntroducerRange.getBegin()' and 'CaptureDefaultLoc' are on different lines. https://reviews.llvm.org/D50927 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51945: [Clang] Fix test to followup https://reviews.llvm.org/rL341977
vsk added a comment. I took care of this in r341985. Repository: rC Clang https://reviews.llvm.org/D51945 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52034: [Clang] Add options -Xclang -coverage-filter and -Xclang -coverage-exclude to filter the files to instrument with gcov
vsk added a comment. Please document the filter behavior (see docs/UsersManual.rst). Comment at: include/clang/Driver/CC1Options.td:236 +def coverage_exclude_EQ : Joined<["-"], "coverage-exclude=">, + Alias; def coverage_exit_block_before_body : Flag<["-"], "coverage-exit-block-before-body">, Have you checked whether gcc supports similar options? If so, it would be great if we could match their name & behavior. Repository: rC Clang https://reviews.llvm.org/D52034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52064: [Sema] Add a note pointing to the first use of an implicit capture
vsk created this revision.
vsk added reviewers: rsmith, erichkeane.
When it's not possible to initialize an implicit capture, add a note
pointing to the first use of the captured variable.
Example (the `note` is new):
lambda-expressions.cpp:81:15: error: no matching constructor for initialization
of 'G'
return [=]{
^
lambda-expressions.cpp:82:24: note: implicitly capturing 'g', first used here
const G* gg = &g;
^
As suggested in https://reviews.llvm.org/D50927.
https://reviews.llvm.org/D52064
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Frontend/FrontendActions.cpp
clang/lib/Sema/SemaLambda.cpp
clang/lib/Sema/SemaTemplateInstantiate.cpp
clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
clang/test/SemaCXX/lambda-expressions.cpp
Index: clang/test/SemaCXX/lambda-expressions.cpp
===
--- clang/test/SemaCXX/lambda-expressions.cpp
+++ clang/test/SemaCXX/lambda-expressions.cpp
@@ -77,8 +77,18 @@
struct G { G(); G(G&); int a; }; // expected-note 6 {{not viable}}
G g;
[=]() { const G* gg = &g; return gg->a; };
-[=]() { return [=]{ const G* gg = &g; return gg->a; }(); }; // expected-error {{no matching constructor for initialization of 'G'}}
-(void)^{ return [=]{ const G* gg = &g; return gg->a; }(); }; // expected-error 2 {{no matching constructor for initialization of 'const G'}}
+[=]() {
+ return [=]{ // expected-error {{no matching constructor for initialization of 'G'}}
+const G* gg = &g; // expected-note {{implicitly capturing 'g', first used here}}
+return gg->a;
+ }();
+};
+(void)^{ // expected-error@+1 {{no matching constructor for initialization of 'const G'}}
+ return [=]{ // expected-error@+1 {{no matching constructor for initialization of 'const G'}}
+const G* gg = &g; // expected-note {{implicitly capturing 'g', first used here}}
+return gg->a;
+ }();
+};
const int h = a; // expected-note {{declared}}
[]() { return h; }; // expected-error {{variable 'h' cannot be implicitly captured in a lambda with no capture-default specified}} expected-note {{lambda expression begins here}}
@@ -378,7 +388,9 @@
namespace PR18473 {
template void f() {
T t(0);
-(void) [=]{ int n = t; }; // expected-error {{deleted}}
+(void)[=] { // expected-error {{call to deleted constructor of 'PR18473::NoCopy'}}
+ int n = t; // expected-note {{implicitly capturing 't', first used here}}
+};
}
template void f();
Index: clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
@@ -16,7 +16,7 @@
void capture_by_copy(NonCopyable nc, NonCopyable &ncr, const NonConstCopy nco) {
(void)[nc] { }; // expected-error{{capture of variable 'nc' as type 'NonCopyable' calls private copy constructor}}
(void)[=] { // expected-error{{capture of variable 'ncr' as type 'NonCopyable' calls private copy constructor}}
-ncr.foo();
+ncr.foo(); // expected-note{{implicitly capturing 'ncr', first used here}}
}();
[nco] {}(); // expected-error{{no matching constructor for initialization of 'const NonConstCopy'}}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -200,6 +200,7 @@
case DeclaringSpecialMember:
case DefiningSynthesizedFunction:
case ExceptionSpecEvaluation:
+ case ImplicitLambdaCaptureInitialization:
return false;
// This function should never be called when Kind's value is Memoization.
@@ -653,6 +654,13 @@
break;
}
+case CodeSynthesisContext::ImplicitLambdaCaptureInitialization: {
+ Diags.Report(Active->PointOfInstantiation,
+ diag::note_implicitly_capturing_var_first_used_here)
+ << cast(Active->Entity);
+ break;
+}
+
case CodeSynthesisContext::Memoization:
break;
}
@@ -698,6 +706,7 @@
case CodeSynthesisContext::DeclaringSpecialMember:
case CodeSynthesisContext::DefiningSynthesizedFunction:
+case CodeSynthesisContext::ImplicitLambdaCaptureInitialization:
// This happens in a context unrelated to template instantiation, so
// there is no SFINAE.
return None;
Index: clang/lib/Sema/SemaLambda.cpp
===
--- clang/lib/Sema/SemaLambda.cpp
+++ clang/lib/Sema/SemaLambda.cpp
@@ -1401,6 +1401,14 @@
SourceLocation Loc =
IsImplicitCapture ? ImplicitCaptureLoc : Capture.getLocation();
+ if (IsImplicitCapture) {
+Sema::CodeSynthesisContext Ctx;
+Ctx.Entity = Var;
+Ctx.Kind
[PATCH] D52064: [Sema] Add a note pointing to the first use of an implicit capture
vsk updated this revision to Diff 165403.
vsk marked an inline comment as done.
https://reviews.llvm.org/D52064
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Frontend/FrontendActions.cpp
clang/lib/Sema/SemaLambda.cpp
clang/lib/Sema/SemaTemplateInstantiate.cpp
clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
clang/test/SemaCXX/lambda-expressions.cpp
Index: clang/test/SemaCXX/lambda-expressions.cpp
===
--- clang/test/SemaCXX/lambda-expressions.cpp
+++ clang/test/SemaCXX/lambda-expressions.cpp
@@ -77,8 +77,18 @@
struct G { G(); G(G&); int a; }; // expected-note 6 {{not viable}}
G g;
[=]() { const G* gg = &g; return gg->a; };
-[=]() { return [=]{ const G* gg = &g; return gg->a; }(); }; // expected-error {{no matching constructor for initialization of 'G'}}
-(void)^{ return [=]{ const G* gg = &g; return gg->a; }(); }; // expected-error 2 {{no matching constructor for initialization of 'const G'}}
+[=]() {
+ return [=]{ // expected-error {{no matching constructor for initialization of 'G'}}
+const G* gg = &g; // expected-note {{implicitly capturing 'g', first used here}}
+return gg->a;
+ }();
+};
+(void)^{
+ return [=]{ // expected-error {{no matching constructor for initialization of 'const G'}}
+const G* gg = &g; // expected-error {{no matching constructor for initialization of 'const G'}} expected-note {{implicitly capturing 'g', first used here}}
+return gg->a;
+ }();
+};
const int h = a; // expected-note {{declared}}
[]() { return h; }; // expected-error {{variable 'h' cannot be implicitly captured in a lambda with no capture-default specified}} expected-note {{lambda expression begins here}}
@@ -378,7 +388,9 @@
namespace PR18473 {
template void f() {
T t(0);
-(void) [=]{ int n = t; }; // expected-error {{deleted}}
+(void)[=] { // expected-error {{call to deleted constructor of 'PR18473::NoCopy'}}
+ int n = t; // expected-note {{implicitly capturing 't', first used here}}
+};
}
template void f();
Index: clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp
@@ -16,7 +16,7 @@
void capture_by_copy(NonCopyable nc, NonCopyable &ncr, const NonConstCopy nco) {
(void)[nc] { }; // expected-error{{capture of variable 'nc' as type 'NonCopyable' calls private copy constructor}}
(void)[=] { // expected-error{{capture of variable 'ncr' as type 'NonCopyable' calls private copy constructor}}
-ncr.foo();
+ncr.foo(); // expected-note{{implicitly capturing 'ncr', first used here}}
}();
[nco] {}(); // expected-error{{no matching constructor for initialization of 'const NonConstCopy'}}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -200,6 +200,7 @@
case DeclaringSpecialMember:
case DefiningSynthesizedFunction:
case ExceptionSpecEvaluation:
+ case ImplicitLambdaCaptureInitialization:
return false;
// This function should never be called when Kind's value is Memoization.
@@ -653,6 +654,13 @@
break;
}
+case CodeSynthesisContext::ImplicitLambdaCaptureInitialization: {
+ Diags.Report(Active->PointOfInstantiation,
+ diag::note_implicitly_capturing_var_first_used_here)
+ << cast(Active->Entity);
+ break;
+}
+
case CodeSynthesisContext::Memoization:
break;
}
@@ -698,6 +706,7 @@
case CodeSynthesisContext::DeclaringSpecialMember:
case CodeSynthesisContext::DefiningSynthesizedFunction:
+case CodeSynthesisContext::ImplicitLambdaCaptureInitialization:
// This happens in a context unrelated to template instantiation, so
// there is no SFINAE.
return None;
Index: clang/lib/Sema/SemaLambda.cpp
===
--- clang/lib/Sema/SemaLambda.cpp
+++ clang/lib/Sema/SemaLambda.cpp
@@ -21,6 +21,7 @@
#include "clang/Sema/ScopeInfo.h"
#include "clang/Sema/SemaInternal.h"
#include "clang/Sema/SemaLambda.h"
+#include "llvm/ADT/ScopeExit.h"
using namespace clang;
using namespace sema;
@@ -1401,6 +1402,18 @@
SourceLocation Loc =
IsImplicitCapture ? ImplicitCaptureLoc : Capture.getLocation();
+ if (IsImplicitCapture) {
+Sema::CodeSynthesisContext Ctx;
+Ctx.Entity = Var;
+Ctx.Kind = Sema::CodeSynthesisContext::ImplicitLambdaCaptureInitialization;
+Ctx.PointOfInstantiation = Capture.getLocation();
+S.pushCodeSynthesisContext(Ctx);
+ }
+ auto PopCodeSynthesisStackOnExit = llvm::make_scope_exit([
[PATCH] D52064: [Sema] Add a note pointing to the first use of an implicit capture
vsk added inline comments.
Comment at: clang/test/SemaCXX/lambda-expressions.cpp:87
+(void)^{ // expected-error@+1 {{no matching constructor for initialization
of 'const G'}}
+ return [=]{ // expected-error@+1 {{no matching constructor for
initialization of 'const G'}}
+const G* gg = &g; // expected-note {{implicitly capturing 'g', first
used here}}
rsmith wrote:
> Why are these @+1?
A 'no matching constructor' error is present on the line containing "[=]"
(pointing to the '=' sign), as well as on the line containing "gg = &g"
(pointing to the last 'g').
I'll try to capture that in a neater way.
Stepping back a bit, I think clang does this to make it clear that an implicit
capture is part of the problem. It does seem strange to me that we'd emit the
same error twice, but according to baseline version of this test, that's the
expected behavior. Let me know if I should try and change that diagnostic.
https://reviews.llvm.org/D52064
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part
vsk added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:97 + is not equal to the original value before the downcast. This kind of issues + may often be caused by an implicit integer promotions. - ``-fsanitize=integer-divide-by-zero``: Integer division by zero. Nitpicks: kind of issues -> issue promotions -> conversions Comment at: docs/UndefinedBehaviorSanitizer.rst:134 + integer promotions, as those may result in an unexpected computation + results, even though no overflow happens (signed or unsigned). - ``-fsanitize=unreachable``: If control flow reaches an unreachable Could you make this more explicit? It would help to point out that this check does not diagnose lossy implicit integer conversions, but that the new check does. Ditto for the comment in the unsigned-integer-overflow section. Comment at: lib/CodeGen/CodeGenFunction.h:383 + // This stack is used/maintained exclusively by the implicit cast sanitizer. + llvm::SmallVector CastExprStack; + Why not 0 instead of 8, given that in the common case, this stack is unused? Comment at: lib/CodeGen/CodeGenFunction.h:388 + +llvm::SmallVector GuardedCasts; + I'm not sure the cost of maintaining an extra vector is worth the benefit of the added assertion. Wouldn't it be cheaper to just store the number of pushed casts? You'd only need one constructor which accepts an ArrayRef. Comment at: test/CodeGen/catch-implicit-integer-truncations.c:29 + // CHECK-SANITIZE-NEXT: %[[TRUNCHECK:.*]] = icmp eq i32 %[[ANYEXT]], %[[SRC]], !nosanitize + // CHECK-SANITIZE-ANYRECOVER-NEXT: br i1 %[[TRUNCHECK]], label %[[CONT:.*]], label %[[HANDLER_IMPLICIT_CAST:.*]], !prof ![[WEIGHT_MD:.*]], !nosanitize + // CHECK-SANITIZE-TRAP-NEXT: br i1 %[[TRUNCHECK]], label %[[CONT:.*]], label %[[HANDLER_IMPLICIT_CAST:.*]], !nosanitize There's no need to check the profile metadata here. Comment at: test/CodeGen/catch-implicit-integer-truncations.c:159 +// == // +// The expected false-negatives. +// == // nit, aren't these true-negatives because we expect to see no errors? Repository: rC Clang https://reviews.llvm.org/D48958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part
vsk added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:159 - ``-fsanitize=undefined``: All of the checks listed above other than ``unsigned-integer-overflow`` and the ``nullability-*`` checks. - ``-fsanitize=undefined-trap``: Deprecated alias of Please add "the `implicit-cast` group of checks" to this list. Comment at: docs/UndefinedBehaviorSanitizer.rst:134 + integer promotions, as those may result in an unexpected computation + results, even though no overflow happens (signed or unsigned). - ``-fsanitize=unreachable``: If control flow reaches an unreachable lebedev.ri wrote: > vsk wrote: > > Could you make this more explicit? It would help to point out that this > > check does not diagnose lossy implicit integer conversions, but that the > > new check does. Ditto for the comment in the unsigned-integer-overflow > > section. > Is this better? Looks good. Comment at: lib/CodeGen/CodeGenFunction.h:383 + // This stack is used/maintained exclusively by the implicit cast sanitizer. + llvm::SmallVector CastExprStack; + lebedev.ri wrote: > vsk wrote: > > Why not 0 instead of 8, given that in the common case, this stack is unused? > No longer relevant. I'm referring to CastExprStack within ScalarExprEmitter, which still allocates space for 8 pointers inline. Repository: rC Clang https://reviews.llvm.org/D48958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part
vsk added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:383 + // This stack is used/maintained exclusively by the implicit cast sanitizer. + llvm::SmallVector CastExprStack; + lebedev.ri wrote: > vsk wrote: > > lebedev.ri wrote: > > > vsk wrote: > > > > Why not 0 instead of 8, given that in the common case, this stack is > > > > unused? > > > No longer relevant. > > I'm referring to CastExprStack within ScalarExprEmitter, which still > > allocates space for 8 pointers inline. > Ah, you mean in the general case when the sanitizer is disabled? > Yes. It's a relatively minor concern, but clang's stack can get pretty deep inside of CodeGenFunction. At one point we needed to outline code by hand to unbreak the ASan build. Later I think we just increased the stack size rlimit. I don't see a countervailing performance benefit of allocating more space inline, at least not here. Repository: rC Clang https://reviews.llvm.org/D48958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part
vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.
LGTM, although I think it'd be helpful to have another +1 just to be safe.
I did two small experiments with this using a Stage1 Rel+Asserts compiler:
Stage2 Rel+Asserts build of llvm-tblgen:
ninja llvm-tblgen 384.27s user 14.98s system 1467% cpu 27.203 total
Stage2 Rel+Asserts build of llvm-tblgen with implicit-cast checking:
ninja llvm-tblgen 385.15s user 15.02s system 1472% cpu 27.170 total
With caveats about having a small sample size here and testing with an
asserts-enabled stage1 build, I don't see any red flags about the compile-time
overhead of the new check. I would have liked to measure the check against more
code, but I couldn't complete a stage2 build due to a diagnostic which might
plausibly point to a real issue in tblgen:
/Users/vsk/src/llvm.org-lldbsan/llvm/utils/TableGen/RegisterInfoEmitter.cpp:604:17:
runtime error: implicit cast from type 'int' of value -1 (32-bit, signed) to
type 'const unsigned short' changed the value to 65535 (16-bit, unsigned)
With -fno-sanitize-recover=all disabled, I found a few more reports:
/Users/vsk/src/llvm.org-lldbsan/llvm/include/llvm/Object/Archive.h:278:38:
runtime error: implicit cast from type 'int' of value -1 (32-bit, signed) to
type 'uint16_t' (aka 'unsigned short') changed the value to 65535 (16-bit,
unsigned)
--> uint16_t FirstRegularStartOfFile = -1;
/Users/vsk/src/llvm.org-lldbsan/llvm/lib/Analysis/MemorySSA.cpp:199:12:
runtime error: implicit cast from type 'size_t' (aka 'unsigned long') of value
4969132974595412838 (64-bit, unsigned) to type 'unsigned int' changed the value
to 3765474150 (32-bit, unsigned)
--> hash_combine() result casted to unsigned
/Users/vsk/src/llvm.org-lldbsan/llvm/lib/CodeGen/TargetLoweringBase.cpp:1212:30:
runtime error: implicit cast from type 'unsigned int' of value 512 (32-bit,
unsigned) to type 'unsigned char' changed the value to 0 (8-bit, unsigned)
--> NumRegistersForVT[i] = getVectorTypeBreakdownMVT(...)
/Users/vsk/src/llvm.org-lldbsan/llvm/lib/Transforms/Scalar/EarlyCSE.cpp:136:12:
runtime error: implicit cast from type 'size_t' (aka 'unsigned long') of value
16583795711468875482 (64-bit, unsigned) to type 'unsigned int' changed the
value to 3116347098 (32-bit, unsigned)
--> hash_combine() result casted to unsigned
...
These four at least don't look like false positives:
- Maybe we should consider special-casing assignments of "-1" to unsigned
values? This seems somewhat idiomatic.
- At least a few of these are due to not being explicit about dropping the high
bits of hash_combine()'s result. Given that this check is opt-in, that that
seems like a legitimate diagnostic (lost entropy).
- The TargetLoweringBase.cpp diagnostic looks a bit scary.
Comment at: lib/CodeGen/CGExprScalar.cpp:986
+ for (auto CastRef : llvm::reverse(CastExprStack)) {
+const CastExpr *Cast = &(CastRef.get());
+// Was there a previous cast?
nit, extra parens?
Repository:
rC Clang
https://reviews.llvm.org/D48958
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49760: [clang:sema] de-duplicate getDepthAndIndex helpers
vsk added a comment. Thanks for doing this! Comment at: include/clang/Sema/SemaInternal.h:120 + if (const TemplateTypeParmType *TTP = + UPP.first.dyn_cast()) +return std::make_pair(TTP->getDepth(), TTP->getIndex()); Perhaps 'const auto *TTP = ...' would read better here, given that the expected type appears once already in the r.h.s of the expression? I have the same comment re: the three other casts above. Repository: rC Clang https://reviews.llvm.org/D49760 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49760: [clang:sema] de-duplicate getDepthAndIndex helpers
vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D49760#1174141, @nickdesaulniers wrote: > Thanks for the review. Today is my first day committing to clang! Welcome :). LGTM. Feel free to commit this yourself once you obtain commit access (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). I'd also be happy to commit this on your behalf. Repository: rC Clang https://reviews.llvm.org/D49760 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions
vsk added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:380 + /// True if sanitizer checks for current pointer value are generated. + bool PointerChecksAreEmitted = false; + rjmccall wrote: > sepavloff wrote: > > rjmccall wrote: > > > I don't think this is a good way of doing this. Using global state makes > > > this unnecessarily difficult to reason about and can have unintended > > > effects. For example, you are unintentionally suppressing any checks > > > that would be done as part of e.g. evaluating default arguments to the > > > default constructor. > > > > > > You should pass a parameter down that says whether checks are necessary. > > > You can also use that parameter to e.g. suppress checks when constructing > > > local variables and temporaries, although you may already have an > > > alternative mechanism for doing that. > > Passing parameter that inctructs whether to generate checks or not hardly > > can be directly implemented. This information is used in > > `EmitCXXConstructorCall` and it is formed during processing of `new` > > expression. The distance between these points can be 10 call frames, in > > some of them (`StmtVisitor` interface of `AggExprEmitter`) we cannot change > > function parameters. > > The case of `new` expression in default arguments indeed handled > > incorrectly, thank you for the catch. The updated version must process > > this case correctly. > I'm not going to accept a patch based on global state here. `AggValueSlot` > already propagates a bunch of flags about the slot into which we're emitting > an expression; I don't think it would be difficult to propagate some sort of > "alignment is statically known or already checked" flag along with that. + 1. @rjmccall offered similar advice in D25448, which led us to find a few more bugs / missed opportunities we otherwise wouldn't have. Repository: rC Clang https://reviews.llvm.org/D49589 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43787: Fix which Darwin versions have ObjC runtime with full subscripting support.
vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks for digging into and addressing this! https://reviews.llvm.org/D43787 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33588: Fix two sources of UB in __next_hash_pow2 (from __hash_table)
vsk created this revision.
There are two sources of undefined behavior in __next_hash_pow2: an
invalid shift (occurs when __n is 0 or 1), and an invalid call to CLZ
(when __n=1).
This patch corrects both issues. It's NFC for all values of __n which do
not trigger UB, and leads to defined results when __n is 0 or 1.
Minimal reproducer:
unordered_map m;
m.reserve(4);
m.reserve(1);
I wrote a miniature 'fuzzer' to test this change and ran it with UBSan
enabled. AFAIK, this is the best we can do for a test. I don't know how
to write a non-flaky test which would fail without this patch applied.
Here is the 'fuzzer' (simply run with -fsanitize=undefined):
void fuzzUnorderedMap(unsigned NumInserts, unsigned NumReserve1,
unsigned NumReserve2) {
unordered_map m;
m.reserve(NumReserve1);
for (unsigned I = 0; I < NumInserts; ++I) { m[I] = 0; }
size_t __n = size_t(ceil(float(m.size()) / m.max_load_factor()));
m.reserve(NumReserve2);
}
...
for (unsigned NumInserts = 0; NumInserts <= 64; ++NumInserts)
for (unsigned NumReserve1 = 1; NumReserve1 <= 64; ++NumReserve1)
for (unsigned NumReserve2 = 1; NumReserve2 <= 64; ++NumReserve2)
fuzzUnorderedMap(NumInserts, NumReserve1, NumReserve2);
rdar://problem/32407328
https://reviews.llvm.org/D33588
Files:
include/__hash_table
Index: include/__hash_table
===
--- include/__hash_table
+++ include/__hash_table
@@ -136,7 +136,7 @@
size_t
__next_hash_pow2(size_t __n)
{
-return size_t(1) << (std::numeric_limits::digits - __clz(__n-1));
+return (__n > 1) ? (size_t(1) << (std::numeric_limits::digits -
__clz(__n-1))) : __n;
}
Index: include/__hash_table
===
--- include/__hash_table
+++ include/__hash_table
@@ -136,7 +136,7 @@
size_t
__next_hash_pow2(size_t __n)
{
-return size_t(1) << (std::numeric_limits::digits - __clz(__n-1));
+return (__n > 1) ? (size_t(1) << (std::numeric_limits::digits - __clz(__n-1))) : __n;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33588: Fix two sources of UB in __next_hash_pow2 (from __hash_table)
vsk added a comment. In https://reviews.llvm.org/D33588#765768, @mclow.lists wrote: > I can reproduce this, but I'd rather figure out why we're calling > `__next_hash_pow2(0)` or `(1)` before deciding how to fix it. It looks like we hit the UB while attempting to shrink a hash table during a rehash. If the current bucket count is a power of two, we try and find a smaller bucket count (also a power of two) large enough to accommodate all entries in the table. The argument passed in to next_hash_pow2 from hash_table::rehash is `__n = size_t(ceil(float(size()) / max_load_factor()))`. I think `__n = 0` if the table is empty. And `__n = 1` when the maximum load factor is (roughly) equal to the table's size. As an alternate fix, it might be worth considering changing the rehashing algorithm. But I'd like to start with a more conservative fix for the UB issue, at least initially. https://reviews.llvm.org/D33588 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33588: Fix two sources of UB in __next_hash_pow2 (from __hash_table)
vsk updated this revision to Diff 100461.
vsk edited the summary of this revision.
vsk added a comment.
Thanks @EricWF for pointing me to the right place to add a test. I've tried to
follow the style used by the existing tests. PTAL.
https://reviews.llvm.org/D33588
Files:
include/__hash_table
test/libcxx/containers/unord/next_pow2.pass.cpp
Index: test/libcxx/containers/unord/next_pow2.pass.cpp
===
--- /dev/null
+++ test/libcxx/containers/unord/next_pow2.pass.cpp
@@ -0,0 +1,80 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// REQUIRES: long_tests
+
+// Not a portable test
+
+// <__hash_table>
+
+// size_t __next_hash_pow2(size_t n);
+
+// If n <= 1, return n. If n is a power of 2, return n.
+// Otherwise, return the next power of 2.
+
+#include <__hash_table>
+#include
+#include
+
+#include
+
+bool
+is_power_of_two(unsigned long n)
+{
+return __builtin_popcount(n) == 1;
+}
+
+void
+test_next_pow2()
+{
+assert(!is_power_of_two(0));
+assert(is_power_of_two(1));
+assert(is_power_of_two(2));
+assert(!is_power_of_two(3));
+
+assert(std::__next_hash_pow2(0) == 0);
+assert(std::__next_hash_pow2(1) == 1);
+
+for (std::size_t n = 2; n < (sizeof(std::size_t) * 8 - 1); ++n)
+{
+std::size_t pow2 = 1ULL << n;
+assert(std::__next_hash_pow2(pow2) == pow2);
+}
+
+for (std::size_t n : {3, 7, 9, 15, 127, 129})
+{
+std::size_t npow2 = std::__next_hash_pow2(n);
+assert(is_power_of_two(npow2) && npow2 > n);
+}
+}
+
+// Note: this is only really useful when run with -fsanitize=undefined.
+void
+fuzz_unordered_map_reserve(unsigned num_inserts,
+ unsigned num_reserve1,
+ unsigned num_reserve2)
+{
+std::unordered_map m;
+m.reserve(num_reserve1);
+for (unsigned I = 0; I < num_inserts; ++I) m[I] = 0;
+m.reserve(num_reserve2);
+assert(m.bucket_count() >= num_reserve2);
+}
+
+int main()
+{
+test_next_pow2();
+
+for (unsigned num_inserts = 0; num_inserts <= 64; ++num_inserts)
+for (unsigned num_reserve1 = 1; num_reserve1 <= 64; ++num_reserve1)
+for (unsigned num_reserve2 = 1; num_reserve2 <= 64; ++num_reserve2)
+fuzz_unordered_map_reserve(num_inserts, num_reserve1,
num_reserve2);
+
+return 0;
+}
Index: include/__hash_table
===
--- include/__hash_table
+++ include/__hash_table
@@ -136,7 +136,7 @@
size_t
__next_hash_pow2(size_t __n)
{
-return size_t(1) << (std::numeric_limits::digits - __clz(__n-1));
+return (__n > 1) ? (size_t(1) << (std::numeric_limits::digits -
__clz(__n-1))) : __n;
}
Index: test/libcxx/containers/unord/next_pow2.pass.cpp
===
--- /dev/null
+++ test/libcxx/containers/unord/next_pow2.pass.cpp
@@ -0,0 +1,80 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// REQUIRES: long_tests
+
+// Not a portable test
+
+// <__hash_table>
+
+// size_t __next_hash_pow2(size_t n);
+
+// If n <= 1, return n. If n is a power of 2, return n.
+// Otherwise, return the next power of 2.
+
+#include <__hash_table>
+#include
+#include
+
+#include
+
+bool
+is_power_of_two(unsigned long n)
+{
+return __builtin_popcount(n) == 1;
+}
+
+void
+test_next_pow2()
+{
+assert(!is_power_of_two(0));
+assert(is_power_of_two(1));
+assert(is_power_of_two(2));
+assert(!is_power_of_two(3));
+
+assert(std::__next_hash_pow2(0) == 0);
+assert(std::__next_hash_pow2(1) == 1);
+
+for (std::size_t n = 2; n < (sizeof(std::size_t) * 8 - 1); ++n)
+{
+std::size_t pow2 = 1ULL << n;
+assert(std::__next_hash_pow2(pow2) == pow2);
+}
+
+for (std::size_t n : {3, 7, 9, 15, 127, 129})
+{
+std::size_t npow2 = std::__next_hash_pow2(n);
+assert(is_power_of_two(npow2) && npow2 > n);
+}
+}
+
+// Note: this is only really useful when run with -fsanitize=undefined.
+void
+fuzz_unordered_map_reserve(unsigned num_inserts,
+ unsigned num_reserve1,
+ unsigned num_reserve2)
+{
+std::unordered_map m;
+m.reserve(num_reserve1);
+for (unsigned I = 0; I < num_inserts; ++I) m[I] = 0;
+m.res
[PATCH] D33305: [ubsan] Add a check for pointer overflow UB
vsk updated this revision to Diff 100475.
vsk edited the summary of this revision.
vsk added a comment.
Ping.
https://reviews.llvm.org/D33305
Files:
docs/UndefinedBehaviorSanitizer.rst
include/clang/Basic/Sanitizers.def
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-pointer-overflow.m
test/Driver/fsanitize.c
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -3,27 +3,27 @@
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang -target x86_64-linux-gnu -fsanitize-undefined-trap-on-error -fsanitize=undefined-trap %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
-// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute|function),?){18}"}}
-// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
-// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute|function),?){19}"}}
+// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED
-// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){19}"}}
+// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){20}"}}
// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN
-// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
+// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){18}"}}
// RUN: %clang -target i386-unknown-openbsd -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-OPENBSD
-// CHECK-UNDEFINED-OPENBSD: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
+// CHECK-UNDEFINED-OPENBSD: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonn
[PATCH] D32842: Specify which sanitizers are covered by a sanitizer blacklist
vsk added a comment. In https://reviews.llvm.org/D32842#768038, @eugenis wrote: > This change scares me a bit, to be honest. Is this really that big of a > problem? Blacklists are not supposed to be big, maybe we can tolerate a few > false negatives? I'd like to make it possible to deploy a larger default blacklist for one sanitizer without inhibiting the other sanitizers. This would be useful if we find a bug in a runtime check: we could temporarily work around the issue by deploying a new blacklist, instead of changing the compiler or build system. It won't be possible to do this if blacklist updates can introduce false negatives. Also, as a separate point, I think a design which permits false negatives is worrisome in and of itself, and is worth fixing. > Consider extending the blacklist syntax instead, the same way Valgrind does. I like this idea. However, I think that it would require most of the changes in this patch, with some additional work to change SpecialCaseList. > Blacklist entries could be annotated with a list of sanitizers they apply to, > like > > asan,ubsan : src: file.cc:line > > or an even less verbose way using sections > > [asan] > src: > src: > [msan] > src: > > As an extra benefit, all default blacklists can be merged into one. It would be nice to combine the default blacklists into one file with separate sections for each sanitizer. I'll look into this (hopefully next week). https://reviews.llvm.org/D32842 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33305: [ubsan] Add a check for pointer overflow UB
vsk added a comment. Thanks for the review! I've rebased the patch and plan on checking it in tomorrow. At the moment I'm getting some additional test coverage (running check-libcxx and testing more backends). https://reviews.llvm.org/D33305 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33588: Fix two sources of UB in __next_hash_pow2 (from __hash_table)
vsk added inline comments.
Comment at: include/__hash_table:139
{
-return size_t(1) << (std::numeric_limits::digits - __clz(__n-1));
+return (__n > 1) ? (size_t(1) << (std::numeric_limits::digits -
__clz(__n-1))) : __n;
}
EricWF wrote:
> Shouldn't this return `__n + 1` when `__n <= 1`, or even 2 in both cases?
I thought "next_hash_pow2(n)" meant "a hash based on n or the first
power-of-two GTE n", but I suppose it's actually "first power-of-two GTE than
n, for hash tables". In this case, returning `1` when `__n <= 1` makes the most
sense to me, since it's the first power of two GTE 0 and 1. What do you think?
https://reviews.llvm.org/D33588
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33910: [ubsan] Detect invalid unsigned pointer index expression (clang)
vsk created this revision.
Adding an unsigned offset to a base pointer has undefined behavior if
the result of the expression would precede the base. An example from
@regehr:
int foo(char *p, unsigned offset) {
return p + offset >= p; // This may be optimized to '1'.
}
foo(p, -1); // UB.
This patch extends the pointer overflow check in ubsan to detect invalid
unsigned pointer index expressions. It changes the instrumentation to
only permit non-negative offsets in pointer index expressions when all
of the GEP indices are unsigned.
Aside: If anyone has a better name for this type of bug, I'm all ears.
Using "unsigned pointer index expression" could be a problem, because it
sounds like an indexing expression with an _unsigned pointer_.
https://reviews.llvm.org/D33910
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-pointer-overflow.m
Index: test/CodeGen/ubsan-pointer-overflow.m
===
--- test/CodeGen/ubsan-pointer-overflow.m
+++ test/CodeGen/ubsan-pointer-overflow.m
@@ -5,9 +5,7 @@
// CHECK: [[BASE:%.*]] = ptrtoint i8* {{.*}} to i64, !nosanitize
// CHECK-NEXT: [[COMPGEP:%.*]] = add i64 [[BASE]], 1, !nosanitize
// CHECK-NEXT: [[POSVALID:%.*]] = icmp uge i64 [[COMPGEP]], [[BASE]], !nosanitize
- // CHECK-NEXT: [[NEGVALID:%.*]] = icmp ult i64 [[COMPGEP]], [[BASE]], !nosanitize
- // CHECK-NEXT: [[DIFFVALID:%.*]] = select i1 true, i1 [[POSVALID]], i1 [[NEGVALID]], !nosanitize
- // CHECK-NEXT: [[VALID:%.*]] = and i1 true, [[DIFFVALID]], !nosanitize
+ // CHECK-NEXT: [[VALID:%.*]] = and i1 true, [[POSVALID]], !nosanitize
// CHECK-NEXT: br i1 [[VALID]]{{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}, i64 [[BASE]], i64 [[COMPGEP]]){{.*}}, !nosanitize
++p;
@@ -34,11 +32,11 @@
// CHECK-NEXT: [[SMULVAL:%.*]] = extractvalue { i64, i1 } [[SMUL]], 0, !nosanitize
// CHECK-NEXT: [[BASE:%.*]] = ptrtoint i8* {{.*}} to i64, !nosanitize
// CHECK-NEXT: [[COMPGEP:%.*]] = add i64 [[BASE]], [[SMULVAL]], !nosanitize
+ // CHECK-NEXT: [[OFFSETVALID:%.*]] = xor i1 [[OFFSETOFLOW]], true, !nosanitize
// CHECK-NEXT: [[POSVALID:%.*]] = icmp uge i64 [[COMPGEP]], [[BASE]], !nosanitize
- // CHECK-NEXT: [[NEGVALID:%.*]] = icmp ult i64 [[COMPGEP]], [[BASE]], !nosanitize
// CHECK-NEXT: [[POSOFFSET:%.*]] = icmp sge i64 [[SMULVAL]], 0, !nosanitize
- // CHECK-DAG: [[OFFSETVALID:%.*]] = xor i1 [[OFFSETOFLOW]], true, !nosanitize
- // CHECK-DAG: [[DIFFVALID:%.*]] = select i1 [[POSOFFSET]], i1 [[POSVALID]], i1 [[NEGVALID]], !nosanitize
+ // CHECK-NEXT: [[NEGVALID:%.*]] = icmp ult i64 [[COMPGEP]], [[BASE]], !nosanitize
+ // CHECK-NEXT: [[DIFFVALID:%.*]] = select i1 [[POSOFFSET]], i1 [[POSVALID]], i1 [[NEGVALID]], !nosanitize
// CHECK: [[VALID:%.*]] = and i1 [[OFFSETVALID]], [[DIFFVALID]], !nosanitize
// CHECK-NEXT: br i1 [[VALID]]{{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}, i64 [[BASE]], i64 [[COMPGEP]]){{.*}}, !nosanitize
@@ -50,6 +48,27 @@
p - i;
}
+// CHECK-LABEL: define void @binary_arith_unsigned
+void binary_arith_unsigned(char *p, unsigned i) {
+ // CHECK: [[SMUL:%.*]] = call { i64, i1 } @llvm.smul.with.overflow.i64(i64 1, i64 %{{.*}}), !nosanitize
+ // CHECK-NEXT: [[SMULOFLOW:%.*]] = extractvalue { i64, i1 } [[SMUL]], 1, !nosanitize
+ // CHECK-NEXT: [[OFFSETOFLOW:%.*]] = or i1 false, [[SMULOFLOW]], !nosanitize
+ // CHECK-NEXT: [[SMULVAL:%.*]] = extractvalue { i64, i1 } [[SMUL]], 0, !nosanitize
+ // CHECK-NEXT: [[BASE:%.*]] = ptrtoint i8* {{.*}} to i64, !nosanitize
+ // CHECK-NEXT: [[COMPGEP:%.*]] = add i64 [[BASE]], [[SMULVAL]], !nosanitize
+ // CHECK-NEXT: [[OFFSETVALID:%.*]] = xor i1 [[OFFSETOFLOW]], true, !nosanitize
+ // CHECK-NEXT: [[POSVALID:%.*]] = icmp uge i64 [[COMPGEP]], [[BASE]], !nosanitize
+ // CHECK: [[VALID:%.*]] = and i1 [[OFFSETVALID]], [[POSVALID]], !nosanitize
+ // CHECK-NEXT: br i1 [[VALID]]{{.*}}, !nosanitize
+ // CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}, i64 [[BASE]], i64 [[COMPGEP]]){{.*}}, !nosanitize
+ p + i;
+
+ // CHECK: [[OFFSET:%.*]] = sub i64 0, {{.*}}
+ // CHECK-NEXT: getelementptr inbounds {{.*}} [[OFFSET]]
+ // CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
+ p - i;
+}
+
// CHECK-LABEL: define void @fixed_len_array
void fixed_len_array(int k) {
// CHECK: getelementptr inbounds [10 x [10 x i32]], [10 x [10 x i32]]* [[ARR:%.*]], i64 0, i64 [[IDXPROM:%.*]]
@@ -59,11 +78,11 @@
// CHECK-NEXT: [[SMULVAL:%.*]] = extractvalue { i64, i1 } [[SMUL]], 0, !nosanitize
// CHECK-NEXT: [[BASE:%.*]] = ptrtoint [10 x [10 x i32]]* [[ARR]] to i64, !nosanitize
// CHECK-NEXT: [[COMPGEP:%.*]] = add i64 [[BASE]], [[SMULVAL]], !nosanitize
+ // CHECK-NEXT: [[OFFSETVALID:%.*]] = xor i1 [[OFFSETOFLOW]], true, !nosanitize
// CHECK-NEXT: [[POSVALID:%.*]] = icmp uge i64 [[COMPGEP]], [[BASE]], !nosanitize
- // CHECK-NEXT:
[PATCH] D33910: [ubsan] Detect invalid unsigned pointer index expression (clang)
vsk updated this revision to Diff 101479.
vsk marked 3 inline comments as done.
vsk added a comment.
Thanks for the review comments. I've changed the calls which use Builder as
suggested, and fixed up the tests.
It sounds like this patch is in good shape, so I'll commit this after two days
provided that there's no blocking feedback.
https://reviews.llvm.org/D33910
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-pointer-overflow.m
Index: test/CodeGen/ubsan-pointer-overflow.m
===
--- test/CodeGen/ubsan-pointer-overflow.m
+++ test/CodeGen/ubsan-pointer-overflow.m
@@ -5,17 +5,13 @@
// CHECK: [[BASE:%.*]] = ptrtoint i8* {{.*}} to i64, !nosanitize
// CHECK-NEXT: [[COMPGEP:%.*]] = add i64 [[BASE]], 1, !nosanitize
// CHECK-NEXT: [[POSVALID:%.*]] = icmp uge i64 [[COMPGEP]], [[BASE]], !nosanitize
- // CHECK-NEXT: [[NEGVALID:%.*]] = icmp ult i64 [[COMPGEP]], [[BASE]], !nosanitize
- // CHECK-NEXT: [[DIFFVALID:%.*]] = select i1 true, i1 [[POSVALID]], i1 [[NEGVALID]], !nosanitize
- // CHECK-NEXT: [[VALID:%.*]] = and i1 true, [[DIFFVALID]], !nosanitize
- // CHECK-NEXT: br i1 [[VALID]]{{.*}}, !nosanitize
+ // CHECK-NEXT: br i1 [[POSVALID]]{{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}, i64 [[BASE]], i64 [[COMPGEP]]){{.*}}, !nosanitize
++p;
// CHECK: ptrtoint i8* {{.*}} to i64, !nosanitize
// CHECK-NEXT: add i64 {{.*}}, -1, !nosanitize
// CHECK: select i1 false{{.*}}, !nosanitize
- // CHECK-NEXT: and i1 true{{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
--p;
@@ -30,16 +26,35 @@
void binary_arith(char *p, int i) {
// CHECK: [[SMUL:%.*]] = call { i64, i1 } @llvm.smul.with.overflow.i64(i64 1, i64 %{{.*}}), !nosanitize
// CHECK-NEXT: [[SMULOFLOW:%.*]] = extractvalue { i64, i1 } [[SMUL]], 1, !nosanitize
- // CHECK-NEXT: [[OFFSETOFLOW:%.*]] = or i1 false, [[SMULOFLOW]], !nosanitize
// CHECK-NEXT: [[SMULVAL:%.*]] = extractvalue { i64, i1 } [[SMUL]], 0, !nosanitize
// CHECK-NEXT: [[BASE:%.*]] = ptrtoint i8* {{.*}} to i64, !nosanitize
// CHECK-NEXT: [[COMPGEP:%.*]] = add i64 [[BASE]], [[SMULVAL]], !nosanitize
+ // CHECK-NEXT: [[OFFSETVALID:%.*]] = xor i1 [[SMULOFLOW]], true, !nosanitize
// CHECK-NEXT: [[POSVALID:%.*]] = icmp uge i64 [[COMPGEP]], [[BASE]], !nosanitize
- // CHECK-NEXT: [[NEGVALID:%.*]] = icmp ult i64 [[COMPGEP]], [[BASE]], !nosanitize
// CHECK-NEXT: [[POSOFFSET:%.*]] = icmp sge i64 [[SMULVAL]], 0, !nosanitize
- // CHECK-DAG: [[OFFSETVALID:%.*]] = xor i1 [[OFFSETOFLOW]], true, !nosanitize
- // CHECK-DAG: [[DIFFVALID:%.*]] = select i1 [[POSOFFSET]], i1 [[POSVALID]], i1 [[NEGVALID]], !nosanitize
- // CHECK: [[VALID:%.*]] = and i1 [[OFFSETVALID]], [[DIFFVALID]], !nosanitize
+ // CHECK-NEXT: [[NEGVALID:%.*]] = icmp ult i64 [[COMPGEP]], [[BASE]], !nosanitize
+ // CHECK-NEXT: [[DIFFVALID:%.*]] = select i1 [[POSOFFSET]], i1 [[POSVALID]], i1 [[NEGVALID]], !nosanitize
+ // CHECK: [[VALID:%.*]] = and i1 [[DIFFVALID]], [[OFFSETVALID]], !nosanitize
+ // CHECK-NEXT: br i1 [[VALID]]{{.*}}, !nosanitize
+ // CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}, i64 [[BASE]], i64 [[COMPGEP]]){{.*}}, !nosanitize
+ p + i;
+
+ // CHECK: [[OFFSET:%.*]] = sub i64 0, {{.*}}
+ // CHECK-NEXT: getelementptr inbounds {{.*}} [[OFFSET]]
+ // CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
+ p - i;
+}
+
+// CHECK-LABEL: define void @binary_arith_unsigned
+void binary_arith_unsigned(char *p, unsigned i) {
+ // CHECK: [[SMUL:%.*]] = call { i64, i1 } @llvm.smul.with.overflow.i64(i64 1, i64 %{{.*}}), !nosanitize
+ // CHECK-NEXT: [[SMULOFLOW:%.*]] = extractvalue { i64, i1 } [[SMUL]], 1, !nosanitize
+ // CHECK-NEXT: [[SMULVAL:%.*]] = extractvalue { i64, i1 } [[SMUL]], 0, !nosanitize
+ // CHECK-NEXT: [[BASE:%.*]] = ptrtoint i8* {{.*}} to i64, !nosanitize
+ // CHECK-NEXT: [[COMPGEP:%.*]] = add i64 [[BASE]], [[SMULVAL]], !nosanitize
+ // CHECK-NEXT: [[OFFSETVALID:%.*]] = xor i1 [[SMULOFLOW]], true, !nosanitize
+ // CHECK-NEXT: [[POSVALID:%.*]] = icmp uge i64 [[COMPGEP]], [[BASE]], !nosanitize
+ // CHECK: [[VALID:%.*]] = and i1 [[POSVALID]], [[OFFSETVALID]], !nosanitize
// CHECK-NEXT: br i1 [[VALID]]{{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}, i64 [[BASE]], i64 [[COMPGEP]]){{.*}}, !nosanitize
p + i;
@@ -55,16 +70,15 @@
// CHECK: getelementptr inbounds [10 x [10 x i32]], [10 x [10 x i32]]* [[ARR:%.*]], i64 0, i64 [[IDXPROM:%.*]]
// CHECK-NEXT: [[SMUL:%.*]] = call { i64, i1 } @llvm.smul.with.overflow.i64(i64 40, i64 [[IDXPROM]]), !nosanitize
// CHECK-NEXT: [[SMULOFLOW:%.*]] = extractvalue { i64, i1 } [[SMUL]], 1, !nosanitize
- // CHECK-NEXT: [[OFFSETOFLOW:%.*]] = or i1 false, [[SMULOFLOW]], !nosanitize
// CHECK-NEXT: [[SMULVAL:%.*]] = extractvalue { i64, i1 } [[SMUL]], 0, !nosanitize
// CHECK-NEXT: [[BAS
[PATCH] D33910: [ubsan] Detect invalid unsigned pointer index expression (clang)
vsk added a comment. I've encountered some new diagnostics when running tests on a stage2 instrumented clang, and will need more time to investigate them. Sorry for the delayed communication, I am a bit swamped this week owing to wwdc and being a build cop. https://reviews.llvm.org/D33910 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34121: [ubsan] Teach the pointer overflow check that "p - <= p" (PR33430)
vsk created this revision.
The pointer overflow check gives false negatives when dealing with
expressions in which an unsigned value is subtracted from a pointer.
This is summarized in PR33430 [1]: ubsan permits the result of the
submission to be greater than "p", but it should not.
To fix the issue, we should track whether or not the pointer expression
is a subtraction. If it is, and the indices are unsigned, we know to
expect "p - <= p".
I've tested this by running check-{llvm,clang} with a stage2 ubsan-enabled
build. I've also added some tests to compiler-rt, which I'll upload in a
separate patch.
[1] https://bugs.llvm.org/show_bug.cgi?id=33430
https://reviews.llvm.org/D34121
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-pointer-overflow.m
Index: test/CodeGen/ubsan-pointer-overflow.m
===
--- test/CodeGen/ubsan-pointer-overflow.m
+++ test/CodeGen/ubsan-pointer-overflow.m
@@ -10,16 +10,20 @@
++p;
// CHECK: ptrtoint i8* {{.*}} to i64, !nosanitize
- // CHECK-NEXT: add i64 {{.*}}, -1, !nosanitize
- // CHECK: select i1 false{{.*}}, !nosanitize
+ // CHECK-NEXT: [[COMPGEP:%.*]] = add i64 {{.*}}, -1, !nosanitize
+ // CHECK: [[NEGVALID:%.*]] = icmp ule i64 [[COMPGEP]], {{.*}}, !nosanitize
+ // CHECK-NOT: select
+ // CHECK: br i1 [[NEGVALID]]{{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
--p;
+ // CHECK: icmp uge i64
// CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p++;
- // CHECK: select
+ // CHECK: icmp ule i64
+ // CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p--;
}
@@ -64,7 +68,8 @@
// CHECK: [[OFFSET:%.*]] = sub i64 0, {{.*}}
// CHECK-NEXT: getelementptr inbounds {{.*}} [[OFFSET]]
- // CHECK: select
+ // CHECK: icmp ule i64
+ // CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p - i;
}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -3555,9 +3555,12 @@
/// Same as IRBuilder::CreateInBoundsGEP, but additionally emits a check to
/// detect undefined behavior when the pointer overflow sanitizer is enabled.
/// \p SignedIndices indicates whether any of the GEP indices are signed.
+ /// \p IsSubtraction indicates whether the expression used to form the GEP
+ /// is a subtraction.
llvm::Value *EmitCheckedInBoundsGEP(llvm::Value *Ptr,
ArrayRef IdxList,
bool SignedIndices,
+ bool IsSubtraction,
SourceLocation Loc,
const Twine &Name = "");
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -1851,7 +1851,6 @@
llvm::Value *input;
int amount = (isInc ? 1 : -1);
- bool signedIndex = !isInc;
if (const AtomicType *atomicTy = type->getAs()) {
type = atomicTy->getValueType();
@@ -1941,8 +1940,9 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, numElts, "vla.inc");
else
-value = CGF.EmitCheckedInBoundsGEP(value, numElts, signedIndex,
- E->getExprLoc(), "vla.inc");
+value = CGF.EmitCheckedInBoundsGEP(
+value, numElts, /*SignedIndices=*/false, /*IsSubtraction=*/!isInc,
+E->getExprLoc(), "vla.inc");
// Arithmetic on function pointers (!) is just +-1.
} else if (type->isFunctionType()) {
@@ -1952,7 +1952,8 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, amt, "incdec.funcptr");
else
-value = CGF.EmitCheckedInBoundsGEP(value, amt, signedIndex,
+value = CGF.EmitCheckedInBoundsGEP(value, amt, /*SignedIndices=*/false,
+ /*IsSubtraction=*/!isInc,
E->getExprLoc(), "incdec.funcptr");
value = Builder.CreateBitCast(value, input->getType());
@@ -1962,7 +1963,8 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, amt, "incdec.ptr");
else
-value = CGF.EmitCheckedInBoundsGEP(value, amt, signedIndex,
+value = CGF.EmitCheckedInBoundsGEP(value, amt, /*SignedIndices=*/false,
+ /*IsSubtraction=*/!isInc,
E->getExprLoc(), "incdec.ptr");
}
@@ -2044,8 +2046,9 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, sizeValue, "incdec.objptr");
[PATCH] D34121: [ubsan] Teach the pointer overflow check that "p - <= p" (PR33430)
vsk updated this revision to Diff 102261.
vsk marked an inline comment as done.
vsk edited the summary of this revision.
vsk added a comment.
Thanks for the review! I'll wait for another 'lgtm'.
https://reviews.llvm.org/D34121
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-pointer-overflow.m
Index: test/CodeGen/ubsan-pointer-overflow.m
===
--- test/CodeGen/ubsan-pointer-overflow.m
+++ test/CodeGen/ubsan-pointer-overflow.m
@@ -10,16 +10,20 @@
++p;
// CHECK: ptrtoint i8* {{.*}} to i64, !nosanitize
- // CHECK-NEXT: add i64 {{.*}}, -1, !nosanitize
- // CHECK: select i1 false{{.*}}, !nosanitize
+ // CHECK-NEXT: [[COMPGEP:%.*]] = add i64 {{.*}}, -1, !nosanitize
+ // CHECK: [[NEGVALID:%.*]] = icmp ule i64 [[COMPGEP]], {{.*}}, !nosanitize
+ // CHECK-NOT: select
+ // CHECK: br i1 [[NEGVALID]]{{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
--p;
+ // CHECK: icmp uge i64
// CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p++;
- // CHECK: select
+ // CHECK: icmp ule i64
+ // CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p--;
}
@@ -64,7 +68,8 @@
// CHECK: [[OFFSET:%.*]] = sub i64 0, {{.*}}
// CHECK-NEXT: getelementptr inbounds {{.*}} [[OFFSET]]
- // CHECK: select
+ // CHECK: icmp ule i64
+ // CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p - i;
}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -3555,9 +3555,12 @@
/// Same as IRBuilder::CreateInBoundsGEP, but additionally emits a check to
/// detect undefined behavior when the pointer overflow sanitizer is enabled.
/// \p SignedIndices indicates whether any of the GEP indices are signed.
+ /// \p IsSubtraction indicates whether the expression used to form the GEP
+ /// is a subtraction.
llvm::Value *EmitCheckedInBoundsGEP(llvm::Value *Ptr,
ArrayRef IdxList,
bool SignedIndices,
+ bool IsSubtraction,
SourceLocation Loc,
const Twine &Name = "");
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -1851,7 +1851,6 @@
llvm::Value *input;
int amount = (isInc ? 1 : -1);
- bool signedIndex = !isInc;
if (const AtomicType *atomicTy = type->getAs()) {
type = atomicTy->getValueType();
@@ -1941,8 +1940,9 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, numElts, "vla.inc");
else
-value = CGF.EmitCheckedInBoundsGEP(value, numElts, signedIndex,
- E->getExprLoc(), "vla.inc");
+value = CGF.EmitCheckedInBoundsGEP(
+value, numElts, /*SignedIndices=*/false, /*IsSubtraction=*/!isInc,
+E->getExprLoc(), "vla.inc");
// Arithmetic on function pointers (!) is just +-1.
} else if (type->isFunctionType()) {
@@ -1952,7 +1952,8 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, amt, "incdec.funcptr");
else
-value = CGF.EmitCheckedInBoundsGEP(value, amt, signedIndex,
+value = CGF.EmitCheckedInBoundsGEP(value, amt, /*SignedIndices=*/false,
+ /*IsSubtraction=*/!isInc,
E->getExprLoc(), "incdec.funcptr");
value = Builder.CreateBitCast(value, input->getType());
@@ -1962,7 +1963,8 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, amt, "incdec.ptr");
else
-value = CGF.EmitCheckedInBoundsGEP(value, amt, signedIndex,
+value = CGF.EmitCheckedInBoundsGEP(value, amt, /*SignedIndices=*/false,
+ /*IsSubtraction=*/!isInc,
E->getExprLoc(), "incdec.ptr");
}
@@ -2044,8 +2046,9 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, sizeValue, "incdec.objptr");
else
- value = CGF.EmitCheckedInBoundsGEP(value, sizeValue, signedIndex,
- E->getExprLoc(), "incdec.objptr");
+ value = CGF.EmitCheckedInBoundsGEP(
+ value, sizeValue, /*SignedIndices=*/false, /*IsSubtraction=*/!isInc,
+ E->getExprLoc(), "incdec.objptr");
value = Builder.CreateBitCast(value, input->getType());
}
@@ -2663,7 +2666,6 @@
}
bool isSigned = indexOperand->getType(
[PATCH] D33305: [ubsan] Add a check for pointer overflow UB
vsk added a comment. @sberg I agree with @regehr's analysis, and do think that this is a real overflow. Once https://reviews.llvm.org/D34121 lands, we will report this issue in a better way: runtime error: addition of unsigned offset to 0x7fff59dfe990 overflowed to 0x7fff59dfe980 Repository: rL LLVM https://reviews.llvm.org/D33305 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34121: [ubsan] Teach the pointer overflow check that "p - <= p" (PR33430)
vsk added a comment. In https://reviews.llvm.org/D34121#779347, @dtzWill wrote: > Don't mean to block this, but just FYI I won't be able to look into this > carefully until later this week (sorry!). > > Kicked off a rebuild using these patches just now, though! O:) No problem, thanks for taking a look. https://reviews.llvm.org/D34121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34121: [ubsan] Teach the pointer overflow check that "p - <= p" (PR33430)
vsk updated this revision to Diff 102603.
vsk marked an inline comment as done.
vsk added a comment.
Address Adrian's comment about using an enum to simplify some calls.
https://reviews.llvm.org/D34121
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-pointer-overflow.m
Index: test/CodeGen/ubsan-pointer-overflow.m
===
--- test/CodeGen/ubsan-pointer-overflow.m
+++ test/CodeGen/ubsan-pointer-overflow.m
@@ -10,16 +10,20 @@
++p;
// CHECK: ptrtoint i8* {{.*}} to i64, !nosanitize
- // CHECK-NEXT: add i64 {{.*}}, -1, !nosanitize
- // CHECK: select i1 false{{.*}}, !nosanitize
+ // CHECK-NEXT: [[COMPGEP:%.*]] = add i64 {{.*}}, -1, !nosanitize
+ // CHECK: [[NEGVALID:%.*]] = icmp ule i64 [[COMPGEP]], {{.*}}, !nosanitize
+ // CHECK-NOT: select
+ // CHECK: br i1 [[NEGVALID]]{{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
--p;
+ // CHECK: icmp uge i64
// CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p++;
- // CHECK: select
+ // CHECK: icmp ule i64
+ // CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p--;
}
@@ -64,7 +68,8 @@
// CHECK: [[OFFSET:%.*]] = sub i64 0, {{.*}}
// CHECK-NEXT: getelementptr inbounds {{.*}} [[OFFSET]]
- // CHECK: select
+ // CHECK: icmp ule i64
+ // CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p - i;
}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -3552,12 +3552,19 @@
/// nonnull, if \p LHS is marked _Nonnull.
void EmitNullabilityCheck(LValue LHS, llvm::Value *RHS, SourceLocation Loc);
+ /// An enumeration which makes it easier to specify whether or not an
+ /// operation is a subtraction.
+ enum { NotSubtraction = false, IsSubtraction = true };
+
/// Same as IRBuilder::CreateInBoundsGEP, but additionally emits a check to
/// detect undefined behavior when the pointer overflow sanitizer is enabled.
/// \p SignedIndices indicates whether any of the GEP indices are signed.
+ /// \p IsSubtraction indicates whether the expression used to form the GEP
+ /// is a subtraction.
llvm::Value *EmitCheckedInBoundsGEP(llvm::Value *Ptr,
ArrayRef IdxList,
bool SignedIndices,
+ bool IsSubtraction,
SourceLocation Loc,
const Twine &Name = "");
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -1851,7 +1851,7 @@
llvm::Value *input;
int amount = (isInc ? 1 : -1);
- bool signedIndex = !isInc;
+ bool isSubtraction = !isInc;
if (const AtomicType *atomicTy = type->getAs()) {
type = atomicTy->getValueType();
@@ -1941,8 +1941,9 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, numElts, "vla.inc");
else
-value = CGF.EmitCheckedInBoundsGEP(value, numElts, signedIndex,
- E->getExprLoc(), "vla.inc");
+value = CGF.EmitCheckedInBoundsGEP(
+value, numElts, /*SignedIndices=*/false, isSubtraction,
+E->getExprLoc(), "vla.inc");
// Arithmetic on function pointers (!) is just +-1.
} else if (type->isFunctionType()) {
@@ -1952,18 +1953,20 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, amt, "incdec.funcptr");
else
-value = CGF.EmitCheckedInBoundsGEP(value, amt, signedIndex,
- E->getExprLoc(), "incdec.funcptr");
+value = CGF.EmitCheckedInBoundsGEP(value, amt, /*SignedIndices=*/false,
+ isSubtraction, E->getExprLoc(),
+ "incdec.funcptr");
value = Builder.CreateBitCast(value, input->getType());
// For everything else, we can just do a simple increment.
} else {
llvm::Value *amt = Builder.getInt32(amount);
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, amt, "incdec.ptr");
else
-value = CGF.EmitCheckedInBoundsGEP(value, amt, signedIndex,
- E->getExprLoc(), "incdec.ptr");
+value = CGF.EmitCheckedInBoundsGEP(value, amt, /*SignedIndices=*/false,
+ isSubtraction, E->getExprLoc(),
+ "incdec.ptr");
}
// Vector increment/decrement.
@@ -2044,7 +2047,8 @@
if (CGF.getLan
[PATCH] D34262: [ubsan] PR33081: Skip the standard type checks for volatile
vsk created this revision.
Skip checks for null dereference, alignment violation, object size
violation, and dynamic type violation if the pointer points to volatile
data.
https://bugs.llvm.org/show_bug.cgi?id=33081
https://reviews.llvm.org/D34262
Files:
lib/CodeGen/CGExpr.cpp
test/CodeGen/ubsan-volatile.c
Index: test/CodeGen/ubsan-volatile.c
===
--- /dev/null
+++ test/CodeGen/ubsan-volatile.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10
-fsanitize=null,alignment,object-size,vptr -S -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: @volatile_null_deref
+void volatile_null_deref() {
+ // CHECK: [[P:%.*]] = alloca i32*
+ // CHECK-NEXT: [[V:%.*]] = load i32*, i32** [[P]]
+ // CHECK-NEXT: load volatile i32, i32* [[V]]
+ // CHECK-NEXT: ret void
+ volatile int *p;
+ *p;
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -549,6 +549,11 @@
if (Ptr->getType()->getPointerAddressSpace())
return;
+ // Don't check pointers to volatile data. The behavior here is
implementation-
+ // defined.
+ if (Ty.isVolatileQualified())
+return;
+
SanitizerScope SanScope(this);
SmallVector, 3> Checks;
Index: test/CodeGen/ubsan-volatile.c
===
--- /dev/null
+++ test/CodeGen/ubsan-volatile.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsanitize=null,alignment,object-size,vptr -S -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: @volatile_null_deref
+void volatile_null_deref() {
+ // CHECK: [[P:%.*]] = alloca i32*
+ // CHECK-NEXT: [[V:%.*]] = load i32*, i32** [[P]]
+ // CHECK-NEXT: load volatile i32, i32* [[V]]
+ // CHECK-NEXT: ret void
+ volatile int *p;
+ *p;
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -549,6 +549,11 @@
if (Ptr->getType()->getPointerAddressSpace())
return;
+ // Don't check pointers to volatile data. The behavior here is implementation-
+ // defined.
+ if (Ty.isVolatileQualified())
+return;
+
SanitizerScope SanScope(this);
SmallVector, 3> Checks;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34262: [ubsan] PR33081: Skip the standard type checks for volatile
vsk added a comment. Thanks for the review. I'll make the suggested test changes and commit. https://reviews.llvm.org/D34262 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk created this revision.
This patch makes ubsan's nonnull return value diagnostics more precise,
which makes the diagnostics more useful when there are multiple return
statements in a function. Example:
1 |__attribute__((returns_nonnull)) char *foo() {
2 | if (...) {
3 |return expr_which_might_evaluate_to_null();
4 | } else {
5 |return another_expr_which_might_evaluate_to_null();
6 | }
7 |} // <- The current diagnostic always points here!
runtime error: Null returned from Line 7, Column 2!
With this patch, the diagnostic would point to either Line 3, Column 5
or Line 5, Column 5.
This is done by emitting source location metadata for each return
statement in a sanitized function. The runtime is passed a pointer to
the appropriate metadata so that it can prepare and deduplicate reports.
Compiler-rt patch (with more tests): https://reviews.llvm.org/D34298
https://reviews.llvm.org/D34299
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGStmt.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGenObjC/ubsan-nonnull-and-nullability.m
test/CodeGenObjC/ubsan-nullability.m
Index: test/CodeGenObjC/ubsan-nullability.m
===
--- test/CodeGenObjC/ubsan-nullability.m
+++ test/CodeGenObjC/ubsan-nullability.m
@@ -2,15 +2,15 @@
// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
-// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 109, i32 1 {{.*}} i32 100, i32 6
+// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 100, i32 6
// CHECK: [[NONNULL_ARG_LOC:@.*]] = private unnamed_addr global {{.*}} i32 204, i32 15 {{.*}} i32 190, i32 23
// CHECK: [[NONNULL_ASSIGN1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 305, i32 9
// CHECK: [[NONNULL_ASSIGN2_LOC:@.*]] = private unnamed_addr global {{.*}} i32 405, i32 10
// CHECK: [[NONNULL_ASSIGN3_LOC:@.*]] = private unnamed_addr global {{.*}} i32 506, i32 10
// CHECK: [[NONNULL_INIT1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 604, i32 25
// CHECK: [[NONNULL_INIT2_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 26
// CHECK: [[NONNULL_INIT2_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 29
-// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 817, i32 1 {{.*}} i32 800, i32 6
+// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 800, i32 6
#define NULL ((void *)0)
#define INULL ((int *)NULL)
@@ -22,7 +22,7 @@
// CHECK: br i1 true, label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC1]]
return p;
// CHECK: [[NONULL]]:
@@ -111,7 +111,7 @@
// CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC2]]
return arg1;
// CHECK: [[NONULL]]:
@@ -132,7 +132,7 @@
// CHECK: br i1 [[DO_RV_CHECK]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}
return arg1;
// CHECK: [[NONULL]]:
@@ -146,7 +146,7 @@
// CHECK: br i1 [[DO_RV_CHECK]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}
return arg1;
// CHECK: [[NONULL]]:
Index: test/CodeGenObjC/ubsan-nonnull-and-nullability.m
===
--- test/CodeGenObjC/ubsan-nonnull-and-nullability.m
+++ test/CodeGenObjC/ubsan-nonnull-and-nullability.m
@@ -8,12 +8,16 @@
__attribute__((returns_nonnull)) int *_Nonnull f1(int *_Nonnull p) {
// CHECK: entry:
// CHECK-NEXT: [[ADDR:%.*]] = alloca i32*
+ // CHECK-NEXT: [[SLOC_PTR:%.*]] = alloca i8*
+ // CHECK-NEXT: store i8* null, i8** [[SLOC_PTR]]
// CHECK-NEXT: store i32
[PATCH] D34212: docs: Document binary compatibility issue due to bug in gcc
vsk added a comment. Could you add links to this document in index.rst and UsersManual.rst? Comment at: docs/BinaryCompatibilityWithOtherCompilers.rst:39 + +https://llvm.org/PR33161 This link is still showing up as 'insecure'. Mind using this?: https://bugs.llvm.org/show_bug.cgi?id=33161 https://reviews.llvm.org/D34212 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34301: [Sema] Make sure the definition of a referenced virtual function is emitted when it is final
vsk added a comment. It looks Comment at: lib/Sema/SemaExpr.cpp:14715 +if (Method->isVirtual() && !(Method->hasAttr() || + Method->getParent()->hasAttr())) OdrUse = false; Do you think it makes sense to eliminate all candidate virtual methods which can be devirtualized? If so, you could make "CanDevirtualizeMemberFunctionCall" a shared utility between Sema and CodeGen, and use it here. That function should give "the truth" about whether or not a call can be devirtualized. Comment at: lib/Sema/SemaExpr.cpp:14717 OdrUse = false; MarkExprReferenced(*this, E->getLocation(), E->getDecl(), E, OdrUse); } "MarkExprReferenced" has what looks like an incomplete version of "CanDevirtualizeMemberFunctionCall". Do you think there is an opportunity to share logic there as well? Comment at: test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp:271 +// Derived::operator() is not emitted, there will be a linker error. +(*ptr)(); + } Have you looked into why "ptr->operator()();" compiles? We are either missing a devirtualization opportunity, or we have inconsistent logic for setting MightBeOdrUse for member calls. Either way, I think this patch is the right vehicle to address the issue. https://reviews.llvm.org/D34301 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk added a comment.
In https://reviews.llvm.org/D34299#787795, @arphaman wrote:
> It looks like if we have a function without the `return` (like the sample
> below), we will pass in a `0` as the location pointer. This will prevent a
> report of a runtime error as your compiler-rt change ignores the location
> pointers that are `nil`. Is this a bug or is this the intended behaviour?
>
> int *_Nonnull nonnull_retval1(int *p) {
> }
>
This is the intended behavior (I'll add a test). Users should not see a "null
return value" diagnostic here. There is another check, -fsanitize=return, which
can catch this issue.
@filcab --
> Splitting the attrloc from the useloc might make sense since we would be able
> to emit attrloc just once. But I don't see why we need to store/load those
> pointers in runtime instead of just caching the Constant* in CodeGenFunction.
The source locations aren't constants. The ubsan runtime uses a bit inside of
source location structures as a flag. When an issue is diagnosed at a
particular source location, that bit is atomically set. This is how ubsan
implements issue deduplication.
> I'd also like to have some asserts and explicit resets to nullptr after use
> on the ReturnLocation variable, if possible.
Resetting Address fields in CodeGenFunction doesn't appear to be an established
practice. Could you explain what this would be in aid of?
https://reviews.llvm.org/D34299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34121: [ubsan] Teach the pointer overflow check that "p - <= p" (PR33430)
vsk updated this revision to Diff 103606.
vsk added a comment.
Fix a typo introduced in emitArraySubscriptGEP while refactoring
/*isSubtraction=false*/ to CodeGenFunction::NotSubtraction, and add CHECK lines
which catch the issue.
https://reviews.llvm.org/D34121
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-pointer-overflow.m
Index: test/CodeGen/ubsan-pointer-overflow.m
===
--- test/CodeGen/ubsan-pointer-overflow.m
+++ test/CodeGen/ubsan-pointer-overflow.m
@@ -10,16 +10,20 @@
++p;
// CHECK: ptrtoint i8* {{.*}} to i64, !nosanitize
- // CHECK-NEXT: add i64 {{.*}}, -1, !nosanitize
- // CHECK: select i1 false{{.*}}, !nosanitize
+ // CHECK-NEXT: [[COMPGEP:%.*]] = add i64 {{.*}}, -1, !nosanitize
+ // CHECK: [[NEGVALID:%.*]] = icmp ule i64 [[COMPGEP]], {{.*}}, !nosanitize
+ // CHECK-NOT: select
+ // CHECK: br i1 [[NEGVALID]]{{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
--p;
+ // CHECK: icmp uge i64
// CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p++;
- // CHECK: select
+ // CHECK: icmp ule i64
+ // CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p--;
}
@@ -64,7 +68,8 @@
// CHECK: [[OFFSET:%.*]] = sub i64 0, {{.*}}
// CHECK-NEXT: getelementptr inbounds {{.*}} [[OFFSET]]
- // CHECK: select
+ // CHECK: icmp ule i64
+ // CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
p - i;
}
@@ -121,8 +126,10 @@
// CHECK-LABEL: define void @pointer_array_unsigned_indices
void pointer_array_unsigned_indices(int **arr, unsigned k) {
+ // CHECK: icmp uge
// CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
+ // CHECK: icmp uge
// CHECK-NOT: select
// CHECK: call void @__ubsan_handle_pointer_overflow{{.*}}
arr[k][k];
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -3580,12 +3580,19 @@
/// nonnull, if \p LHS is marked _Nonnull.
void EmitNullabilityCheck(LValue LHS, llvm::Value *RHS, SourceLocation Loc);
+ /// An enumeration which makes it easier to specify whether or not an
+ /// operation is a subtraction.
+ enum { NotSubtraction = false, IsSubtraction = true };
+
/// Same as IRBuilder::CreateInBoundsGEP, but additionally emits a check to
/// detect undefined behavior when the pointer overflow sanitizer is enabled.
/// \p SignedIndices indicates whether any of the GEP indices are signed.
+ /// \p IsSubtraction indicates whether the expression used to form the GEP
+ /// is a subtraction.
llvm::Value *EmitCheckedInBoundsGEP(llvm::Value *Ptr,
ArrayRef IdxList,
bool SignedIndices,
+ bool IsSubtraction,
SourceLocation Loc,
const Twine &Name = "");
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -1851,7 +1851,7 @@
llvm::Value *input;
int amount = (isInc ? 1 : -1);
- bool signedIndex = !isInc;
+ bool isSubtraction = !isInc;
if (const AtomicType *atomicTy = type->getAs()) {
type = atomicTy->getValueType();
@@ -1941,8 +1941,9 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, numElts, "vla.inc");
else
-value = CGF.EmitCheckedInBoundsGEP(value, numElts, signedIndex,
- E->getExprLoc(), "vla.inc");
+value = CGF.EmitCheckedInBoundsGEP(
+value, numElts, /*SignedIndices=*/false, isSubtraction,
+E->getExprLoc(), "vla.inc");
// Arithmetic on function pointers (!) is just +-1.
} else if (type->isFunctionType()) {
@@ -1952,18 +1953,20 @@
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, amt, "incdec.funcptr");
else
-value = CGF.EmitCheckedInBoundsGEP(value, amt, signedIndex,
- E->getExprLoc(), "incdec.funcptr");
+value = CGF.EmitCheckedInBoundsGEP(value, amt, /*SignedIndices=*/false,
+ isSubtraction, E->getExprLoc(),
+ "incdec.funcptr");
value = Builder.CreateBitCast(value, input->getType());
// For everything else, we can just do a simple increment.
} else {
llvm::Value *amt = Builder.getInt32(amount);
if (CGF.getLangOpts().isSignedOverflowDefined())
value = Builder.CreateGEP(value, amt, "incdec.ptr");
else
-
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk added a comment. In https://reviews.llvm.org/D34299#788379, @filcab wrote: > In https://reviews.llvm.org/D34299#788152, @vsk wrote: > > > The source locations aren't constants. The ubsan runtime uses a bit inside > > of source location structures as a flag. When an issue is diagnosed at a > > particular source location, that bit is atomically set. This is how ubsan > > implements issue deduplication. > > > It's still an `llvm::Constant`. Just like in StaticData, in line 2966. > Basically, I don't see why we need to add the store/load and an additional > indirection, since the pointer is constant, and we can just emit the static > data as before. My earlier response made the assumption that you wanted a copy of the source location passed to the runtime handler, by-value. I see now that you're wondering why the source locations aren't part of the static data structure. That's because the location of the return statement isn't known at compile time, i.e it's not static data. The location depends on which return statement is executed. > We're already doing `Data->Loc.acquire();` for the current version (and all > the other checks). The other checks do not allow the source location within a static data object to change. >>> I'd also like to have some asserts and explicit resets to nullptr after use >>> on the ReturnLocation variable, if possible. >> >> Resetting Address fields in CodeGenFunction doesn't appear to be an >> established practice. Could you explain what this would be in aid of? > > It would be a sanity check and help with code reading/keeping in mind the > lifetime of the information. I'm ok with that happening only on `!NDEBUG` > builds. > > Reading the code, I don't know if a `ReturnLocation` might end up being used > for more than one checks. If it's supposed to be a different one per check, > etc. > If it's only supposed to be used once, I'd rather set it to `nullptr` right > after use (at least when `!NDEBUG`), and `assert(!ReturnLocation)` before > setting. It's not a big deal, but would help with making sense of the flow of > information when debugging. Sure, I'll reset it to Address::invalid(). https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk updated this revision to Diff 103632.
vsk marked an inline comment as done.
vsk added a comment.
Handle functions without return statements correctly (fixing an issue pointed
out by @arphaman). Now, the instrumentation always checks that we have a valid
return location before calling the runtime. I added tests for this on the clang
side: we can't test it on the compiler-rt side, because functions without
return statements can cause stack corruption / crashes on Darwin.
https://reviews.llvm.org/D34299
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGStmt.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGenObjC/ubsan-nonnull-and-nullability.m
test/CodeGenObjC/ubsan-nullability.m
Index: test/CodeGenObjC/ubsan-nullability.m
===
--- test/CodeGenObjC/ubsan-nullability.m
+++ test/CodeGenObjC/ubsan-nullability.m
@@ -2,31 +2,28 @@
// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
-// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 109, i32 1 {{.*}} i32 100, i32 6
+// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 100, i32 6
// CHECK: [[NONNULL_ARG_LOC:@.*]] = private unnamed_addr global {{.*}} i32 204, i32 15 {{.*}} i32 190, i32 23
// CHECK: [[NONNULL_ASSIGN1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 305, i32 9
// CHECK: [[NONNULL_ASSIGN2_LOC:@.*]] = private unnamed_addr global {{.*}} i32 405, i32 10
// CHECK: [[NONNULL_ASSIGN3_LOC:@.*]] = private unnamed_addr global {{.*}} i32 506, i32 10
// CHECK: [[NONNULL_INIT1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 604, i32 25
// CHECK: [[NONNULL_INIT2_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 26
// CHECK: [[NONNULL_INIT2_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 29
-// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 817, i32 1 {{.*}} i32 800, i32 6
+// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 800, i32 6
#define NULL ((void *)0)
#define INULL ((int *)NULL)
#define INNULL ((int *_Nonnull)NULL)
// CHECK-LABEL: define i32* @{{.*}}nonnull_retval1
#line 100
int *_Nonnull nonnull_retval1(int *p) {
- // CHECK: br i1 true, label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
- // CHECK: [[NULL]]:
// CHECK: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC1]]
return p;
- // CHECK: [[NONULL]]:
- // CHECK-NEXT: ret i32*
+ // CHECK: ret i32*
}
#line 190
@@ -108,10 +105,13 @@
// CHECK-NEXT: [[DO_RV_CHECK_1:%.*]] = and i1 true, [[ARG1CMP]], !nosanitize
// CHECK: [[ARG2CMP:%.*]] = icmp ne i32* %arg2, null, !nosanitize
// CHECK-NEXT: [[DO_RV_CHECK_2:%.*]] = and i1 [[DO_RV_CHECK_1]], [[ARG2CMP]]
- // CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
+ // CHECK: [[SLOC_PTR:%.*]] = load i8*, i8** %return.sloc.ptr
+ // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC_PTR]], null
+ // CHECK-NEXT: [[DO_RV_CHECK_3:%.*]] = and i1 [[SLOC_NONNULL]], [[DO_RV_CHECK_2]]
+ // CHECK: br i1 [[DO_RV_CHECK_3]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC2]]
return arg1;
// CHECK: [[NONULL]]:
@@ -129,10 +129,13 @@
+(int *_Nonnull) objc_clsmethod: (int *_Nonnull) arg1 {
// CHECK: [[ARG1CMP:%.*]] = icmp ne i32* %arg1, null, !nosanitize
// CHECK-NEXT: [[DO_RV_CHECK:%.*]] = and i1 true, [[ARG1CMP]]
- // CHECK: br i1 [[DO_RV_CHECK]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
+ // CHECK: [[SLOC_PTR:%.*]] = load i8*, i8** %return.sloc.ptr
+ // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC_PTR]], null
+ // CHECK-NEXT: [[DO_RV_CHECK_2:%.*]] = and i1 [[SLOC_NONNULL]], [[DO_RV_CHECK]]
+ // CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}
return arg1;
// CHECK: [[NONULL]]:
@@ -143,10 +146,13 @@
-(int *_Nonnull) objc_method: (int *_Nonnull) arg1 {
// CHECK: [[ARG1CMP:%.*]
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk added inline comments. Comment at: lib/CodeGen/CGStmt.cpp:1035 +assert(ReturnLocation.isValid() && "No valid return location"); +Builder.CreateStore(Builder.CreateBitCast(SLocPtr, Int8PtrTy), +ReturnLocation); filcab wrote: > Can't you just keep the `Constant*` around and use it later for the static > data? Instead of creating a global var and have runtime store/load? I hope I've cleared this up, but: we need to store the source location constant _somewhere_, before we emit the return value check. That's because we can't infer which return location to use at compile time. Comment at: lib/CodeGen/CodeGenFunction.h:1412 + /// source location for diagnostics. + Address ReturnLocation = Address::invalid(); + filcab wrote: > Maybe `CurrentReturnLocation`? I'd prefer to keep it the way it is, for consistency with the "ReturnValue" field. https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk added a comment. In https://reviews.llvm.org/D34299#788908, @arphaman wrote: > Ok, so now the null check `return.sloc.load` won't call the checker in > compiler-rt and so the program won't `abort` and won't hit the `unreachable`. > I have one question tough: > > This patch changes the behavior of this sanitizer for the example that I gave > above. Yes, in the case where there is no explicit return, and the return value happens to be null. > Previously a runtime diagnostic was emitted, but now there is none. While I'm > not saying that the previous behaviour was correct, I'm wondering if the new > behaviour is right. I think that for C++ it makes sense, but I don't know > the right answer for C. I'm leaning more towards the new behaviour, since > technically in C falling off without returning a value is not UB unless that > return value is used by the caller. But at the same time, since we don't > diagnose `return` UB for C, maybe it's still worth diagnosing this particular > issue? The user might not catch it otherwise at all (or they might catch it > later when they try to access it, but by that point they might not know where > the pointer came from). WDYT? Without seeing what the caller does with the result, the return value check can't do a good job of diagnosing missing returns. I think clang should emit a diagnostic in the example above, and -Wreturn-type does. So I think the new behavior is appropriate. https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34563: [ubsan] Disable the object-size check at -O0
vsk created this revision.
This is motivated by the thread:
[cfe-dev] Disabling ubsan's object size check at -O0
I think the driver is the best place to disable a sanitizer check at particular
optimization levels. Doing so in the frontend is messy, and makes it really
hard to test IR generation for the check. Making the change in CodeGen has the
same issues.
https://reviews.llvm.org/D34563
Files:
lib/Driver/SanitizerArgs.cpp
test/Driver/fsanitize-object-size.c
test/Driver/fsanitize.c
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -3,27 +3,27 @@
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang -target x86_64-linux-gnu -fsanitize-undefined-trap-on-error -fsanitize=undefined-trap %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
-// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute|function),?){19}"}}
-// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
-// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute|function),?){18}"}}
+// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED
-// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){20}"}}
+// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){19}"}}
// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN
-// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){18}"}}
+// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
// RUN: %clang -target i386-unknown-openbsd -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-OPENBSD
-// CHECK-UNDEFINED-OPENBSD: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){18}"}}
+// CHECK-UNDEFINED-OPENBSD: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-bas
[PATCH] D34563: [ubsan] Disable the object-size check at -O0
vsk updated this revision to Diff 103778.
vsk added a comment.
Add a diagnostic for users who explicitly turn the object-size check on at -O0,
and tighten up the test a bit.
https://reviews.llvm.org/D34563
Files:
include/clang/Basic/DiagnosticDriverKinds.td
lib/Driver/SanitizerArgs.cpp
test/Driver/fsanitize-object-size.c
test/Driver/fsanitize.c
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -3,27 +3,27 @@
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang -target x86_64-linux-gnu -fsanitize-undefined-trap-on-error -fsanitize=undefined-trap %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
-// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute|function),?){19}"}}
-// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
-// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute|function),?){18}"}}
+// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED
-// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){20}"}}
+// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){19}"}}
// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN
-// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){18}"}}
+// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
// RUN: %clang -target i386-unknown-openbsd -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-OPENBSD
-// CHECK-UNDEFINED-OPENBSD: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){18}"}}
+// CHECK-UNDEFINED-OPENBSD: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attrib
[PATCH] D34590: [ubsan] Diagnose invalid uses of builtins (clang)
vsk created this revision.
On some targets, passing zero to the clz() or ctz() builtins has
undefined behavior. I ran into this issue while debugging UB in
__hash_table from libcxx: the bug I was seeing manifested itself
differently under -O0 vs -Os, due to a UB call to clz() (see:
libcxx/r304617).
This patch introduces a check which can detect UB calls to builtins.
llvm.org/PR26979
https://reviews.llvm.org/D34590
Files:
docs/UndefinedBehaviorSanitizer.rst
include/clang/Basic/Sanitizers.def
lib/CodeGen/CGBuiltin.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-builtin-checks.c
test/Driver/fsanitize.c
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -3,27 +3,27 @@
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang -target x86_64-linux-gnu -fsanitize-undefined-trap-on-error -fsanitize=undefined-trap %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
-// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute|function),?){18}"}}
-// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
-// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function),?){19}"}}
+// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED
-// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){19}"}}
+// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){20}"}}
// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN
-// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
+// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){18}"}}
// RUN: %clang -target i386-unknown-openbsd -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-OPENBSD
-// CHECK-UNDEFINED-OPENBSD: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
+// CHECK-UNDEFINED-OPENBSD
[PATCH] D34591: [ubsan] Diagnose invalid uses of builtins (compiler-rt)
vsk created this revision.
Herald added subscribers: dberris, kubamracek.
Compiler-rt changes and tests to go along with: https://reviews.llvm.org/D34590
https://reviews.llvm.org/D34591
Files:
lib/ubsan/ubsan_checks.inc
lib/ubsan/ubsan_handlers.cc
lib/ubsan/ubsan_handlers.h
lib/ubsan/ubsan_interface.inc
test/ubsan/TestCases/Misc/builtins.cpp
test/ubsan/lit.common.cfg
Index: test/ubsan/lit.common.cfg
===
--- test/ubsan/lit.common.cfg
+++ test/ubsan/lit.common.cfg
@@ -77,3 +77,5 @@
# because the test hangs or fails on one configuration and not the other.
if config.target_arch.startswith('arm') == False and config.target_arch != 'aarch64':
config.available_features.add('stable-runtime')
+
+config.available_features.add('arch=' + config.target_arch)
Index: test/ubsan/TestCases/Misc/builtins.cpp
===
--- /dev/null
+++ test/ubsan/TestCases/Misc/builtins.cpp
@@ -0,0 +1,35 @@
+// REQUIRES: arch=x86_64
+//
+// RUN: %clangxx -fsanitize=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
+// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
+// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
+
+void check_ctz(int n) {
+ // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
+ __builtin_ctz(n);
+
+ // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
+ __builtin_ctzl(n);
+
+ // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
+ __builtin_ctzll(n);
+}
+
+void check_clz(int n) {
+ // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
+ __builtin_clz(n);
+
+ // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
+ __builtin_clzl(n);
+
+ // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
+ __builtin_clzll(n);
+}
+
+int main() {
+ check_ctz(0);
+ check_clz(0);
+ return 0;
+}
Index: lib/ubsan/ubsan_interface.inc
===
--- lib/ubsan/ubsan_interface.inc
+++ lib/ubsan/ubsan_interface.inc
@@ -19,6 +19,8 @@
INTERFACE_FUNCTION(__ubsan_handle_float_cast_overflow_abort)
INTERFACE_FUNCTION(__ubsan_handle_function_type_mismatch)
INTERFACE_FUNCTION(__ubsan_handle_function_type_mismatch_abort)
+INTERFACE_FUNCTION(__ubsan_handle_invalid_builtin)
+INTERFACE_FUNCTION(__ubsan_handle_invalid_builtin_abort)
INTERFACE_FUNCTION(__ubsan_handle_load_invalid_value)
INTERFACE_FUNCTION(__ubsan_handle_load_invalid_value_abort)
INTERFACE_FUNCTION(__ubsan_handle_missing_return)
Index: lib/ubsan/ubsan_handlers.h
===
--- lib/ubsan/ubsan_handlers.h
+++ lib/ubsan/ubsan_handlers.h
@@ -122,6 +122,21 @@
/// \brief Handle a load of an invalid value for the type.
RECOVERABLE(load_invalid_value, InvalidValueData *Data, ValueHandle Val)
+/// Known builtin check kinds.
+/// Keep in sync with the enum of the same name in CodeGenFunction.h
+enum BuiltinCheckKind : unsigned char {
+ BCK_CTZPassedZero,
+ BCK_CLZPassedZero,
+};
+
+struct InvalidBuiltinData {
+ SourceLocation Loc;
+ unsigned char Kind;
+};
+
+/// Handle a builtin called in an invalid way.
+RECOVERABLE(invalid_builtin, InvalidBuiltinData *Data)
+
struct FunctionTypeMismatchData {
SourceLocation Loc;
const TypeDescriptor &Type;
Index: lib/ubsan/ubsan_handlers.cc
===
--- lib/ubsan/ubsan_handlers.cc
+++ lib/ubsan/ubsan_handlers.cc
@@ -437,6 +437,30 @@
Die();
}
+static void handleInvalidBuiltin(InvalidBuiltinData *Data, ReportOptions Opts) {
+ SourceLocation Loc = Data->Loc.acquire();
+ ErrorType ET = ErrorType::InvalidBuiltin;
+
+ if (ignoreReport(Loc, Opts, ET))
+return;
+
+ ScopedReport R(Opts, Loc, ET);
+
+ Diag(Loc, DL_Error,
+ "passing zero to %0, which is not a valid argument")
+<< ((Data->Kind == BCK_CTZPassedZero) ? "ctz()" : "clz()");
+}
+
+void __ubsan::__ubsan_handle_invalid_builtin(InvalidBuiltinData *Data) {
+ GET_REPORT_OPTIONS(true);
+ handleInvalidBuiltin(Data, Opts);
+}
+void __ubsan::__ubsan_handle_invalid_builtin_abort(InvalidBuiltinData *Data) {
+ GET_REPORT_OPTIONS(true);
+ handleInvalidBuiltin(Data, Opts);
+ Die();
+}
+
static void handleFunctionTypeMismatch(FunctionTypeMismatchData *Data,
ValueHandle Function,
ReportOptions Opts) {
Index: lib/ubsan/ubsan_checks.inc
=
[PATCH] D41149: [CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920)
vsk marked an inline comment as done. vsk added a comment. Thanks for the review! Comment at: lib/CodeGen/CGBuiltin.cpp:912 + auto IntMax = + llvm::APInt::getMaxValue(ResultInfo.Width).zextOrSelf(Op1Info.Width); + llvm::Value *TruncOverflow = CGF.Builder.CreateICmpUGT( efriedma wrote: > zext() rather than zextOrSelf(). Will fix before committing. https://reviews.llvm.org/D41149 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40720: No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17
vsk added a comment. Please add a test. Repository: rC Clang https://reviews.llvm.org/D40720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40720: No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17
vsk added a comment. Copying my comment from the diffusion thread to keep things in one place: > You could make FunctionTypeMismatch an 'AlwaysRecoverable' check, just like > the Vptr check, and remove the _abort handlers from the ubsan runtimes. Repository: rC Clang https://reviews.llvm.org/D40720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40720: No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17
vsk added a comment. In https://reviews.llvm.org/D40720#958697, @sberg wrote: > In https://reviews.llvm.org/D40720#958677, @vsk wrote: > > > Please add a test. > > > Note that the bot upon the first closing of this review changed the shown > diff from the combined cfe+compiler-rt diff to just the cfe part. See > https://reviews.llvm.org/rL320977 for the compiler-rt part, including tests > in compiler-rt/trunk/test/ubsan/TestCases/TypeCheck/Function/function.cpp. Ah sorry, I'd missed that. Still, it's always nice to have a test at the IR-gen level as well as the runtime test, since those can be a bit more stringent. > Would it be possible to fix this by stripping the noexcept specifiers from > both the function type used in the check and the one that is embedded in the > prefix data? The downside is that we won't catch the case where the caller > has a noexcept specifier and the callee doesn't, but that seems like an edge > case to me, and we can think about fixing it in other ways later. This sounds fine to me, and it avoids breaking the trapping mode. Repository: rC Clang https://reviews.llvm.org/D40720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40698: [ubsan] Diagnose noreturn functions which return
vsk updated this revision to Diff 127424.
vsk added a comment.
- Make the patch cleaner by introducing an overload of EmitCall() which doesn't
require a SourceLocation argument.
https://reviews.llvm.org/D40698
Files:
docs/UndefinedBehaviorSanitizer.rst
lib/CodeGen/CGBuiltin.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-noreturn.c
test/CodeGenCXX/ubsan-unreachable.cpp
Index: test/CodeGenCXX/ubsan-unreachable.cpp
===
--- /dev/null
+++ test/CodeGenCXX/ubsan-unreachable.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=unreachable | FileCheck %s
+
+extern void __attribute__((noreturn)) abort();
+
+// CHECK-LABEL: define void @_Z14calls_noreturnv
+void calls_noreturn() {
+ abort();
+
+ // Check that there are no attributes on the call site.
+ // CHECK-NOT: call void @_Z5abortv{{.*}}#
+
+ // CHECK: __ubsan_handle_builtin_unreachable
+ // CHECK: unreachable
+}
+
+struct A {
+ // Test regular members.
+ void __attribute__((noreturn)) does_not_return1() {
+// CHECK-NOT: call void @_Z5abortv(){{.*}}#
+abort();
+ }
+
+ // CHECK: declare void @_Z5abortv{{.*}} [[ABORT_ATTR:#[0-9]+]]
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A5call1Ev
+ void call1() {
+// CHECK-NOT: call void @_ZN1A16does_not_return1Ev{{.*}}#
+does_not_return1();
+
+// CHECK: __ubsan_handle_builtin_unreachable
+// CHECK: unreachable
+ }
+
+ // Test static members.
+ static void __attribute__((noreturn)) does_not_return2() {
+// CHECK-NOT: call void @_Z5abortv{{.*}}#
+abort();
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A5call2Ev
+ void call2() {
+// CHECK-NOT: call void @_ZN1A16does_not_return2Ev{{.*}}#
+does_not_return2();
+
+// CHECK: __ubsan_handle_builtin_unreachable
+// CHECK: unreachable
+ }
+
+ // Test calls through pointers to non-static member functions.
+ typedef void __attribute__((noreturn)) (A::*MemFn)();
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A5call3Ev
+ void call3() {
+MemFn MF = &A::does_not_return1;
+(this->*MF)();
+
+// CHECK-NOT: call void %{{.*}}#
+// CHECK: __ubsan_handle_builtin_unreachable
+// CHECK: unreachable
+ }
+};
+
+// CHECK: define linkonce_odr void @_ZN1A16does_not_return1Ev{{.*}} [[DOES_NOT_RETURN_ATTR:#[0-9]+]]
+// CHECK: define linkonce_odr void @_ZN1A16does_not_return2Ev{{.*}} [[DOES_NOT_RETURN_ATTR]]
+
+void force_irgen() {
+ A a;
+ a.call1();
+ a.call2();
+ a.call3();
+}
+
+// CHECK-NOT: [[ABORT_ATTR]] = {{[^}]+}}noreturn
+// CHECK-NOT: [[DOES_NOT_RETURN_ATTR]] = {{[^}]+}}noreturn
Index: test/CodeGen/ubsan-noreturn.c
===
--- /dev/null
+++ test/CodeGen/ubsan-noreturn.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -emit-llvm -fsanitize=unreachable -o - | FileCheck %s
+
+// CHECK-LABEL: @f(
+void __attribute__((noreturn)) f() {
+ // CHECK: __ubsan_handle_builtin_unreachable
+ // CHECK: unreachable
+}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -3288,11 +3288,15 @@
/// LLVM arguments and the types they were derived from.
RValue EmitCall(const CGFunctionInfo &CallInfo, const CGCallee &Callee,
ReturnValueSlot ReturnValue, const CallArgList &Args,
- llvm::Instruction **callOrInvoke = nullptr);
-
+ llvm::Instruction **callOrInvoke, SourceLocation Loc);
+ RValue EmitCall(const CGFunctionInfo &CallInfo, const CGCallee &Callee,
+ ReturnValueSlot ReturnValue, const CallArgList &Args,
+ llvm::Instruction **callOrInvoke = nullptr) {
+return EmitCall(CallInfo, Callee, ReturnValue, Args, callOrInvoke,
+SourceLocation());
+ }
RValue EmitCall(QualType FnType, const CGCallee &Callee, const CallExpr *E,
- ReturnValueSlot ReturnValue,
- llvm::Value *Chain = nullptr);
+ ReturnValueSlot ReturnValue, llvm::Value *Chain = nullptr);
RValue EmitCallExpr(const CallExpr *E,
ReturnValueSlot ReturnValue = ReturnValueSlot());
RValue EmitSimpleCallExpr(const CallExpr *E, ReturnValueSlot ReturnValue);
@@ -3747,6 +3751,10 @@
llvm::ConstantInt *TypeId, llvm::Value *Ptr,
ArrayRef StaticArgs);
+ /// Emit a reached-unreachable diagnostic if \p Loc is valid and runtime
+ /// checking is enabled. Otherwise, just emit an unreachable instruction.
+ void EmitUnreachable(SourceLocation Loc);
+
/// \brief Create a basic block that will call the trap intrinsic, and emit a
/// conditional branch to it, for the -ftrapv checks.
void EmitTrapCheck(llvm::Value *Checked);
Index
[PATCH] D40698: [ubsan] Diagnose noreturn functions which return
vsk updated this revision to Diff 127426.
vsk added a comment.
- Tighten the IR test case.
https://reviews.llvm.org/D40698
Files:
docs/UndefinedBehaviorSanitizer.rst
lib/CodeGen/CGBuiltin.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/ubsan-noreturn.c
test/CodeGenCXX/ubsan-unreachable.cpp
Index: test/CodeGenCXX/ubsan-unreachable.cpp
===
--- /dev/null
+++ test/CodeGenCXX/ubsan-unreachable.cpp
@@ -0,0 +1,81 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=unreachable | FileCheck %s
+
+extern void __attribute__((noreturn)) abort();
+
+// CHECK-LABEL: define void @_Z14calls_noreturnv
+void calls_noreturn() {
+ abort();
+
+ // Check that there are no attributes on the call site.
+ // CHECK-NOT: call void @_Z5abortv{{.*}}#
+
+ // CHECK: __ubsan_handle_builtin_unreachable
+ // CHECK: unreachable
+}
+
+struct A {
+ // CHECK: declare void @_Z5abortv{{.*}} [[ABORT_ATTR:#[0-9]+]]
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A5call1Ev
+ void call1() {
+// CHECK-NOT: call void @_ZN1A16does_not_return2Ev{{.*}}#
+does_not_return2();
+
+// CHECK: __ubsan_handle_builtin_unreachable
+// CHECK: unreachable
+ }
+
+ // Test static members.
+ static void __attribute__((noreturn)) does_not_return1() {
+// CHECK-NOT: call void @_Z5abortv{{.*}}#
+abort();
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A5call2Ev
+ void call2() {
+// CHECK-NOT: call void @_ZN1A16does_not_return1Ev{{.*}}#
+does_not_return1();
+
+// CHECK: __ubsan_handle_builtin_unreachable
+// CHECK: unreachable
+ }
+
+ // Test calls through pointers to non-static member functions.
+ typedef void __attribute__((noreturn)) (A::*MemFn)();
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A5call3Ev
+ void call3() {
+MemFn MF = &A::does_not_return2;
+(this->*MF)();
+
+// CHECK-NOT: call void %{{.*}}#
+// CHECK: __ubsan_handle_builtin_unreachable
+// CHECK: unreachable
+ }
+
+ // Test regular members.
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return2Ev({{.*}})
+ // CHECK-SAME: [[DOES_NOT_RETURN_ATTR:#[0-9]+]]
+ void __attribute__((noreturn)) does_not_return2() {
+// CHECK-NOT: call void @_Z5abortv(){{.*}}#
+abort();
+
+// CHECK: call void @__ubsan_handle_builtin_unreachable
+// CHECK: unreachable
+
+// CHECK: call void @__ubsan_handle_builtin_unreachable
+// CHECK: unreachable
+ }
+};
+
+// CHECK: define linkonce_odr void @_ZN1A16does_not_return1Ev() [[DOES_NOT_RETURN_ATTR]]
+
+void force_irgen() {
+ A a;
+ a.call1();
+ a.call2();
+ a.call3();
+}
+
+// CHECK-NOT: [[ABORT_ATTR]] = {{[^}]+}}noreturn
+// CHECK-NOT: [[DOES_NOT_RETURN_ATTR]] = {{[^}]+}}noreturn
Index: test/CodeGen/ubsan-noreturn.c
===
--- /dev/null
+++ test/CodeGen/ubsan-noreturn.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -emit-llvm -fsanitize=unreachable -o - | FileCheck %s
+
+// CHECK-LABEL: @f(
+void __attribute__((noreturn)) f() {
+ // CHECK: __ubsan_handle_builtin_unreachable
+ // CHECK: unreachable
+}
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -3288,11 +3288,15 @@
/// LLVM arguments and the types they were derived from.
RValue EmitCall(const CGFunctionInfo &CallInfo, const CGCallee &Callee,
ReturnValueSlot ReturnValue, const CallArgList &Args,
- llvm::Instruction **callOrInvoke = nullptr);
-
+ llvm::Instruction **callOrInvoke, SourceLocation Loc);
+ RValue EmitCall(const CGFunctionInfo &CallInfo, const CGCallee &Callee,
+ ReturnValueSlot ReturnValue, const CallArgList &Args,
+ llvm::Instruction **callOrInvoke = nullptr) {
+return EmitCall(CallInfo, Callee, ReturnValue, Args, callOrInvoke,
+SourceLocation());
+ }
RValue EmitCall(QualType FnType, const CGCallee &Callee, const CallExpr *E,
- ReturnValueSlot ReturnValue,
- llvm::Value *Chain = nullptr);
+ ReturnValueSlot ReturnValue, llvm::Value *Chain = nullptr);
RValue EmitCallExpr(const CallExpr *E,
ReturnValueSlot ReturnValue = ReturnValueSlot());
RValue EmitSimpleCallExpr(const CallExpr *E, ReturnValueSlot ReturnValue);
@@ -3747,6 +3751,10 @@
llvm::ConstantInt *TypeId, llvm::Value *Ptr,
ArrayRef StaticArgs);
+ /// Emit a reached-unreachable diagnostic if \p Loc is valid and runtime
+ /// checking is enabled. Otherwise, just emit an unreachable instruction.
+ void EmitUnreachable(SourceLocation Loc);
+
/// \brief Create a basic block that will call the trap intrinsic, an
[PATCH] D41374: [Coverage] Fix use-after free in coverage emission
vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, this lgtm as a stopgap. As I mentioned offline, I think the goal should be to have non-modules and modules-enabled builds of a project produce identical coverage reports. I'll look into what exactly we're adding to the deferred decls map while iterating over it, and see if we're dropping any coverage mappings. Repository: rC Clang https://reviews.llvm.org/D41374 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40295: -fsanitize=vptr warnings on bad static types in dynamic_cast and typeid
vsk added a comment. I don't think any checks can be skipped in the newly-introduced calls to EmitTypeCheck. Clang uses EmitDynamicCast on arbitrary addresses, not just addresses which are known to be checked for alignment/etc. Regarding the test update, I think it makes sense to extend the runtime test in vptr.cpp, but that we'd also benefit from a small/narrow IR test (e.g in test/CodeGenCXX/ubsan-vtable-checks.cpp). With the added test I think this patch would be in great shape. https://reviews.llvm.org/D40295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40295: -fsanitize=vptr warnings on bad static types in dynamic_cast and typeid
vsk added reviewers: vsk, thakis. vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Thanks, lgtm. Could you wait a day or two before committing? IIRC Richard or Nico have a -fsanitize=vptr Chromium bot, and they may have further comments. https://reviews.llvm.org/D40295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41044: Implementation of -fextend-lifetimes and -fextend-this-ptr to aid with debugging of optimized code
vsk added a comment.
I have some results from the development build of our kernel ('-O2 -g -flto').
According to dwarfdump -statistics, when compiled with -fextend-lifetimes, the
percentage of covered scope bytes increases from 62% to 69%. The number of
inlined scopes decreases by 4%, and (I think relatedly) the size of the binary
increases by 14%. There is a small increase in the number of unique source
variables (under 1%). I'd be happy to report back on any other suggested
quantitative measures.
My qualitative feedback based on spot-checking a few frames is that
-fextend-lifetimes noticeably improves the overall debugging experience. More
argument values and local variables tend to be available. I'm not sure how best
to put a number to this.
https://reviews.llvm.org/D41044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41717: [CGBuiltin] Handle unsigned mul overflow properly (PR35750)
vsk created this revision.
vsk added reviewers: efriedma, arphaman.
r320902 fixed the IRGen for some types of checked multiplications. It
did not handle unsigned overflow correctly in the case where the signed
operand is negative (PR35750).
Eli pointed out that on overflow, the result must be equal to the unique
value that is equivalent to the mathematically-correct result modulo two
raised to the k power, where k is the number of bits in the result type.
This patch fixes the specialized IRGen from r320902 accordingly.
Testing: Apart from check-clang, I modified the test harness from
r320902 to validate the results of all multiplications -- not just the
ones which don't overflow:
https://gist.github.com/vedantk/3eb9c88f82e5c32f2e590555b4af5081
llvm.org/PR35750, rdar://34963321
https://reviews.llvm.org/D41717
Files:
lib/CodeGen/CGBuiltin.cpp
test/CodeGen/builtins-overflow.c
Index: test/CodeGen/builtins-overflow.c
===
--- test/CodeGen/builtins-overflow.c
+++ test/CodeGen/builtins-overflow.c
@@ -373,7 +373,9 @@
// CHECK-NEXT: [[NotNull:%.*]] = icmp ne i32 [[UnsignedResult]], 0
// CHECK-NEXT: [[Underflow:%.*]] = and i1 [[IsNeg]], [[NotNull]]
// CHECK-NEXT: [[OFlow:%.*]] = or i1 [[UnsignedOFlow]], [[Underflow]]
-// CHECK-NEXT: store i32 [[UnsignedResult]], i32* %{{.*}}, align 4
+// CHECK-NEXT: [[NegatedResult:%.*]] = sub i32 0, [[UnsignedResult]]
+// CHECK-NEXT: [[Result:%.*]] = select i1 [[IsNeg]], i32 [[NegatedResult]],
i32 [[UnsignedResult]]
+// CHECK-NEXT: store i32 [[Result]], i32* %{{.*}}, align 4
// CHECK: br i1 [[OFlow]]
unsigned result;
@@ -432,7 +434,9 @@
// CHECK-NEXT: [[OVERFLOW_PRE_TRUNC:%.*]] = or i1 {{.*}}, [[UNDERFLOW]]
// CHECK-NEXT: [[TRUNC_OVERFLOW:%.*]] = icmp ugt i64 [[UNSIGNED_RESULT]],
4294967295
// CHECK-NEXT: [[OVERFLOW:%.*]] = or i1 [[OVERFLOW_PRE_TRUNC]],
[[TRUNC_OVERFLOW]]
-// CHECK-NEXT: trunc i64 [[UNSIGNED_RESULT]] to i32
+// CHECK-NEXT: [[NEGATED:%.*]] = sub i64 0, [[UNSIGNED_RESULT]]
+// CHECK-NEXT: [[RESULT:%.*]] = select i1 {{.*}}, i64 [[NEGATED]], i64
[[UNSIGNED_RESULT]]
+// CHECK-NEXT: trunc i64 [[RESULT]] to i32
// CHECK-NEXT: store
unsigned result;
if (__builtin_mul_overflow(y, x, &result))
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -915,7 +915,11 @@
Overflow = CGF.Builder.CreateOr(Overflow, TruncOverflow);
}
-Result = CGF.Builder.CreateTrunc(UnsignedResult, ResTy);
+// Negate the product if it would be negative in infinite precision.
+Result = CGF.Builder.CreateSelect(
+IsNegative, CGF.Builder.CreateNeg(UnsignedResult), UnsignedResult);
+
+Result = CGF.Builder.CreateTrunc(Result, ResTy);
}
assert(Overflow && Result && "Missing overflow or result");
Index: test/CodeGen/builtins-overflow.c
===
--- test/CodeGen/builtins-overflow.c
+++ test/CodeGen/builtins-overflow.c
@@ -373,7 +373,9 @@
// CHECK-NEXT: [[NotNull:%.*]] = icmp ne i32 [[UnsignedResult]], 0
// CHECK-NEXT: [[Underflow:%.*]] = and i1 [[IsNeg]], [[NotNull]]
// CHECK-NEXT: [[OFlow:%.*]] = or i1 [[UnsignedOFlow]], [[Underflow]]
-// CHECK-NEXT: store i32 [[UnsignedResult]], i32* %{{.*}}, align 4
+// CHECK-NEXT: [[NegatedResult:%.*]] = sub i32 0, [[UnsignedResult]]
+// CHECK-NEXT: [[Result:%.*]] = select i1 [[IsNeg]], i32 [[NegatedResult]], i32 [[UnsignedResult]]
+// CHECK-NEXT: store i32 [[Result]], i32* %{{.*}}, align 4
// CHECK: br i1 [[OFlow]]
unsigned result;
@@ -432,7 +434,9 @@
// CHECK-NEXT: [[OVERFLOW_PRE_TRUNC:%.*]] = or i1 {{.*}}, [[UNDERFLOW]]
// CHECK-NEXT: [[TRUNC_OVERFLOW:%.*]] = icmp ugt i64 [[UNSIGNED_RESULT]], 4294967295
// CHECK-NEXT: [[OVERFLOW:%.*]] = or i1 [[OVERFLOW_PRE_TRUNC]], [[TRUNC_OVERFLOW]]
-// CHECK-NEXT: trunc i64 [[UNSIGNED_RESULT]] to i32
+// CHECK-NEXT: [[NEGATED:%.*]] = sub i64 0, [[UNSIGNED_RESULT]]
+// CHECK-NEXT: [[RESULT:%.*]] = select i1 {{.*}}, i64 [[NEGATED]], i64 [[UNSIGNED_RESULT]]
+// CHECK-NEXT: trunc i64 [[RESULT]] to i32
// CHECK-NEXT: store
unsigned result;
if (__builtin_mul_overflow(y, x, &result))
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -915,7 +915,11 @@
Overflow = CGF.Builder.CreateOr(Overflow, TruncOverflow);
}
-Result = CGF.Builder.CreateTrunc(UnsignedResult, ResTy);
+// Negate the product if it would be negative in infinite precision.
+Result = CGF.Builder.CreateSelect(
+IsNegative, CGF.Builder.CreateNeg(UnsignedResult), UnsignedResult);
+
+Result = CGF.Builder.CreateTrunc(Result, ResTy);
}
assert(Overflow && Result && "Missing overflow or result");
___
cfe-commits
[PATCH] D41921: [Parse] Forward brace locations to TypeConstructExpr
vsk created this revision.
vsk added reviewers: rsmith, aaron.ballman, faisalv.
When parsing C++ type construction expressions with list initialization,
forward the locations of the braces to Sema.
Without these locations, the code coverage pass crashes on the given test
case, because the pass relies on getLocEnd() returning a valid location.
Here is what this patch does in more detail:
- Forwards init-list brace locations to Sema (ParseExprCXX),
- Builds an InitializationKind with these locations (SemaExprCXX), and
- Uses these locations for constructor initialization (SemaInit).
The remaining changes fall out of introducing a new overload for
creating direct-list InitializationKinds.
Testing: check-clang, and a stage2 coverage-enabled build of clang with
asserts enabled.
https://reviews.llvm.org/D41921
Files:
include/clang/AST/ExprCXX.h
include/clang/Sema/Initialization.h
include/clang/Sema/Sema.h
lib/CodeGen/CoverageMappingGen.cpp
lib/Parse/ParseExprCXX.cpp
lib/Sema/SemaDeclCXX.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaInit.cpp
lib/Sema/TreeTransform.h
test/CoverageMapping/classtemplate.cpp
test/SemaCXX/sourceranges.cpp
Index: test/SemaCXX/sourceranges.cpp
===
--- test/SemaCXX/sourceranges.cpp
+++ test/SemaCXX/sourceranges.cpp
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -triple i686-mingw32 -ast-dump %s | FileCheck %s
+// RUN: %clang_cc1 -triple i686-mingw32 -std=c++1z -ast-dump %s | FileCheck %s -check-prefix=CHECK-1Z
template
class P {
@@ -12,7 +13,7 @@
typedef int C;
}
-// CHECK: VarDecl {{0x[0-9a-fA-F]+}} col:15 ImplicitConstrArray 'foo::A [2]'
+// CHECK: VarDecl {{0x[0-9a-fA-F]+}} col:15 ImplicitConstrArray 'foo::A [2]'
static foo::A ImplicitConstrArray[2];
int main() {
@@ -50,3 +51,89 @@
D d = D(12);
// CHECK: CXXConstructExpr {{0x[0-9a-fA-F]+}} 'D' 'void (int){{( __attribute__\(\(thiscall\)\))?}}'
}
+
+void abort() __attribute__((noreturn));
+
+namespace std {
+typedef decltype(sizeof(int)) size_t;
+
+template struct initializer_list {
+ const E *p;
+ size_t n;
+ initializer_list(const E *p, size_t n) : p(p), n(n) {}
+};
+
+template struct pair {
+ F f;
+ S s;
+ pair(const F &f, const S &s) : f(f), s(s) {}
+};
+
+struct string {
+ const char *str;
+ string() { abort(); }
+ string(const char *S) : str(S) {}
+ ~string() { abort(); }
+};
+
+template
+struct map {
+ using T = pair;
+ map(initializer_list i, const string &s = string()) {}
+ ~map() { abort(); }
+};
+
+}; // namespace std
+
+#if __cplusplus >= 201703L
+// CHECK-1Z: FunctionDecl {{.*}} construct_with_init_list
+std::map construct_with_init_list() {
+ // CHECK-1Z-NEXT: CompoundStmt
+ // CHECK-1Z-NEXT: ReturnStmt {{.*}} {{0, 0}};
+}
+
+// CHECK-1Z: NamespaceDecl {{.*}} in_class_init
+namespace in_class_init {
+ struct A {};
+
+ // CHECK-1Z: CXXRecordDecl {{.*}} struct B definition
+ struct B {
+// CHECK-1Z: FieldDecl {{.*}} a 'in_class_init::A'
+// CHECK-1Z-NEXT: InitListExpr {{.*}}
class Test {
@@ -44,11 +45,51 @@
void unmangleable(UninstantiatedClassWithTraits x) {}
};
+void abort() __attribute__((noreturn));
+
+namespace std {
+typedef decltype(sizeof(int)) size_t;
+
+template struct initializer_list {
+ const E *p;
+ size_t n;
+ initializer_list(const E *p, size_t n) : p(p), n(n) {}
+};
+
+template struct pair {
+ F f;
+ S s;
+ pair(const F &f, const S &s) : f(f), s(s) {}
+};
+
+struct string {
+ const char *str;
+ string() { abort(); }
+ string(const char *S) : str(S) {}
+ ~string() { abort(); }
+};
+
+template
+struct map {
+ using T = pair;
+ map(initializer_list i, const string &s = string()) {}
+ ~map() { abort(); }
+};
+
+}; // namespace std
+
+// CHECK-INIT-LIST-LABEL: _Z5Test4v:
+std::map Test4() { // CHECK-INIT-LIST: File 0, [[@LINE]]:28 -> [[@LINE+3]]:2 = #0
+ abort();
+ return std::map{{0, 0}}; // CHECK-INIT-LIST-NEXT: [[@LINE]]:3 -> [[@LINE]]:36 = 0
+}
+
int main() {
Test t;
t.set(Test::A, 5.5);
t.set(Test::T, 5.6);
t.set(Test::G, 5.7);
t.set(Test::C, 5.8);
+ Test4();
return 0;
}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -2591,10 +2591,11 @@
ExprResult RebuildCXXFunctionalCastExpr(TypeSourceInfo *TInfo,
SourceLocation LParenLoc,
Expr *Sub,
- SourceLocation RParenLoc) {
+ SourceLocation RParenLoc,
+ bool ListInitialization) {
return getSema().BuildCXXTypeConstructExpr(TInfo, LParenLoc,
- MultiExprArg(&Sub, 1),
- RParenLoc);
+ MultiExprArg(&Sub, 1), RParenL
