From 79663c1910ebf9bda8f758388eaa3171fb9a5134 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Wed, 14 Aug 2013 08:56:41 +0000 Subject: [PATCH] Fix a really terrifying but improbable bug in mem2reg. If you have seen extremely subtle miscompilations (such as a load getting replaced with the value stored *below* the load within a basic block) related to promoting an alloca to an SSA value, there is the dim possibility that you hit this. Please let me know if you won this unfortunate lottery. The first half of mem2reg's core logic (as it is used both in the standalone mem2reg pass and in SROA) builds up a mapping from 'Instruction *' to the index of that instruction within its basic block. This allows quickly establishing which store dominate a particular load even for large basic blocks. We cache this information throughout the run of mem2reg over a function in order to amortize the cost of computing it. This is not in and of itself a strange pattern in LLVM. However, it introduces a very important constraint: absolutely no instruction can be deleted from the program without updating the mapping. Otherwise a newly allocated instruction might get the same pointer address, and then end up with a wrong index. Yes, LLVM routinely suffers from a *single threaded* variant of the ABA problem. Most places in LLVM don't find avoiding this an imposition because they don't both delete and create new instructions iteratively, but mem2reg *loves* to do this... All the time. Fortunately, the mem2reg code was really careful about updating this cache to handle this eventuallity... except when it comes to the debug declare intrinsic. Oops. The fix is to invalidate that pointer in the cache when we delete it, the same as we do when deleting alloca instructions and other instructions. I've also caused the same bug in new code while working on a fix to PR16867, so this seems to be a really unfortunate pattern. Hopefully in subsequent patches the deletion of dead instructions can be consolidated sufficiently to make it less likely that we'll see future occurences of this bug. Sorry for not having a test case, but I have literally no idea how to reliably trigger this kind of thing. It may be single-threaded, but it remains an ABA problem. It would require a really amazing number of stars to align. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@188367 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/Utils/PromoteMemoryToRegister.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/Transforms/Utils/PromoteMemoryToRegister.cpp b/lib/Transforms/Utils/PromoteMemoryToRegister.cpp index 1b51255251a..368b1fa5b2e 100644 --- a/lib/Transforms/Utils/PromoteMemoryToRegister.cpp +++ b/lib/Transforms/Utils/PromoteMemoryToRegister.cpp @@ -418,6 +418,7 @@ static bool rewriteSingleStoreAlloca(AllocaInst *AI, AllocaInfo &Info, DIBuilder DIB(*AI->getParent()->getParent()->getParent()); ConvertDebugDeclareToDebugValue(DDI, Info.OnlyStore, DIB); DDI->eraseFromParent(); + LBI.deleteValue(DDI); } // Remove the (now dead) store and alloca. Info.OnlyStore->eraseFromParent(); @@ -521,8 +522,10 @@ static void promoteSingleBlockAlloca(AllocaInst *AI, const AllocaInfo &Info, LBI.deleteValue(AI); // The alloca's debuginfo can be removed as well. - if (DbgDeclareInst *DDI = Info.DbgDeclare) + if (DbgDeclareInst *DDI = Info.DbgDeclare) { DDI->eraseFromParent(); + LBI.deleteValue(DDI); + } ++NumLocalPromoted; } -- 2.34.1