6502/llvm-6502
1
0
Fork 0
mirror of https://github.com/c64scene-ar/llvm-6502.git synced 2024-12-14 11:32:34 +00:00
Code Issues Projects Releases Wiki Activity

Fix a really terrifying but improbable bug in mem2reg. If you have seen

Browse Source 
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
This commit is contained in:
Chandler Carruth 2013-08-14 08:56:41 +00:00
parent 35e194fbad
commit 79663c1910
1 changed files with 4 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
lib/Transforms/Utils/PromoteMemoryToRegister.cpp
View File

@ -418,6 +418,7 @@ static bool rewriteSingleStoreAlloca(AllocaInst *AI, AllocaInfo &Info,
DIBuilder DIB(*AI->getParent()->getParent()->getParent()); DIBuilder DIB(*AI->getParent()->getParent()->getParent());
ConvertDebugDeclareToDebugValue(DDI, Info.OnlyStore, DIB); ConvertDebugDeclareToDebugValue(DDI, Info.OnlyStore, DIB);
DDI->eraseFromParent(); DDI->eraseFromParent();
LBI.deleteValue(DDI);
} }
// Remove the (now dead) store and alloca. // Remove the (now dead) store and alloca.
Info.OnlyStore->eraseFromParent(); Info.OnlyStore->eraseFromParent();
@ -521,8 +522,10 @@ static void promoteSingleBlockAlloca(AllocaInst *AI, const AllocaInfo &Info,
LBI.deleteValue(AI); LBI.deleteValue(AI);
// The alloca's debuginfo can be removed as well. // The alloca's debuginfo can be removed as well.
if (DbgDeclareInst *DDI = Info.DbgDeclare) if (DbgDeclareInst *DDI = Info.DbgDeclare) {
DDI->eraseFromParent(); DDI->eraseFromParent();
LBI.deleteValue(DDI);
}
++NumLocalPromoted; ++NumLocalPromoted;
} }