-
Notifications
You must be signed in to change notification settings - Fork 818
[EH] Make rethrow's target a try label #3568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
513c04f
6484eb3
21d808b
dd18b40
9cbff53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2067,8 +2067,8 @@ Expression* SExpressionWasmBuilder::makeTry(Element& s) { | |
// We create a different name for the wrapping block, because try's name can | ||
// be used by internal delegates | ||
block->name = nameMapper.pushLabelName(sName); | ||
// For simplicity, try's name canonly be targeted by delegates. Make the | ||
// branches target the new wrapping block instead. | ||
// For simplicity, try's name can only be targeted by delegates and | ||
// rethrows. Make the branches target the new wrapping block instead. | ||
BranchUtils::replaceBranchTargets(ret, ret->name, block->name); | ||
block->list.push_back(ret); | ||
nameMapper.popLabelName(block->name); | ||
|
@@ -2078,29 +2078,6 @@ Expression* SExpressionWasmBuilder::makeTry(Element& s) { | |
return ret; | ||
} | ||
|
||
Expression* | ||
SExpressionWasmBuilder::makeTryOrCatchBody(Element& s, Type type, bool isTry) { | ||
if (isTry && !elementStartsWith(s, "do")) { | ||
throw ParseException("invalid try do clause", s.line, s.col); | ||
} | ||
if (!isTry && !elementStartsWith(s, "catch") && | ||
!elementStartsWith(s, "catch_all")) { | ||
throw ParseException("invalid catch clause", s.line, s.col); | ||
} | ||
if (s.size() == 1) { // (do) / (catch) / (catch_all) without instructions | ||
return makeNop(); | ||
} | ||
auto ret = allocator.alloc<Block>(); | ||
for (size_t i = 1; i < s.size(); i++) { | ||
ret->list.push_back(parseExpression(s[i])); | ||
} | ||
if (ret->list.size() == 1) { | ||
return ret->list[0]; | ||
} | ||
ret->finalize(type); | ||
return ret; | ||
} | ||
|
||
Comment on lines
-2081
to
-2103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method was already unused; it became unused in #3487 but wasn't deleted there. |
||
Expression* SExpressionWasmBuilder::makeThrow(Element& s) { | ||
auto ret = allocator.alloc<Throw>(); | ||
Index i = 1; | ||
|
@@ -2118,7 +2095,7 @@ Expression* SExpressionWasmBuilder::makeThrow(Element& s) { | |
|
||
Expression* SExpressionWasmBuilder::makeRethrow(Element& s) { | ||
auto ret = allocator.alloc<Rethrow>(); | ||
ret->depth = atoi(s[1]->str().c_str()); | ||
ret->target = getLabel(*s[1], LabelType::Exception); | ||
ret->finalize(); | ||
return ret; | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -236,7 +236,8 @@ struct FunctionValidator : public WalkerPass<PostWalker<FunctionValidator>> { | |||||||||||
}; | ||||||||||||
|
||||||||||||
std::unordered_map<Name, BreakInfo> breakInfos; | ||||||||||||
std::unordered_set<Name> exceptionTargetNames; | ||||||||||||
std::unordered_set<Name> delegateTargetNames; | ||||||||||||
std::unordered_set<Name> rethrowTargetNames; | ||||||||||||
|
||||||||||||
std::set<Type> returnTypes; // types used in returns | ||||||||||||
|
||||||||||||
|
@@ -280,7 +281,7 @@ struct FunctionValidator : public WalkerPass<PostWalker<FunctionValidator>> { | |||||||||||
static void visitPreTry(FunctionValidator* self, Expression** currp) { | ||||||||||||
auto* curr = (*currp)->cast<Try>(); | ||||||||||||
if (curr->name.is()) { | ||||||||||||
self->exceptionTargetNames.insert(curr->name); | ||||||||||||
self->delegateTargetNames.insert(curr->name); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
|
@@ -300,7 +301,8 @@ struct FunctionValidator : public WalkerPass<PostWalker<FunctionValidator>> { | |||||||||||
static void visitPreCatch(FunctionValidator* self, Expression** currp) { | ||||||||||||
auto* curr = (*currp)->cast<Try>(); | ||||||||||||
if (curr->name.is()) { | ||||||||||||
self->exceptionTargetNames.erase(curr->name); | ||||||||||||
self->delegateTargetNames.erase(curr->name); | ||||||||||||
self->rethrowTargetNames.insert(curr->name); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
|
@@ -376,7 +378,8 @@ struct FunctionValidator : public WalkerPass<PostWalker<FunctionValidator>> { | |||||||||||
void visitRefIs(RefIs* curr); | ||||||||||||
void visitRefFunc(RefFunc* curr); | ||||||||||||
void visitRefEq(RefEq* curr); | ||||||||||||
void noteException(Name name, Expression* curr); | ||||||||||||
void noteDelegate(Name name, Expression* curr); | ||||||||||||
void noteRethrow(Name name, Expression* curr); | ||||||||||||
void visitTry(Try* curr); | ||||||||||||
void visitThrow(Throw* curr); | ||||||||||||
void visitRethrow(Rethrow* curr); | ||||||||||||
|
@@ -2073,14 +2076,20 @@ void FunctionValidator::visitRefEq(RefEq* curr) { | |||||||||||
"ref.eq's right argument should be a subtype of eqref"); | ||||||||||||
} | ||||||||||||
|
||||||||||||
void FunctionValidator::noteException(Name name, Expression* curr) { | ||||||||||||
void FunctionValidator::noteDelegate(Name name, Expression* curr) { | ||||||||||||
if (name != DELEGATE_CALLER_TARGET) { | ||||||||||||
shouldBeTrue(exceptionTargetNames.find(name) != exceptionTargetNames.end(), | ||||||||||||
shouldBeTrue(delegateTargetNames.find(name) != delegateTargetNames.end(), | ||||||||||||
tlively marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||
curr, | ||||||||||||
"all delegate targets must be valid"); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
void FunctionValidator::noteRethrow(Name name, Expression* curr) { | ||||||||||||
shouldBeTrue(rethrowTargetNames.find(name) != rethrowTargetNames.end(), | ||||||||||||
curr, | ||||||||||||
"all rethrow targets must be valid"); | ||||||||||||
} | ||||||||||||
|
||||||||||||
void FunctionValidator::visitTry(Try* curr) { | ||||||||||||
shouldBeTrue(getModule()->features.hasExceptionHandling(), | ||||||||||||
curr, | ||||||||||||
|
@@ -2122,8 +2131,10 @@ void FunctionValidator::visitTry(Try* curr) { | |||||||||||
"try should have either catches or a delegate"); | ||||||||||||
|
||||||||||||
if (curr->isDelegate()) { | ||||||||||||
noteException(curr->delegateTarget, curr); | ||||||||||||
noteDelegate(curr->delegateTarget, curr); | ||||||||||||
} | ||||||||||||
|
||||||||||||
rethrowTargetNames.erase(curr->name); | ||||||||||||
} | ||||||||||||
|
||||||||||||
void FunctionValidator::visitThrow(Throw* curr) { | ||||||||||||
|
@@ -2167,8 +2178,7 @@ void FunctionValidator::visitRethrow(Rethrow* curr) { | |||||||||||
Type(Type::unreachable), | ||||||||||||
curr, | ||||||||||||
"rethrow's type must be unreachable"); | ||||||||||||
// TODO Validate the depth field. The current LLVM toolchain only generates | ||||||||||||
// depth 0 for C++ support. | ||||||||||||
noteRethrow(curr->target, curr); | ||||||||||||
} | ||||||||||||
|
||||||||||||
void FunctionValidator::visitTupleMake(TupleMake* curr) { | ||||||||||||
|
@@ -2533,9 +2543,12 @@ void FunctionValidator::visitFunction(Function* curr) { | |||||||||||
|
||||||||||||
shouldBeTrue( | ||||||||||||
breakInfos.empty(), curr->body, "all named break targets must exist"); | ||||||||||||
shouldBeTrue(exceptionTargetNames.empty(), | ||||||||||||
shouldBeTrue(delegateTargetNames.empty(), | ||||||||||||
curr->body, | ||||||||||||
"all named delegate targets must exist"); | ||||||||||||
shouldBeTrue(rethrowTargetNames.empty(), | ||||||||||||
curr->body, | ||||||||||||
"all named rethrow targets must exist"); | ||||||||||||
|
shouldBeTrue( | |
breakInfos.empty(), curr->body, "all named break targets must exist"); | |
shouldBeTrue(exceptionTargetNames.empty(), | |
curr->body, | |
"all named delegate targets must exist"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, is it possible to construct an invalid Binaryen IR module that would trigger this error? (I would ask the same questions of any of the other checks we have)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think so. Some flawed pass can assign a block
label to delegate
, or even a random string (unlikely), but anyway I don't think it's impossible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that would be caught by https://github.com/WebAssembly/binaryen/pull/3568/files#diff-2149be365b66a82038f1d3fa8f9fb1b4dcd40ab535c986f4bf0a4c37e669a1ceR2079-R2092, right? These checks are just checking that the target names were properly removed from their sets at the end of the corresponding scopes AFAICT, and it shouldn't be possible to create an invalid Binaryen module in which a try scope or catch scope never ends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think you're right. Then maybe I'll remove all those checks..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, you suggested to make them assertions... Will change them to assertions.
Uh oh!
There was an error while loading. Please reload this page.