[PATCH] D68056: [Polly][NFC][ScopBuilder] Move RecordedAssumptions vector to ScopBuilder.

[Dominik Adamski via Phabricator via llvm-commits]

domada marked 3 inline comments as done.
domada added inline comments.


================
Comment at: polly/lib/Analysis/ScopInfo.cpp:1693
       R(R), name(None), HasSingleExitEdge(R.getExitingBlock()), DC(DC),
-      ORE(ORE), Affinator(this, LI),
+      ORE(ORE), Affinator(this, LI, RecordedAssumptions),
       ID(getNextID((*R.getEntry()->getParent()).getName().str())) {
----------------
domada wrote:
> Meinersbur wrote:
> > The `RecordedAssumptions` stored inside `ScopBuilder` has a shorter lifetime than the `Affinator` stored inside this `Scop` object. Meaning that any use of `Affinator` after the destruction of `ScopBuilder` will lead to a use-after-free error. It should not happen since adding an assumption after the Scop has been constructed will have no effect. Now new assumptions should be detectable using Valgrind/AddressSanitizer, which might be a good thing, but the potential memory corruption worries me.
> > 
> > Do you have any plans how to ensure no use-after-free happens? E.g. set the `RecordedAssumption` to `nullptr` after the Scop has been built. We might even work on even moving the Affinator into ScopBuilder. I would at least appreciate a comment in the code explaining this.
> Thank you for your comment. Initially I will set `RecordedAssumption` to `nullptr`. Finally I will try to move Affinator into ScopBuilder. I think that it should be the most elegant way.
Initial change is reverted.


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

https://reviews.llvm.org/D68056