From 69287da9aca4ad8d4e6633b0545347223df74ae4 Mon Sep 17 00:00:00 2001 From: baldurk Date: Mon, 15 Oct 2018 16:19:54 +0100 Subject: [PATCH] Handle inserting from rdcarray into itself * This self-insertion has the same kind of problem as overlapping ranges in memcpy, the act of inserting items can affect the input range by shifting things around. For inserting a single object we just copy it, for inserting a range we duplicate the whole array and then do the insert from the old range (and destruct it). * Clearly this is not the most efficient implementation, a better solution would be to append onto the existing array (potentially not even reallocating then) and doing a rotate/shift in place. --- renderdoc/api/replay/basic_types.h | 27 +++++++- renderdoc/replay/basic_types_tests.cpp | 93 ++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 1 deletion(-) diff --git a/renderdoc/api/replay/basic_types.h b/renderdoc/api/replay/basic_types.h index 5110abdbc..da8ab385a 100644 --- a/renderdoc/api/replay/basic_types.h +++ b/renderdoc/api/replay/basic_types.h @@ -381,6 +381,20 @@ public: void insert(size_t offs, const T *el, size_t count) { + if(el + count >= begin() && end() >= el) + { + // we're inserting from ourselves, so if we did this blindly we'd potentially change the + // contents of the inserted range while doing the insertion. + // To fix that, we store our original data in a temp and copy into ourselves again. Then we + // insert from the range (which now points to the copy) and let it be destroyed. + // This could be more efficient as an append and then a rotate, but this is simpler for now. + rdcarray copy; + copy.swap(*this); + this->reserve(copy.capacity()); + *this = copy; + return insert(offs, el, count); + } + const size_t oldSize = size(); // invalid size @@ -487,7 +501,18 @@ public: insert(offs, in.begin(), in.size()); } inline void insert(size_t offs, const rdcarray &in) { insert(offs, in.data(), in.size()); } - inline void insert(size_t offs, const T &in) { insert(offs, &in, 1); } + inline void insert(size_t offs, const T &in) + { + if(&in < begin() || &in > end()) + { + insert(offs, &in, 1); + } + else + { + T copy(in); + insert(offs, ©, 1); + } + } // helpful shortcut for 'append at end', basically a multi-element push_back inline void append(const T *el, size_t count) { insert(size(), el, count); } void erase(size_t offs, size_t count = 1) diff --git a/renderdoc/replay/basic_types_tests.cpp b/renderdoc/replay/basic_types_tests.cpp index 2ac2922c3..0586fad8d 100644 --- a/renderdoc/replay/basic_types_tests.cpp +++ b/renderdoc/replay/basic_types_tests.cpp @@ -473,6 +473,99 @@ TEST_CASE("Test array type", "[basictypes]") CHECK(valueConstructor == 1); CHECK(copyConstructor == 3); }; + + SECTION("Inserting from array into itself") + { + constructor = 0; + valueConstructor = 0; + copyConstructor = 0; + destructor = 0; + + rdcarray test; + + // ensure no re-allocations due to size + test.reserve(100); + + test.resize(5); + test[0].value = 10; + test[1].value = 20; + test[2].value = 30; + test[3].value = 40; + test[4].value = 50; + + CHECK(constructor == 5); + CHECK(valueConstructor == 0); + CHECK(copyConstructor == 0); + CHECK(destructor == 0); + + CHECK(test.capacity() == 100); + CHECK(test.size() == 5); + + ConstructorCounter tmp; + tmp.value = 999; + + // 5 constructed objects in the array, and tmp + CHECK(constructor == 6); + CHECK(valueConstructor == 0); + CHECK(copyConstructor == 0); + CHECK(destructor == 0); + + // this should shift everything up, and copy-construct the element into place + test.insert(0, tmp); + + CHECK(test.capacity() == 100); + CHECK(test.size() == 6); + + // 5 copies and 5 destructs to shift the array contents up, then another copy for inserting tmp + CHECK(constructor == 6); + CHECK(valueConstructor == 0); + CHECK(copyConstructor == 5 + 1); + CHECK(destructor == 5); + + CHECK(test[0].value == 999); + CHECK(test[1].value == 10); + + // this should copy the value, then do an insert + test.insert(0, test[0]); + + CHECK(test.capacity() == 100); + CHECK(test.size() == 7); + + // on top of the above, another 6 copies & destructs to shift the array contents, 1 copy for + // inserting test[0], and a copy&destruct of the temporary copy + CHECK(constructor == 6); + CHECK(valueConstructor == 0); + CHECK(copyConstructor == (5 + 1) + 6 + 1 + 1); + CHECK(destructor == (5) + 6 + 1); + + CHECK(test[0].value == 999); + CHECK(test[1].value == 999); + CHECK(test[2].value == 10); + + // this should detect the overlapped range, and duplicate the whole object + test.insert(0, test.data(), 3); + + // ensure the correct size and allocated space + CHECK(test.capacity() == 100); + CHECK(test.size() == 10); + + CHECK(test[0].value == 999); + CHECK(test[1].value == 999); + CHECK(test[2].value == 10); + CHECK(test[3].value == 999); + CHECK(test[4].value == 999); + CHECK(test[5].value == 10); + + // on top of the above: + // - 7 copies and destructs for the duplication (copies into the new storage, destructs from the + // old storage) + // - 7 copies and destructs for shifting the array contents + // - 3 copies for the inserted items + CHECK(constructor == 6); + CHECK(valueConstructor == 0); + CHECK(copyConstructor == (5 + 1 + 6 + 1 + 1) + 7 + 7 + 3); + CHECK(destructor == (5 + 6 + 1) + 7 + 7); + }; }; #define CHECK_NULL_TERM(str) CHECK(str.c_str()[str.size()] == '\0');