[llvm-bugs] [Bug 28911] New: Scalarizer is crashing due to use-after-free

[via llvm-bugs]

https://llvm.org/bugs/show_bug.cgi?id=28911

            Bug ID: 28911
           Summary: Scalarizer is crashing due to use-after-free
           Product: libraries
           Version: trunk
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P
         Component: Scalar Optimizations
          Assignee: unassignedbugs at nondot.org
          Reporter: david.stenberg at ericsson.com
                CC: llvm-bugs at lists.llvm.org
    Classification: Unclassified

Created attachment 16917
  --> https://llvm.org/bugs/attachment.cgi?id=16917&action=edit
Reproducer

When running the following command:

opt -lint -S -mtriple=x86-64 -scalarizer -verify bugpoint-reduced-simplified.ll

opt crashes due to the following error:

  Instruction does not dominate all uses!
    <badref> = extractelement
    %h.10.2.i0 = phi i16 [ <badref>, %bb3 ]
  LLVM ERROR: Broken function found, compilation aborted!

Running opt with valgrind shows a bunch of invalid reads for an ExtractElement
instruction that has been already deleted:

  [...]
  ==42662== Invalid read of size 8
  ==42662==    at 0x103E118: llvm::Value::getType() const (Value.h:227)
  ==42662==    by 0x13ED204: llvm::PHINode::setIncomingValue(unsigned int,
llvm::Value*) (Instructions.h:2563)
  ==42662==    by 0x13ED317: llvm::PHINode::addIncoming(llvm::Value*,
llvm::BasicBlock*) (Instructions.h:2607)
  ==42662==    by 0x1E8E947: (anonymous
namespace)::Scalarizer::visitPHINode(llvm::PHINode&) (Scalarizer.cpp:663)
  ==42662==    by 0x1E916D0: llvm::InstVisitor<(anonymous
namespace)::Scalarizer, bool>::visitPHI(llvm::PHINode&) (Instruction.def:185)
  ==42662==    by 0x1E90AC8: llvm::InstVisitor<(anonymous
namespace)::Scalarizer, bool>::visit(llvm::Instruction&) (Instruction.def:185)
  ==42662==    by 0x1E8F664: llvm::InstVisitor<(anonymous
namespace)::Scalarizer, bool>::visit(llvm::Instruction*) (InstVisitor.h:114)
  ==42662==    by 0x1E8B035: (anonymous
namespace)::Scalarizer::runOnFunction(llvm::Function&) (Scalarizer.cpp:265)
  ==42662==    by 0x19E145F:
llvm::FPPassManager::runOnFunction(llvm::Function&)
(LegacyPassManager.cpp:1526)
  ==42662==    by 0x19E15F2: llvm::FPPassManager::runOnModule(llvm::Module&)
(LegacyPassManager.cpp:1547)
  ==42662==    by 0x19E198D: (anonymous
namespace)::MPPassManager::runOnModule(llvm::Module&)
(LegacyPassManager.cpp:1603)
  ==42662==    by 0x19E20DD: llvm::legacy::PassManagerImpl::run(llvm::Module&)
(LegacyPassManager.cpp:1706)
  ==42662==  Address 0x62e45d8 is 56 bytes inside a block of size 112 free'd
  ==42662==    at 0x4C2C2BC: operator delete(void*) (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==42662==    by 0x1A4273B: llvm::User::operator delete(void*) (User.cpp:193)
  ==42662==    by 0x19BF14F: llvm::ExtractElementInst::~ExtractElementInst()
(Instructions.h:2005)
  ==42662==    by 0x168F934:
llvm::ilist_node_traits<llvm::Instruction>::deleteNode(llvm::Instruction*) (in
/local/repo/edasten/llvm-upstream/build-latest2/bin/opt)
  ==42662==    by 0x168F3F6: llvm::iplist<llvm::Instruction,
llvm::SymbolTableListTraits<llvm::Instruction>
>::erase(llvm::ilist_iterator<llvm::Instruction>) (ilist.h:470)
  ==42662==    by 0x19A68CA: llvm::Instruction::eraseFromParent()
(Instruction.cpp:76)
  ==42662==    by 0x1E8B666: (anonymous
namespace)::Scalarizer::gather(llvm::Instruction*,
llvm::SmallVector<llvm::Value*, 8u> const&) (Scalarizer.cpp:320)
  ==42662==    by 0x1E8E9A8: (anonymous
namespace)::Scalarizer::visitPHINode(llvm::PHINode&) (Scalarizer.cpp:665)
  [...]

An initial analysis:

As the shufflevector instruction is visited before the %h.10.1 phi node, an
extractvalue instruction will be created for the %h.10.1 node. This instruction
will be stored in Scattered and Gathered for the shufflevector instruction.

When later visiting the %h.10.1 phi node, the extractvalue instruction will be
deleted and replaced by a scalarized phi node, but it will not be removed from
Scattered and Gathered, leading to stale pointers.

This could be fixed by adding the following code to gather() to make sure that
Scattered and Gathered contains the new instruction:

@@ -317,6 +317,17 @@ void Scalarizer::gather(Instruction *Op, const ValueVector
&CV) {
       Instruction *Old = cast<Instruction>(V);
       CV[I]->takeName(Old);
       Old->replaceAllUsesWith(CV[I]);
+
+      for (auto &P : Scattered) {
+        ValueVector &VV = P.second;
+        std::replace(VV.begin(), VV.end(), V, CV[I]);
+      }
+
+      for (auto &P : Gathered) {
+        ValueVector &VV = *P.second;
+        std::replace(VV.begin(), VV.end(), V, CV[I]);
+      }
+
       Old->eraseFromParent();
     }
   }

I do not have enough knowledge of the Scalarizer to tell if this is the best
way to fix this.
The output does at least look reasonable.

Performance wise this fix is not ideal, but as we have no use information since
the
operands for the previously visited instructions are removed, this was easiest.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20160809/8f928b92/attachment-0001.html>