[PATCH] D38201: Use a BumpPtrAllocator for Loop objects

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

In https://reviews.llvm.org/D38201#880864, @chandlerc wrote:

> In https://reviews.llvm.org/D38201#880392, @sanjoy wrote:
>
> > There is a problem with this patch -- the legacy pass manager checks if a loop is valid or not by querying `isInvalid`, which is incorrect; since we call the destructor for loop objects to mark them as "invalid" calling `isInvalid` for possibly invalid loop objects has UB.  We already have this UB in `User::operator delete` though, and so given that and that the legacy pass manager is going away at some point, perhaps this is okay?  Alternatively, I could add a mechanism on the legacy loop pass manager that lets passes denote loop objects to have been deleted (by remembering the pointers in a set) and use that to decide if a Loop object is valid or not (and with that, I can guard `isInvalid` with `#ifndef NDEBUG`).
>
>
> Sadly, I think this would be problematic. I think it'll trip ASan's use-after-scope or use-after-lifetime checks.... =/ But maybe there is a way around that. Or maybe just implement the solution you suggest. Dunno, but definitely worth checking.


We already do this for `User` instances -- we read from fields in the `User` object to be destroyed in its `operator delete`, after the destructors have run.  However,  two wrongs don't make a right so I'll first try to see if I can fix this in the old pass manager.

> 
> 
>> The asserts are using `isInvalid` correctly, as far as I can tell; they should never have UB in an execution that uses the loop objects correctly.
> 
> Cool.




https://reviews.llvm.org/D38201