[BitcodeReader] Fix for PR23310: llvm-dis crashes when trying to upgrade an intrinsic.

[philip.pfaffe]

Summary

When trying to upgrade @llvm.x86.sse2.psrl.dq while parsing a module, BitcodeReader adds the function to its worklist twice, resulting in a crash when accessing it the second time.

This patch replaces the worklist vector by a map.

[filcab]

lib/Bitcode/Reader/BitcodeReader.cpp

4464

Please do the range-loop patch as a separate one (before the switch from vector to DenseMap).

4540

Do the range-for change separately.

[philip.pfaffe]

Moved range-loops into seperate patch.

[• rafael]

LGTM

[philip.pfaffe]

Added git diff --binary

[philip.pfaffe]

Commited in rL241281

lib/Bitcode/Reader/BitcodeReader.cpp

class BitcodeReader : public GVMaterializer {
   std::vector<BasicBlock*> FunctionBBs;
 
   // When reading the module header, this list is populated with functions that
   // have bodies later in the file.
   std::vector<Function*> FunctionsWithBodies;
 
   // When intrinsic functions are encountered which require upgrading they are
   // stored here with their replacement function.
-  typedef std::vector<std::pair<Function*, Function*> > UpgradedIntrinsicMap;
+  typedef DenseMap<Function*, Function*> UpgradedIntrinsicMap;
   UpgradedIntrinsicMap UpgradedIntrinsics;
 
   // Map the bitcode's custom MDKind ID to the Module's MDKind ID.
   DenseMap<unsigned, unsigned> MDKindMap;
 
   // Several operations happen after the module header has been read, but
   // before function bodies are processed. This keeps track of whether
   // we've done this yet.
std::error_code BitcodeReader::globalCleanup() {
   resolveGlobalAndAliasInits();
   if (!GlobalInits.empty() || !AliasInits.empty())
     return error("Malformed global initializer set");
 
   // Look for intrinsic functions which need to be upgraded at some point
   for (Function &F : *TheModule) {
     Function *NewFn;
     if (UpgradeIntrinsicFunction(&F, NewFn))
-      UpgradedIntrinsics.push_back(std::make_pair(&F, NewFn));
+      UpgradedIntrinsics[&F] = NewFn;
   }
 
   // Look for global variables which need to be renamed.
   for (GlobalVariable &GV : TheModule->globals())
     UpgradeGlobalVariable(&GV);
 
   // Force deallocation of memory for these vectors to favor the client that
   // want lazy deserialization.
std::error_code BitcodeReader::materialize(GlobalValue *GV) {
   if (std::error_code EC = parseFunctionBody(F))
     return EC;
   F->setIsMaterializable(false);
 
   if (StripDebugInfo)
     stripDebugInfo(*F);
 
   // Upgrade any old intrinsic calls in the function.
-  for (UpgradedIntrinsicMap::iterator I = UpgradedIntrinsics.begin(),
-       E = UpgradedIntrinsics.end(); I != E; ++I) {
-    if (I->first != I->second) {
-      for (auto UI = I->first->user_begin(), UE = I->first->user_end();
-           UI != UE;) {
-        if (CallInst* CI = dyn_cast<CallInst>(*UI++))
-          UpgradeIntrinsicCall(CI, I->second);
+  for (auto &I : UpgradedIntrinsics) {
+    if (I.first != I.second) {
+      for (auto *UI : I.first->users()) {
+        if (CallInst* CI = dyn_cast<CallInst>(UI))
+          UpgradeIntrinsicCall(CI, I.second);
       }
     }
   }
 
   // Bring in any functions that this function forward-referenced via
   // blockaddresses.
   return materializeForwardReferencedFunctions();
 }
std::error_code BitcodeReader::materializeModule(Module *M) {
   // promised above).
   if (!BasicBlockFwdRefs.empty())
     return error("Never resolved function from blockaddress");
 
   // Upgrade any intrinsic calls that slipped through (should not happen!) and
   // delete the old functions to clean up. We can't do this unless the entire
   // module is materialized because there could always be another function body
   // with calls to the old function.
-  for (std::vector<std::pair<Function*, Function*> >::iterator I =
-       UpgradedIntrinsics.begin(), E = UpgradedIntrinsics.end(); I != E; ++I) {
-    if (I->first != I->second) {
-      for (auto UI = I->first->user_begin(), UE = I->first->user_end();
-           UI != UE;) {
-        if (CallInst* CI = dyn_cast<CallInst>(*UI++))
-          UpgradeIntrinsicCall(CI, I->second);
-      }
-      if (!I->first->use_empty())
-        I->first->replaceAllUsesWith(I->second);
-      I->first->eraseFromParent();
+  for (auto &I : UpgradedIntrinsics) {
+    if (I.first != I.second) {
+      for (auto *U : I.first->users()) {
+        if (CallInst* CI = dyn_cast<CallInst>(U))
+          UpgradeIntrinsicCall(CI, I.second);
+      }
+      if (!I.first->use_empty())
+        I.first->replaceAllUsesWith(I.second);
+      I.first->eraseFromParent();
     }
   }
-  std::vector<std::pair<Function*, Function*> >().swap(UpgradedIntrinsics);
+  UpgradedIntrinsics.clear();
 
   for (unsigned I = 0, E = InstsWithTBAATag.size(); I < E; I++)
     UpgradeInstWithTBAATag(InstsWithTBAATag[I]);
 
   UpgradeDebugInfo(*M);
   return std::error_code();
 }
 

test/Bitcode/PR23310.test

+RUN: llvm-dis -disable-output %p/Inputs/PR23310.bc