[PATCH] D93532: [ADT] Add resize_for_overwrite method to SmallVector.

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM. I'm hopeful we can somehow keep the tests once we instrument SmallVector for the sanitizers (see my comment below); even if not, I suppose we can drop the tests at that time.

In D93532#2464555 <https://reviews.llvm.org/D93532#2464555>, @dblaikie wrote:

> In D93532#2464225 <https://reviews.llvm.org/D93532#2464225>, @dexonsmith wrote:
>
>> In D93532#2463995 <https://reviews.llvm.org/D93532#2463995>, @njames93 wrote:
>>
>>> Was thinking of a good way to test what is essentially undefined behaviour, how will this work under msan??
>>
>> Given that we own `SmallVector` and `destroy_range` is a no-op, is it undefined behaviour?
>
> Ish. We have used __msan_* and __asan_* functions to annotate LLVM's allocators so that uses of memory that hasn't been allocated into the pool, but not assigned to any user of the allocator can be detected (or memory used after it's returned to the allocator). We can/possibly should add such annotations to SmallVector and I think it could catch bugs like this.

When that happens, will there be a way to update this testcase, maybe to something like this?

  V.push_back(5);
  V.pop_back();
  V.resize_for_overwrite(V.size() + 1);
  if (!is_sanitizer_poisoning_pop_back())
    EXPECT_EQ(5, V.back());

or:

  V.resize_for_overwrite(V.size() + 1);
  if (is_sanitizer_poisoning_pop_back())
    EXPECT_TRUE(is_sanitizer_poison(V.back()));
  else
    EXPECT_EQ(5, V.back());



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:466
     if (N < this->size()) {
-      this->destroy_range(this->begin()+N, this->end());
+      this->destroy_range(this->begin() + N, this->end());
       this->set_size(N);
----------------
njames93 wrote:
> dexonsmith wrote:
> > njames93 wrote:
> > > dexonsmith wrote:
> > > > This whitespace change seems unrelated; can you commit separately?
> > > It's not strictly unrelated, but it was a clang format artefact 
> > Hmm... is it possible that git-diff gives clang-format a different diff than Phab is showing? Phab's version doesn't have this statement changing except for the whitespace... if clang-format is getting the same diff and formatting the line, that sounds like a bug in clang-format; alternatively, I wonder if you ran `clang-format` when the patch was different and at the time it looked like the statement had changed?
> Thats what I mean when I say an artefact from clang-format. I often format as I go.
Ah, right, thanks.


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:346
+  {
+    // Heap allocated storage
+    SmallVector<unsigned, 0> V;
----------------
Nit: period at end of sentence.


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:357
+  {
+    // Inline storage
+    SmallVector<unsigned, 2> V;
----------------
Nit: period at end of sentence.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93532/new/

https://reviews.llvm.org/D93532