Skip to content

Commit 1331bd3

Browse files
tlivelyradekdoulik
authored andcommitted
[Parser] Parse br_if correctly (WebAssembly#6202)
The new text parser and IRBuilder were previously not differentiating between `br` and `br_if`. Handle `br_if` correctly by popping and assigning a condition.
1 parent eabde28 commit 1331bd3

9 files changed

Lines changed: 234 additions & 146 deletions

File tree

scripts/gen-s-parser.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@
2424
("if", "makeIf(s)"),
2525
("then", "makeThenOrElse(s)"),
2626
("else", "makeThenOrElse(s)"),
27-
("br", "makeBreak(s)"),
28-
("br_if", "makeBreak(s)"),
27+
("br", "makeBreak(s, false)"),
28+
("br_if", "makeBreak(s, true)"),
2929
("br_table", "makeBreakTable(s)"),
3030
("return", "makeReturn(s)"),
3131
("call", "makeCall(s, /*isReturn=*/false)"),

src/gen-s-parser.inc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -102,12 +102,12 @@ switch (buf[0]) {
102102
case 'r': {
103103
switch (buf[2]) {
104104
case '\0':
105-
if (op == "br"sv) { return makeBreak(s); }
105+
if (op == "br"sv) { return makeBreak(s, false); }
106106
goto parse_error;
107107
case '_': {
108108
switch (buf[3]) {
109109
case 'i':
110-
if (op == "br_if"sv) { return makeBreak(s); }
110+
if (op == "br_if"sv) { return makeBreak(s, true); }
111111
goto parse_error;
112112
case 'o': {
113113
switch (buf[6]) {
@@ -3749,15 +3749,15 @@ switch (buf[0]) {
37493749
switch (buf[2]) {
37503750
case '\0':
37513751
if (op == "br"sv) {
3752-
CHECK_ERR(makeBreak(ctx, pos));
3752+
CHECK_ERR(makeBreak(ctx, pos, false));
37533753
return Ok{};
37543754
}
37553755
goto parse_error;
37563756
case '_': {
37573757
switch (buf[3]) {
37583758
case 'i':
37593759
if (op == "br_if"sv) {
3760-
CHECK_ERR(makeBreak(ctx, pos));
3760+
CHECK_ERR(makeBreak(ctx, pos, true));
37613761
return Ok{};
37623762
}
37633763
goto parse_error;

src/parser/contexts.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -411,7 +411,7 @@ struct NullInstrParserCtx {
411411
Result<> makeCallIndirect(Index, TableIdxT*, TypeUseT, bool) {
412412
return Ok{};
413413
}
414-
Result<> makeBreak(Index, LabelIdxT) { return Ok{}; }
414+
Result<> makeBreak(Index, LabelIdxT, bool) { return Ok{}; }
415415
Result<> makeSwitch(Index, const std::vector<LabelIdxT>&, LabelIdxT) {
416416
return Ok{};
417417
}
@@ -1586,8 +1586,8 @@ struct ParseDefsCtx : TypeParserCtx<ParseDefsCtx> {
15861586
return withLoc(pos, irBuilder.makeCallIndirect(*t, type, isReturn));
15871587
}
15881588

1589-
Result<> makeBreak(Index pos, Index label) {
1590-
return withLoc(pos, irBuilder.makeBreak(label));
1589+
Result<> makeBreak(Index pos, Index label, bool isConditional) {
1590+
return withLoc(pos, irBuilder.makeBreak(label, isConditional));
15911591
}
15921592

15931593
Result<>

src/parser/parsers.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ template<typename Ctx> Result<> makeMemoryFill(Ctx&, Index);
104104
template<typename Ctx> Result<> makePop(Ctx&, Index);
105105
template<typename Ctx> Result<> makeCall(Ctx&, Index, bool isReturn);
106106
template<typename Ctx> Result<> makeCallIndirect(Ctx&, Index, bool isReturn);
107-
template<typename Ctx> Result<> makeBreak(Ctx&, Index);
107+
template<typename Ctx> Result<> makeBreak(Ctx&, Index, bool isConditional);
108108
template<typename Ctx> Result<> makeBreakTable(Ctx&, Index);
109109
template<typename Ctx> Result<> makeReturn(Ctx&, Index);
110110
template<typename Ctx> Result<> makeRefNull(Ctx&, Index);
@@ -1432,10 +1432,11 @@ Result<> makeCallIndirect(Ctx& ctx, Index pos, bool isReturn) {
14321432
return ctx.makeCallIndirect(pos, table.getPtr(), *type, isReturn);
14331433
}
14341434

1435-
template<typename Ctx> Result<> makeBreak(Ctx& ctx, Index pos) {
1435+
template<typename Ctx>
1436+
Result<> makeBreak(Ctx& ctx, Index pos, bool isConditional) {
14361437
auto label = labelidx(ctx);
14371438
CHECK_ERR(label);
1438-
return ctx.makeBreak(pos, *label);
1439+
return ctx.makeBreak(pos, *label, isConditional);
14391440
}
14401441

14411442
template<typename Ctx> Result<> makeBreakTable(Ctx& ctx, Index pos) {

src/wasm-ir-builder.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ class IRBuilder : public UnifiedExpressionVisitor<IRBuilder, Result<>> {
8787
[[nodiscard]] Result<> makeBlock(Name label, Type type);
8888
[[nodiscard]] Result<> makeIf(Name label, Type type);
8989
[[nodiscard]] Result<> makeLoop(Name label, Type type);
90-
[[nodiscard]] Result<> makeBreak(Index label);
90+
[[nodiscard]] Result<> makeBreak(Index label, bool isConditional);
9191
[[nodiscard]] Result<> makeSwitch(const std::vector<Index>& labels,
9292
Index defaultLabel);
9393
// Unlike Builder::makeCall, this assumes the function already exists.

src/wasm-s-parser.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ class SExpressionWasmBuilder {
270270
}
271271
enum class LabelType { Break, Exception };
272272
Name getLabel(Element& s, LabelType labelType = LabelType::Break);
273-
Expression* makeBreak(Element& s);
273+
Expression* makeBreak(Element& s, bool isConditional);
274274
Expression* makeBreakTable(Element& s);
275275
Expression* makeReturn(Element& s);
276276
Expression* makeRefNull(Element& s);

src/wasm/wasm-ir-builder.cpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,11 @@ Result<Expression*> IRBuilder::getBranchValue(Name labelName,
430430
}
431431

432432
Result<> IRBuilder::visitBreak(Break* curr, std::optional<Index> label) {
433+
if (curr->condition) {
434+
auto cond = pop();
435+
CHECK_ERR(cond);
436+
curr->condition = *cond;
437+
}
433438
auto value = getBranchValue(curr->name, label);
434439
CHECK_ERR(value);
435440
curr->value = *value;
@@ -961,13 +966,15 @@ Result<> IRBuilder::makeLoop(Name label, Type type) {
961966
return visitLoopStart(loop);
962967
}
963968

964-
Result<> IRBuilder::makeBreak(Index label) {
969+
Result<> IRBuilder::makeBreak(Index label, bool isConditional) {
965970
auto name = getLabelName(label);
966971
CHECK_ERR(name);
967972
Break curr;
968973
curr.name = *name;
974+
// Use a dummy condition value if we need to pop a condition.
975+
curr.condition = isConditional ? &curr : nullptr;
969976
CHECK_ERR(visitBreak(&curr, label));
970-
push(builder.makeBreak(curr.name, curr.value));
977+
push(builder.makeBreak(curr.name, curr.value, curr.condition));
971978
return Ok{};
972979
}
973980

src/wasm/wasm-s-parser.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2580,15 +2580,15 @@ Name SExpressionWasmBuilder::getLabel(Element& s, LabelType labelType) {
25802580
}
25812581
}
25822582

2583-
Expression* SExpressionWasmBuilder::makeBreak(Element& s) {
2583+
Expression* SExpressionWasmBuilder::makeBreak(Element& s, bool isConditional) {
25842584
auto ret = allocator.alloc<Break>();
25852585
size_t i = 1;
25862586
ret->name = getLabel(*s[i]);
25872587
i++;
25882588
if (i == s.size()) {
25892589
return ret;
25902590
}
2591-
if (elementStartsWith(s, BR_IF)) {
2591+
if (isConditional) {
25922592
if (i + 1 < s.size()) {
25932593
ret->value = parseExpression(s[i]);
25942594
i++;

0 commit comments

Comments
 (0)