[FIX] Make LSan happy by *not* leaking memory

[jdoerfert]

rebase

[jdoerfert]

Pushed as cb19ea45a71b74c72ad5e8ceaa42a0b6c8168576

[dblaikie]

Looks like this was sent for review but no one reviewed/approved it? Generally, if something is sent for review, it's not great to submit it without review. (if you felt it needed review to begin with, time shouldn't change that)

Any chance of using unique_ptrs here, instead of raw pointers and DeleteContainerPointers?

[jdoerfert]

In D69674#1752604, @dblaikie wrote:

Looks like this was sent for review but no one reviewed/approved it? Generally, if something is sent for review, it's not great to submit it without review. (if you felt it needed review to begin with, time shouldn't change that)

The review was intended to share the patch and validate the fix, that is why I didn't add reviewers. I will not open a review next time.

Any chance of using unique_ptrs here, instead of raw pointers and DeleteContainerPointers?

I can do that if you want me to. FWIW, it is a printer pass so I don't feel a strong need to improve performance here and I doubt there is "active development" on this part.

[dblaikie]

In D69674#1753862, @jdoerfert wrote:

In D69674#1752604, @dblaikie wrote:

Looks like this was sent for review but no one reviewed/approved it? Generally, if something is sent for review, it's not great to submit it without review. (if you felt it needed review to begin with, time shouldn't change that)

The review was intended to share the patch and validate the fix, that is why I didn't add reviewers. I will not open a review next time.

Validate in what sense? There didn't appear to be any discussion/validation from other developers here.

Any chance of using unique_ptrs here, instead of raw pointers and DeleteContainerPointers?

I can do that if you want me to. FWIW, it is a printer pass so I don't feel a strong need to improve performance here and I doubt there is "active development" on this part.

Ah, it wasn't so much about performance (I don't expect the unique_ptr solution to be any faster) but readability/maintainability - unique_ptr makes it harder to reintroduce bugs if someone adds an early return to that function or otherwise refactors it.

[jdoerfert]

In D69674#1753895, @dblaikie wrote:

In D69674#1753862, @jdoerfert wrote:

In D69674#1752604, @dblaikie wrote:

Looks like this was sent for review but no one reviewed/approved it? Generally, if something is sent for review, it's not great to submit it without review. (if you felt it needed review to begin with, time shouldn't change that)

The review was intended to share the patch and validate the fix, that is why I didn't add reviewers. I will not open a review next time.

Validate in what sense? There didn't appear to be any discussion/validation from other developers here.

As far as I can recall, I was pinged on IRC and made aware of the leak, the patch was put here, run on their end to verify, then pushed and closed.

Any chance of using unique_ptrs here, instead of raw pointers and DeleteContainerPointers?

I can do that if you want me to. FWIW, it is a printer pass so I don't feel a strong need to improve performance here and I doubt there is "active development" on this part.

Ah, it wasn't so much about performance (I don't expect the unique_ptr solution to be any faster) but readability/maintainability - unique_ptr makes it harder to reintroduce bugs if someone adds an early return to that function or otherwise refactors it.

I do not disagree. Will try to make it happen once I find the time.

llvm/lib/Analysis/MustExecute.cpp

INITIALIZE_PASS_END(MustBeExecutedContextPrinter,
                     false, true)
 
 ModulePass *llvm::createMustBeExecutedContextPrinter() {
   return new MustBeExecutedContextPrinter();
 }
 
 bool MustBeExecutedContextPrinter::runOnModule(Module &M) {
   // We provide non-PM analysis here because the old PM doesn't like to query
-  // function passes from a module pass. Given that this is a printer, we don't
-  // care much about memory leaks.
-  GetterTy<LoopInfo> LIGetter = [](const Function &F) {
+  // function passes from a module pass.
+  SmallVector<PostDominatorTree *, 8> PDTs;
+  SmallVector<DominatorTree *, 8> DTs;
+  SmallVector<LoopInfo *, 8> LIs;
+
+  GetterTy<LoopInfo> LIGetter = [&](const Function &F) {
     DominatorTree *DT = new DominatorTree(const_cast<Function &>(F));
     LoopInfo *LI = new LoopInfo(*DT);
+    DTs.push_back(DT);
+    LIs.push_back(LI);
     return LI;
   };
-  GetterTy<PostDominatorTree> PDTGetter = [](const Function &F) {
+  GetterTy<PostDominatorTree> PDTGetter = [&](const Function &F) {
     PostDominatorTree *PDT = new PostDominatorTree(const_cast<Function &>(F));
+    PDTs.push_back(PDT);
     return PDT;
   };
   MustBeExecutedContextExplorer Explorer(true, LIGetter, PDTGetter);
   for (Function &F : M) {
     for (Instruction &I : instructions(F)) {
       dbgs() << "-- Explore context of: " << I << "\n";
       for (const Instruction *CI : Explorer.range(&I))
         dbgs() << "  [F: " << CI->getFunction()->getName() << "] " << *CI
                << "\n";
     }
   }
 
+  DeleteContainerPointers(PDTs);
+  DeleteContainerPointers(LIs);
+  DeleteContainerPointers(DTs);
   return false;
 }
 
 static bool isMustExecuteIn(const Instruction &I, Loop *L, DominatorTree *DT) {
   // TODO: merge these two routines.  For the moment, we display the best
   // result obtained by *either* implementation.  This is a bit unfair since no
   // caller actually gets the full power at the moment.
   SimpleLoopSafetyInfo LSI;