Clear converters map after X86 Domain Reassignment to avoid crashes

[dim]

Summary

As reported in PR37264, in some cases the X86 Domain Reassignment
runOnMachineFunction() is called twice. Because it only deletes the
.second members of its InstrConverterBaseMap, and does not clean up
the map itself, this can lead to double frees and crashes.

Cleanup the Converters map after use, so it can safely be reinitialized
and its members re-deleted for each X86 Domain Reassignment pass.

[craig.topper]

lib/Target/X86/X86DomainReassignment.cpp

753 ↗

(On Diff #145177)

Use DeleteContainerSeconds from STLExtras.h, or better yet change Converts to hold std::unique_ptrs and then just call clear.

[dim]

Ping.

[craig.topper]

I left a comment on Monday that hasn't been addressed.

[dim]

Use DeleteContainerSeconds instead of rolling our own.

[dim]

lib/Target/X86/X86DomainReassignment.cpp

753 ↗

(On Diff #145177)

Sorry about that, I hadn't seen your remark earlier. I tried using std::unique_ptr for Converters, but that doesn't work:

/home/dim/src/llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp:453:27: error: no viable conversion from 'std::__1::unique_ptr<(anonymous namespace)::InstrConverterBase, std::__1::default_delete<(anonymous namespace)::InstrConverterBase> >' to '(anonymous namespace)::InstrConverterBase *'
      InstrConverterBase *IC = Converters.lookup({i, MI->getOpcode()});
                          ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/dim/src/llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp:17:
In file included from /home/dim/src/llvm/trunk/lib/Target/X86/X86InstrInfo.h:17:
In file included from /home/dim/src/llvm/trunk/lib/Target/X86/MCTargetDesc/X86BaseInfo.h:20:
In file included from /home/dim/src/llvm/trunk/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h:17:
In file included from /home/dim/src/llvm/trunk/include/llvm/MC/MCStreamer.h:18:
/home/dim/src/llvm/trunk/include/llvm/ADT/DenseMap.h:184:14: error: call to implicitly-deleted copy constructor of 'std::__1::unique_ptr<(anonymous namespace)::InstrConverterBase, std::__1::default_delete<(anonymous namespace)::InstrConverterBase> >'
      return TheBucket->getSecond();
             ^~~~~~~~~~~~~~~~~~~~~~
/home/dim/src/llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp:453:43: note: in instantiation of member function 'llvm::DenseMapBase<llvm::DenseMap<std::__1::pair<int, unsigned int>, std::__1::unique_ptr<(anonymous namespace)::InstrConverterBase, std::__1::default_delete<(anonymous namespace)::InstrConverterBase> >, llvm::DenseMapInfo<std::__1::pair<int, unsigned int> >, llvm::detail::DenseMapPair<std::__1::pair<int, unsigned int>, std::__1::unique_ptr<(anonymous namespace)::InstrConverterBase, std::__1::default_delete<(anonymous namespace)::InstrConverterBase> > > >, std::__1::pair<int, unsigned int>, std::__1::unique_ptr<(anonymous namespace)::InstrConverterBase, std::__1::default_delete<(anonymous namespace)::InstrConverterBase> >, llvm::DenseMapInfo<std::__1::pair<int, unsigned int> >, llvm::detail::DenseMapPair<std::__1::pair<int, unsigned int>, std::__1::unique_ptr<(anonymous namespace)::InstrConverterBase, std::__1::default_delete<(anonymous namespace)::InstrConverterBase> > > >::lookup' requested here
      InstrConverterBase *IC = Converters.lookup({i, MI->getOpcode()});
                                          ^
/usr/include/c++/v1/memory:2440:3: note: copy constructor is implicitly deleted because 'unique_ptr<(anonymous namespace)::InstrConverterBase, std::__1::default_delete<(anonymous namespace)::InstrConverterBase> >' has a user-declared move constructor
  unique_ptr(unique_ptr&& __u) noexcept
  ^
2 errors generated.

It looks like DenseMap::lookup attempts to copy the object when returning it, and I don't want to mess with DenseMap for such a minor fix as this.

[craig.topper]

LGTM

llvm/trunk/lib/Target/X86/X86DomainReassignment.cpp

bool X86DomainReassignment::runOnMachineFunction(MachineFunction &MF) {
 
   for (Closure &C : Closures)
     if (isReassignmentProfitable(C, MaskDomain)) {
       reassign(C, MaskDomain);
       ++NumClosuresConverted;
       Changed = true;
     }
 
-  for (auto I : Converters)
-    delete I.second;
+  DeleteContainerSeconds(Converters);
 
   DEBUG(dbgs() << "***** Machine Function after Domain Reassignment *****\n");
   DEBUG(MF.print(dbgs()));
 
   return Changed;
 }
 
 INITIALIZE_PASS(X86DomainReassignment, "x86-domain-reassignment",
                 "X86 Domain Reassignment Pass", false, false)
 
 /// Returns an instance of the Domain Reassignment pass.
 FunctionPass *llvm::createX86DomainReassignmentPass() {
   return new X86DomainReassignment();
 }