Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions scripts/fuzz_opt.py
Original file line number Diff line number Diff line change
Expand Up @@ -350,10 +350,24 @@ def is_git_repo():
'typed_continuations_contnew.wast',
'typed_continuations_contbind.wast',
'typed_continuations_suspend.wast',
# New EH implementation is in progress
# TODO: make sure the fuzzer supports the new EH
'coalesce-locals-eh.wast',
'code-folding-eh.wast',
'code-pushing-eh.wast',
'dce-eh.wast',
'eh.wast',
'eh-gc.wast',
'exception-handling.wast',
'gufa-eh.wast',
'simplify-locals-eh.wast',
'translate-to-new-eh.wast',
'rse-eh.wast',
'vacuum-eh.wast',
# These contain parts with new EH.
# TODO: split those parts out if we see that fuzzing new EH is far out.
'global-effects.wast',
'local-subtyping.wast',
'renamings.wat',
]


Expand Down Expand Up @@ -1643,6 +1657,9 @@ def get_random_opts():
print('avoiding --flatten due to multivalue + reference types not supporting it (spilling of non-nullable tuples)')
print('TODO: Resolving https://github.com/WebAssembly/binaryen/issues/4824 may fix this')
continue
if '--enable-exception-handling' in FEATURE_OPTS:
print('avoiding --flatten due to exception-handling not supporting it (requires blocks with results)')
continue
if '--gc' not in FEATURE_OPTS:
print('avoiding --flatten due to GC not supporting it (spilling of non-nullable locals)')
continue
Expand Down Expand Up @@ -1701,7 +1718,7 @@ def get_random_opts():
# some features depend on other features, so if a required feature is
# disabled, its dependent features need to be disabled as well.
IMPLIED_FEATURE_OPTS = {
'--disable-reference-types': ['--disable-gc', '--disable-strings'],
'--disable-reference-types': ['--disable-gc', '--disable-exception-handling', '--disable-strings'],
'--disable-gc': ['--disable-strings'],
}

Expand Down
7 changes: 6 additions & 1 deletion src/ir/ReFinalize.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,12 @@ void ReFinalize::visitTableGrow(TableGrow* curr) { curr->finalize(); }
void ReFinalize::visitTableFill(TableFill* curr) { curr->finalize(); }
void ReFinalize::visitTableCopy(TableCopy* curr) { curr->finalize(); }
void ReFinalize::visitTry(Try* curr) { curr->finalize(); }
void ReFinalize::visitTryTable(TryTable* curr) { curr->finalize(); }
void ReFinalize::visitTryTable(TryTable* curr) {
curr->finalize();
for (size_t i = 0; i < curr->catchDests.size(); i++) {
updateBreakValueType(curr->catchDests[i], curr->sentTypes[i]);
}
}
void ReFinalize::visitThrow(Throw* curr) { curr->finalize(); }
void ReFinalize::visitRethrow(Rethrow* curr) { curr->finalize(); }
void ReFinalize::visitThrowRef(ThrowRef* curr) { curr->finalize(); }
Expand Down
26 changes: 26 additions & 0 deletions src/ir/effects.h
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,14 @@ class EffectAnalyzer {
self->pushTask(doStartTry, currp);
return;
}
if (auto* tryTable = curr->dynCast<TryTable>()) {
// We need to increment try depth before starting.
self->pushTask(doEndTryTable, currp);
self->pushTask(doVisitTryTable, currp);
self->pushTask(scan, &tryTable->body);
self->pushTask(doStartTryTable, currp);
return;
}
PostWalker<InternalAnalyzer, OverriddenVisitor<InternalAnalyzer>>::scan(
self, currp);
}
Expand Down Expand Up @@ -472,6 +480,24 @@ class EffectAnalyzer {
self->parent.catchDepth--;
}

static void doStartTryTable(InternalAnalyzer* self, Expression** currp) {
auto* curr = (*currp)->cast<TryTable>();
// We only count 'try_table's with a 'catch_all' because instructions
// within a 'try_table' without a 'catch_all' can still throw outside of
// the try.
if (curr->hasCatchAll()) {
self->parent.tryDepth++;
}
}

static void doEndTryTable(InternalAnalyzer* self, Expression** currp) {
auto* curr = (*currp)->cast<TryTable>();
if (curr->hasCatchAll()) {
assert(self->parent.tryDepth > 0 && "try depth cannot be negative");
self->parent.tryDepth--;
}
}

void visitBlock(Block* curr) {
if (curr->name.is()) {
parent.breakTargets.erase(curr->name); // these were internal breaks
Expand Down
6 changes: 6 additions & 0 deletions src/ir/linear-execution.h
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,12 @@ struct LinearExecutionWalker : public PostWalker<SubType, VisitorType> {
self->pushTask(SubType::scan, &curr->cast<Try>()->body);
break;
}
case Expression::Id::TryTableId: {
self->pushTask(SubType::doVisitTryTable, currp);
self->pushTask(SubType::doNoteNonLinear, currp);
self->pushTask(SubType::scan, &curr->cast<TryTable>()->body);
break;
}
case Expression::Id::ThrowId: {
self->pushTask(SubType::doVisitThrow, currp);
self->pushTask(SubType::doNoteNonLinear, currp);
Expand Down
34 changes: 32 additions & 2 deletions src/ir/possible-contents.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1134,8 +1134,38 @@ struct InfoCollector
}
}
void visitTryTable(TryTable* curr) {
// TODO: optimize when possible
addRoot(curr);
receiveChildValue(curr->body, curr);

// Connect caught tags with their branch targets, and materialize non-null
// exnref values.
auto numTags = curr->catchTags.size();
for (Index tagIndex = 0; tagIndex < numTags; tagIndex++) {
auto tag = curr->catchTags[tagIndex];
auto target = curr->catchDests[tagIndex];

Index exnrefIndex = 0;
if (tag.is()) {
auto params = getModule()->getTag(tag)->sig.params;

for (Index i = 0; i < params.size(); i++) {
if (isRelevant(params[i])) {
info.links.push_back(
{TagLocation{tag, i},
BreakTargetLocation{getFunction(), target, i}});
}
}

exnrefIndex = params.size();
}

if (curr->catchRefs[tagIndex]) {
Location location = CaughtExnRefLocation{};
addRoot(location,
PossibleContents::fromType(Type(HeapType::exn, NonNullable)));
info.links.push_back(
{location, BreakTargetLocation{getFunction(), target, exnrefIndex}});
}
}
}
void visitThrow(Throw* curr) {
auto& operands = curr->operands;
Expand Down
11 changes: 11 additions & 0 deletions src/ir/possible-contents.h
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,12 @@ struct TagLocation {
}
};

// The location of an exnref materialized by a catch_ref or catch_all_ref clause
// of a try_table.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// of a try_table.
// of a try_table. No data is stored here (exnrefs contain a stack at runtime, but we don't track that), so this is the same as NullLocation in a way: we just need *a* source of contents for places that receive an exnref.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I read GUFA code so my understanding is bad, but why are all exnrefs treated the same? All nulls are actually the same value and can be used interchangeably, but each exnref contains a different payload and tag, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the payload separate from the exnref? When we catch with an exnref we jump to a block that gets the payload as multivalues + an exnref at the end IIUC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GUFA doesn't do any abstract interpretation, AFAICT. It only tracks specific literals, and unknown things of certain types. So all unknown (ref exn) are the same anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exnref conceptually contains the payload, even though it is opaque, no? I think we can treat it in the same way we treat externref or anyref, given that they also contain something opaque. How do we treat them in GUFA? Do we treat all of them as the same literal?

Copy link
Member

@aheejin aheejin Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new EH (exnref) scheme, when we rethrow an exnref using throw_ref, we rethrow the whole package, including a tag and a payload. So exnref contains both.

Oh, I didn't realize that. So when we catch, we are sending the payload as values, and also sending it wrapped in the exnref object (which also has the stack and tag id)? So we send it both wrapped and unwrapped?

No I think, at least conceptually (because the actual implementation in the engines can differ), we just send a package (=exnref). And when caught, depending on the kind of catch clauses, we provide the exnref as is or extract values from it and provide the values.

I think we can treat it in the same way we treat externref or anyref, given that they also contain something opaque.

Makes sense, yeah. That is basically what the code does. E.g. if an import returns an anyref, then we know nothing about it but the type, so we fill that ResultLocation (result of a function) with addRoot, which just sets it to an unknown value of that type. The code here does the same, but we do need a Location to read the exnref from (it isn't a ResultLocation or anything else we have, yet), so we add that as well.

Still not sure why we need a separate location kind, given that externref/anyref doesn't have any. exnref here is just a return value from a block, so it can be treated as such, no? Do we need a separate Location kind for every kind of block return value? Or it can be treated the same way as a return value of a br, because it sends the info as the block return value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is, where does the block return value receive the value from? anyref arrives from e.g. a function result, which we already have a Location type for. But here we are not just adding a new type but also a new control flow construct that sends that value.

But maybe we could instead add a root on the target of the branch, though, is that what you mean? I suppose there is just one such target, and we know exactly what it is. I think that would be safe and correct to do here (that location can't be marked as a root for any other reason), but it does seem a little cleaner to me to keep the code as it is, since the branch target is not conceptually a "root" (e.g. it may receive values from other branches than this, so it needs to combine them all. We do know that this unknown value will "win", but conceptually we are still combining there).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see. Then I guess even though all exnrefs are treated as the same constant it wouldn't matter in GUFA because it doesn't do anything with constants, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we just move constants around, but don't do anything with the values otherwise. So this should be ok as is, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see. Then I guess even though all exnrefs are treated as the same constant it wouldn't matter in GUFA because it doesn't do anything with constants, right?

They're not treated like the same constant. They're all treated as some unknown value of type (ref exn). GUFA makes a difference between actual constants and unknown values of some type. If we did register them as the same constant, that would be bad, as GUFA would then propagate that constant at all destinations.

struct CaughtExnRefLocation {
bool operator==(const CaughtExnRefLocation& other) const { return true; }
};

// A null value. This is used as the location of the default value of a var in a
// function, a null written to a struct field in struct.new_with_default, etc.
struct NullLocation {
Expand Down Expand Up @@ -520,6 +526,7 @@ using Location = std::variant<ExpressionLocation,
SignatureResultLocation,
DataLocation,
TagLocation,
CaughtExnRefLocation,
NullLocation,
ConeReadLocation>;

Expand Down Expand Up @@ -608,6 +615,10 @@ template<> struct hash<wasm::TagLocation> {
}
};

template<> struct hash<wasm::CaughtExnRefLocation> {
size_t operator()(const wasm::CaughtExnRefLocation& loc) const { return 0; }
};

template<> struct hash<wasm::NullLocation> {
size_t operator()(const wasm::NullLocation& loc) const {
return std::hash<wasm::Type>{}(loc.type);
Expand Down
19 changes: 19 additions & 0 deletions src/literal.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ namespace wasm {

class Literals;
struct GCData;
struct ExnData;

class Literal {
// store only integers, whose bits are deterministic. floats
Expand All @@ -44,6 +45,7 @@ class Literal {
int64_t i64;
uint8_t v128[16];
// funcref function name. `isNull()` indicates a `null` value.
// TODO: handle cross-module calls using something other than a Name here.
Name func;
// A reference to GC data, either a Struct or an Array. For both of those we
// store the referred data as a Literals object (which is natural for an
Expand All @@ -56,6 +58,8 @@ class Literal {
// reference as its sole value even though internal i31 references do not
// have a gcData.
std::shared_ptr<GCData> gcData;
// A reference to Exn data.
std::shared_ptr<ExnData> exnData;
};

public:
Expand Down Expand Up @@ -85,6 +89,7 @@ class Literal {
assert(type.isSignature());
}
explicit Literal(std::shared_ptr<GCData> gcData, HeapType type);
explicit Literal(std::shared_ptr<ExnData> exnData);
explicit Literal(std::string_view string);
Literal(const Literal& other);
Literal& operator=(const Literal& other);
Expand All @@ -96,6 +101,7 @@ class Literal {
// Whether this is GC data, that is, something stored on the heap (aside from
// a null or i31). This includes structs, arrays, and also strings.
bool isData() const { return type.isData(); }
bool isExn() const { return type.isExn(); }
bool isString() const { return type.isString(); }

bool isNull() const { return type.isNull(); }
Expand Down Expand Up @@ -303,6 +309,7 @@ class Literal {
return func;
}
std::shared_ptr<GCData> getGCData() const;
std::shared_ptr<ExnData> getExnData() const;

// careful!
int32_t* geti32Ptr() {
Expand Down Expand Up @@ -732,6 +739,18 @@ struct GCData {
GCData(HeapType type, Literals values) : type(type), values(values) {}
};

// The data of a (ref exn) literal.
struct ExnData {
// The tag of this exn data.
// TODO: handle cross-module calls using something other than a Name here.
Name tag;

// The payload of this exn data.
Literals payload;

ExnData(Name tag, Literals payload) : tag(tag), payload(payload) {}
};

} // namespace wasm

namespace std {
Expand Down
21 changes: 12 additions & 9 deletions src/passes/CodeFolding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -343,15 +343,18 @@ struct CodeFolding : public WalkerPass<ControlFlowWalker<CodeFolding>> {
if (effects.danglingPop) {
return false;
}
// When an expression can throw and it is within a try scope, taking it
// out of the try scope changes the program's behavior, because the
// expression that would otherwise have been caught by the try now
// throws up to the next try scope or even up to the caller. We restrict
// the move if 'outOf' contains a 'try' anywhere in it. This is a
// conservative approximation because there can be cases that 'try' is
// within the expression that may throw so it is safe to take the
// expression out.
if (effects.throws() && !FindAll<Try>(outOf).list.empty()) {
// When an expression can throw and it is within a try/try_table scope,
// taking it out of the try/try_table scope changes the program's
// behavior, because the expression that would otherwise have been
// caught by the try/try_table now throws up to the next try/try_table
// scope or even up to the caller. We restrict the move if 'outOf'
// contains a 'try' or 'try_table' anywhere in it. This is a
// conservative approximation because there can be cases that
// 'try'/'try_table' is within the expression that may throw so it is
// safe to take the expression out.
// TODO: optimize this check to avoid two FindAlls.
if (effects.throws() &&
(FindAll<Try>(outOf).has() || FindAll<TryTable>(outOf).has())) {
return false;
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/passes/DeadCodeElimination.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,12 @@ struct DeadCodeElimination
tryy->body->type == Type::unreachable && allCatchesUnreachable) {
typeUpdater.changeType(tryy, Type::unreachable);
}
} else if (auto* tryTable = curr->dynCast<TryTable>()) {
// try_table can finish normally only if its body finishes normally.
if (tryTable->type != Type::unreachable &&
tryTable->body->type == Type::unreachable) {
typeUpdater.changeType(tryTable, Type::unreachable);
}
} else {
WASM_UNREACHABLE("unimplemented DCE control flow structure");
}
Expand Down
6 changes: 3 additions & 3 deletions src/passes/SimplifyLocals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,9 @@ struct SimplifyLocals
Expression** currp) {
Expression* curr = *currp;

// Certain expressions cannot be sinked into 'try', and so at the start of
// 'try' we forget about them.
if (curr->is<Try>()) {
// Certain expressions cannot be sinked into 'try'/'try_table', and so at
// the start of 'try'/'try_table' we forget about them.
if (curr->is<Try>() || curr->is<TryTable>()) {
std::vector<Index> invalidated;
for (auto& [index, info] : self->sinkables) {
// Expressions that may throw cannot be moved into a try (which might
Expand Down
11 changes: 10 additions & 1 deletion src/passes/Vacuum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ struct Vacuum : public WalkerPass<ExpressionStackWalker<Vacuum>> {
// Some instructions have special handling in visit*, and we should do
// nothing for them here.
if (curr->is<Drop>() || curr->is<Block>() || curr->is<If>() ||
curr->is<Loop>() || curr->is<Try>()) {
curr->is<Loop>() || curr->is<Try>() || curr->is<TryTable>()) {
return curr;
}
// Check if this expression itself has side effects, ignoring children.
Expand Down Expand Up @@ -435,6 +435,15 @@ struct Vacuum : public WalkerPass<ExpressionStackWalker<Vacuum>> {
}
}

void visitTryTable(TryTable* curr) {
// If try_table's body does not throw, the whole try_table can be replaced
// with the try_table's body.
if (!EffectAnalyzer(getPassOptions(), *getModule(), curr->body).throws()) {
replaceCurrent(curr->body);
return;
}
}

void visitFunction(Function* curr) {
auto* optimized =
optimize(curr->body, curr->getResults() != Type::none, true);
Expand Down
Loading