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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >>> >>> Ping! :) >>> >>> ~Will >>> >>> On Mon, Nov 4, 2013 at 4:32 PM, Will Dietz <[email protected]> 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 <[email protected]> >>> > 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 >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >>
From 941368075b2dc46431c742c30c9caa2193b7a9c6 Mon Sep 17 00:00:00 2001 From: Will Dietz <[email protected]> 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_cast<unsigned>(this->size() + NumToInsert)); @@ -284,14 +284,15 @@ public: template<typename 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_cast<unsigned>(this->size() + NumToInsert)); diff --git a/unittests/AST/ASTVectorTest.cpp b/unittests/AST/ASTVectorTest.cpp index ce6d0a0..5f0360a 100644 --- a/unittests/AST/ASTVectorTest.cpp +++ b/unittests/AST/ASTVectorTest.cpp @@ -14,11 +14,78 @@ #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; + IntrusiveRefCntPtr<DiagnosticIDs> DiagID; + DiagnosticsEngine Diags; + SourceManager SourceMgr; + LangOptions LangOpts; + IdentifierTable Idents; + SelectorTable Sels; + Builtin::Context Builtins; + ASTContext Ctxt; +}; +} // unnamed namespace + +TEST_F(ASTVectorTest, Compile) { ASTVector<int> V; - V.insert(*C, V.begin(), 0); + V.insert(Ctxt, V.begin(), 0); +} + +TEST_F(ASTVectorTest, InsertFill) { + ASTVector<double> 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); } + +TEST_F(ASTVectorTest, InsertEmpty) { + ASTVector<double> V; + + // Ensure no pointer overflow when inserting empty range + std::vector<int> IntVec{0, 1, 2, 3}; + auto I = V.insert(Ctxt, V.begin(), IntVec.begin(), IntVec.begin()); + ASSERT_EQ(I, V.begin()); + ASSERT_TRUE(V.empty()); + + // Non-empty range + I = V.insert(Ctxt, V.begin(), IntVec.begin(), IntVec.end()); + ASSERT_EQ(I, V.begin()); + + // Non-Empty Vector, empty range + I = V.insert(Ctxt, V.end(), IntVec.begin(), IntVec.begin()); + ASSERT_EQ(I, V.begin() + IntVec.size()); + + // Non-Empty Vector, non-empty range + I = V.insert(Ctxt, V.end(), IntVec.begin(), IntVec.end()); + ASSERT_EQ(I, V.begin() + IntVec.size()); +} + +} // end namespace ast +} // end namespace clang + -- 2.1.0
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
