Skip to content

Commit 4a58116

Browse files
aheejinaheejin
committed
[WebAssembly] Fix more ExceptionInfo grouping bugs
This fixes two bugs in `WebAssemblyExceptionInfo` grouping, created by D97247. These two bugs are not easy to split into two different CLs, because tests that fail for one also tend to fail for the other. - In D97247, when fixing `ExceptionInfo` grouping by taking out the unwind destination' exception from the unwind src's exception, we just iterated the BBs in the function order, but this was incorrect; this changes it to dominator tree preorder. Please refer to the comments in the code for the reason and an example. - After this subexception-taking-out fix, there still can be remaining BBs we have to take out. When Exception B is taken out of Exception A (because EHPad B is the unwind destination of EHPad A), there can still be BBs within Exception A that are reachable from Exception B, which also should be taken out. Please refer to the comments in the code for more detailed explanation on why this can happen. To make this possible, this splits `WebAssemblyException::addBlock` into two parts: adding to a set and adding to a vector. We need to iterate on BBs within a `WebAssemblyException` to fix this, so we add BBs to sets first. But we add BBs to vectors later after we fix all incorrectness because deleting BBs from vectors is expensive. I considered removing the vector from `WebAssemblyException`, but it was not easy because this class has to maintain a similar interface with `MachineLoop` to be wrapped into a single interface `SortRegion`, which is used in CFGSort. Other misc. drive-by fixes: - Make `WebAssemblyExceptionInfo` do not even run when wasm EH is not used or the function doesn't have any EH pads, not to waste time - Add `LLVM_DEBUG` lines for easy debugging - Fix `preds` comments in cfg-stackify-eh.ll - Fix `__cxa_throw`'s signature in cfg-stackify-eh.ll Fixes emscripten-core/emscripten#13554. Reviewed By: dschuff, tlively Differential Revision: https://reviews.llvm.org/D97677
1 parent 7b6fc9a commit 4a58116

File tree

3 files changed

+315
-21
lines changed

3 files changed

+315
-21
lines changed

‎llvm/lib/Target/WebAssembly/WebAssemblyExceptionInfo.cpp

Lines changed: 112 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
#include "llvm/CodeGen/MachineDominators.h"
2020
#include "llvm/CodeGen/WasmEHFuncInfo.h"
2121
#include "llvm/InitializePasses.h"
22+
#include "llvm/MC/MCAsmInfo.h"
23+
#include "llvm/Target/TargetMachine.h"
2224

2325
using namespace llvm;
2426

@@ -38,9 +40,37 @@ bool WebAssemblyExceptionInfo::runOnMachineFunction(MachineFunction &MF) {
3840
"********** Function: "
3941
<< MF.getName() << '\n');
4042
releaseMemory();
43+
if (MF.getTarget().getMCAsmInfo()->getExceptionHandlingType() !=
44+
ExceptionHandling::Wasm ||
45+
!MF.getFunction().hasPersonalityFn())
46+
return false;
4147
auto &MDT = getAnalysis<MachineDominatorTree>();
4248
auto &MDF = getAnalysis<MachineDominanceFrontier>();
4349
recalculate(MF, MDT, MDF);
50+
LLVM_DEBUG(dump());
51+
return false;
52+
}
53+
54+
// Check if Dst is reachable from Src using BFS. Search only within BBs
55+
// dominated by Header.
56+
static bool isReachableAmongDominated(const MachineBasicBlock *Src,
57+
const MachineBasicBlock *Dst,
58+
const MachineBasicBlock *Header,
59+
const MachineDominatorTree &MDT) {
60+
assert(MDT.dominates(Header, Dst));
61+
SmallVector<const MachineBasicBlock *, 8> WL;
62+
SmallPtrSet<const MachineBasicBlock *, 8> Visited;
63+
WL.push_back(Src);
64+
65+
while (!WL.empty()) {
66+
const auto *MBB = WL.pop_back_val();
67+
if (MBB == Dst)
68+
return true;
69+
Visited.insert(MBB);
70+
for (auto *Succ : MBB->successors())
71+
if (!Visited.count(Succ) && MDT.dominates(Header, Succ))
72+
WL.push_back(Succ);
73+
}
4474
return false;
4575
}
4676

@@ -83,30 +113,103 @@ void WebAssemblyExceptionInfo::recalculate(
83113
// Here we extract those unwind destinations from their (incorrect) parent
84114
// exception. Note that the unwind destinations may not be an immediate
85115
// children of the parent exception, so we have to traverse the parent chain.
116+
//
117+
// We should traverse BBs in the preorder of the dominator tree, because
118+
// otherwise the result can be incorrect. For example, when there are three
119+
// exceptions A, B, and C and A > B > C (> is subexception relationship here),
120+
// and A's unwind destination is B and B's is C. When we visit B before A, we
121+
// end up extracting C only out of B but not out of A.
86122
const auto *EHInfo = MF.getWasmEHFuncInfo();
87-
for (auto &MBB : MF) {
88-
if (!MBB.isEHPad())
123+
DenseMap<WebAssemblyException *, WebAssemblyException *> UnwindWEMap;
124+
for (auto *DomNode : depth_first(&MDT)) {
125+
MachineBasicBlock *EHPad = DomNode->getBlock();
126+
if (!EHPad->isEHPad())
89127
continue;
90-
auto *EHPad = &MBB;
91128
if (!EHInfo->hasUnwindDest(EHPad))
92129
continue;
93130
auto *UnwindDest = EHInfo->getUnwindDest(EHPad);
94131
auto *WE = getExceptionFor(EHPad);
95-
auto *UnwindDestWE = getExceptionFor(UnwindDest);
96-
if (WE->contains(UnwindDestWE)) {
132+
auto *UnwindWE = getExceptionFor(UnwindDest);
133+
if (WE->contains(UnwindWE)) {
134+
UnwindWEMap[WE] = UnwindWE;
135+
LLVM_DEBUG(dbgs() << "ExceptionInfo fix: " << WE->getEHPad()->getNumber()
136+
<< "." << WE->getEHPad()->getName()
137+
<< "'s exception is taken out of "
138+
<< UnwindWE->getEHPad()->getNumber() << "."
139+
<< UnwindWE->getEHPad()->getName() << "'s exception\n");
97140
if (WE->getParentException())
98-
UnwindDestWE->setParentException(WE->getParentException());
141+
UnwindWE->setParentException(WE->getParentException());
99142
else
100-
UnwindDestWE->setParentException(nullptr);
143+
UnwindWE->setParentException(nullptr);
144+
}
145+
}
146+
147+
// Add BBs to exceptions' block set first
148+
for (auto *DomNode : post_order(&MDT)) {
149+
MachineBasicBlock *MBB = DomNode->getBlock();
150+
WebAssemblyException *WE = getExceptionFor(MBB);
151+
for (; WE; WE = WE->getParentException())
152+
WE->addToBlocksSet(MBB);
153+
}
154+
155+
// After fixing subexception relationship between unwind destinations above,
156+
// there can still be remaining discrepancies.
157+
//
158+
// For example, suppose Exception A is dominated by EHPad A and Exception B is
159+
// dominated by EHPad B. EHPad A's unwind destination is EHPad B, but because
160+
// EHPad B is dominated by EHPad A, the initial grouping makes Exception B a
161+
// subexception of Exception A, and we fix it by taking Exception B out of
162+
// Exception A above. But there can still be remaining BBs within Exception A
163+
// that are reachable from Exception B. These BBs semantically don't belong
164+
// to Exception A and were not a part of 'catch' clause or cleanup code in the
165+
// original code, but they just happened to be grouped within Exception A
166+
// because they were dominated by EHPad A. We fix this case by taking those
167+
// BBs out of the incorrect exception and all its subexceptions that it
168+
// belongs to.
169+
for (auto &KV : UnwindWEMap) {
170+
WebAssemblyException *WE = KV.first;
171+
WebAssemblyException *UnwindWE = KV.second;
172+
173+
for (auto *MBB : WE->getBlocksSet()) {
174+
if (MBB->isEHPad()) {
175+
// If this assertion is triggered, it would be a violation of scoping
176+
// rules in ll files, because this means an instruction in an outer
177+
// scope tries to unwind to an EH pad in an inner scope.
178+
assert(!isReachableAmongDominated(UnwindWE->getEHPad(), MBB,
179+
WE->getEHPad(), MDT) &&
180+
"Outer scope unwinds to inner scope. Bug in scope rules?");
181+
continue;
182+
}
183+
if (isReachableAmongDominated(UnwindWE->getEHPad(), MBB, WE->getEHPad(),
184+
MDT)) {
185+
LLVM_DEBUG(dbgs() << "Remainder BB: " << MBB->getNumber() << "."
186+
<< MBB->getName() << " is ");
187+
WebAssemblyException *InnerWE = getExceptionFor(MBB);
188+
while (InnerWE != WE) {
189+
LLVM_DEBUG(dbgs()
190+
<< " removed from " << InnerWE->getEHPad()->getNumber()
191+
<< "." << InnerWE->getEHPad()->getName()
192+
<< "'s exception\n");
193+
InnerWE->removeFromBlocksSet(MBB);
194+
InnerWE = InnerWE->getParentException();
195+
}
196+
WE->removeFromBlocksSet(MBB);
197+
LLVM_DEBUG(dbgs() << " removed from " << WE->getEHPad()->getNumber()
198+
<< "." << WE->getEHPad()->getName()
199+
<< "'s exception\n");
200+
changeExceptionFor(MBB, WE->getParentException());
201+
if (WE->getParentException())
202+
WE->getParentException()->addToBlocksSet(MBB);
203+
}
101204
}
102205
}
103206

104-
// Add BBs to exceptions
207+
// Add BBs to exceptions' block vector
105208
for (auto DomNode : post_order(&MDT)) {
106209
MachineBasicBlock *MBB = DomNode->getBlock();
107210
WebAssemblyException *WE = getExceptionFor(MBB);
108211
for (; WE; WE = WE->getParentException())
109-
WE->addBlock(MBB);
212+
WE->addToBlocksVector(MBB);
110213
}
111214

112215
SmallVector<WebAssemblyException*, 8> ExceptionPointers;

‎llvm/lib/Target/WebAssembly/WebAssemblyExceptionInfo.h

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ class WebAssemblyException {
4545
WebAssemblyException *ParentException = nullptr;
4646
std::vector<std::unique_ptr<WebAssemblyException>> SubExceptions;
4747
std::vector<MachineBasicBlock *> Blocks;
48-
SmallPtrSet<const MachineBasicBlock *, 8> BlockSet;
48+
SmallPtrSet<MachineBasicBlock *, 8> BlockSet;
4949

5050
public:
5151
WebAssemblyException(MachineBasicBlock *EHPad) : EHPad(EHPad) {}
@@ -68,6 +68,9 @@ class WebAssemblyException {
6868
return BlockSet.count(MBB);
6969
}
7070

71+
void addToBlocksSet(MachineBasicBlock *MBB) { BlockSet.insert(MBB); }
72+
void removeFromBlocksSet(MachineBasicBlock *MBB) { BlockSet.erase(MBB); }
73+
void addToBlocksVector(MachineBasicBlock *MBB) { Blocks.push_back(MBB); }
7174
void addBlock(MachineBasicBlock *MBB) {
7275
Blocks.push_back(MBB);
7376
BlockSet.insert(MBB);
@@ -81,8 +84,10 @@ class WebAssemblyException {
8184
}
8285
unsigned getNumBlocks() const { return Blocks.size(); }
8386
std::vector<MachineBasicBlock *> &getBlocksVector() { return Blocks; }
87+
SmallPtrSetImpl<MachineBasicBlock *> &getBlocksSet() { return BlockSet; }
8488

85-
const std::vector<std::unique_ptr<WebAssemblyException>> &getSubExceptions() const {
89+
const std::vector<std::unique_ptr<WebAssemblyException>> &
90+
getSubExceptions() const {
8691
return SubExceptions;
8792
}
8893
std::vector<std::unique_ptr<WebAssemblyException>> &getSubExceptions() {
@@ -149,7 +154,8 @@ class WebAssemblyExceptionInfo final : public MachineFunctionPass {
149154
return BBMap.lookup(MBB);
150155
}
151156

152-
void changeExceptionFor(MachineBasicBlock *MBB, WebAssemblyException *WE) {
157+
void changeExceptionFor(const MachineBasicBlock *MBB,
158+
WebAssemblyException *WE) {
153159
if (!WE) {
154160
BBMap.erase(MBB);
155161
return;

0 commit comments

Comments
 (0)