Re: [WIP][PATCH] pointer-overflow sanitizer
Hi all! Attached are updated copies of the patches, previous ones no longer apply cleanly to ToT. Also cleaned up the clang patch a bit. Enjoy, feedback/review requested :). ~Will On Tue, Oct 28, 2014 at 7:42 PM, Will Dietz wdie...@illinois.edu wrote: Hi all, Attached are updated patches for adding -fsanitize=pointer-overflow. Now with quite a bit more thorough testing of various constructs :). On my blog I wrote a post detailing a few bugs found with this tool[1][2]. At least some developers care: * LLVM accepted a patch to ASTVector to fix this behavior[3] (disclosure: I committed this one, but no one objected O:)) * ffmpeg fixed reported issue[4] I haven't reported other issues yet, probably will do that soon :). Anyway, please review and let me know any thoughts you have about this checker :). Enjoy! ~Will [1] http://wdtz.org/catching-pointer-overflow-bugs.html [2] http://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html [3] http://llvm.org/viewvc/llvm-project?view=revisionrevision=216385 [4] https://trac.ffmpeg.org/ticket/3152 ~Will On Mon, Nov 18, 2013 at 11:13 PM, Will Dietz wdie...@illinois.edu wrote: Attached are updated patches, please take a look :). For now not checking struct indexing as doing so caught no additional bugs on my test programs and doing so requires a fair bit of plumbing to get the SourceLocation down into the struct indexing helpers. That said, this has been useful in catching bugs in LLVM and elsewhere, as previously reported. Thanks! ~Will On Mon, Oct 28, 2013 at 7:56 PM, Will Dietz wdie...@illinois.edu wrote: Glad there's some interest. I have no test coverage of anything other than the Driver component, that will be included. I also need to do some plumbing work to support adding checks to struct indexing. I've tried this on: * LLVM/Clang * ImageMagick * binutils * curl * ffmpeg (w/FATE samples) * openldap * openssh * pcre * postgresql * sqlite And the programs seem to build and at least pass their own non-trivial test-suites. So far detected bugs in: * binutils (what inspired this sanitizer) * clang (reported earlier today) * curl (unreported) * pcre (unreported) * ffmpeg (unreported) With a single bug location per software so far :). I also expect this to work particularly well with fuzz testing. ~Will On Mon, Oct 28, 2013 at 5:44 PM, Richard Smith rich...@metafoo.co.uk wrote: Seems like a nice idea to me. (Your test coverage is pretty weak, though.) Have you tried this much on large codebases? Does this find many bugs? (I can imagine it would be effective when combined with fuzz testing...) On Mon, Oct 28, 2013 at 3:39 PM, Will Dietz wdie...@illinois.edu wrote: Hi all, Recently I thought it would be useful to have a sanitizer for detecting overflows in pointer expressions. Such overflows are undefined behavior and are pretty much always bugs. While it's true that if such an overflowed pointer is dereferenced a tool such as ASan will catch the error, detection of these bugs when the occur helps fix them without requiring an input that triggers a crash. Two examples of this in the wild: * binutils undefined behavior bug that leads to segfault when built with clang[1] * ASTVector bug I just submitted patch for, discovered using this sanitizer[2] Attached are patches for clang and compiler-rt that implement this sanitizer and seem to work well in my testing so far. There is some work to do yet: * Adding lit tests to clang/compiler-rt * Finalizing what constructs are useful/worth checking (iterative process, I imagine) * More testing/benchmarking Before tackling the above, I was hoping to get some early feedback: * Is this something the community is interested in/would find useful? * Code review (the current implementation should be complete in terms of the checking code itself) Thank you for your time, here's to finding even more bugs! :) ~Will [1] http://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html [2] http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131028/091878.html ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits From 146e68b827d0581cb1fbb5000ae6ce9d5feeb65c Mon Sep 17 00:00:00 2001 From: Will Dietz w...@wdtz.org Date: Tue, 28 Oct 2014 21:45:44 + Subject: [PATCH] Add handler for printing pointer overflow diagnostics. --- lib/ubsan/ubsan_handlers.cc | 30 + lib/ubsan/ubsan_handlers.h | 7 ++ test/ubsan/TestCases/Pointer/index-overflow.cpp | 18 +++ 3 files changed, 55 insertions(+) create mode 100644 test/ubsan/TestCases/Pointer/index-overflow.cpp diff --git a/lib/ubsan/ubsan_handlers.cc b/lib/ubsan/ubsan_handlers.cc index a0ecff9..04b10ef 100644 --- a/lib/ubsan/ubsan_handlers.cc +++ b/lib/ubsan
Re: [WIP][PATCH] pointer-overflow sanitizer
Hi all,
Attached are updated patches for adding -fsanitize=pointer-overflow.
Now with quite a bit more thorough testing of various constructs :).
On my blog I wrote a post detailing a few bugs found with this tool[1][2].
At least some developers care:
* LLVM accepted a patch to ASTVector to fix this behavior[3]
(disclosure: I committed this one, but no one objected O:))
* ffmpeg fixed reported issue[4]
I haven't reported other issues yet, probably will do that soon :).
Anyway, please review and let me know any thoughts you have about this
checker :).
Enjoy!
~Will
[1] http://wdtz.org/catching-pointer-overflow-bugs.html
[2] http://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html
[3] http://llvm.org/viewvc/llvm-project?view=revisionrevision=216385
[4] https://trac.ffmpeg.org/ticket/3152
~Will
On Mon, Nov 18, 2013 at 11:13 PM, Will Dietz wdie...@illinois.edu wrote:
Attached are updated patches, please take a look :).
For now not checking struct indexing as doing so caught no additional bugs
on my test programs and doing so requires a fair bit of plumbing to get
the SourceLocation down into the struct indexing helpers.
That said, this has been useful in catching bugs in LLVM and
elsewhere, as previously reported.
Thanks!
~Will
On Mon, Oct 28, 2013 at 7:56 PM, Will Dietz wdie...@illinois.edu wrote:
Glad there's some interest.
I have no test coverage of anything other than the Driver component,
that will be included.
I also need to do some plumbing work to support adding checks to
struct indexing.
I've tried this on:
* LLVM/Clang
* ImageMagick
* binutils
* curl
* ffmpeg (w/FATE samples)
* openldap
* openssh
* pcre
* postgresql
* sqlite
And the programs seem to build and at least pass their own non-trivial
test-suites.
So far detected bugs in:
* binutils (what inspired this sanitizer)
* clang (reported earlier today)
* curl (unreported)
* pcre (unreported)
* ffmpeg (unreported)
With a single bug location per software so far :).
I also expect this to work particularly well with fuzz testing.
~Will
On Mon, Oct 28, 2013 at 5:44 PM, Richard Smith rich...@metafoo.co.uk wrote:
Seems like a nice idea to me. (Your test coverage is pretty weak, though.)
Have you tried this much on large codebases? Does this find many bugs? (I
can imagine it would be effective when combined with fuzz testing...)
On Mon, Oct 28, 2013 at 3:39 PM, Will Dietz wdie...@illinois.edu wrote:
Hi all,
Recently I thought it would be useful to have a sanitizer for
detecting overflows in pointer expressions. Such overflows are
undefined behavior and are pretty much always bugs. While it's true
that if such an overflowed pointer is dereferenced a tool such as ASan
will catch the error, detection of these bugs when the occur helps fix
them without requiring an input that triggers a crash.
Two examples of this in the wild:
* binutils undefined behavior bug that leads to segfault when built
with clang[1]
* ASTVector bug I just submitted patch for, discovered using this
sanitizer[2]
Attached are patches for clang and compiler-rt that implement this
sanitizer and seem to work well in my testing so far.
There is some work to do yet:
* Adding lit tests to clang/compiler-rt
* Finalizing what constructs are useful/worth checking (iterative
process, I imagine)
* More testing/benchmarking
Before tackling the above, I was hoping to get some early feedback:
* Is this something the community is interested in/would find useful?
* Code review (the current implementation should be complete in terms
of the checking code itself)
Thank you for your time, here's to finding even more bugs! :)
~Will
[1] http://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html
[2]
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131028/091878.html
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
From c86b9ed640b33d1b80b122ee8701edbc1bcb9f64 Mon Sep 17 00:00:00 2001
From: Will Dietz w...@wdtz.org
Date: Tue, 28 Oct 2014 21:45:44 +
Subject: [PATCH] Add handler for printing pointer overflow diagnostics.
---
lib/ubsan/ubsan_handlers.cc | 30 +
lib/ubsan/ubsan_handlers.h | 7 ++
test/ubsan/TestCases/Pointer/index-overflow.cpp | 20 +
3 files changed, 57 insertions(+)
create mode 100644 test/ubsan/TestCases/Pointer/index-overflow.cpp
diff --git a/lib/ubsan/ubsan_handlers.cc b/lib/ubsan/ubsan_handlers.cc
index a0ecff9..04b10ef 100644
--- a/lib/ubsan/ubsan_handlers.cc
+++ b/lib/ubsan/ubsan_handlers.cc
@@ -410,3 +410,33 @@ void __ubsan::__ubsan_handle_nonnull_arg_abort(NonNullArgData *Data) {
handleNonNullArg(Data, Opts);
Die();
}
+
+static void handlePointerOverflowImpl(PointerOverflowData *Data,
+ ValueHandle
Re: r216463 - Switching from std::vector to llvm::ArrayRef per post-commit review suggestion.
Thanks, apologies for the breakage!
I'll review the Coding standards regarding the C++11 changes carefully... :).
~Will
On Tue, Aug 26, 2014 at 12:05 PM, Aaron Ballman aa...@aaronballman.com wrote:
Author: aaronballman
Date: Tue Aug 26 12:05:57 2014
New Revision: 216463
URL: http://llvm.org/viewvc/llvm-project?rev=216463view=rev
Log:
Switching from std::vector to llvm::ArrayRef per post-commit review
suggestion.
Modified:
cfe/trunk/unittests/AST/ASTVectorTest.cpp
Modified: cfe/trunk/unittests/AST/ASTVectorTest.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/ASTVectorTest.cpp?rev=216463r1=216462r2=216463view=diff
==
--- cfe/trunk/unittests/AST/ASTVectorTest.cpp (original)
+++ cfe/trunk/unittests/AST/ASTVectorTest.cpp Tue Aug 26 12:05:57 2014
@@ -18,8 +18,6 @@
#include gtest/gtest.h
-#include vector
-
using namespace clang;
namespace clang {
@@ -73,7 +71,7 @@ TEST_F(ASTVectorTest, InsertEmpty) {
// Ensure no pointer overflow when inserting empty range
int Values[] = { 0, 1, 2, 3 };
- std::vectorint IntVec(Values, Values + 4);
+ ArrayRefint IntVec(Values);
auto I = V.insert(Ctxt, V.begin(), IntVec.begin(), IntVec.begin());
ASSERT_EQ(V.begin(), I);
ASSERT_TRUE(V.empty());
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r216385 - ASTVector: Fix return value of various insert() methods.
Author: wdietz2
Date: Mon Aug 25 11:09:51 2014
New Revision: 216385
URL: http://llvm.org/viewvc/llvm-project?rev=216385view=rev
Log:
ASTVector: Fix return value of various insert() methods.
Error caught using -fsanitize=pointer-overflow.
Expand ASTVectorTest to verify basic behavior,
test fails without functionality in this patch.
Modified:
cfe/trunk/include/clang/AST/ASTVector.h
cfe/trunk/unittests/AST/ASTVectorTest.cpp
Modified: cfe/trunk/include/clang/AST/ASTVector.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTVector.h?rev=216385r1=216384r2=216385view=diff
==
--- cfe/trunk/include/clang/AST/ASTVector.h (original)
+++ cfe/trunk/include/clang/AST/ASTVector.h Mon Aug 25 11:09:51 2014
@@ -236,14 +236,14 @@ public:
iterator insert(const ASTContext C, iterator I, size_type NumToInsert,
const T Elt) {
-if (I == this-end()) { // Important special case for empty vector.
- append(C, NumToInsert, Elt);
- return this-end()-1;
-}
-
// Convert iterator to elt# to avoid invalidating iterator when we
reserve()
size_t InsertElt = I - this-begin();
+if (I == this-end()) { // Important special case for empty vector.
+ append(C, NumToInsert, Elt);
+ return this-begin() + InsertElt;
+}
+
// Ensure there is enough space.
reserve(C, static_castunsigned(this-size() + NumToInsert));
@@ -284,14 +284,15 @@ public:
templatetypename ItTy
iterator insert(const ASTContext C, iterator I, ItTy From, ItTy To) {
-if (I == this-end()) { // Important special case for empty vector.
+// Convert iterator to elt# to avoid invalidating iterator when we
reserve()
+size_t InsertElt = I - this-begin();
+
+if (I == this-end()) { // Important special case for empty vector.
append(C, From, To);
- return this-end()-1;
+ return this-begin() + InsertElt;
}
size_t NumToInsert = std::distance(From, To);
-// Convert iterator to elt# to avoid invalidating iterator when we
reserve()
-size_t InsertElt = I - this-begin();
// Ensure there is enough space.
reserve(C, static_castunsigned(this-size() + NumToInsert));
Modified: cfe/trunk/unittests/AST/ASTVectorTest.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/ASTVectorTest.cpp?rev=216385r1=216384r2=216385view=diff
==
--- cfe/trunk/unittests/AST/ASTVectorTest.cpp (original)
+++ cfe/trunk/unittests/AST/ASTVectorTest.cpp Mon Aug 25 11:09:51 2014
@@ -14,11 +14,81 @@
#include llvm/Support/Compiler.h
#include clang/AST/ASTContext.h
#include clang/AST/ASTVector.h
+#include clang/Basic/Builtins.h
+
+#include gtest/gtest.h
+
+#include vector
using namespace clang;
-LLVM_ATTRIBUTE_UNUSED void CompileTest() {
- ASTContext *C = nullptr;
+namespace clang {
+namespace ast {
+
+namespace {
+class ASTVectorTest : public ::testing::Test {
+protected:
+ ASTVectorTest()
+ : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), Idents(LangOpts, nullptr),
+Ctxt(LangOpts, SourceMgr, Idents, Sels, Builtins) {}
+
+ FileSystemOptions FileMgrOpts;
+ FileManager FileMgr;
+ IntrusiveRefCntPtrDiagnosticIDs DiagID;
+ DiagnosticsEngine Diags;
+ SourceManager SourceMgr;
+ LangOptions LangOpts;
+ IdentifierTable Idents;
+ SelectorTable Sels;
+ Builtin::Context Builtins;
+ ASTContext Ctxt;
+};
+} // unnamed namespace
+
+TEST_F(ASTVectorTest, Compile) {
ASTVectorint V;
- V.insert(*C, V.begin(), 0);
+ V.insert(Ctxt, V.begin(), 0);
}
+
+TEST_F(ASTVectorTest, InsertFill) {
+ ASTVectordouble V;
+
+ // Ensure returned iterator points to first of inserted elements
+ auto I = V.insert(Ctxt, V.begin(), 5, 1.0);
+ ASSERT_EQ(V.begin(), I);
+
+ // Check non-empty case as well
+ I = V.insert(Ctxt, V.begin() + 1, 5, 1.0);
+ ASSERT_EQ(V.begin() + 1, I);
+
+ // And insert-at-end
+ I = V.insert(Ctxt, V.end(), 5, 1.0);
+ ASSERT_EQ(V.end() - 5, I);
+}
+
+TEST_F(ASTVectorTest, InsertEmpty) {
+ ASTVectordouble V;
+
+ // Ensure no pointer overflow when inserting empty range
+ std::vectorint IntVec{0, 1, 2, 3};
+ auto I = V.insert(Ctxt, V.begin(), IntVec.begin(), IntVec.begin());
+ ASSERT_EQ(V.begin(), I);
+ ASSERT_TRUE(V.empty());
+
+ // Non-empty range
+ I = V.insert(Ctxt, V.begin(), IntVec.begin(), IntVec.end());
+ ASSERT_EQ(V.begin(), I);
+
+ // Non-Empty Vector, empty range
+ I = V.insert(Ctxt, V.end(), IntVec.begin(), IntVec.begin());
+ ASSERT_EQ(V.begin() + IntVec.size(), I);
+
+ // Non-Empty Vector, non-empty range
+ I = V.insert(Ctxt, V.end(), IntVec.begin(), IntVec.end());
+ ASSERT_EQ(V.begin() + IntVec.size(), I);
+}
+
+} // end namespace ast
+} // end namespace clang
Re: [PATCH] ASTVector: Fix return value of various insert() methods
Thank you, good call.
Added mentioned test and committed as r216385.
Thanks!
~Will
On Mon, Aug 25, 2014 at 12:24 AM, Richard Smith rich...@metafoo.co.uk wrote:
+TEST_F(ASTVectorTest, InsertFill) {
+ ASTVectordouble V;
+
+ // Ensure returned iterator points to first of inserted elements
+ auto I = V.insert(Ctxt, V.begin(), 5, 1.0);
+ ASSERT_EQ(I, V.begin());
+
+ // Check non-empty case as well
+ auto J = V.insert(Ctxt, V.begin() + 1, 5, 1.0);
+ ASSERT_EQ(J, V.begin() + 1);
}
You don't have any tests for the insert-at-the-end, non-empty case for this
form of 'insert', as far as I can see. Looks like the test would still pass
if I did this:
iterator insert(const ASTContext C, iterator I, size_type NumToInsert,
const T Elt) {
// Convert iterator to elt# to avoid invalidating iterator when we
reserve()
size_t InsertElt = I - this-begin();
if (I == this-end()) { // Important special case for empty vector.
append(C, NumToInsert, Elt);
- return this-begin() + InsertElt;
+ return this-begin();
}
LGTM with one more test case for the above.
On Thu, Aug 21, 2014 at 4:17 PM, Will Dietz wdie...@illinois.edu wrote:
Hi Richard, @cfe-commits,
Attached is an updated patch, including actual tests for ASTVector.
There's still a long way to go to verify other aspects of ASTVector's
behavior as a well-behaved container, but this covers the functionality
changes made in this commit and should serve as a basis for more thorough
testing in the future should someone tackle such a task :).
Please let me know okay to commit or if there's any questions or comments
:).
Thanks!
~Will
On Tue, Nov 19, 2013 at 9:18 AM, Will Dietz wdie...@illinois.edu wrote:
Closest we have is a test to ensure ASTVector compiles correctly with
basic usage[1] due to complications in creating a fake ASTContext to
construct it with. I'd be happy to write some functionality tests if
anyone has a good way to construct one for testing purposes, as I'm
not very familiar with the related code.
Or should I commit this as improving quality regardless?
~Will
[1]
https://github.com/llvm-mirror/clang/blob/f475bf83a45435a211edb4e0ef6ac3481ce7b3fe/unittests/AST/ASTVectorTest.cpp
On Tue, Nov 19, 2013 at 2:06 AM, Richard Smith rich...@metafoo.co.uk
wrote:
Change LGTM. It'd be nice to have test coverage for this that doesn't
require running a sanitizer. Do we have any direct tests for ASTVector?
On Mon, Nov 18, 2013 at 9:35 PM, Will Dietz wdie...@illinois.edu
wrote:
Ping! :)
~Will
On Mon, Nov 4, 2013 at 4:32 PM, Will Dietz wdie...@illinois.edu
wrote:
Ping.
It's easy to get clang to trigger this bug which results in an
invalid
iterator to be returned (which the current code happens to ignore,
but
that's just a lucky coincidence), as this regularly occurs during
execution of the lit tests.
On a related note, any suggestions on how to create a simple dummy
ASTContext for testing? As noted in
the commit that originally added ASTVectorTest.cpp (r186253) this
blocks the creation of even basic
functionality tests for this data structure.
~Will
On Mon, Oct 28, 2013 at 5:11 PM, Will Dietz wdie...@illinois.edu
wrote:
Error caught -fsanitize=pointer-overflow[1], curiously enough :).
The pointer overflow occurred when insert() was invoked with
From==To,
which is done in quite a few places. While std::vector::insert
requires [From,To) to be valid, it looks like here From==To is
intended to be supported[2], making the bug in the container not in
its use.
This patch fixes the overflow when From==To, as well as the return
value in this variant as well as the fill variant, changing them
to
return an iterator pointing to the first of the inserted elements
(like SmallVector does).
See attached.
~Will
[1] Patches coming soon.
[2] See the implementation of append(), for example.
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] ASTVector: Fix return value of various insert() methods
Hi Richard, @cfe-commits,
Attached is an updated patch, including actual tests for ASTVector.
There's still a long way to go to verify other aspects of ASTVector's
behavior as a well-behaved container, but this covers the functionality
changes made in this commit and should serve as a basis for more thorough
testing in the future should someone tackle such a task :).
Please let me know okay to commit or if there's any questions or comments :).
Thanks!
~Will
On Tue, Nov 19, 2013 at 9:18 AM, Will Dietz wdie...@illinois.edu wrote:
Closest we have is a test to ensure ASTVector compiles correctly with
basic usage[1] due to complications in creating a fake ASTContext to
construct it with. I'd be happy to write some functionality tests if
anyone has a good way to construct one for testing purposes, as I'm
not very familiar with the related code.
Or should I commit this as improving quality regardless?
~Will
[1]
https://github.com/llvm-mirror/clang/blob/f475bf83a45435a211edb4e0ef6ac3481ce7b3fe/unittests/AST/ASTVectorTest.cpp
On Tue, Nov 19, 2013 at 2:06 AM, Richard Smith rich...@metafoo.co.uk wrote:
Change LGTM. It'd be nice to have test coverage for this that doesn't
require running a sanitizer. Do we have any direct tests for ASTVector?
On Mon, Nov 18, 2013 at 9:35 PM, Will Dietz wdie...@illinois.edu wrote:
Ping! :)
~Will
On Mon, Nov 4, 2013 at 4:32 PM, Will Dietz wdie...@illinois.edu wrote:
Ping.
It's easy to get clang to trigger this bug which results in an invalid
iterator to be returned (which the current code happens to ignore, but
that's just a lucky coincidence), as this regularly occurs during
execution of the lit tests.
On a related note, any suggestions on how to create a simple dummy
ASTContext for testing? As noted in
the commit that originally added ASTVectorTest.cpp (r186253) this
blocks the creation of even basic
functionality tests for this data structure.
~Will
On Mon, Oct 28, 2013 at 5:11 PM, Will Dietz wdie...@illinois.edu
wrote:
Error caught -fsanitize=pointer-overflow[1], curiously enough :).
The pointer overflow occurred when insert() was invoked with From==To,
which is done in quite a few places. While std::vector::insert
requires [From,To) to be valid, it looks like here From==To is
intended to be supported[2], making the bug in the container not in
its use.
This patch fixes the overflow when From==To, as well as the return
value in this variant as well as the fill variant, changing them to
return an iterator pointing to the first of the inserted elements
(like SmallVector does).
See attached.
~Will
[1] Patches coming soon.
[2] See the implementation of append(), for example.
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
From 941368075b2dc46431c742c30c9caa2193b7a9c6 Mon Sep 17 00:00:00 2001
From: Will Dietz w...@wdtz.org
Date: Mon, 28 Oct 2013 08:10:34 -0500
Subject: [PATCH] ASTVector: Fix return value of various insert() methods.
Error caught using -fsanitize=pointer-overflow.
Expand ASTVectorTest to verify basic behavior,
test fails without functionality in this patch.
---
include/clang/AST/ASTVector.h | 19 ++-
unittests/AST/ASTVectorTest.cpp | 73 +++--
2 files changed, 80 insertions(+), 12 deletions(-)
diff --git a/include/clang/AST/ASTVector.h b/include/clang/AST/ASTVector.h
index 3b856a7..6ec0545 100644
--- a/include/clang/AST/ASTVector.h
+++ b/include/clang/AST/ASTVector.h
@@ -236,14 +236,14 @@ public:
iterator insert(const ASTContext C, iterator I, size_type NumToInsert,
const T Elt) {
-if (I == this-end()) { // Important special case for empty vector.
- append(C, NumToInsert, Elt);
- return this-end()-1;
-}
-
// Convert iterator to elt# to avoid invalidating iterator when we reserve()
size_t InsertElt = I - this-begin();
+if (I == this-end()) { // Important special case for empty vector.
+ append(C, NumToInsert, Elt);
+ return this-begin() + InsertElt;
+}
+
// Ensure there is enough space.
reserve(C, static_castunsigned(this-size() + NumToInsert));
@@ -284,14 +284,15 @@ public:
templatetypename ItTy
iterator insert(const ASTContext C, iterator I, ItTy From, ItTy To) {
-if (I == this-end()) { // Important special case for empty vector.
+// Convert iterator to elt# to avoid invalidating iterator when we reserve()
+size_t InsertElt = I - this-begin();
+
+if (I == this-end()) { // Important special case for empty vector.
append(C, From, To);
- return this-end()-1;
+ return this-begin() + InsertElt;
}
size_t NumToInsert = std::distance(From, To);
-// Convert iterator to elt# to avoid invalidating iterator when we reserve()
-size_t InsertElt = I - this-begin
Re: [PATCH] ASTVector: Fix return value of various insert() methods
Closest we have is a test to ensure ASTVector compiles correctly with basic usage[1] due to complications in creating a fake ASTContext to construct it with. I'd be happy to write some functionality tests if anyone has a good way to construct one for testing purposes, as I'm not very familiar with the related code. Or should I commit this as improving quality regardless? ~Will [1] https://github.com/llvm-mirror/clang/blob/f475bf83a45435a211edb4e0ef6ac3481ce7b3fe/unittests/AST/ASTVectorTest.cpp On Tue, Nov 19, 2013 at 2:06 AM, Richard Smith rich...@metafoo.co.uk wrote: Change LGTM. It'd be nice to have test coverage for this that doesn't require running a sanitizer. Do we have any direct tests for ASTVector? On Mon, Nov 18, 2013 at 9:35 PM, Will Dietz wdie...@illinois.edu wrote: Ping! :) ~Will On Mon, Nov 4, 2013 at 4:32 PM, Will Dietz wdie...@illinois.edu wrote: Ping. It's easy to get clang to trigger this bug which results in an invalid iterator to be returned (which the current code happens to ignore, but that's just a lucky coincidence), as this regularly occurs during execution of the lit tests. On a related note, any suggestions on how to create a simple dummy ASTContext for testing? As noted in the commit that originally added ASTVectorTest.cpp (r186253) this blocks the creation of even basic functionality tests for this data structure. ~Will On Mon, Oct 28, 2013 at 5:11 PM, Will Dietz wdie...@illinois.edu wrote: Error caught -fsanitize=pointer-overflow[1], curiously enough :). The pointer overflow occurred when insert() was invoked with From==To, which is done in quite a few places. While std::vector::insert requires [From,To) to be valid, it looks like here From==To is intended to be supported[2], making the bug in the container not in its use. This patch fixes the overflow when From==To, as well as the return value in this variant as well as the fill variant, changing them to return an iterator pointing to the first of the inserted elements (like SmallVector does). See attached. ~Will [1] Patches coming soon. [2] See the implementation of append(), for example. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [WIP][PATCH] pointer-overflow sanitizer
Attached are updated patches, please take a look :). For now not checking struct indexing as doing so caught no additional bugs on my test programs and doing so requires a fair bit of plumbing to get the SourceLocation down into the struct indexing helpers. That said, this has been useful in catching bugs in LLVM and elsewhere, as previously reported. Thanks! ~Will On Mon, Oct 28, 2013 at 7:56 PM, Will Dietz wdie...@illinois.edu wrote: Glad there's some interest. I have no test coverage of anything other than the Driver component, that will be included. I also need to do some plumbing work to support adding checks to struct indexing. I've tried this on: * LLVM/Clang * ImageMagick * binutils * curl * ffmpeg (w/FATE samples) * openldap * openssh * pcre * postgresql * sqlite And the programs seem to build and at least pass their own non-trivial test-suites. So far detected bugs in: * binutils (what inspired this sanitizer) * clang (reported earlier today) * curl (unreported) * pcre (unreported) * ffmpeg (unreported) With a single bug location per software so far :). I also expect this to work particularly well with fuzz testing. ~Will On Mon, Oct 28, 2013 at 5:44 PM, Richard Smith rich...@metafoo.co.uk wrote: Seems like a nice idea to me. (Your test coverage is pretty weak, though.) Have you tried this much on large codebases? Does this find many bugs? (I can imagine it would be effective when combined with fuzz testing...) On Mon, Oct 28, 2013 at 3:39 PM, Will Dietz wdie...@illinois.edu wrote: Hi all, Recently I thought it would be useful to have a sanitizer for detecting overflows in pointer expressions. Such overflows are undefined behavior and are pretty much always bugs. While it's true that if such an overflowed pointer is dereferenced a tool such as ASan will catch the error, detection of these bugs when the occur helps fix them without requiring an input that triggers a crash. Two examples of this in the wild: * binutils undefined behavior bug that leads to segfault when built with clang[1] * ASTVector bug I just submitted patch for, discovered using this sanitizer[2] Attached are patches for clang and compiler-rt that implement this sanitizer and seem to work well in my testing so far. There is some work to do yet: * Adding lit tests to clang/compiler-rt * Finalizing what constructs are useful/worth checking (iterative process, I imagine) * More testing/benchmarking Before tackling the above, I was hoping to get some early feedback: * Is this something the community is interested in/would find useful? * Code review (the current implementation should be complete in terms of the checking code itself) Thank you for your time, here's to finding even more bugs! :) ~Will [1] http://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html [2] http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131028/091878.html ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits From b83d172f45e3a69081a5f6604f368a573d694616 Mon Sep 17 00:00:00 2001 From: Will Dietz w...@wdtz.org Date: Sun, 27 Oct 2013 14:53:17 -0500 Subject: [PATCH] Add 'pointer-overflow' sanitizer for undefined overflow in GEP's. Handles things like: char *p = NULL; --p;. --- include/clang/Basic/Sanitizers.def |9 ++-- lib/CodeGen/CGExpr.cpp | 94 +++- lib/CodeGen/CGExprAgg.cpp |7 +++ lib/CodeGen/CGExprCXX.cpp |3 + lib/CodeGen/CGExprScalar.cpp | 21 ++-- lib/CodeGen/CodeGenFunction.h |3 + test/CodeGen/pointer-overflow.c| 39 +++ test/Driver/fsanitize.c|8 ++-- 8 files changed, 169 insertions(+), 15 deletions(-) create mode 100644 test/CodeGen/pointer-overflow.c diff --git a/include/clang/Basic/Sanitizers.def b/include/clang/Basic/Sanitizers.def index c9b31a3..188e62f 100644 --- a/include/clang/Basic/Sanitizers.def +++ b/include/clang/Basic/Sanitizers.def @@ -68,6 +68,7 @@ SANITIZER(function, Function) SANITIZER(integer-divide-by-zero, IntegerDivideByZero) SANITIZER(null, Null) SANITIZER(object-size, ObjectSize) +SANITIZER(pointer-overflow, PointerOverflow) SANITIZER(return, Return) SANITIZER(shift, Shift) SANITIZER(signed-integer-overflow, SignedIntegerOverflow) @@ -86,8 +87,8 @@ SANITIZER(dataflow, DataFlow) SANITIZER_GROUP(undefined, Undefined, Alignment | Bool | ArrayBounds | Enum | FloatCastOverflow | FloatDivideByZero | Function | IntegerDivideByZero | Null | -ObjectSize | Return | Shift | SignedIntegerOverflow | -Unreachable | VLABound | Vptr) +ObjectSize | PointerOverflow | Return | Shift | +SignedIntegerOverflow | Unreachable | VLABound | Vptr) // -fsanitize=undefined-trap (and its alias -fcatch
Re: [PATCH] ASTVector: Fix return value of various insert() methods
Ping! :) ~Will On Mon, Nov 4, 2013 at 4:32 PM, Will Dietz wdie...@illinois.edu wrote: Ping. It's easy to get clang to trigger this bug which results in an invalid iterator to be returned (which the current code happens to ignore, but that's just a lucky coincidence), as this regularly occurs during execution of the lit tests. On a related note, any suggestions on how to create a simple dummy ASTContext for testing? As noted in the commit that originally added ASTVectorTest.cpp (r186253) this blocks the creation of even basic functionality tests for this data structure. ~Will On Mon, Oct 28, 2013 at 5:11 PM, Will Dietz wdie...@illinois.edu wrote: Error caught -fsanitize=pointer-overflow[1], curiously enough :). The pointer overflow occurred when insert() was invoked with From==To, which is done in quite a few places. While std::vector::insert requires [From,To) to be valid, it looks like here From==To is intended to be supported[2], making the bug in the container not in its use. This patch fixes the overflow when From==To, as well as the return value in this variant as well as the fill variant, changing them to return an iterator pointing to the first of the inserted elements (like SmallVector does). See attached. ~Will [1] Patches coming soon. [2] See the implementation of append(), for example. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[PATCH] ubsan: Only emit constants for filenames and type descriptors once.
Produces neater IR in significantly less time.
(~18% faster -O0 compile time for sqlite3 with -fsanitize=undefined)
-
Patch attached, both of these were marked as FIXME's previously.
Okay to commit?
~Will
From eb61ff9d5f83be6d828ed0769732bb06d8478a19 Mon Sep 17 00:00:00 2001
From: Will Dietz w...@wdtz.org
Date: Thu, 7 Nov 2013 15:27:03 -0600
Subject: [PATCH] ubsan: Only emit constants for filenames and type descriptors
once.
Produces neater IR in significantly less time.
(~18% faster -O0 compile time for sqlite3 with -fsanitize=undefined)
---
lib/CodeGen/CGExpr.cpp | 13 +
lib/CodeGen/CodeGenModule.h | 10 ++
test/CodeGen/compound-assign-overflow.c | 3 +--
3 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/lib/CodeGen/CGExpr.cpp b/lib/CodeGen/CGExpr.cpp
index 96d6a01..be50f4e 100644
--- a/lib/CodeGen/CGExpr.cpp
+++ b/lib/CodeGen/CGExpr.cpp
@@ -2027,7 +2027,10 @@ LValue CodeGenFunction::EmitPredefinedLValue(const PredefinedExpr *E) {
/// followed by an array of i8 containing the type name. TypeKind is 0 for an
/// integer, 1 for a floating point value, and -1 for anything else.
llvm::Constant *CodeGenFunction::EmitCheckTypeDescriptor(QualType T) {
- // FIXME: Only emit each type's descriptor once.
+ // Only emit each type's descriptor once.
+ if (llvm::Constant *C = CGM.getTypeDescriptor(T))
+return C;
+
uint16_t TypeKind = -1;
uint16_t TypeInfo = 0;
@@ -2060,6 +2063,10 @@ llvm::Constant *CodeGenFunction::EmitCheckTypeDescriptor(QualType T) {
llvm::GlobalVariable::PrivateLinkage,
Descriptor);
GV-setUnnamedAddr(true);
+
+ // Remember the descriptor for this type.
+ CGM.setTypeDescriptor(T, GV);
+
return GV;
}
@@ -2102,9 +2109,7 @@ llvm::Constant *CodeGenFunction::EmitCheckSourceLocation(SourceLocation Loc) {
PresumedLoc PLoc = getContext().getSourceManager().getPresumedLoc(Loc);
llvm::Constant *Data[] = {
-// FIXME: Only emit each file name once.
-PLoc.isValid() ? castllvm::Constant(
- Builder.CreateGlobalStringPtr(PLoc.getFilename()))
+PLoc.isValid() ? CGM.GetAddrOfConstantCString(PLoc.getFilename(), .src)
: llvm::Constant::getNullValue(Int8PtrTy),
Builder.getInt32(PLoc.isValid() ? PLoc.getLine() : 0),
Builder.getInt32(PLoc.isValid() ? PLoc.getColumn() : 0)
diff --git a/lib/CodeGen/CodeGenModule.h b/lib/CodeGen/CodeGenModule.h
index 02b9ce6..c161224 100644
--- a/lib/CodeGen/CodeGenModule.h
+++ b/lib/CodeGen/CodeGenModule.h
@@ -318,6 +318,9 @@ class CodeGenModule : public CodeGenTypeCache {
llvm::DenseMapQualType, llvm::Constant * AtomicSetterHelperFnMap;
llvm::DenseMapQualType, llvm::Constant * AtomicGetterHelperFnMap;
+ /// Map used to get unique type descriptor constants for sanitizers.
+ llvm::DenseMapQualType, llvm::Constant * TypeDescriptorMap;
+
/// Map used to track internal linkage functions declared within
/// extern C regions.
typedef llvm::MapVectorIdentifierInfo *,
@@ -498,6 +501,13 @@ public:
AtomicGetterHelperFnMap[Ty] = Fn;
}
+ llvm::Constant *getTypeDescriptor(QualType Ty) {
+return TypeDescriptorMap[Ty];
+ }
+ void setTypeDescriptor(QualType Ty, llvm::Constant *C) {
+TypeDescriptorMap[Ty] = C;
+ }
+
CGDebugInfo *getModuleDebugInfo() { return DebugInfo; }
llvm::MDNode *getNoObjCARCExceptionsMetadata() {
diff --git a/test/CodeGen/compound-assign-overflow.c b/test/CodeGen/compound-assign-overflow.c
index e82061b..1533429 100644
--- a/test/CodeGen/compound-assign-overflow.c
+++ b/test/CodeGen/compound-assign-overflow.c
@@ -7,8 +7,7 @@
// CHECK: @[[LINE_100:.*]] = private unnamed_addr global {{.*}}, i32 100, i32 5 {{.*}} @[[INT]]
// CHECK: @[[UINT:.*]] = private unnamed_addr constant { i16, i16, [15 x i8] } { i16 0, i16 10, [15 x i8] c'unsigned int'\00 }
// CHECK: @[[LINE_200:.*]] = private unnamed_addr global {{.*}}, i32 200, i32 5 {{.*}} @[[UINT]]
-// CHECK: @[[DIVINT:.*]] = private unnamed_addr constant { i16, i16, [6 x i8] } { i16 0, i16 11, [6 x i8] c'int'\00 }
-// CHECK: @[[LINE_300:.*]] = private unnamed_addr global {{.*}}, i32 300, i32 5 {{.*}} @[[DIVINT]]
+// CHECK: @[[LINE_300:.*]] = private unnamed_addr global {{.*}}, i32 300, i32 5 {{.*}} @[[INT]]
int32_t x;
--
1.8.4.2
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] Fix toolchain detection on SL6
Looks like this is unnecessary and was actually only temporarily broken by r193528, which was reverted quickly and fixed shortly thereafter in r193554 and others. AFAICT --no-add-needed is not used by default, so it seems the default handling for an unknown target is appropriate. Thanks for the feedback, and hooray my SL6 builds work again :). ~Will On Mon, Oct 28, 2013 at 5:30 PM, Rafael Espíndola rafael.espind...@gmail.com wrote: Since we don't seem to need to know the difference from ScientificLinux6 to ScientificLinux5, I would suggest starting with just ScientificLinux. Can you add a test? You should be able to create tree with a redhat-realease file and, for example, test that we pass --no-add-needed to the linker (assuming that is correct behavior for SL6). Thanks, Rafael ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r194231 - ubsan: Only emit constants for filenames and type descriptors once.
Author: wdietz2
Date: Thu Nov 7 19:09:22 2013
New Revision: 194231
URL: http://llvm.org/viewvc/llvm-project?rev=194231view=rev
Log:
ubsan: Only emit constants for filenames and type descriptors once.
Produces neater IR in significantly less time.
(~18% faster -O0 compile time for sqlite3 with -fsanitize=undefined)
Modified:
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/test/CodeGen/compound-assign-overflow.c
Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=194231r1=194230r2=194231view=diff
==
--- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp Thu Nov 7 19:09:22 2013
@@ -2027,7 +2027,10 @@ LValue CodeGenFunction::EmitPredefinedLV
/// followed by an array of i8 containing the type name. TypeKind is 0 for an
/// integer, 1 for a floating point value, and -1 for anything else.
llvm::Constant *CodeGenFunction::EmitCheckTypeDescriptor(QualType T) {
- // FIXME: Only emit each type's descriptor once.
+ // Only emit each type's descriptor once.
+ if (llvm::Constant *C = CGM.getTypeDescriptor(T))
+return C;
+
uint16_t TypeKind = -1;
uint16_t TypeInfo = 0;
@@ -2060,6 +2063,10 @@ llvm::Constant *CodeGenFunction::EmitChe
llvm::GlobalVariable::PrivateLinkage,
Descriptor);
GV-setUnnamedAddr(true);
+
+ // Remember the descriptor for this type.
+ CGM.setTypeDescriptor(T, GV);
+
return GV;
}
@@ -2102,9 +2109,7 @@ llvm::Constant *CodeGenFunction::EmitChe
PresumedLoc PLoc = getContext().getSourceManager().getPresumedLoc(Loc);
llvm::Constant *Data[] = {
-// FIXME: Only emit each file name once.
-PLoc.isValid() ? castllvm::Constant(
- Builder.CreateGlobalStringPtr(PLoc.getFilename()))
+PLoc.isValid() ? CGM.GetAddrOfConstantCString(PLoc.getFilename(), .src)
: llvm::Constant::getNullValue(Int8PtrTy),
Builder.getInt32(PLoc.isValid() ? PLoc.getLine() : 0),
Builder.getInt32(PLoc.isValid() ? PLoc.getColumn() : 0)
Modified: cfe/trunk/lib/CodeGen/CodeGenModule.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.h?rev=194231r1=194230r2=194231view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.h (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.h Thu Nov 7 19:09:22 2013
@@ -318,6 +318,9 @@ class CodeGenModule : public CodeGenType
llvm::DenseMapQualType, llvm::Constant * AtomicSetterHelperFnMap;
llvm::DenseMapQualType, llvm::Constant * AtomicGetterHelperFnMap;
+ /// Map used to get unique type descriptor constants for sanitizers.
+ llvm::DenseMapQualType, llvm::Constant * TypeDescriptorMap;
+
/// Map used to track internal linkage functions declared within
/// extern C regions.
typedef llvm::MapVectorIdentifierInfo *,
@@ -498,6 +501,13 @@ public:
AtomicGetterHelperFnMap[Ty] = Fn;
}
+ llvm::Constant *getTypeDescriptor(QualType Ty) {
+return TypeDescriptorMap[Ty];
+ }
+ void setTypeDescriptor(QualType Ty, llvm::Constant *C) {
+TypeDescriptorMap[Ty] = C;
+ }
+
CGDebugInfo *getModuleDebugInfo() { return DebugInfo; }
llvm::MDNode *getNoObjCARCExceptionsMetadata() {
Modified: cfe/trunk/test/CodeGen/compound-assign-overflow.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/compound-assign-overflow.c?rev=194231r1=194230r2=194231view=diff
==
--- cfe/trunk/test/CodeGen/compound-assign-overflow.c (original)
+++ cfe/trunk/test/CodeGen/compound-assign-overflow.c Thu Nov 7 19:09:22 2013
@@ -7,8 +7,7 @@
// CHECK: @[[LINE_100:.*]] = private unnamed_addr global {{.*}}, i32 100, i32
5 {{.*}} @[[INT]]
// CHECK: @[[UINT:.*]] = private unnamed_addr constant { i16, i16, [15 x i8] }
{ i16 0, i16 10, [15 x i8] c'unsigned int'\00 }
// CHECK: @[[LINE_200:.*]] = private unnamed_addr global {{.*}}, i32 200, i32
5 {{.*}} @[[UINT]]
-// CHECK: @[[DIVINT:.*]] = private unnamed_addr constant { i16, i16, [6 x i8]
} { i16 0, i16 11, [6 x i8] c'int'\00 }
-// CHECK: @[[LINE_300:.*]] = private unnamed_addr global {{.*}}, i32 300, i32
5 {{.*}} @[[DIVINT]]
+// CHECK: @[[LINE_300:.*]] = private unnamed_addr global {{.*}}, i32 300, i32
5 {{.*}} @[[INT]]
int32_t x;
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] ubsan: Only emit constants for filenames and type descriptors once.
My pleasure :). r194231, thanks! ~Will On Thu, Nov 7, 2013 at 7:07 PM, Richard Smith rich...@metafoo.co.uk wrote: On Thu, Nov 7, 2013 at 3:34 PM, Will Dietz wdie...@illinois.edu wrote: Produces neater IR in significantly less time. (~18% faster -O0 compile time for sqlite3 with -fsanitize=undefined) - Patch attached, both of these were marked as FIXME's previously. Okay to commit? Yes, thank you! ~Will ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] ASTVector: Fix return value of various insert() methods
Ping. It's easy to get clang to trigger this bug which results in an invalid iterator to be returned (which the current code happens to ignore, but that's just a lucky coincidence), as this regularly occurs during execution of the lit tests. On a related note, any suggestions on how to create a simple dummy ASTContext for testing? As noted in the commit that originally added ASTVectorTest.cpp (r186253) this blocks the creation of even basic functionality tests for this data structure. ~Will On Mon, Oct 28, 2013 at 5:11 PM, Will Dietz wdie...@illinois.edu wrote: Error caught -fsanitize=pointer-overflow[1], curiously enough :). The pointer overflow occurred when insert() was invoked with From==To, which is done in quite a few places. While std::vector::insert requires [From,To) to be valid, it looks like here From==To is intended to be supported[2], making the bug in the container not in its use. This patch fixes the overflow when From==To, as well as the return value in this variant as well as the fill variant, changing them to return an iterator pointing to the first of the inserted elements (like SmallVector does). See attached. ~Will [1] Patches coming soon. [2] See the implementation of append(), for example. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[PATCH] ASTVector: Fix return value of various insert() methods
Error caught -fsanitize=pointer-overflow[1], curiously enough :).
The pointer overflow occurred when insert() was invoked with From==To,
which is done in quite a few places. While std::vector::insert
requires [From,To) to be valid, it looks like here From==To is
intended to be supported[2], making the bug in the container not in
its use.
This patch fixes the overflow when From==To, as well as the return
value in this variant as well as the fill variant, changing them to
return an iterator pointing to the first of the inserted elements
(like SmallVector does).
See attached.
~Will
[1] Patches coming soon.
[2] See the implementation of append(), for example.
From d89275825fdfa88c78719df5489433277e56e735 Mon Sep 17 00:00:00 2001
From: Will Dietz w...@wdtz.org
Date: Mon, 28 Oct 2013 08:10:34 -0500
Subject: [PATCH] ASTVector: Fix return value of various insert() methods.
Error caught using -fsanitize=pointer-overflow.
---
include/clang/AST/ASTVector.h | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/include/clang/AST/ASTVector.h b/include/clang/AST/ASTVector.h
index 6db918e..c6dc37b 100644
--- a/include/clang/AST/ASTVector.h
+++ b/include/clang/AST/ASTVector.h
@@ -245,14 +245,14 @@ public:
iterator insert(const ASTContext C, iterator I, size_type NumToInsert,
const T Elt) {
+// Convert iterator to elt# to avoid invalidating iterator when we reserve()
+size_t InsertElt = I - this-begin();
+
if (I == this-end()) { // Important special case for empty vector.
append(C, NumToInsert, Elt);
- return this-end()-1;
+ return this-begin()+InsertElt;
}
-// Convert iterator to elt# to avoid invalidating iterator when we reserve()
-size_t InsertElt = I - this-begin();
-
// Ensure there is enough space.
reserve(C, static_castunsigned(this-size() + NumToInsert));
@@ -293,14 +293,15 @@ public:
templatetypename ItTy
iterator insert(const ASTContext C, iterator I, ItTy From, ItTy To) {
+// Convert iterator to elt# to avoid invalidating iterator when we reserve()
+size_t InsertElt = I - this-begin();
+
if (I == this-end()) { // Important special case for empty vector.
append(C, From, To);
- return this-end()-1;
+ return this-begin()+InsertElt;
}
size_t NumToInsert = std::distance(From, To);
-// Convert iterator to elt# to avoid invalidating iterator when we reserve()
-size_t InsertElt = I - this-begin();
// Ensure there is enough space.
reserve(C, static_castunsigned(this-size() + NumToInsert));
--
1.8.4.1
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[PATCH] Fix toolchain detection on SL6
See attached, thanks!
~Will
From 13bc7bb6b76cd884469004f864e5f53803554350 Mon Sep 17 00:00:00 2001
From: Will Dietz w...@wdtz.org
Date: Mon, 28 Oct 2013 16:49:43 -0500
Subject: [PATCH] Fix toolchain detection on SL6.
* Add SL6 Distro type, detect from /etc/redhat-release
* Ignore /etc/lsb-release if does not contain DISTRIB_CODENAME
Fixes clang misdetecting library locations on 64bit.
---
lib/Driver/ToolChains.cpp | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/lib/Driver/ToolChains.cpp b/lib/Driver/ToolChains.cpp
index e99c1f0..1182a89 100644
--- a/lib/Driver/ToolChains.cpp
+++ b/lib/Driver/ToolChains.cpp
@@ -2096,6 +2096,7 @@ enum Distro {
RHEL6,
Fedora,
OpenSUSE,
+ ScientificLinux6,
UbuntuHardy,
UbuntuIntrepid,
UbuntuJaunty,
@@ -2112,7 +2113,8 @@ enum Distro {
};
static bool IsRedhat(enum Distro Distro) {
- return Distro == Fedora || (Distro = RHEL4 Distro = RHEL6);
+ return Distro == Fedora || (Distro = RHEL4 Distro = RHEL6) ||
+Distro == ScientificLinux6;
}
static bool IsOpenSUSE(enum Distro Distro) {
@@ -2133,10 +2135,9 @@ static Distro DetectDistro(StringRef Prefix, llvm::Triple::ArchType Arch) {
StringRef Data = File.get()-getBuffer();
SmallVectorStringRef, 8 Lines;
Data.split(Lines, \n);
-Distro Version = UnknownDistro;
for (unsigned i = 0, s = Lines.size(); i != s; ++i)
- if (Version == UnknownDistro Lines[i].startswith(DISTRIB_CODENAME=))
-Version = llvm::StringSwitchDistro(Lines[i].substr(17))
+ if (Lines[i].startswith(DISTRIB_CODENAME=))
+return llvm::StringSwitchDistro(Lines[i].substr(17))
.Case(hardy, UbuntuHardy)
.Case(intrepid, UbuntuIntrepid)
.Case(jaunty, UbuntuJaunty)
@@ -2150,7 +2151,6 @@ static Distro DetectDistro(StringRef Prefix, llvm::Triple::ArchType Arch) {
.Case(raring, UbuntuRaring)
.Case(saucy, UbuntuSaucy)
.Default(UnknownDistro);
-return Version;
}
if (!llvm::MemoryBuffer::getFile(Prefix + /etc/redhat-release, File)) {
@@ -2168,6 +2168,9 @@ static Distro DetectDistro(StringRef Prefix, llvm::Triple::ArchType Arch) {
Data.startswith(CentOS))
Data.find(release 4) != StringRef::npos)
return RHEL4;
+else if (Data.startswith(Scientific Linux)
+ Data.find(release 6) != StringRef::npos)
+ return ScientificLinux6;
return UnknownDistro;
}
--
1.8.4.1
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[WIP][PATCH] pointer-overflow sanitizer
Hi all,
Recently I thought it would be useful to have a sanitizer for
detecting overflows in pointer expressions. Such overflows are
undefined behavior and are pretty much always bugs. While it's true
that if such an overflowed pointer is dereferenced a tool such as ASan
will catch the error, detection of these bugs when the occur helps fix
them without requiring an input that triggers a crash.
Two examples of this in the wild:
* binutils undefined behavior bug that leads to segfault when built
with clang[1]
* ASTVector bug I just submitted patch for, discovered using this sanitizer[2]
Attached are patches for clang and compiler-rt that implement this
sanitizer and seem to work well in my testing so far.
There is some work to do yet:
* Adding lit tests to clang/compiler-rt
* Finalizing what constructs are useful/worth checking (iterative
process, I imagine)
* More testing/benchmarking
Before tackling the above, I was hoping to get some early feedback:
* Is this something the community is interested in/would find useful?
* Code review (the current implementation should be complete in terms
of the checking code itself)
Thank you for your time, here's to finding even more bugs! :)
~Will
[1] http://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html
[2]
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131028/091878.html
From bf46609c78ccdd27253dcaeee39ffcac7a156456 Mon Sep 17 00:00:00 2001
From: Will Dietz w...@wdtz.org
Date: Sun, 27 Oct 2013 20:19:55 -0500
Subject: [PATCH] Add handler for pointer overflow sanitizer.
---
lib/ubsan/ubsan_handlers.cc | 19 +++
lib/ubsan/ubsan_handlers.h | 6 ++
2 files changed, 25 insertions(+)
diff --git a/lib/ubsan/ubsan_handlers.cc b/lib/ubsan/ubsan_handlers.cc
index d556431..fbaa8c2 100644
--- a/lib/ubsan/ubsan_handlers.cc
+++ b/lib/ubsan/ubsan_handlers.cc
@@ -279,3 +279,22 @@ void __ubsan::__ubsan_handle_function_type_mismatch_abort(
__ubsan_handle_function_type_mismatch(Data, Function);
Die();
}
+
+void __ubsan::__ubsan_handle_pointer_overflow(PointerOverflowData *Data,
+ ValueHandle Base,
+ ValueHandle Result) {
+ SourceLocation Loc = Data-Loc.acquire();
+ if (Loc.isDisabled())
+return;
+
+ Diag(Loc, DL_Error,
+ pointer index expression with base %0 overflowed to %1)
+ (void *)Base (void*)Result;
+}
+
+void __ubsan::__ubsan_handle_pointer_overflow_abort(PointerOverflowData *Data,
+ValueHandle Base,
+ValueHandle Result) {
+ __ubsan_handle_pointer_overflow(Data, Base, Result);
+ Die();
+}
diff --git a/lib/ubsan/ubsan_handlers.h b/lib/ubsan/ubsan_handlers.h
index 14e6f04..d56bfa7 100644
--- a/lib/ubsan/ubsan_handlers.h
+++ b/lib/ubsan/ubsan_handlers.h
@@ -121,6 +121,12 @@ RECOVERABLE(function_type_mismatch,
FunctionTypeMismatchData *Data,
ValueHandle Val)
+struct PointerOverflowData {
+ SourceLocation Loc;
+};
+
+RECOVERABLE(pointer_overflow, PointerOverflowData *Data, ValueHandle Base,
+ValueHandle Result)
}
#endif // UBSAN_HANDLERS_H
--
1.8.4.1
From 2cd6f1c679864ffc88a8b258d23ef1c5836cbce1 Mon Sep 17 00:00:00 2001
From: Will Dietz w...@wdtz.org
Date: Sun, 27 Oct 2013 14:53:17 -0500
Subject: [PATCH] Add 'pointer-overflow' sanitizer for undefined overflow in GEP's.
Handles things like: char *p = NULL; --p;.
---
include/clang/Basic/Sanitizers.def |9 ++--
lib/CodeGen/CGExpr.cpp | 93 +++-
lib/CodeGen/CGExprAgg.cpp |7 +++
lib/CodeGen/CGExprCXX.cpp |3 +
lib/CodeGen/CGExprScalar.cpp | 21 ++--
lib/CodeGen/CodeGenFunction.h |3 +
test/Driver/fsanitize.c|6 +-
7 files changed, 128 insertions(+), 14 deletions(-)
diff --git a/include/clang/Basic/Sanitizers.def b/include/clang/Basic/Sanitizers.def
index c9b31a3..188e62f 100644
--- a/include/clang/Basic/Sanitizers.def
+++ b/include/clang/Basic/Sanitizers.def
@@ -68,6 +68,7 @@ SANITIZER(function, Function)
SANITIZER(integer-divide-by-zero, IntegerDivideByZero)
SANITIZER(null, Null)
SANITIZER(object-size, ObjectSize)
+SANITIZER(pointer-overflow, PointerOverflow)
SANITIZER(return, Return)
SANITIZER(shift, Shift)
SANITIZER(signed-integer-overflow, SignedIntegerOverflow)
@@ -86,8 +87,8 @@ SANITIZER(dataflow, DataFlow)
SANITIZER_GROUP(undefined, Undefined,
Alignment | Bool | ArrayBounds | Enum | FloatCastOverflow |
FloatDivideByZero | Function | IntegerDivideByZero | Null |
-ObjectSize | Return | Shift | SignedIntegerOverflow |
-Unreachable | VLABound | Vptr)
+ObjectSize | PointerOverflow | Return | Shift |
+SignedIntegerOverflow | Unreachable | VLABound | Vptr)
// -fsanitize=undefined-trap
Re: [WIP][PATCH] pointer-overflow sanitizer
Glad there's some interest. I have no test coverage of anything other than the Driver component, that will be included. I also need to do some plumbing work to support adding checks to struct indexing. I've tried this on: * LLVM/Clang * ImageMagick * binutils * curl * ffmpeg (w/FATE samples) * openldap * openssh * pcre * postgresql * sqlite And the programs seem to build and at least pass their own non-trivial test-suites. So far detected bugs in: * binutils (what inspired this sanitizer) * clang (reported earlier today) * curl (unreported) * pcre (unreported) * ffmpeg (unreported) With a single bug location per software so far :). I also expect this to work particularly well with fuzz testing. ~Will On Mon, Oct 28, 2013 at 5:44 PM, Richard Smith rich...@metafoo.co.uk wrote: Seems like a nice idea to me. (Your test coverage is pretty weak, though.) Have you tried this much on large codebases? Does this find many bugs? (I can imagine it would be effective when combined with fuzz testing...) On Mon, Oct 28, 2013 at 3:39 PM, Will Dietz wdie...@illinois.edu wrote: Hi all, Recently I thought it would be useful to have a sanitizer for detecting overflows in pointer expressions. Such overflows are undefined behavior and are pretty much always bugs. While it's true that if such an overflowed pointer is dereferenced a tool such as ASan will catch the error, detection of these bugs when the occur helps fix them without requiring an input that triggers a crash. Two examples of this in the wild: * binutils undefined behavior bug that leads to segfault when built with clang[1] * ASTVector bug I just submitted patch for, discovered using this sanitizer[2] Attached are patches for clang and compiler-rt that implement this sanitizer and seem to work well in my testing so far. There is some work to do yet: * Adding lit tests to clang/compiler-rt * Finalizing what constructs are useful/worth checking (iterative process, I imagine) * More testing/benchmarking Before tackling the above, I was hoping to get some early feedback: * Is this something the community is interested in/would find useful? * Code review (the current implementation should be complete in terms of the checking code itself) Thank you for your time, here's to finding even more bugs! :) ~Will [1] http://wdtz.org/undefined-behavior-in-binutils-causes-segfault.html [2] http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20131028/091878.html ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[clang-tools-extra] r192713 - ModuleAssistant: Fix warning, don't return bool as a pointer.
Author: wdietz2
Date: Tue Oct 15 10:45:00 2013
New Revision: 192713
URL: http://llvm.org/viewvc/llvm-project?rev=192713view=rev
Log:
ModuleAssistant: Fix warning, don't return bool as a pointer.
No functionality change intended.
Modified:
clang-tools-extra/trunk/modularize/ModuleAssistant.cpp
Modified: clang-tools-extra/trunk/modularize/ModuleAssistant.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/modularize/ModuleAssistant.cpp?rev=192713r1=192712r2=192713view=diff
==
--- clang-tools-extra/trunk/modularize/ModuleAssistant.cpp (original)
+++ clang-tools-extra/trunk/modularize/ModuleAssistant.cpp Tue Oct 15 10:45:00
2013
@@ -219,7 +219,7 @@ static Module *loadModuleDescriptions(
I != E; ++I) {
// Add as a module.
if (!addModuleDescription(RootModule, *I, HeaderPrefix, Dependencies))
- return false;
+ return NULL;
}
return RootModule;
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[PATCH] [ubsan] Emit single check for Shl
See attached. This is important to avoid warning twice on shifts that fail both checks, like 1 -1. The branching is done to avoid executing the second check's shift with invalid operands (poisoning the result), especially since we already know the shift is invalid. Thanks! ~Will 0001-ubsan-Emit-single-check-for-left-shift.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
r176056 - [ubsan] Emit single check for left shift.
Author: wdietz2
Date: Mon Feb 25 16:37:49 2013
New Revision: 176056
URL: http://llvm.org/viewvc/llvm-project?rev=176056view=rev
Log:
[ubsan] Emit single check for left shift.
Avoids warning twice on same shift.
Modified:
cfe/trunk/lib/CodeGen/CGExprScalar.cpp
cfe/trunk/test/CodeGen/catch-undef-behavior.c
cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp
Modified: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprScalar.cpp?rev=176056r1=176055r2=176056view=diff
==
--- cfe/trunk/lib/CodeGen/CGExprScalar.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Mon Feb 25 16:37:49 2013
@@ -2407,13 +2407,17 @@ Value *ScalarExprEmitter::EmitShl(const
if (CGF.SanOpts-Shift !CGF.getLangOpts().OpenCL
isallvm::IntegerType(Ops.LHS-getType())) {
llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, RHS);
-// FIXME: Emit the branching explicitly rather than emitting the check
-// twice.
-EmitBinOpCheck(Builder.CreateICmpULE(RHS, WidthMinusOne), Ops);
+llvm::Value *Valid = Builder.CreateICmpULE(RHS, WidthMinusOne);
if (Ops.Ty-hasSignedIntegerRepresentation()) {
+ llvm::BasicBlock *Orig = Builder.GetInsertBlock();
+ llvm::BasicBlock *Cont = CGF.createBasicBlock(cont);
+ llvm::BasicBlock *CheckBitsShifted = CGF.createBasicBlock(check);
+ Builder.CreateCondBr(Valid, CheckBitsShifted, Cont);
+
// Check whether we are shifting any non-zero bits off the top of the
// integer.
+ CGF.EmitBlock(CheckBitsShifted);
llvm::Value *BitsShiftedOff =
Builder.CreateLShr(Ops.LHS,
Builder.CreateSub(WidthMinusOne, RHS, shl.zeros,
@@ -2428,8 +2432,15 @@ Value *ScalarExprEmitter::EmitShl(const
BitsShiftedOff = Builder.CreateLShr(BitsShiftedOff, One);
}
llvm::Value *Zero = llvm::ConstantInt::get(BitsShiftedOff-getType(), 0);
- EmitBinOpCheck(Builder.CreateICmpEQ(BitsShiftedOff, Zero), Ops);
+ llvm::Value *SecondCheck = Builder.CreateICmpEQ(BitsShiftedOff, Zero);
+ CGF.EmitBlock(Cont);
+ llvm::PHINode *P = Builder.CreatePHI(Valid-getType(), 2);
+ P-addIncoming(Valid, Orig);
+ P-addIncoming(SecondCheck, CheckBitsShifted);
+ Valid = P;
}
+
+EmitBinOpCheck(Valid, Ops);
}
// OpenCL 6.3j: shift values are effectively % word size of LHS.
if (CGF.getLangOpts().OpenCL)
Modified: cfe/trunk/test/CodeGen/catch-undef-behavior.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/catch-undef-behavior.c?rev=176056r1=176055r2=176056view=diff
==
--- cfe/trunk/test/CodeGen/catch-undef-behavior.c (original)
+++ cfe/trunk/test/CodeGen/catch-undef-behavior.c Mon Feb 25 16:37:49 2013
@@ -8,8 +8,7 @@
// FIXME: When we only emit each type once, use [[INT]] more below.
// CHECK: @[[LINE_100:.*]] = private unnamed_addr global {{.*}}, i32 100, i32
5 {{.*}} @[[INT]], i64 4, i8 1
// CHECK: @[[LINE_200:.*]] = {{.*}}, i32 200, i32 10 {{.*}}, i64 4, i8 0
-// CHECK: @[[LINE_300_A:.*]] = {{.*}}, i32 300, i32 12 {{.*}} @{{.*}}, {{.*}}
@{{.*}}
-// CHECK: @[[LINE_300_B:.*]] = {{.*}}, i32 300, i32 12 {{.*}} @{{.*}}, {{.*}}
@{{.*}}
+// CHECK: @[[LINE_300:.*]] = {{.*}}, i32 300, i32 12 {{.*}} @{{.*}}, {{.*}}
@{{.*}}
// CHECK: @[[LINE_400:.*]] = {{.*}}, i32 400, i32 12 {{.*}} @{{.*}}, {{.*}}
@{{.*}}
// CHECK: @[[LINE_500:.*]] = {{.*}}, i32 500, i32 10 {{.*}} @{{.*}}, i64 4, i8
0 }
// CHECK: @[[LINE_600:.*]] = {{.*}}, i32 600, i32 3 {{.*}} @{{.*}}, i64 4, i8
1 }
@@ -106,35 +105,36 @@ int addr_space(int __attribute__((addres
// CHECK-TRAP: @lsh_overflow
int lsh_overflow(int a, int b) {
// CHECK: %[[INBOUNDS:.*]] = icmp ule i32 %[[RHS:.*]], 31
- // CHECK-NEXT: br i1 %[[INBOUNDS]]
+ // CHECK-NEXT: br i1 %[[INBOUNDS]], label %[[CHECKBB:.*]], label
%[[CONTBB:.*]]
// CHECK-TRAP: %[[INBOUNDS:.*]] = icmp ule i32 %[[RHS:.*]], 31
- // CHECK-TRAP-NEXT: br i1 %[[INBOUNDS]]
-
- // FIXME: Only emit one trap block here.
- // CHECK: %[[ARG1:.*]] = zext
- // CHECK-NEXT: %[[ARG2:.*]] = zext
- // CHECK-NEXT: call void @__ubsan_handle_shift_out_of_bounds(i8* bitcast
({{.*}} @[[LINE_300_A]] to i8*), i64 %[[ARG1]], i64 %[[ARG2]])
-
- // CHECK-TRAP: call void @llvm.trap() [[NR_NUW]]
- // CHECK-TRAP-NEXT: unreachable
+ // CHECK-TRAP-NEXT: br i1 %[[INBOUNDS]], label %[[CHECKBB:.*]], label
%[[CONTBB:.*]]
// CHECK: %[[SHIFTED_OUT_WIDTH:.*]] = sub nuw nsw i32 31, %[[RHS]]
// CHECK-NEXT: %[[SHIFTED_OUT:.*]] = lshr i32 %[[LHS:.*]],
%[[SHIFTED_OUT_WIDTH]]
// CHECK-NEXT: %[[NO_OVERFLOW:.*]] = icmp eq i32 %[[SHIFTED_OUT]], 0
- // CHECK-NEXT: br i1 %[[NO_OVERFLOW]], {{.*}} !prof ![[WEIGHT_MD]]
+ // CHECK-NEXT: br label %[[CONTBB]]
// CHECK-TRAP: %[[SHIFTED_OUT_WIDTH:.*]] = sub nuw nsw i32 31,
Re: [PATCH] [ubsan] Emit single check for Shl
Great, thanks for taking a look. Didn't know about omitting labels in -Asserts, updated tests accordingly and committed as r176056. (After confirming works on an -Asserts build) Thanks! ~Will On Mon, Feb 25, 2013 at 3:51 PM, Richard Smith rich...@metafoo.co.uk wrote: On Mon, Feb 25, 2013 at 1:02 PM, Will Dietz wdie...@illinois.edu wrote: See attached. This is important to avoid warning twice on shifts that fail both checks, like 1 -1. The branching is done to avoid executing the second check's shift with invalid operands (poisoning the result), especially since we already know the shift is invalid. Thanks, this generally looks good, except that you shouldn't test for labels in the IR, since we don't emit them in -Asserts builds. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] -fcatch-undefined-behavior with trapping implementation
On Mon, Jan 28, 2013 at 7:10 PM, Richard Smith rich...@metafoo.co.uk wrote: On Mon, Jan 28, 2013 at 5:01 PM, Will Dietz wdie...@uiuc.edu wrote: Glad this is going in, although I would prefer to see compiler-rt become more widely used (shipped by default, etc) instead. However, that's not the case yet and it's good to make these checks available to users that either don't want to or can't use it (kernel work, no compiler-rt readily available, etc). One thought: is there anything to be done to better clarify the different roles played by -f[no-]sanitize-recover and -f[no]-sanitize-undefined-trap-on-error? This seems like it could be awfully confusing. Would -fsanitize-undefined-standalone or similar fit this and better identify the reasons one might use the -fsanitize-undefined-trap-on-error flag? I like the synergy between -fsanitize=undefined-trap and -fsanitize-undefined-trap-on-error, but if you can suggest an equally harmonious pair of option names which better explain the purpose, that'd be great! Hmm, nope I don't have a better naming suggestion. Thanks, carry on! :) ~Will ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [PATCH] -fcatch-undefined-behavior with trapping implementation
Glad this is going in, although I would prefer to see compiler-rt
become more widely used (shipped by default, etc) instead. However,
that's not the case yet and it's good to make these checks available
to users that either don't want to or can't use it (kernel work, no
compiler-rt readily available, etc).
One thought: is there anything to be done to better clarify the
different roles played by -f[no-]sanitize-recover and
-f[no]-sanitize-undefined-trap-on-error? This seems like it could be
awfully confusing. Would -fsanitize-undefined-standalone or similar
fit this and better identify the reasons one might use the
-fsanitize-undefined-trap-on-error flag?
~Will
On Mon, Jan 28, 2013 at 6:24 PM, Richard Smith rich...@metafoo.co.uk wrote:
On Mon, Jan 28, 2013 at 3:58 PM, Chad Rosier mcros...@apple.com wrote:
All,
The attached patch implements the -fcatch-undefined-behavior flag using a
trapping implementation; this is much more inline with the original
implementation (i.e., pre-Ubsan) and does not require run-time library
support.
I've added a the 'undefined-trap' SANITIZER_GROUP that includes all the
sanitizers which have low overhead, no ABI or address space layout
implications, only catch undefined behavior, *and* do not require run-time
library support. This group should be used in conjunction with the new
-fsanitize-undefined-trap-on-error flag. Thus, the trapping implementation
can be invoked using either '-fcatch-undefined-behavior' or
'-fsanitize=undefined-trap -fsanitize-undefined-trap-on-error'. Per a
discussion between Richard, Chandler, and I, we are going to continue
deprecating the -fcatch-undefined-behavior flag, but don't expect the flag
to be removed until after a few releases.
Please take a look.
Thanks to Richard for a great deal of feedback prior to my submission to the
list.
Patch file seems to have DOS line endings?
+// RUN: %clang -target x86_64-linux-gnu -fcatch-undefined-behavior
-fno-sanitize-undefined-trap-on-error %s -### 21 | FileCheck %s
--check-prefix=CHECK-UNDEFINED-NO-TRAP-ERROR
+// CHECK-UNDEFINED-NO-TRAP-ERROR: '-fcatch-undefined-behavior' not
allowed with '-fno-sanitize-undefined-trap-on-error'
OK, so -fcatch-undefined-behavior isn't exactly equivalent to
-fsanitize=undefined-trap -fsanitize-undefined-trap-on-error? (Its
-fsanitize= effect can be reversed by later command-line options, but
its -fsanitize-undefined-trap-on-error effect cannot be.) That's
probably fine, since we don't have a -fno-catch-undefined-behavior,
but might be a little surprising.
+++ include/clang/Basic/Sanitizers.def (working copy)
@@ -74,15 +74,24 @@
// IntegerSanitizer
SANITIZER(unsigned-integer-overflow, UnsignedIntegerOverflow)
-// -fsanitize=undefined (and its alias -fcatch-undefined-behavior). This
should
-// include all the sanitizers which have low overhead, no ABI or address
space
-// layout implications, and only catch undefined behavior.
+// -fsanitize=undefined include all the sanitizers which have low overhead,
no
+// ABI or address space layout implications, and only catch undefined
behavior.
Typo, -fsanitize=undefined include*s* all [...]
+++ lib/Driver/Tools.cpp(working copy)
[...]
+ if (UbsanTrapOnError) {
+// Warn about undefined sanitizer options that require runtime support.
+if (Kind Vptr) {
+ if (Args.hasArg(options::OPT_fcatch_undefined_behavior))
+D.Diag(diag::err_drv_argument_not_allowed_with)
+ -fsanitize=undefined|vptr -fcatch-undefined-behavior;
+ if (Args.hasFlag(options::OPT_fsanitize_undefined_trap_on_error,
else if?
+
options::OPT_fno_sanitize_undefined_trap_on_error, false))
+D.Diag(diag::err_drv_argument_not_allowed_with)
+ -fsanitize=undefined|vptr
-fsanitize-undefined-trap-on-error;
+}
+ }
Use SanitizerArgs::lastArgumentForKind(..., Vptr) to find which
argument to print instead of using -fsanitize=undefined|vptr here.
Also, please add a named symbolic constant for things which are
incompatible with -fsanitize-undefined-trap-on-error, rather than
hardcoding Vptr.
+++ lib/CodeGen/CGExpr.cpp (working copy)
@@ -1975,6 +1975,13 @@
ArrayRefllvm::Value * DynamicArgs,
CheckRecoverableKind RecoverKind) {
assert(SanOpts != SanitizerOptions::Disabled);
+
+ if (CGM.getCodeGenOpts().SanitizeUndefinedTrapOnError) {
+assert (RecoverKind != CRK_AlwaysRecoverable
+Runtime call required for AlwaysRecoverable kind!);
+return EmitTrapvCheck(Checked);
Please rename this to something more general (maybe just remove the 'v'?)
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
Re: [cfe-commits] [PATCH] [ubsan] Add support for -fsanitize-blacklist
Sorry about that, attached is a patch that should fix the tests. Can you confirm that this fixes the failures you're seeing? Thanks! ~Will On Fri, Jan 18, 2013 at 9:12 PM, NAKAMURA Takumi geek4ci...@gmail.com wrote: It behaves unexpectedly to pass src:x:\path\to\file.cpp in the blacklist, due to try to match whole DOSish path without escaping with Regex. Two tests have been suppressed on win32 since r172820, FYI. ...Takumi 2013/1/18 Will Dietz wdie...@illinois.edu: Committed as r172806 (llvm) and r172808 (clang). Thanks! ~Will On Fri, Jan 18, 2013 at 2:28 AM, Alexey Samsonov samso...@google.com wrote: LGTM. Thanks! On Fri, Jan 18, 2013 at 2:28 AM, Richard Smith rich...@metafoo.co.uk wrote: Clang side looks great to me, thanks! Alexey: Does this look OK from your end? On Thu, Jan 17, 2013 at 8:53 AM, Will Dietz wdie...@illinois.edu wrote: Great, thank you both for your feedback. I also agree the approach originally submitted was messy, and Richard you nailed the conflict that drove me to accept it. Thank you for giving me a better alternative :). Updated patches attached. Notes: * It was unclear to what IR clang should generate for a blacklisted function/module when using address sanitizer. I've added a test verifying a blacklisted function does /not/ get given the address-safety attribute, which is a deviation from existing functionality. Looking at the Asan LLVM transform suggests this is fine (and seems like the right thing to do from clang's perspective regardless), but would appreciate confirmation this is correct. * I extended the AST serialization code to write out the sanitization flags, only because that's what the code does presently. Note that all users of ASTReader seem to ignore benign language options (such as sanitization flags). One result is that AFAICT testing this would require creating additional code to dump LangOpts or similar. Is this worth pursuing? * At the time of CodeGenFunction construction we presently do not give the ctor sufficient information to query the blacklist (function name), so a conditionally bound reference didn't work. Using a pointer seemed preferable to the refactoring required to alleviate this issue. Thanks! ~Will On Thu, Jan 17, 2013 at 2:24 AM, Alexey Samsonov samso...@google.com wrote: I think that making blacklist available in Clang is a good idea. I agree with Richard's comment as well. On Thu, Jan 17, 2013 at 12:50 AM, Richard Smith rich...@metafoo.co.uk wrote: On the clang side, it seems messy and error-prone to distribute the checking for blacklisting across all the callers of EmitCheck, but I understand that you're trying to avoid generating the check condition for blacklisted functions. Maybe we could make a separate struct containing just the Sanitize* flags, and in the CodeGenFunction constructor bind a reference to either the global set of flags or a set of all-false flags, depending on whether we're blacklisted? This would also make it easier to blacklist specific sanitizers in specific functions later. On Wed, Jan 16, 2013 at 12:08 PM, Will Dietz wdie...@illinois.edu wrote: Minor touchup to clang patch, thanks! ~Will On Wed, Jan 16, 2013 at 11:37 AM, Will Dietz wdie...@illinois.edu wrote: Attached are two patches. First applies to clang and uses the blacklist to avoid instrumenting the source files or functions specified. Lit test included. The other is a small change to llvm to make the Blacklist class visible to Clang. Not sure I understand the header organization system well enough, let me know if it belongs elsewhere. Thanks! ~Will ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits -- Alexey Samsonov, MSK -- Alexey Samsonov, MSK ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits 0001-Use-wildcards-to-match-paths-in-tests-using-fsanitiz.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] r172808 - in /cfe/trunk: include/clang/Basic/ lib/Basic/ lib/CodeGen/ lib/Frontend/ lib/Lex/ lib/Serialization/ test/CodeGen/
Author: wdietz2
Date: Fri Jan 18 05:30:38 2013
New Revision: 172808
URL: http://llvm.org/viewvc/llvm-project?rev=172808view=rev
Log:
[ubsan] Add support for -fsanitize-blacklist
Added:
cfe/trunk/test/CodeGen/ubsan-blacklist.c
Modified:
cfe/trunk/include/clang/Basic/LangOptions.def
cfe/trunk/include/clang/Basic/LangOptions.h
cfe/trunk/lib/Basic/LangOptions.cpp
cfe/trunk/lib/Basic/Targets.cpp
cfe/trunk/lib/CodeGen/BackendUtil.cpp
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/CGExprScalar.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/lib/Lex/PPMacroExpansion.cpp
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/CodeGen/address-safety-attr.cpp
Modified: cfe/trunk/include/clang/Basic/LangOptions.def
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=172808r1=172807r2=172808view=diff
==
--- cfe/trunk/include/clang/Basic/LangOptions.def (original)
+++ cfe/trunk/include/clang/Basic/LangOptions.def Fri Jan 18 05:30:38 2013
@@ -170,11 +170,6 @@
BENIGN_LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, retain documentation
comments from system headers in the AST)
-/// Runtime sanitizers.
-#define SANITIZER(NAME, ID) \
-BENIGN_LANGOPT(Sanitize##ID, 1, 0, NAME sanitizer)
-#include clang/Basic/Sanitizers.def
-
#undef LANGOPT
#undef VALUE_LANGOPT
#undef BENIGN_LANGOPT
Modified: cfe/trunk/include/clang/Basic/LangOptions.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.h?rev=172808r1=172807r2=172808view=diff
==
--- cfe/trunk/include/clang/Basic/LangOptions.h (original)
+++ cfe/trunk/include/clang/Basic/LangOptions.h Fri Jan 18 05:30:38 2013
@@ -23,6 +23,14 @@
namespace clang {
+struct SanitizerOptions {
+#define SANITIZER(NAME, ID) unsigned ID : 1;
+#include clang/Basic/Sanitizers.def
+
+ /// \brief Cached set of sanitizer options with all sanitizers disabled.
+ static const SanitizerOptions Disabled;
+};
+
/// Bitfields of LangOptions, split out from LangOptions in order to ensure
that
/// this large collection of bitfields is a trivial class type.
class LangOptionsBase {
@@ -32,6 +40,7 @@
#define ENUM_LANGOPT(Name, Type, Bits, Default, Description)
#include clang/Basic/LangOptions.def
+ SanitizerOptions Sanitize;
protected:
// Define language options of enumeration type. These are private, and will
// have accessors (below).
Modified: cfe/trunk/lib/Basic/LangOptions.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/LangOptions.cpp?rev=172808r1=172807r2=172808view=diff
==
--- cfe/trunk/lib/Basic/LangOptions.cpp (original)
+++ cfe/trunk/lib/Basic/LangOptions.cpp Fri Jan 18 05:30:38 2013
@@ -14,10 +14,14 @@
using namespace clang;
+const SanitizerOptions SanitizerOptions::Disabled = {};
+
LangOptions::LangOptions() {
#define LANGOPT(Name, Bits, Default, Description) Name = Default;
#define ENUM_LANGOPT(Name, Type, Bits, Default, Description)
set##Name(Default);
#include clang/Basic/LangOptions.def
+
+ Sanitize = SanitizerOptions::Disabled;
}
void LangOptions::resetNonModularOptions() {
@@ -26,7 +30,9 @@
#define BENIGN_ENUM_LANGOPT(Name, Type, Bits, Default, Description) \
Name = Default;
#include clang/Basic/LangOptions.def
-
+
+ Sanitize = SanitizerOptions::Disabled;
+
CurrentModule.clear();
}
Modified: cfe/trunk/lib/Basic/Targets.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=172808r1=172807r2=172808view=diff
==
--- cfe/trunk/lib/Basic/Targets.cpp (original)
+++ cfe/trunk/lib/Basic/Targets.cpp Fri Jan 18 05:30:38 2013
@@ -94,7 +94,7 @@
Builder.defineMacro(OBJC_NEW_PROPERTIES);
// AddressSanitizer doesn't play well with source fortification, which is on
// by default on Darwin.
- if (Opts.SanitizeAddress) Builder.defineMacro(_FORTIFY_SOURCE, 0);
+ if (Opts.Sanitize.Address) Builder.defineMacro(_FORTIFY_SOURCE, 0);
if (!Opts.ObjCAutoRefCount) {
// __weak is always defined, for use in blocks and with objc pointers.
Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=172808r1=172807r2=172808view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++
Re: [cfe-commits] [PATCH] [ubsan] Add support for -fsanitize-blacklist
Committed as r172806 (llvm) and r172808 (clang). Thanks! ~Will On Fri, Jan 18, 2013 at 2:28 AM, Alexey Samsonov samso...@google.com wrote: LGTM. Thanks! On Fri, Jan 18, 2013 at 2:28 AM, Richard Smith rich...@metafoo.co.uk wrote: Clang side looks great to me, thanks! Alexey: Does this look OK from your end? On Thu, Jan 17, 2013 at 8:53 AM, Will Dietz wdie...@illinois.edu wrote: Great, thank you both for your feedback. I also agree the approach originally submitted was messy, and Richard you nailed the conflict that drove me to accept it. Thank you for giving me a better alternative :). Updated patches attached. Notes: * It was unclear to what IR clang should generate for a blacklisted function/module when using address sanitizer. I've added a test verifying a blacklisted function does /not/ get given the address-safety attribute, which is a deviation from existing functionality. Looking at the Asan LLVM transform suggests this is fine (and seems like the right thing to do from clang's perspective regardless), but would appreciate confirmation this is correct. * I extended the AST serialization code to write out the sanitization flags, only because that's what the code does presently. Note that all users of ASTReader seem to ignore benign language options (such as sanitization flags). One result is that AFAICT testing this would require creating additional code to dump LangOpts or similar. Is this worth pursuing? * At the time of CodeGenFunction construction we presently do not give the ctor sufficient information to query the blacklist (function name), so a conditionally bound reference didn't work. Using a pointer seemed preferable to the refactoring required to alleviate this issue. Thanks! ~Will On Thu, Jan 17, 2013 at 2:24 AM, Alexey Samsonov samso...@google.com wrote: I think that making blacklist available in Clang is a good idea. I agree with Richard's comment as well. On Thu, Jan 17, 2013 at 12:50 AM, Richard Smith rich...@metafoo.co.uk wrote: On the clang side, it seems messy and error-prone to distribute the checking for blacklisting across all the callers of EmitCheck, but I understand that you're trying to avoid generating the check condition for blacklisted functions. Maybe we could make a separate struct containing just the Sanitize* flags, and in the CodeGenFunction constructor bind a reference to either the global set of flags or a set of all-false flags, depending on whether we're blacklisted? This would also make it easier to blacklist specific sanitizers in specific functions later. On Wed, Jan 16, 2013 at 12:08 PM, Will Dietz wdie...@illinois.edu wrote: Minor touchup to clang patch, thanks! ~Will On Wed, Jan 16, 2013 at 11:37 AM, Will Dietz wdie...@illinois.edu wrote: Attached are two patches. First applies to clang and uses the blacklist to avoid instrumenting the source files or functions specified. Lit test included. The other is a small change to llvm to make the Blacklist class visible to Clang. Not sure I understand the header organization system well enough, let me know if it belongs elsewhere. Thanks! ~Will ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits -- Alexey Samsonov, MSK -- Alexey Samsonov, MSK ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] [PATCH] [ubsan] Add support for -fsanitize-blacklist
Attached are two patches. First applies to clang and uses the blacklist to avoid instrumenting the source files or functions specified. Lit test included. The other is a small change to llvm to make the Blacklist class visible to Clang. Not sure I understand the header organization system well enough, let me know if it belongs elsewhere. Thanks! ~Will 0001-ubsan-Add-support-for-fsanitize-blacklist.patch Description: Binary data 0001-Move-Blacklist.h-to-include-to-enable-use-from-clang.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [PATCH] [ubsan] Add support for -fsanitize-blacklist
Minor touchup to clang patch, thanks! ~Will On Wed, Jan 16, 2013 at 11:37 AM, Will Dietz wdie...@illinois.edu wrote: Attached are two patches. First applies to clang and uses the blacklist to avoid instrumenting the source files or functions specified. Lit test included. The other is a small change to llvm to make the Blacklist class visible to Clang. Not sure I understand the header organization system well enough, let me know if it belongs elsewhere. Thanks! ~Will 0001-ubsan-Add-support-for-fsanitize-blacklist.patch Description: Binary data 0001-Move-Blacklist.h-to-include-to-enable-use-from-clang.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [Review] [ubsan] Deduplication
On Mon, Jan 14, 2013 at 1:27 AM, Alexey Samsonov samso...@google.com wrote:
On Wed, Jan 9, 2013 at 6:01 AM, Will Dietz wdie...@illinois.edu wrote:
On Tue, Jan 8, 2013 at 3:02 PM, Richard Smith rich...@metafoo.co.uk
wrote:
On Tue, Jan 8, 2013 at 12:21 AM, Will Dietz wdie...@illinois.edu
wrote:
Thanks for taking a look. Responses inline.
On Mon, Jan 7, 2013 at 10:16 PM, Richard Smith rich...@metafoo.co.uk
wrote:
Thanks! This patch is highlighting a pressing need to refactor out the
repetition in ubsan_handlers.cc...
Heh, yep. One day... :).
The atomic compare-exchange isn't sufficient to remove the data race,
since the copy to the local Location object is non-atomic. How about
something like...
SourceLocation SourceLocation::acquire() {
return SourceLocation(Filename, Line,
__sync_val_compare_and_swap(Column, Column, ~u32(0)));
Oops, this is actually wrong, too, since the load of Column is
non-atomic. What we actually want here is
__atomic_exchange_n(Column, ~u32(0), __ATOMIC_RELAXED)
... and this should be much quicker than __sync_* on some platforms,
since it's not a synchronization operation. The downside is that this
is pretty new (g++ 4.7 and Clang 3.1), so we should wrap it in #ifdef
__ATOMIC_RELAXED and use the __sync_ builtin otherwise.
Sounds good to me, and thank you for your explanation! Certainly
clears things up about the UB.
Accordingly, I've removed all racing accesses to the Column field
(including the misguided optimization) and
switched to using __sync_lock_test_and_set when __atomic_exchange_n is
unavailable. This avoids the UB load and avoids the need for a full
barrier (or a compare) as compared to the CAS approach. TAS is
guaranteed at least acquire semantics, which is more than we need but
sufficient (Clang however gives it seq_cst, not sure why).
Tricky business, thanks for helping me get this right! :)
Can you re-use atomic_exchange defined in sanitizer_common library? I took a
glance at
atomic_exchange in sanitizer_atomic_clang.h and it uses
__sync_lock_test_and_set.
Maybe, we should put optional use of __atomic_exchange_n there?
Was looking at this, went ahead and moved to shared impl in r172429, thanks.
Looks like using __atomic_exchange_n was already discussed and its use
postponed[1].
As I discovered with a previous revision, checking for __ATOMIC_* to
be defined is not sufficient on some platforms[2].
Not sure how to best test for support, some mix of
__GXX_EXPERIMENTAL_CXX0X__ and
__has_extension(cxx_atomic)/has_feature(cxx_atomic) seems like the
best we can do.
Thoughts? Perhaps not worth it?
~Will
[1] http://comments.gmane.org/gmane.comp.gcc.patches/277763
[2]
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130107/161480.html
}
bool SourceLocation::isDisabled() {
return Column == ~u32(0);
}
// ... then ...
SourceLocation Loc = Data-Loc.acquire();
if (Loc.isDisabled())
return;
I like how this forces the copying of the SourceLocation in order to
use the deduplication feature, something I didn't like about the
approach I used.
That said, I'm not sure I understand your concern about the local
SourceLocation copy. Would you mind briefly explaining? As far as I
can tell, the load used for the copy must happen before our own CAS,
and if the load in the local copy contains anything other than the
original pristine Column value then we'll see the full ~u32(0) value
in the CAS and discard the copy/loaded value without using it. I
suppose the question is if such a raced load is UB (unused or not,
this is wrong) or simply an 'undef' value (like uninitialized,
harmless if not used). My reading says it's 'undef', but it's quite
possible I'm missing something. Thoughts? Thanks!
In theory, C++11 [intro.multithread]p21: The execution of a program
contains a data race if it contains two conflicting actions in
different threads, at least one of which is not atomic, and neither
happens before the other. Any such data race results in
undefined behavior.
In practice, since the compiler knows that a non-atomic load of Column
dominates the atomic exchange of Column, it also knows there can be no
concurrent modifications, so it can transform the atomic exchange into
an atomic store, which defeats your attempt to emit only one
diagnostic. I'm not sure whether there are compilers which actually do
that (yet).
Again, thanks for the detailed explanation. Makes good sense, and
much appreciated! :)
Additionally, one intentional feature of the approach used in my patch
is to avoid the call to __sync_bool_compare_and_swap() in the
potentially 'common' case of the diagnostic indeed being disabled.
This helps performance on programs with many concurrent duplicate
overflows. Does this seem valid to you?
No, for the same reason. But __atomic_exchange_n might be faster than
a branch
[cfe-commits] [PATCH] ToolsChains: Minor touchup to use correct type, avoid truncation
Please see attached. ~Will 0001-ToolChains-Minor-touchup-to-use-correct-type-avoid-t.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [PATCH] ToolsChains: Minor touchup to use correct type, avoid truncation
Heh, agreed. r172127, thanks! ~Will On Thu, Jan 10, 2013 at 4:19 PM, Richard Smith rich...@metafoo.co.uk wrote: It would be alarming if this made a difference in practice, but LGTM On Thu, Jan 10, 2013 at 2:13 PM, Will Dietz wdie...@uiuc.edu wrote: Please see attached. ~Will ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] r172127 - /cfe/trunk/lib/Driver/ToolChains.cpp
Author: wdietz2
Date: Thu Jan 10 16:20:02 2013
New Revision: 172127
URL: http://llvm.org/viewvc/llvm-project?rev=172127view=rev
Log:
ToolChains: Minor touchup to use correct type, avoid truncation.
Truncation happens regularly when find_first_not_of returns npos,
strings long enough to trigger bug here are implausible.
No functionality change intended (ignoring absurd string lengths).
Modified:
cfe/trunk/lib/Driver/ToolChains.cpp
Modified: cfe/trunk/lib/Driver/ToolChains.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains.cpp?rev=172127r1=172126r2=172127view=diff
==
--- cfe/trunk/lib/Driver/ToolChains.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains.cpp Thu Jan 10 16:20:02 2013
@@ -952,7 +952,7 @@
// And retains any patch number it finds.
StringRef PatchText = GoodVersion.PatchSuffix = Second.second.str();
if (!PatchText.empty()) {
-if (unsigned EndNumber = PatchText.find_first_not_of(0123456789)) {
+if (size_t EndNumber = PatchText.find_first_not_of(0123456789)) {
// Try to parse the number and any suffix.
if (PatchText.slice(0, EndNumber).getAsInteger(10, GoodVersion.Patch) ||
GoodVersion.Patch 0)
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] r171801 - in /cfe/trunk: lib/CodeGen/CGExprScalar.cpp test/CodeGen/compound-assign-overflow.c
Sorry about that! Can you apply the attached patch (patch -p1 patchfile) and confirm this fixes the issue for you? Thanks! ~Will On Wed, Jan 9, 2013 at 8:08 AM, İsmail Dönmez ism...@donmez.ws wrote: Hi; On Mon, Jan 7, 2013 at 11:25 PM, Will Dietz wdie...@illinois.edu wrote: Author: wdietz2 Date: Mon Jan 7 16:25:52 2013 New Revision: 171801 URL: http://llvm.org/viewvc/llvm-project?rev=171801view=rev Log: [ubsan] Use correct type for compound assignment ops. Added: cfe/trunk/test/CodeGen/compound-assign-overflow.c This test seems to fail on openSUSE 12.2 32bit because... Added: cfe/trunk/test/CodeGen/compound-assign-overflow.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/compound-assign-overflow.c?rev=171801view=auto == --- cfe/trunk/test/CodeGen/compound-assign-overflow.c (added) +++ cfe/trunk/test/CodeGen/compound-assign-overflow.c Mon Jan 7 16:25:52 2013 @@ -0,0 +1,36 @@ +// Verify proper type emitted for compound assignments +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=signed-integer-overflow,unsigned-integer-overflow | FileCheck %s ^^ We have an 64bit target here. + +#include stdint.h This includes features.h which in turn includes gnu/stubs.h which says: #if defined __x86_64__ defined __LP64__ # include gnu/stubs-64.h #endif Since the target is 64bit it tries to include gnu/stubs-64.h but it doesn't exist on 32bit host. Maybe XFAIL on 32bit? Regards. 0001-Drop-use-of-stdint.h-in-test-to-fix-on-platforms-wit.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] r171801 - in /cfe/trunk: lib/CodeGen/CGExprScalar.cpp test/CodeGen/compound-assign-overflow.c
Poor internet here, my apologies for skipping a message in the thread. Can you check if this still fails on trunk as Dmitri suggested? Thanks! ~Will On Wed, Jan 9, 2013 at 1:48 PM, Will Dietz wdie...@illinois.edu wrote: Sorry about that! Can you apply the attached patch (patch -p1 patchfile) and confirm this fixes the issue for you? Thanks! ~Will On Wed, Jan 9, 2013 at 8:08 AM, İsmail Dönmez ism...@donmez.ws wrote: Hi; On Mon, Jan 7, 2013 at 11:25 PM, Will Dietz wdie...@illinois.edu wrote: Author: wdietz2 Date: Mon Jan 7 16:25:52 2013 New Revision: 171801 URL: http://llvm.org/viewvc/llvm-project?rev=171801view=rev Log: [ubsan] Use correct type for compound assignment ops. Added: cfe/trunk/test/CodeGen/compound-assign-overflow.c This test seems to fail on openSUSE 12.2 32bit because... Added: cfe/trunk/test/CodeGen/compound-assign-overflow.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/compound-assign-overflow.c?rev=171801view=auto == --- cfe/trunk/test/CodeGen/compound-assign-overflow.c (added) +++ cfe/trunk/test/CodeGen/compound-assign-overflow.c Mon Jan 7 16:25:52 2013 @@ -0,0 +1,36 @@ +// Verify proper type emitted for compound assignments +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=signed-integer-overflow,unsigned-integer-overflow | FileCheck %s ^^ We have an 64bit target here. + +#include stdint.h This includes features.h which in turn includes gnu/stubs.h which says: #if defined __x86_64__ defined __LP64__ # include gnu/stubs-64.h #endif Since the target is 64bit it tries to include gnu/stubs-64.h but it doesn't exist on 32bit host. Maybe XFAIL on 32bit? Regards. ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [Review] [ubsan] Deduplication
Thanks for taking a look. Responses inline.
On Mon, Jan 7, 2013 at 10:16 PM, Richard Smith rich...@metafoo.co.uk wrote:
Thanks! This patch is highlighting a pressing need to refactor out the
repetition in ubsan_handlers.cc...
Heh, yep. One day... :).
The atomic compare-exchange isn't sufficient to remove the data race,
since the copy to the local Location object is non-atomic. How about
something like...
SourceLocation SourceLocation::acquire() {
return SourceLocation(Filename, Line,
__sync_val_compare_and_swap(Column, Column, ~u32(0)));
}
bool SourceLocation::isDisabled() {
return Column == ~u32(0);
}
// ... then ...
SourceLocation Loc = Data-Loc.acquire();
if (Loc.isDisabled())
return;
I like how this forces the copying of the SourceLocation in order to
use the deduplication feature, something I didn't like about the
approach I used.
That said, I'm not sure I understand your concern about the local
SourceLocation copy. Would you mind briefly explaining? As far as I
can tell, the load used for the copy must happen before our own CAS,
and if the load in the local copy contains anything other than the
original pristine Column value then we'll see the full ~u32(0) value
in the CAS and discard the copy/loaded value without using it. I
suppose the question is if such a raced load is UB (unused or not,
this is wrong) or simply an 'undef' value (like uninitialized,
harmless if not used). My reading says it's 'undef', but it's quite
possible I'm missing something. Thoughts? Thanks!
Additionally, one intentional feature of the approach used in my patch
is to avoid the call to __sync_bool_compare_and_swap() in the
potentially 'common' case of the diagnostic indeed being disabled.
This helps performance on programs with many concurrent duplicate
overflows. Does this seem valid to you? It's the primary reason for
having any of the non-atomic accesses to the Columns field in this
code.
Thank you for your time,
~Will
On Mon, Jan 7, 2013 at 3:17 PM, Will Dietz wdie...@illinois.edu wrote:
Updated, slightly neater patches attached.
Thanks!
~Will
On Tue, Jan 1, 2013 at 9:49 PM, Will Dietz wdie...@illinois.edu wrote:
Updated to apply cleanly to latest clang/compiler-rt.
Thanks!
~Will
On Sun, Dec 30, 2012 at 6:22 PM, Will Dietz wdie...@illinois.edu wrote:
(Moving to cfe-commits@, was previously:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-December/026519.html)
Please see attached! :)
Thank you,
~Will
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [Review] [ubsan] Deduplication
On Tue, Jan 8, 2013 at 3:02 PM, Richard Smith rich...@metafoo.co.uk wrote:
On Tue, Jan 8, 2013 at 12:21 AM, Will Dietz wdie...@illinois.edu wrote:
Thanks for taking a look. Responses inline.
On Mon, Jan 7, 2013 at 10:16 PM, Richard Smith rich...@metafoo.co.uk wrote:
Thanks! This patch is highlighting a pressing need to refactor out the
repetition in ubsan_handlers.cc...
Heh, yep. One day... :).
The atomic compare-exchange isn't sufficient to remove the data race,
since the copy to the local Location object is non-atomic. How about
something like...
SourceLocation SourceLocation::acquire() {
return SourceLocation(Filename, Line,
__sync_val_compare_and_swap(Column, Column, ~u32(0)));
Oops, this is actually wrong, too, since the load of Column is
non-atomic. What we actually want here is
__atomic_exchange_n(Column, ~u32(0), __ATOMIC_RELAXED)
... and this should be much quicker than __sync_* on some platforms,
since it's not a synchronization operation. The downside is that this
is pretty new (g++ 4.7 and Clang 3.1), so we should wrap it in #ifdef
__ATOMIC_RELAXED and use the __sync_ builtin otherwise.
Sounds good to me, and thank you for your explanation! Certainly
clears things up about the UB.
Accordingly, I've removed all racing accesses to the Column field
(including the misguided optimization) and
switched to using __sync_lock_test_and_set when __atomic_exchange_n is
unavailable. This avoids the UB load and avoids the need for a full
barrier (or a compare) as compared to the CAS approach. TAS is
guaranteed at least acquire semantics, which is more than we need but
sufficient (Clang however gives it seq_cst, not sure why).
Tricky business, thanks for helping me get this right! :)
}
bool SourceLocation::isDisabled() {
return Column == ~u32(0);
}
// ... then ...
SourceLocation Loc = Data-Loc.acquire();
if (Loc.isDisabled())
return;
I like how this forces the copying of the SourceLocation in order to
use the deduplication feature, something I didn't like about the
approach I used.
That said, I'm not sure I understand your concern about the local
SourceLocation copy. Would you mind briefly explaining? As far as I
can tell, the load used for the copy must happen before our own CAS,
and if the load in the local copy contains anything other than the
original pristine Column value then we'll see the full ~u32(0) value
in the CAS and discard the copy/loaded value without using it. I
suppose the question is if such a raced load is UB (unused or not,
this is wrong) or simply an 'undef' value (like uninitialized,
harmless if not used). My reading says it's 'undef', but it's quite
possible I'm missing something. Thoughts? Thanks!
In theory, C++11 [intro.multithread]p21: The execution of a program
contains a data race if it contains two conflicting actions in
different threads, at least one of which is not atomic, and neither
happens before the other. Any such data race results in
undefined behavior.
In practice, since the compiler knows that a non-atomic load of Column
dominates the atomic exchange of Column, it also knows there can be no
concurrent modifications, so it can transform the atomic exchange into
an atomic store, which defeats your attempt to emit only one
diagnostic. I'm not sure whether there are compilers which actually do
that (yet).
Again, thanks for the detailed explanation. Makes good sense, and
much appreciated! :)
Additionally, one intentional feature of the approach used in my patch
is to avoid the call to __sync_bool_compare_and_swap() in the
potentially 'common' case of the diagnostic indeed being disabled.
This helps performance on programs with many concurrent duplicate
overflows. Does this seem valid to you?
No, for the same reason. But __atomic_exchange_n might be faster than
a branch anyway.
Sounds good to me. This is likely plenty fast.
Updated patches attached, thanks!
~Will
On Mon, Jan 7, 2013 at 3:17 PM, Will Dietz wdie...@illinois.edu wrote:
Updated, slightly neater patches attached.
Thanks!
~Will
On Tue, Jan 1, 2013 at 9:49 PM, Will Dietz wdie...@illinois.edu wrote:
Updated to apply cleanly to latest clang/compiler-rt.
Thanks!
~Will
On Sun, Dec 30, 2012 at 6:22 PM, Will Dietz wdie...@illinois.edu wrote:
(Moving to cfe-commits@, was previously:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-December/026519.html)
Please see attached! :)
Thank you,
~Will
0001-ubsan-Add-deduplication-functionality-always-enabled.patch
Description: Binary data
0001-ubsan-Make-static-check-data-non-const-so-it-can-be-.patch
Description: Binary data
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] r171947 - in /cfe/trunk: lib/CodeGen/CGExpr.cpp test/CodeGen/catch-undef-behavior.c test/CodeGen/compound-assign-overflow.c
Author: wdietz2
Date: Tue Jan 8 21:39:41 2013
New Revision: 171947
URL: http://llvm.org/viewvc/llvm-project?rev=171947view=rev
Log:
[ubsan] Make static check data non-const so it can be used for deduplication.
Modified:
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/test/CodeGen/catch-undef-behavior.c
cfe/trunk/test/CodeGen/compound-assign-overflow.c
Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=171947r1=171946r2=171947view=diff
==
--- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp Tue Jan 8 21:39:41 2013
@@ -1992,7 +1992,7 @@
llvm::Constant *Info = llvm::ConstantStruct::getAnon(StaticArgs);
llvm::GlobalValue *InfoPtr =
- new llvm::GlobalVariable(CGM.getModule(), Info-getType(), true,
+ new llvm::GlobalVariable(CGM.getModule(), Info-getType(), false,
llvm::GlobalVariable::PrivateLinkage, Info);
InfoPtr-setUnnamedAddr(true);
Modified: cfe/trunk/test/CodeGen/catch-undef-behavior.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/catch-undef-behavior.c?rev=171947r1=171946r2=171947view=diff
==
--- cfe/trunk/test/CodeGen/catch-undef-behavior.c (original)
+++ cfe/trunk/test/CodeGen/catch-undef-behavior.c Tue Jan 8 21:39:41 2013
@@ -5,7 +5,7 @@
// CHECK: @[[INT:.*]] = private unnamed_addr constant { i16, i16, [6 x i8] } {
i16 0, i16 11, [6 x i8] c'int'\00 }
// FIXME: When we only emit each type once, use [[INT]] more below.
-// CHECK: @[[LINE_100:.*]] = private unnamed_addr constant {{.*}}, i32 100,
i32 5 {{.*}} @[[INT]], i64 4, i8 1
+// CHECK: @[[LINE_100:.*]] = private unnamed_addr global {{.*}}, i32 100, i32
5 {{.*}} @[[INT]], i64 4, i8 1
// CHECK: @[[LINE_200:.*]] = {{.*}}, i32 200, i32 10 {{.*}}, i64 4, i8 0
// CHECK: @[[LINE_300_A:.*]] = {{.*}}, i32 300, i32 12 {{.*}} @{{.*}}, {{.*}}
@{{.*}}
// CHECK: @[[LINE_300_B:.*]] = {{.*}}, i32 300, i32 12 {{.*}} @{{.*}}, {{.*}}
@{{.*}}
@@ -19,7 +19,7 @@
// CHECK: @[[LINE_800:.*]] = {{.*}}, i32 800, i32 12 {{.*}} @{{.*}} }
// CHECK: @[[LINE_900:.*]] = {{.*}}, i32 900, i32 11 {{.*}} @{{.*}} }
-// CHECK-NULL: @[[LINE_100:.*]] = private unnamed_addr constant {{.*}}, i32
100, i32 5 {{.*}}
+// CHECK-NULL: @[[LINE_100:.*]] = private unnamed_addr global {{.*}}, i32 100,
i32 5 {{.*}}
// PR6805
// CHECK: @foo
Modified: cfe/trunk/test/CodeGen/compound-assign-overflow.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/compound-assign-overflow.c?rev=171947r1=171946r2=171947view=diff
==
--- cfe/trunk/test/CodeGen/compound-assign-overflow.c (original)
+++ cfe/trunk/test/CodeGen/compound-assign-overflow.c Tue Jan 8 21:39:41 2013
@@ -4,11 +4,11 @@
#include stdint.h
// CHECK: @[[INT:.*]] = private unnamed_addr constant { i16, i16, [6 x i8] } {
i16 0, i16 11, [6 x i8] c'int'\00 }
-// CHECK: @[[LINE_100:.*]] = private unnamed_addr constant {{.*}}, i32 100,
i32 5 {{.*}} @[[INT]]
+// CHECK: @[[LINE_100:.*]] = private unnamed_addr global {{.*}}, i32 100, i32
5 {{.*}} @[[INT]]
// CHECK: @[[UINT:.*]] = private unnamed_addr constant { i16, i16, [15 x i8] }
{ i16 0, i16 10, [15 x i8] c'unsigned int'\00 }
-// CHECK: @[[LINE_200:.*]] = private unnamed_addr constant {{.*}}, i32 200,
i32 5 {{.*}} @[[UINT]]
+// CHECK: @[[LINE_200:.*]] = private unnamed_addr global {{.*}}, i32 200, i32
5 {{.*}} @[[UINT]]
// CHECK: @[[DIVINT:.*]] = private unnamed_addr constant { i16, i16, [6 x i8]
} { i16 0, i16 11, [6 x i8] c'int'\00 }
-// CHECK: @[[LINE_300:.*]] = private unnamed_addr constant {{.*}}, i32 300,
i32 5 {{.*}} @[[DIVINT]]
+// CHECK: @[[LINE_300:.*]] = private unnamed_addr global {{.*}}, i32 300, i32
5 {{.*}} @[[DIVINT]]
int32_t x;
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] r171947 - in /cfe/trunk: lib/CodeGen/CGExpr.cpp test/CodeGen/catch-undef-behavior.c test/CodeGen/compound-assign-overflow.c
On Tue, Jan 8, 2013 at 9:46 PM, Eli Friedman eli.fried...@gmail.com wrote: On Tue, Jan 8, 2013 at 7:39 PM, Will Dietz wdie...@illinois.edu wrote: Author: wdietz2 Date: Tue Jan 8 21:39:41 2013 New Revision: 171947 URL: http://llvm.org/viewvc/llvm-project?rev=171947view=rev Log: [ubsan] Make static check data non-const so it can be used for deduplication. Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp cfe/trunk/test/CodeGen/catch-undef-behavior.c cfe/trunk/test/CodeGen/compound-assign-overflow.c Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=171947r1=171946r2=171947view=diff == --- cfe/trunk/lib/CodeGen/CGExpr.cpp (original) +++ cfe/trunk/lib/CodeGen/CGExpr.cpp Tue Jan 8 21:39:41 2013 @@ -1992,7 +1992,7 @@ llvm::Constant *Info = llvm::ConstantStruct::getAnon(StaticArgs); llvm::GlobalValue *InfoPtr = - new llvm::GlobalVariable(CGM.getModule(), Info-getType(), true, + new llvm::GlobalVariable(CGM.getModule(), Info-getType(), false, llvm::GlobalVariable::PrivateLinkage, Info); InfoPtr-setUnnamedAddr(true); If you want the address to be unique, just get rid of the setUnnamedAddr() call... no need to fiddle with the const-ness. -Eli Interesting, didn't know that, was wondering the reason for the setUnnamedAddr() :). For this case however, they need to be non-const since the runtime marks them in-place to track that they've already triggered. Thanks! ~Will ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] r171718 - in /cfe/trunk: lib/Analysis/CFG.cpp test/Analysis/cfg.cpp
Author: wdietz2
Date: Mon Jan 7 03:51:17 2013
New Revision: 171718
URL: http://llvm.org/viewvc/llvm-project?rev=171718view=rev
Log:
CFG.cpp: Fix wrapping logic when printing block preds/succs.
First check only wrapped with i==8, second wrapped at i==2,8,18,28,...
This fix restores the intended behavior: i==8,18,28,...
Found with -fsanitize=integer.
Added:
cfe/trunk/test/Analysis/cfg.cpp
Modified:
cfe/trunk/lib/Analysis/CFG.cpp
Modified: cfe/trunk/lib/Analysis/CFG.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=171718r1=171717r2=171718view=diff
==
--- cfe/trunk/lib/Analysis/CFG.cpp (original)
+++ cfe/trunk/lib/Analysis/CFG.cpp Mon Jan 7 03:51:17 2013
@@ -3893,7 +3893,7 @@
for (CFGBlock::const_pred_iterator I = B.pred_begin(), E = B.pred_end();
I != E; ++I, ++i) {
-if (i == 8 || (i-8) == 0)
+if (i % 10 == 8)
OS \n ;
OS B (*I)-getBlockID();
@@ -3922,7 +3922,7 @@
for (CFGBlock::const_succ_iterator I = B.succ_begin(), E = B.succ_end();
I != E; ++I, ++i) {
-if (i == 8 || (i-8) % 10 == 0)
+if (i % 10 == 8)
OS \n;
if (*I)
Added: cfe/trunk/test/Analysis/cfg.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/cfg.cpp?rev=171718view=auto
==
--- cfe/trunk/test/Analysis/cfg.cpp (added)
+++ cfe/trunk/test/Analysis/cfg.cpp Mon Jan 7 03:51:17 2013
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -fsyntax-only -analyze -analyzer-checker=debug.DumpCFG %s
21 | FileCheck %s
+// Check the wrapping behavior when dumping the CFG.
+
+// CHECK: ENTRY
+// CHECK-NEXT: Succs (1): B1
+// CHECK: [B1]
+// CHECK: Succs (21): B2 B3 B4 B5 B6 B7 B8 B9
+// CHECK: B10 B11 B12 B13 B14 B15 B16 B17 B18 B19
+// CHECK: B20 B21 B0
+// CHECK: [B0 (EXIT)]
+// CHECK-NEXT: Preds (21): B2 B3 B4 B5 B6 B7 B8 B9
+// CHECK-NEXT: B10 B11 B12 B13 B14 B15 B16 B17 B18 B19
+// CHECK-NEXT: B20 B21 B1
+void test(int i) {
+ switch(i) {
+case 0: break;
+case 1: break;
+case 2: break;
+case 3: break;
+case 4: break;
+case 5: break;
+case 6: break;
+case 7: break;
+case 8: break;
+case 9: break;
+case 10: break;
+case 11: break;
+case 12: break;
+case 13: break;
+case 14: break;
+case 15: break;
+case 16: break;
+case 17: break;
+case 18: break;
+case 19: break;
+ }
+}
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [Review] [ubsan] Fix type reported in compound assignment operations
Ping :). Updated patches for ToT attached. ~Will On Sun, Dec 30, 2012 at 4:13 PM, Will Dietz wdie...@uiuc.edu wrote: Thanks for the feedback, updated patches attached. Moved regression test to clang, and also fix similar issue with /= using the wrong type. On Sun, Dec 30, 2012 at 2:48 AM, Richard Smith rich...@metafoo.co.uk wrote: - StaticData.push_back(CGF.EmitCheckTypeDescriptor(Info.E-getType())); + QualType ResTy = Info.E-getType(); + if (const CompoundAssignOperator *C = +dyn_castCompoundAssignOperator(Info.E)) +ResTy = C-getComputationResultType(); + StaticData.push_back(CGF.EmitCheckTypeDescriptor(ResTy)); Could you just use Info.Ty here? Good call, much better. Please add a test for Clang's test suite too. The tests in compiler-rt are intended to test compiler-rt itself (the formatting of the diagnostics, etc), not for testing that Clang produces correct data blocks (in particular, we should have sufficient coverage that we can refactor Clang's IRGen without running the compiler-rt tests). Understood, thanks for the explanation. Makes good sense. On Sun, Dec 30, 2012 at 12:13 AM, Will Dietz wdie...@uiuc.edu wrote: See attached patches, thanks! Description: When checking a += b we were using the type of 'a' in the diagnostic, instead of the type of the overflowing expression a+b. This was particularly problematic when 'a' was signed and 'b' was unsigned. Okay to commit? ~Will ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits 0001-ubsan-Use-correct-type-for-compound-assignment-ops.patch Description: Binary data 0001-ubsan-Check-for-appropriate-types-on-compound-assign.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] r171801 - in /cfe/trunk: lib/CodeGen/CGExprScalar.cpp test/CodeGen/compound-assign-overflow.c
Author: wdietz2
Date: Mon Jan 7 16:25:52 2013
New Revision: 171801
URL: http://llvm.org/viewvc/llvm-project?rev=171801view=rev
Log:
[ubsan] Use correct type for compound assignment ops.
Added:
cfe/trunk/test/CodeGen/compound-assign-overflow.c
Modified:
cfe/trunk/lib/CodeGen/CGExprScalar.cpp
Modified: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprScalar.cpp?rev=171801r1=171800r2=171801view=diff
==
--- cfe/trunk/lib/CodeGen/CGExprScalar.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Mon Jan 7 16:25:52 2013
@@ -827,7 +827,7 @@
} else if (Opcode == BO_Div || Opcode == BO_Rem) {
// Divide or modulo by zero, or signed overflow (eg INT_MAX / -1).
CheckName = divrem_overflow;
- StaticData.push_back(CGF.EmitCheckTypeDescriptor(Info.E-getType()));
+ StaticData.push_back(CGF.EmitCheckTypeDescriptor(Info.Ty));
} else {
// Signed arithmetic overflow (+, -, *).
switch (Opcode) {
@@ -836,7 +836,7 @@
case BO_Mul: CheckName = mul_overflow; break;
default: llvm_unreachable(unexpected opcode for bin op check);
}
- StaticData.push_back(CGF.EmitCheckTypeDescriptor(Info.E-getType()));
+ StaticData.push_back(CGF.EmitCheckTypeDescriptor(Info.Ty));
}
DynamicData.push_back(Info.LHS);
DynamicData.push_back(Info.RHS);
Added: cfe/trunk/test/CodeGen/compound-assign-overflow.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/compound-assign-overflow.c?rev=171801view=auto
==
--- cfe/trunk/test/CodeGen/compound-assign-overflow.c (added)
+++ cfe/trunk/test/CodeGen/compound-assign-overflow.c Mon Jan 7 16:25:52 2013
@@ -0,0 +1,36 @@
+// Verify proper type emitted for compound assignments
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s
-fsanitize=signed-integer-overflow,unsigned-integer-overflow | FileCheck %s
+
+#include stdint.h
+
+// CHECK: @[[INT:.*]] = private unnamed_addr constant { i16, i16, [6 x i8] } {
i16 0, i16 11, [6 x i8] c'int'\00 }
+// CHECK: @[[LINE_100:.*]] = private unnamed_addr constant {{.*}}, i32 100,
i32 5 {{.*}} @[[INT]]
+// CHECK: @[[UINT:.*]] = private unnamed_addr constant { i16, i16, [15 x i8] }
{ i16 0, i16 10, [15 x i8] c'unsigned int'\00 }
+// CHECK: @[[LINE_200:.*]] = private unnamed_addr constant {{.*}}, i32 200,
i32 5 {{.*}} @[[UINT]]
+// CHECK: @[[DIVINT:.*]] = private unnamed_addr constant { i16, i16, [6 x i8]
} { i16 0, i16 11, [6 x i8] c'int'\00 }
+// CHECK: @[[LINE_300:.*]] = private unnamed_addr constant {{.*}}, i32 300,
i32 5 {{.*}} @[[DIVINT]]
+
+int32_t x;
+
+// CHECK: @compaddsigned
+void compaddsigned() {
+#line 100
+ x += ((int32_t)1);
+ // CHECK: @__ubsan_handle_add_overflow(i8* bitcast ({{.*}} @[[LINE_100]] to
i8*), {{.*}})
+}
+
+// CHECK: @compaddunsigned
+void compaddunsigned() {
+#line 200
+ x += ((uint32_t)1U);
+ // CHECK: @__ubsan_handle_add_overflow(i8* bitcast ({{.*}} @[[LINE_200]] to
i8*), {{.*}})
+}
+
+int8_t a, b;
+
+// CHECK: @compdiv
+void compdiv() {
+#line 300
+ a /= b;
+ // CHECK: @__ubsan_handle_divrem_overflow(i8* bitcast ({{.*}} @[[LINE_300]]
to i8*), {{.*}})
+}
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [Review] [ubsan] Deduplication
Updated, slightly neater patches attached. Thanks! ~Will On Tue, Jan 1, 2013 at 9:49 PM, Will Dietz wdie...@illinois.edu wrote: Updated to apply cleanly to latest clang/compiler-rt. Thanks! ~Will On Sun, Dec 30, 2012 at 6:22 PM, Will Dietz wdie...@illinois.edu wrote: (Moving to cfe-commits@, was previously: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-December/026519.html) Please see attached! :) Thank you, ~Will 0001-ubsan-Add-deduplication-functionality-always-enabled.patch Description: Binary data 0001-ubsan-Make-static-check-data-non-const-so-it-can-be-.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [Review] [ubsan] Deduplication
Updated to apply cleanly to latest clang/compiler-rt. Thanks! ~Will On Sun, Dec 30, 2012 at 6:22 PM, Will Dietz wdie...@illinois.edu wrote: (Moving to cfe-commits@, was previously: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-December/026519.html) Please see attached! :) Thank you, ~Will 0001-ubsan-Add-deduplication-functionality-always-enabled.patch Description: Binary data 0001-ubsan-Make-static-check-data-non-const-so-it-can-be-.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] [PATCH] CFG.cpp: Fix wrapping logic when printing block preds/succs.
Attached, testcase included. Found with -fsanitize=integer. ~Will 0001-CFG.cpp-Fix-wrapping-logic-when-printing-block-preds.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] [PATCH] DiagnosticIds: Fix offset/ID calculation, no impact outside this code.
See attached. Minor code touchup, no externally-visible functionality change. Assert added causes major check-clang failures without the corresponding code change in the patch. Found with -fsanitize=integer. ~Will 0001-DiagnosticIds-Fix-offset-ID-calculation-no-impact-ou.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] [Review] [ubsan] Fix type reported in compound assignment operations
See attached patches, thanks! Description: When checking a += b we were using the type of 'a' in the diagnostic, instead of the type of the overflowing expression a+b. This was particularly problematic when 'a' was signed and 'b' was unsigned. Okay to commit? ~Will 0001-ubsan-Use-correct-type-for-compound-assignment-ops.patch Description: Binary data 0001-ubsan-Check-for-appropriate-types-on-compound-assign.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] r171264 - in /cfe/trunk: include/clang/Driver/CC1Options.td include/clang/Driver/Options.td include/clang/Frontend/CodeGenOptions.def lib/Driver/Tools.cpp lib/Frontend/CompilerInvocation
Author: wdietz2 Date: Sun Dec 30 14:53:28 2012 New Revision: 171264 URL: http://llvm.org/viewvc/llvm-project?rev=171264view=rev Log: [ubsan] Recover by default, use -fno-sanitize-recover to disable. Modified: cfe/trunk/include/clang/Driver/CC1Options.td cfe/trunk/include/clang/Driver/Options.td cfe/trunk/include/clang/Frontend/CodeGenOptions.def cfe/trunk/lib/Driver/Tools.cpp cfe/trunk/lib/Frontend/CompilerInvocation.cpp cfe/trunk/test/CodeGen/catch-undef-behavior.c cfe/trunk/test/CodeGen/sanitize-recover.c cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp cfe/trunk/test/Driver/fsanitize.c Modified: cfe/trunk/include/clang/Driver/CC1Options.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CC1Options.td?rev=171264r1=171263r2=171264view=diff == --- cfe/trunk/include/clang/Driver/CC1Options.td (original) +++ cfe/trunk/include/clang/Driver/CC1Options.td Sun Dec 30 14:53:28 2012 @@ -201,8 +201,6 @@ HelpTextEmit complete constructors and destructors as aliases when possible; def mlink_bitcode_file : Separate[-], mlink-bitcode-file, HelpTextLink the given bitcode file before performing optimizations.; -def fsanitize_recover : Flag[-], fsanitize-recover, - HelpTextAttempt to recover from failed sanitizer checks when possible; //===--===// // Dependency Output Options Modified: cfe/trunk/include/clang/Driver/Options.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=171264r1=171263r2=171264view=diff == --- cfe/trunk/include/clang/Driver/Options.td (original) +++ cfe/trunk/include/clang/Driver/Options.td Sun Dec 30 14:53:28 2012 @@ -404,6 +404,11 @@ def fno_sanitize_memory_track_origins : Flag[-], fno-sanitize-memory-track-origins, Groupf_clang_Group, HelpTextDisable origins tracking in MemorySanitizer; +def fsanitize_recover : Flag[-], fsanitize-recover, +Groupf_clang_Group; +def fno_sanitize_recover : Flag[-], fno-sanitize-recover, + Groupf_clang_Group, Flags[CC1Option], + HelpTextDisable sanitizer check recovery; def funsafe_math_optimizations : Flag[-], funsafe-math-optimizations, Groupf_Group; def fno_unsafe_math_optimizations : Flag[-], fno-unsafe-math-optimizations, Modified: cfe/trunk/include/clang/Frontend/CodeGenOptions.def URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CodeGenOptions.def?rev=171264r1=171263r2=171264view=diff == --- cfe/trunk/include/clang/Frontend/CodeGenOptions.def (original) +++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def Sun Dec 30 14:53:28 2012 @@ -127,7 +127,7 @@ /// The default TLS model to use. ENUM_CODEGENOPT(DefaultTLSModel, TLSModel, 2, GeneralDynamicTLSModel) -CODEGENOPT(SanitizeRecover, 1, 0) /// Attempt to recover from sanitizer checks +CODEGENOPT(SanitizeRecover, 1, 1) /// Attempt to recover from sanitizer checks /// by continuing execution when possible #undef CODEGENOPT Modified: cfe/trunk/lib/Driver/Tools.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=171264r1=171263r2=171264view=diff == --- cfe/trunk/lib/Driver/Tools.cpp (original) +++ cfe/trunk/lib/Driver/Tools.cpp Sun Dec 30 14:53:28 2012 @@ -2480,6 +2480,11 @@ SanitizerArgs Sanitize(D, Args); Sanitize.addArgs(Args, CmdArgs); + if (!Args.hasFlag(options::OPT_fsanitize_recover, +options::OPT_fno_sanitize_recover, +true)) +CmdArgs.push_back(-fno-sanitize-recover); + // Report and error for -faltivec on anything other then PowerPC. if (const Arg *A = Args.getLastArg(options::OPT_faltivec)) if (!(getToolChain().getTriple().getArch() == llvm::Triple::ppc || Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=171264r1=171263r2=171264view=diff == --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original) +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Sun Dec 30 14:53:28 2012 @@ -392,7 +392,7 @@ Opts.MainFileName = Args.getLastArgValue(OPT_main_file_name); Opts.VerifyModule = !Args.hasArg(OPT_disable_llvm_verifier); - Opts.SanitizeRecover = Args.hasArg(OPT_fsanitize_recover); + Opts.SanitizeRecover = !Args.hasArg(OPT_fno_sanitize_recover); Opts.InstrumentFunctions = Args.hasArg(OPT_finstrument_functions);
Re: [cfe-commits] [Review] [ubsan] Fix type reported in compound assignment operations
Thanks for the feedback, updated patches attached. Moved regression test to clang, and also fix similar issue with /= using the wrong type. On Sun, Dec 30, 2012 at 2:48 AM, Richard Smith rich...@metafoo.co.uk wrote: - StaticData.push_back(CGF.EmitCheckTypeDescriptor(Info.E-getType())); + QualType ResTy = Info.E-getType(); + if (const CompoundAssignOperator *C = +dyn_castCompoundAssignOperator(Info.E)) +ResTy = C-getComputationResultType(); + StaticData.push_back(CGF.EmitCheckTypeDescriptor(ResTy)); Could you just use Info.Ty here? Good call, much better. Please add a test for Clang's test suite too. The tests in compiler-rt are intended to test compiler-rt itself (the formatting of the diagnostics, etc), not for testing that Clang produces correct data blocks (in particular, we should have sufficient coverage that we can refactor Clang's IRGen without running the compiler-rt tests). Understood, thanks for the explanation. Makes good sense. On Sun, Dec 30, 2012 at 12:13 AM, Will Dietz wdie...@uiuc.edu wrote: See attached patches, thanks! Description: When checking a += b we were using the type of 'a' in the diagnostic, instead of the type of the overflowing expression a+b. This was particularly problematic when 'a' was signed and 'b' was unsigned. Okay to commit? ~Will ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits 0001-ubsan-Use-correct-type-for-compound-assignment-ops.patch Description: Binary data 0001-ubsan-Check-for-appropriate-types-on-compound-assign.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] [Review] [ubsan] Deduplication
(Moving to cfe-commits@, was previously: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-December/026519.html) Please see attached! :) Thank you, ~Will 0001-ubsan-Add-deduplication-functionality-always-enabled.patch Description: Binary data 0001-ubsan-Make-static-check-data-non-const-so-it-can-be-.patch Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] r170263 - in /cfe/trunk: lib/CodeGen/CGExpr.cpp test/CodeGen/catch-undef-behavior.c
Author: wdietz2
Date: Fri Dec 14 19:39:14 2012
New Revision: 170263
URL: http://llvm.org/viewvc/llvm-project?rev=170263view=rev
Log:
[ubsan] Emit branch weight metadata to hint towards common case.
Results in better block placement that helps close the
performance gap when making ubsan checks recoverable.
Modified:
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/test/CodeGen/catch-undef-behavior.c
Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=170263r1=170262r2=170263view=diff
==
--- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp Fri Dec 14 19:39:14 2012
@@ -1973,7 +1973,15 @@
llvm::BasicBlock *Cont = createBasicBlock(cont);
llvm::BasicBlock *Handler = createBasicBlock(handler. + CheckName);
- Builder.CreateCondBr(Checked, Cont, Handler);
+
+ llvm::Instruction *Branch = Builder.CreateCondBr(Checked, Cont, Handler);
+
+ // Give hint that we very much don't expect to execute the handler
+ // Value chosen to match UR_NONTAKEN_WEIGHT, see BranchProbabilityInfo.cpp
+ llvm::MDBuilder MDHelper(getLLVMContext());
+ llvm::MDNode *Node = MDHelper.createBranchWeights((1U 20) - 1, 1);
+ Branch-setMetadata(llvm::LLVMContext::MD_prof, Node);
+
EmitBlock(Handler);
llvm::Constant *Info = llvm::ConstantStruct::getAnon(StaticArgs);
Modified: cfe/trunk/test/CodeGen/catch-undef-behavior.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/catch-undef-behavior.c?rev=170263r1=170262r2=170263view=diff
==
--- cfe/trunk/test/CodeGen/catch-undef-behavior.c (original)
+++ cfe/trunk/test/CodeGen/catch-undef-behavior.c Fri Dec 14 19:39:14 2012
@@ -38,7 +38,7 @@
// CHECK-NEXT: %[[CHECK2:.*]] = icmp eq i64 %[[MISALIGN]], 0
// CHECK: %[[OK:.*]] = and i1 %[[CHECK01]], %[[CHECK2]]
- // CHECK-NEXT: br i1 %[[OK]]
+ // CHECK-NEXT: br i1 %[[OK]], {{.*}} !prof ![[WEIGHT_MD:.*]]
// CHECK: %[[ARG:.*]] = ptrtoint {{.*}} %[[PTR]] to i64
// CHECK-NEXT: call void @__ubsan_handle_type_mismatch_abort(i8* bitcast
({{.*}} @[[LINE_100]] to i8*), i64 %[[ARG]]) noreturn nounwind
@@ -85,7 +85,7 @@
// CHECK: %[[SHIFTED_OUT_WIDTH:.*]] = sub nuw nsw i32 31, %[[RHS]]
// CHECK-NEXT: %[[SHIFTED_OUT:.*]] = lshr i32 %[[LHS:.*]],
%[[SHIFTED_OUT_WIDTH]]
// CHECK-NEXT: %[[NO_OVERFLOW:.*]] = icmp eq i32 %[[SHIFTED_OUT]], 0
- // CHECK-NEXT: br i1 %[[NO_OVERFLOW]]
+ // CHECK-NEXT: br i1 %[[NO_OVERFLOW]], {{.*}} !prof ![[WEIGHT_MD]]
// CHECK: %[[ARG1:.*]] = zext
// CHECK-NEXT: %[[ARG2:.*]] = zext
@@ -254,3 +254,5 @@
// CHECK: call void @__ubsan_handle_load_invalid_value_abort(i8* bitcast
({{.*}}), i64 {{.*}})
return *p;
}
+
+// CHECK: ![[WEIGHT_MD]] = metadata !{metadata !branch_weights, i32 1048575,
i32 1}
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] r169114 - in /cfe/trunk: include/clang/Driver/CC1Options.td include/clang/Frontend/CodeGenOptions.def lib/CodeGen/CGBuiltin.cpp lib/CodeGen/CGExpr.cpp lib/CodeGen/CGExprScalar.cpp lib/Co
Author: wdietz2
Date: Sun Dec 2 13:50:33 2012
New Revision: 169114
URL: http://llvm.org/viewvc/llvm-project?rev=169114view=rev
Log:
[ubsan] Add flag to enable recovery from checks when possible.
Added:
cfe/trunk/test/CodeGen/sanitize-recover.c
Modified:
cfe/trunk/include/clang/Driver/CC1Options.td
cfe/trunk/include/clang/Frontend/CodeGenOptions.def
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/CGExprScalar.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/test/CodeGen/catch-undef-behavior.c
cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp
Modified: cfe/trunk/include/clang/Driver/CC1Options.td
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CC1Options.td?rev=169114r1=169113r2=169114view=diff
==
--- cfe/trunk/include/clang/Driver/CC1Options.td (original)
+++ cfe/trunk/include/clang/Driver/CC1Options.td Sun Dec 2 13:50:33 2012
@@ -201,6 +201,8 @@
HelpTextEmit complete constructors and destructors as aliases when
possible;
def mlink_bitcode_file : Separate[-], mlink-bitcode-file,
HelpTextLink the given bitcode file before performing optimizations.;
+def fsanitize_recover : Flag[-], fsanitize-recover,
+ HelpTextAttempt to recover from failed sanitizer checks when possible;
//===--===//
// Dependency Output Options
Modified: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CodeGenOptions.def?rev=169114r1=169113r2=169114view=diff
==
--- cfe/trunk/include/clang/Frontend/CodeGenOptions.def (original)
+++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def Sun Dec 2 13:50:33 2012
@@ -125,6 +125,9 @@
/// The default TLS model to use.
ENUM_CODEGENOPT(DefaultTLSModel, TLSModel, 2, GeneralDynamicTLSModel)
+CODEGENOPT(SanitizeRecover, 1, 0) /// Attempt to recover from sanitizer checks
+ /// by continuing execution when possible
+
#undef CODEGENOPT
#undef ENUM_CODEGENOPT
#undef VALUE_CODEGENOPT
Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=169114r1=169113r2=169114view=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Sun Dec 2 13:50:33 2012
@@ -409,7 +409,7 @@
if (getLangOpts().SanitizeUnreachable)
EmitCheck(Builder.getFalse(), builtin_unreachable,
EmitCheckSourceLocation(E-getExprLoc()),
-llvm::ArrayRefllvm::Value *());
+llvm::ArrayRefllvm::Value *(), CRK_Unrecoverable);
else
Builder.CreateUnreachable();
Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=169114r1=169113r2=169114view=diff
==
--- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp Sun Dec 2 13:50:33 2012
@@ -533,7 +533,7 @@
llvm::ConstantInt::get(SizeTy, AlignVal),
llvm::ConstantInt::get(Int8Ty, TCK)
};
-EmitCheck(Cond, type_mismatch, StaticData, Address);
+EmitCheck(Cond, type_mismatch, StaticData, Address, CRK_Recoverable);
}
// If possible, check that the vptr indicates that there is a subobject of
@@ -586,7 +586,8 @@
};
llvm::Value *DynamicData[] = { Address, Hash };
EmitCheck(Builder.CreateICmpEQ(CacheVal, Hash),
- dynamic_type_cache_miss, StaticData, DynamicData, true);
+ dynamic_type_cache_miss, StaticData, DynamicData,
+ CRK_AlwaysRecoverable);
}
}
@@ -2023,7 +2024,7 @@
void CodeGenFunction::EmitCheck(llvm::Value *Checked, StringRef CheckName,
llvm::ArrayRefllvm::Constant * StaticArgs,
llvm::ArrayRefllvm::Value * DynamicArgs,
-bool Recoverable) {
+CheckRecoverableKind RecoverKind) {
llvm::BasicBlock *Cont = createBasicBlock(cont);
llvm::BasicBlock *Handler = createBasicBlock(handler. + CheckName);
@@ -2051,20 +2052,29 @@
ArgTypes.push_back(IntPtrTy);
}
+ bool Recover = (RecoverKind == CRK_AlwaysRecoverable) ||
+ ((RecoverKind == CRK_Recoverable)
+ CGM.getCodeGenOpts().SanitizeRecover);
+
llvm::FunctionType *FnType =
llvm::FunctionType::get(CGM.VoidTy, ArgTypes, false);
llvm::AttrBuilder B;
- if (!Recoverable) {
+ if (!Recover) {
[cfe-commits] r168701 - in /cfe/trunk: docs/UsersManual.html include/clang/Basic/Sanitizers.def lib/CodeGen/CGExprScalar.cpp lib/Driver/SanitizerArgs.h test/CodeGen/catch-undef-behavior.c test/CodeGen
Author: wdietz2 Date: Tue Nov 27 09:01:55 2012 New Revision: 168701 URL: http://llvm.org/viewvc/llvm-project?rev=168701view=rev Log: Add -fsanitize=integer for reporting suspicious integer behaviors. Introduces new sanitizer unsigned-integer-overflow. Added: cfe/trunk/test/CodeGen/unsigned-overflow.c cfe/trunk/test/CodeGen/unsigned-promotion.c cfe/trunk/test/CodeGen/unsigned-trapv.c Modified: cfe/trunk/docs/UsersManual.html cfe/trunk/include/clang/Basic/Sanitizers.def cfe/trunk/lib/CodeGen/CGExprScalar.cpp cfe/trunk/lib/Driver/SanitizerArgs.h cfe/trunk/test/CodeGen/catch-undef-behavior.c cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp cfe/trunk/test/Driver/fsanitize.c Modified: cfe/trunk/docs/UsersManual.html URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.html?rev=168701r1=168700r2=168701view=diff == --- cfe/trunk/docs/UsersManual.html (original) +++ cfe/trunk/docs/UsersManual.html Tue Nov 27 09:01:55 2012 @@ -875,21 +875,27 @@ !-- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -- dl dt id=opt_fsanitizeb-fsanitize=check1,check2/b: Turn on runtime checks -for various forms of undefined behavior./dt +for various forms of undefined or suspicious behavior./dt ddThis option controls whether Clang adds runtime checks for various forms of -undefined behavior, and is disabled by default. If a check fails, a diagnostic -message is produced at runtime explaining the problem. The main checks are: +undefined or suspicious behavior, and is disabled by default. If a check +fails, a diagnostic message is produced at runtime explaining the problem. The +main checks are: ul li id=opt_fsanitize_addresstt-fsanitize=address/tt: a href=AddressSanitizer.htmlAddressSanitizer/a, a memory error detector./li +li id=opt_fsanitize_integertt-fsanitize=integer/tt: +Enables checks for undefined or suspicious integer behavior./li li id=opt_fsanitize_threadtt-fsanitize=thread/tt: a href=ThreadSanitizer.htmlThreadSanitizer/a, an emexperimental/em data race detector. Not ready for widespread use./li li id=opt_fsanitize_undefinedtt-fsanitize=undefined/tt: -Enables all the checks listed below./li +Fast and compatible undefined behavior checker. Enables the undefined behavior +checks that have small runtime cost and no impact on address space layout +or ABI. This includes all of the checks listed below other than unsigned +integer overflow./li /ul The following more fine-grained checks are also available: @@ -897,11 +903,13 @@ ul li id=opt_fsanitize_alignmenttt-fsanitize=alignment/tt: Use of a misaligned pointer or creation of a misaligned reference./li -li id=opt_fsanitize_divide-by-zerott-fsanitize=divide-by-zero/tt: -Division by zero./li li id=opt_fsanitize_float-cast-overflowtt-fsanitize=float-cast-overflow/tt: Conversion to, from, or between floating-point types which would overflow the destination./li +li id=opt_fsanitize_float-divide-by-zerott-fsanitize=float-divide-by-zero/tt: +Floating point division by zero./li +li id=opt_fsanitize_integer-divide-by-zerott-fsanitize=integer-divide-by-zero/tt: +Integer division by zero./li li id=opt_fsanitize_nulltt-fsanitize=null/tt: Use of a null pointer or creation of a null reference./li li id=opt_fsanitize_object-sizett-fsanitize=object-size/tt: @@ -923,6 +931,8 @@ and checking for overflow in signed division (ttINT_MIN / -1/tt)./li li id=opt_fsanitize_unreachablett-fsanitize=unreachable/tt: If control flow reaches __builtin_unreachable./li +li id=opt_fsanitize_unsigned-integer-overflowtt-fsanitize=unsigned-integer-overflow/tt: +Unsigned integer overflows./li li id=opt_fsanitize_vla-boundtt-fsanitize=vla-bound/tt: A variable-length array whose bound does not evaluate to a positive value./li li id=opt_fsanitize_vptrtt-fsanitize=vptr/tt: Modified: cfe/trunk/include/clang/Basic/Sanitizers.def URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Sanitizers.def?rev=168701r1=168700r2=168701view=diff == --- cfe/trunk/include/clang/Basic/Sanitizers.def (original) +++ cfe/trunk/include/clang/Basic/Sanitizers.def Tue Nov 27 09:01:55 2012 @@ -45,26 +45,34 @@ SANITIZER(thread, Thread) // UndefinedBehaviorSanitizer -SANITIZER(signed-integer-overflow, SignedIntegerOverflow) -SANITIZER(divide-by-zero, DivideByZero) +SANITIZER(alignment, Alignment) +SANITIZER(bounds, Bounds) +SANITIZER(float-cast-overflow, FloatCastOverflow) +SANITIZER(float-divide-by-zero, FloatDivideByZero) +SANITIZER(integer-divide-by-zero, IntegerDivideByZero) +SANITIZER(null, Null) +SANITIZER(object-size, ObjectSize) +SANITIZER(return, Return) SANITIZER(shift, Shift) +SANITIZER(signed-integer-overflow, SignedIntegerOverflow)
