[Mlir-commits] [mlir] [mlir] Fix use-after-free bugs in {RankedTensorType|VectorType}::Builder (PR #68969)

[Andrzej Warzyński]

================
@@ -277,7 +277,7 @@ class RankedTensorType::Builder {
     if (storage.empty())
       storage.append(shape.begin(), shape.end());
     storage.erase(storage.begin() + pos);
-    shape = {storage.data(), storage.size()};
+    shape = {};
----------------
banach-space wrote:

> And the question was about instead making sure that the "shape" is always consistent.

As in consistent with `storage`? Do we need that? With this change (*):
* if `shape` **is not** empty then it's consistent with `storage`,
* if `shape` **is** empty then it's inconsistent with `storage` and one should be usiing the latter instead.

This would makes sense to me, but IMHO requires a bit more documentation. For example, you could decorate `shape` with a comment hinting that it shouldn't be used directly.

> I think it'll just wrap this pattern in a helper CopyOnWriteArrayRef<T> class, which can be safer, and simplify the builder's code (and ensure consistency there).

It feels that that's adding complexity to something that was meant to be fairly simple. 

> we're just dropping information on the floor??

The information would be preserved in `storage`, so not really?

Anyway, mostly my 2p. @joker-eph will have a much better intuition what the design goals should be here.

(*) Apologies if I am stating the obvious, but want to make sure that I follow the logic here.

https://github.com/llvm/llvm-project/pull/68969