[PATCH] D91837: ADT: Fix reference invalidation in SmallVector APIs that pass in a value

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:141
+  bool isReferenceToStorage(const void *V) const {
+    return V >= this->begin() && V < this->end();
+  }
----------------
Absurd C++ pedantry, but I believe op< on pointers to unrelated objects is unspecified in C++? ("If two pointers are not specified to compare greater or compare equal, the result of the comparison is unspecified. An unspecified result may be nondeterministic, and need not be consistent even for multiple evaluations of the same expression with the same operands in the same execution of the program" - https://en.cppreference.com/w/cpp/language/operator_comparison) but std::less is specified to handle this case: "A specialization of std::less for any pointer type yields a strict total order, even if the built-in operator< does not." - https://en.cppreference.com/w/cpp/utility/functional/less)

Because C++ is fun like that.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:334
+      ReferencesStorage = true;
+      Index = &Elt - this->begin();
+    }
----------------
Is there any case where 'Elt' could be contained within a subobject of an element of the SmallVector, while not being an element itself? I guess not, because T can't contain (directly) another T - but asking just in case I've missed a possibility (if this were true, then an index would be inadequate for persisting 'Elt' across the grow operation).


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:454-467
+  T *reserveForAndGetAddress(const T &Elt, size_t N = 1) {
+    size_t NewSize = this->size() + N;
+    if (LLVM_LIKELY(NewSize <= this->capacity()))
+      return const_cast<T *>(&Elt);
+
+    bool ReferencesStorage = false;
+    int64_t Index = -1;
----------------
Is this text identical between the trivially copyable and non-trivially copyable specializations? If so, can it be shared at all? I guess it'd need to be in a CRTP base placed between these specializations and SmallVectorTemplateCommon, and possibly not worth the added complexity?


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:677-686
+  template <
+      class ArgType,
+      std::enable_if_t<
+          std::is_same<std::remove_const_t<std::remove_reference_t<ArgType>>,
+                       T>::value &&
+              !TakesParamByValue,
+          bool> = false>
----------------
Perhaps all this infrastructure and SFINAE might merit a separate patch? (breaking this patch up into separate changes for adding reference validity for each independent operation might be nice in general, but especially this part/set of changes, if they can be separate - it'd also make some of the other changes a bit clearer, once the reserveForAndGetAddress is added, subsequent changes would be "port to use that, and add test")

But I know that can be a hassle to make, manage the reviews, etc, so not vital by any means.


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1083-1085
+  // Append enough more elements that V will grow again. This time the old
+  // storage will have been freed at time of access and sanitizers can catch
+  // the use-after-free.
----------------
By this you mean "sanitizers would catch use-after-free if this support were missing/broken"? Otherwise this reads as though it might be a sanitizer-only death test, specifically checking that misuse fails/is diagnosed, which it isn't.


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

https://reviews.llvm.org/D91837