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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D93532#2466993 <https://reviews.llvm.org/D93532#2466993>, @dexonsmith wrote:

> 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());



In D93532#2467643 <https://reviews.llvm.org/D93532#2467643>, @njames93 wrote:

> In D93532#2466993 <https://reviews.llvm.org/D93532#2466993>, @dexonsmith wrote:
>
>> 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.
>>
>> 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());
>
> We can detect MSAN using
>
>   #if LLVM_MEMORY_SANITIZER_BUILD
>   // We have msan, don't run tests.
>   #else
>   <The test code>
>   #endif
>
> If these tests are causing issues under msan, then just don't run them.
> As an added bonus if the tests are failing under msan, that would mean that msan will help catch bugs when people abuse this by not writing to the new storage before reading.

Yep. Sounds good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93532