[PATCH] D117356: InstructionCombining: avoid eliding mismatched alloc/free pairs

[Dávid Bolvanský via Phabricator via llvm-commits]

xbolva00 added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2666
         if (isFreeCall(I, &TLI)) {
+          if (getAllocationFamily(I, &TLI) != Family)
+            return false;
----------------
durin42 wrote:
> jyknight wrote:
> > durin42 wrote:
> > > reames wrote:
> > > > With the semantics you're proposing, having two unannotated alloc/free be removed seems inconsistent.  
> > > > 
> > > > Also, use compound if clause.
> > > I think we can't fuse the conditions, because if we see a `free()` call on a `new`ed pointer we should give up on the instruction combining, not allow it to possibly proceed on other methods, right?
> > > 
> > > Agreed on the unannotated functions meshing being inconsistent. Today that can't happen, but I guess when we move this to supporting attributes it could. Should the logic be something more like:
> > > 
> > > if (isFreeCall(...)) {
> > >   auto FreeFamily = getAllocationFamily(...);
> > >   if (FreeFamily == None || FreeFamily != Family) 
> > >     return false;
> > >   ...
> > > }
> > > 
> > > or is there more to it than that that I'm missing?
> > I think you can make that an `assert(FreeFamily)` instead (and `assert(Family)` at the top).
> > 
> > As you say, today it can't happen. And in the future, when we move this stuff into an attribute, I think it still shouldn't be able to happen -- in that future, part of the definition of "is this a free call" should be "has an allocator family attribute".
> To my surprise, I can't add an `assert(Family)` at the top: it fails many tests (over 200!) so I've included it here. I don't think we need to have an assert on `FreeFamily` since we already know it equals whatever `Family` is, though I can add that if you'd like.
Example of crashed tests? Maybe the fix is easy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117356