[PATCH] D98602: [SCCP] Avoid modifying AdditionalUsers while iterating over it

Sun Mar 14 11:02:49 PDT 2021

dim added a comment.

In D98602#2624871 <https://reviews.llvm.org/D98602#2624871>, @lebedev.ri wrote:

> Test missing.

I haven't yet reduced the test case, will see to that. (One issue is that any failure would only show up on FreeBSD and OpenBSD...)

> Am i understanding correctly that the problem is *only* that the inner `SmallPtrSet` may get new entries added to it?

I think so, the problem is that `SCCPSolver::AdditionalUsers` is a `DenseMap` which is iterated over:

  auto Iter = AdditionalUsers.find(I);
  if (Iter != AdditionalUsers.end()) {
    for (User *U : Iter->second)
      if (auto *UI = dyn_cast<Instruction>(U))
        OperandChangedState(UI);
  }

meanwhile, `OperandChangedState()` can change the `Instruction` pointed to, via `visit()`:

  void OperandChangedState(Instruction *I) {
    if (BBExecutable.count(I->getParent()))   // Inst is executable?
      visit(*I);
  }

Apparently, `visit()` can cause the `SmallPtrSet` to grow, which invalidates the `U` iterator, as it shows in valgrind output:

  ==1366860== Invalid read of size 8
  ==1366860==    at 0x18CD850: AdvanceIfNotValid (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:250)
  ==1366860==    by 0x18CD850: operator++ (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:301)
  ==1366860==    by 0x18CD850: (anonymous namespace)::SCCPSolver::markUsersAsChanged(llvm::Value*) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:536)
  ==1366860==    by 0x18C773A: (anonymous namespace)::SCCPSolver::Solve() (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1420)
  ==1366860==    by 0x18C4EE4: llvm::runIPSCCP(llvm::Module&, llvm::DataLayout const&, std::function<llvm::TargetLibraryInfo const& (llvm::Function&)>, llvm::function_ref<llvm::AnalysisResultsForFn (llvm::Function&)>) (sr
  c/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:0)
  ==1366860==    by 0x153E53E: llvm::IPSCCPPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm-project/llvm/lib/Transforms/IPO/SCCP.cpp:24)
  ==1366860==    by 0x28BF9BC: llvm::detail::PassModel<llvm::Module, llvm::IPSCCPPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm
  -project/llvm/include/llvm/IR/PassManagerInternal.h:85)
  ==1366860==    by 0x13E9D75: llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm-project/llvm/include/llvm/IR/PassManager.h:517)
  ==1366860==    by 0x1C254A7: (anonymous namespace)::EmitAssemblyHelper::EmitAssemblyWithNewPassManager(clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (src/
  llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1465)
  ==1366860==    by 0x1C1F9CF: clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout
  const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (src/llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1634)
  ==1366860==    by 0x27FE6DF: clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (src/llvm/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:334)
  ==1366860==    by 0x32CF2C2: clang::ParseAST(clang::Sema&, bool, bool) (src/llvm/llvm-project/clang/lib/Parse/ParseAST.cpp:171)
  ==1366860==    by 0x21D8AC3: clang::FrontendAction::Execute() (src/llvm/llvm-project/clang/lib/Frontend/FrontendAction.cpp:949)
  ==1366860==    by 0x2154162: clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (src/llvm/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:955)
  ==1366860==  Address 0x7c9c678 is 8 bytes inside a block of size 1,024 free'd
  ==1366860==    at 0x697BA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==1366860==    by 0x197F06A: llvm::SmallPtrSetImplBase::Grow(unsigned int) (src/llvm/llvm-project/llvm/lib/Support/SmallPtrSet.cpp:116)
  ==1366860==    by 0x197EEF5: llvm::SmallPtrSetImplBase::insert_imp_big(void const*) (src/llvm/llvm-project/llvm/lib/Support/SmallPtrSet.cpp:0)
  ==1366860==    by 0xDA841E: insert_imp (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:154)
  ==1366860==    by 0xDA841E: llvm::SmallPtrSetImpl<llvm::User*>::insert(llvm::User*) (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:365)
  ==1366860==    by 0x18CF349: (anonymous namespace)::SCCPSolver::addAdditionalUser(llvm::Value*, llvm::User*) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:514)
  ==1366860==    by 0x18CE9BB: (anonymous namespace)::SCCPSolver::handleCallResult(llvm::CallBase&) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1332)
  ==1366860==    by 0x18D3F5B: (anonymous namespace)::SCCPSolver::visitCallBase(llvm::CallBase&) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1188)
  ==1366860==    by 0x18CD827: OperandChangedState (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:508)
  ==1366860==    by 0x18CD827: (anonymous namespace)::SCCPSolver::markUsersAsChanged(llvm::Value*) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:538)
  ==1366860==    by 0x18C773A: (anonymous namespace)::SCCPSolver::Solve() (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1420)
  ==1366860==    by 0x18C4EE4: llvm::runIPSCCP(llvm::Module&, llvm::DataLayout const&, std::function<llvm::TargetLibraryInfo const& (llvm::Function&)>, llvm::function_ref<llvm::AnalysisResultsForFn (llvm::Function&)>) (sr
  c/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:0)
  ==1366860==    by 0x153E53E: llvm::IPSCCPPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm-project/llvm/lib/Transforms/IPO/SCCP.cpp:24)
  ==1366860==    by 0x28BF9BC: llvm::detail::PassModel<llvm::Module, llvm::IPSCCPPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm
  -project/llvm/include/llvm/IR/PassManagerInternal.h:85)
  ==1366860==  Block was alloc'd at
  ==1366860==    at 0x697A7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==1366860==    by 0x197EF77: safe_malloc (src/llvm/llvm-project/llvm/include/llvm/Support/MemAlloc.h:26)
  ==1366860==    by 0x197EF77: llvm::SmallPtrSetImplBase::Grow(unsigned int) (src/llvm/llvm-project/llvm/lib/Support/SmallPtrSet.cpp:100)
  ==1366860==    by 0x197EEF5: llvm::SmallPtrSetImplBase::insert_imp_big(void const*) (src/llvm/llvm-project/llvm/lib/Support/SmallPtrSet.cpp:0)
  ==1366860==    by 0xDA841E: insert_imp (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:154)
  ==1366860==    by 0xDA841E: llvm::SmallPtrSetImpl<llvm::User*>::insert(llvm::User*) (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:365)
  ==1366860==    by 0x18CF349: (anonymous namespace)::SCCPSolver::addAdditionalUser(llvm::Value*, llvm::User*) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:514)
  ==1366860==    by 0x18CE9BB: (anonymous namespace)::SCCPSolver::handleCallResult(llvm::CallBase&) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1332)
  ==1366860==    by 0x18D3F5B: (anonymous namespace)::SCCPSolver::visitCallBase(llvm::CallBase&) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1188)
  ==1366860==    by 0x18C7A6B: visit<llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void>, false, false> > (src/llvm/llvm-project/llvm/include/llvm/IR/InstVisitor.h:90)
  ==1366860==    by 0x18C7A6B: visit (src/llvm/llvm-project/llvm/include/llvm/IR/InstVisitor.h:105)
  ==1366860==    by 0x18C7A6B: visit (src/llvm/llvm-project/llvm/include/llvm/IR/InstVisitor.h:111)
  ==1366860==    by 0x18C7A6B: (anonymous namespace)::SCCPSolver::Solve() (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1448)
  ==1366860==    by 0x18C4EE4: llvm::runIPSCCP(llvm::Module&, llvm::DataLayout const&, std::function<llvm::TargetLibraryInfo const& (llvm::Function&)>, llvm::function_ref<llvm::AnalysisResultsForFn (llvm::Function&)>) (sr
  c/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:0)
  ==1366860==    by 0x153E53E: llvm::IPSCCPPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm-project/llvm/lib/Transforms/IPO/SCCP.cpp:24)
  ==1366860==    by 0x28BF9BC: llvm::detail::PassModel<llvm::Module, llvm::IPSCCPPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm
  -project/llvm/include/llvm/IR/PassManagerInternal.h:85)
  ==1366860==    by 0x13E9D75: llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm-project/llvm/include/llvm/IR/PassManager.h:517)

> I think the fix depends on the expected behavior here, in particular, two things stand out:
>
> - when looping over that set there, if a new unique entry is added in process, should it be ignored, or handled?
> - what if said new entry isn't unique? should it be re-handled?
>
> I'm not sure about the latter, but not handling the former //seems// like a bug.
> I think we should instead change `AdditionalUsers` from `DenseMap<Value *, SmallPtrSet<User *, 2>>` to `DenseMap<Value *, SetVector<User *, 2>>`,
> and change the loop to
>
>   // NOTE: new entries may be added to the vector in-process, invalidating iterators.
>   for(int I = 0; I != Iter->second.size(); ++I) {
>     User *U = Iter->second[I];
>     <...>
>   }
>
> But it is also possible that i don't know what i'm talking about.

I'm not familiar enough with the semantics of this code to have an informed opinion. :)

The solution proposed by @mortimer was just trying to work around the iterator invalidation, but preserves the original logic. In that respect, it is a minimalistic fix, but if there is a better solution, I'm all for it.

In D98602#2624876 <https://reviews.llvm.org/D98602#2624876>, @fhahn wrote:

> Does the crash trigger with ASan/MSan? Would it be possible to add a reduced test case from the bug report?

I've only run it under valgrind, but I'll attempt to reduce a test case from the bug report. On FreeBSD it gets a SIGBUS, and on OpenBSD apparently a SIGSEGV, but this type of memory error is typically non-portable. :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98602