Skip to content

[RT] Add type to tables and element segments #3763

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Apr 6, 2021
Merged

[RT] Add type to tables and element segments #3763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7f4bc4c
[RT] Add type to tables and elem segments
martianboy Mar 29, 2021
bde8f94
update tests
martianboy Apr 2, 2021
fb2abda
a little bit of polish
martianboy Apr 2, 2021
a5da3d4
wasm2js: error out if there's a non-function table
martianboy Apr 2, 2021
eb76258
make FlatTable invalid if it's non-function type
martianboy Apr 2, 2021
2b07dc9
fix the fuzzer
martianboy Apr 5, 2021
8c3e93e
trigger another build
martianboy Apr 5, 2021
900b176
and another...
martianboy Apr 5, 2021
af98cc6
Merge remote-tracking branch 'upstream/main' into table-types
martianboy Apr 6, 2021
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
2 changes: 1 addition & 1 deletion src/binaryen-c.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3359,7 +3359,7 @@ BinaryenTableRef BinaryenAddTable(BinaryenModuleRef module,
const char* name,
BinaryenIndex initial,
BinaryenIndex maximum) {
auto table = Builder::makeTable(name, initial, maximum);
auto table = Builder::makeTable(name, Type::funcref, initial, maximum);
table->hasExplicitName = true;
return ((Module*)module)->addTable(std::move(table));
}
Expand Down
5 changes: 4 additions & 1 deletion src/ir/element-utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ namespace ElementUtils {
template<typename T>
inline void iterElementSegmentFunctionNames(ElementSegment* segment,
T visitor) {
// TODO(reference-types): return early if segment type is non-funcref
if (!segment->type.isFunction()) {
return;
}

for (Index i = 0; i < segment->data.size(); i++) {
if (auto* get = segment->data[i]->dynCast<RefFunc>()) {
visitor(get->func, i);
Expand Down
19 changes: 13 additions & 6 deletions src/ir/module-splitting.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,16 @@ void TableSlotManager::addSlot(Name func, Slot slot) {
TableSlotManager::TableSlotManager(Module& module) : module(module) {
// TODO: Reject or handle passive element segments

if (module.tables.empty()) {
auto it = std::find_if(module.tables.begin(),
module.tables.end(),
[&](std::unique_ptr<Table>& table) {
return table->type == Type::funcref;
});
if (it == module.tables.end()) {
return;
}

activeTable = module.tables.front().get();
activeTable = it->get();
ModuleUtils::iterTableSegments(
module, activeTable->name, [&](ElementSegment* segment) {
activeTableSegments.push_back(segment);
Expand All @@ -172,6 +177,7 @@ TableSlotManager::TableSlotManager(Module& module) : module(module) {
// append new items at constant offsets after all existing items at constant
// offsets.
if (activeTableSegments.size() == 1 &&
activeTableSegments[0]->type == Type::funcref &&
!activeTableSegments[0]->offset->is<Const>()) {
assert(activeTableSegments[0]->offset->is<GlobalGet>() &&
"Unexpected initializer instruction");
Expand Down Expand Up @@ -204,7 +210,8 @@ TableSlotManager::TableSlotManager(Module& module) : module(module) {
}

Table* TableSlotManager::makeTable() {
return module.addTable(Builder::makeTable(Name::fromInt(0)));
return module.addTable(
Builder::makeTable(Names::getValidTableName(module, Name::fromInt(0))));
}

TableSlotManager::Slot TableSlotManager::getSlot(RefFunc* entry) {
Expand Down Expand Up @@ -533,7 +540,7 @@ void ModuleSplitter::setupTablePatching() {

auto offset = ExpressionManipulator::copy(primarySeg->offset, secondary);
auto secondarySeg = std::make_unique<ElementSegment>(
secondaryTable->name, offset, secondaryElems);
secondaryTable->name, offset, secondaryTable->type, secondaryElems);
secondarySeg->setName(primarySeg->name, primarySeg->hasExplicitName);
secondary.addElementSegment(std::move(secondarySeg));
return;
Expand All @@ -545,8 +552,8 @@ void ModuleSplitter::setupTablePatching() {
std::vector<Expression*> currData;
auto finishSegment = [&]() {
auto* offset = Builder(secondary).makeConst(int32_t(currBase));
auto secondarySeg =
std::make_unique<ElementSegment>(secondaryTable->name, offset, currData);
auto secondarySeg = std::make_unique<ElementSegment>(
secondaryTable->name, offset, secondaryTable->type, currData);
secondarySeg->setName(Name::fromInt(secondary.elementSegments.size()),
false);
secondary.addElementSegment(std::move(secondarySeg));
Expand Down
11 changes: 10 additions & 1 deletion src/ir/module-utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ inline ElementSegment* copyElementSegment(const ElementSegment* segment,
auto copy = [&](std::unique_ptr<ElementSegment>&& ret) {
ret->name = segment->name;
ret->hasExplicitName = segment->hasExplicitName;
ret->type = segment->type;
ret->data.reserve(segment->data.size());
for (auto* item : segment->data) {
ret->data.push_back(ExpressionManipulator::copy(item, out));
Expand All @@ -92,9 +93,11 @@ inline ElementSegment* copyElementSegment(const ElementSegment* segment,
}
}

inline Table* copyTable(Table* table, Module& out) {
inline Table* copyTable(const Table* table, Module& out) {
auto ret = std::make_unique<Table>();
ret->name = table->name;
ret->hasExplicitName = table->hasExplicitName;
ret->type = table->type;
ret->module = table->module;
ret->base = table->base;

Expand Down Expand Up @@ -510,6 +513,12 @@ inline void collectHeapTypes(Module& wasm,
for (auto& curr : wasm.events) {
counts.note(curr->sig);
}
for (auto& curr : wasm.tables) {
counts.maybeNote(curr->type);
}
for (auto& curr : wasm.elementSegments) {
counts.maybeNote(curr->type);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tlively is this enough for making sure table & elem types are counted correctly everywhere, or have I missed something?

Copy link
Member

Choose a reason for hiding this comment

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

This looks good to me!


// Collect info from functions in parallel.
ModuleUtils::ParallelFunctionAnalysis<Counts> analysis(
Expand Down
2 changes: 1 addition & 1 deletion src/ir/table-utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ struct FlatTable {
ModuleUtils::iterTableSegments(
wasm, table.name, [&](ElementSegment* segment) {
auto offset = segment->offset;
if (!offset->is<Const>()) {
if (!offset->is<Const>() || !segment->type.isFunction()) {
// TODO: handle some non-constant segments
valid = false;
return;
Expand Down
8 changes: 4 additions & 4 deletions src/passes/Print.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2619,7 +2619,8 @@ struct PrintSExpression : public UnifiedExpressionVisitor<PrintSExpression> {
if (curr->hasMax()) {
o << ' ' << curr->max;
}
o << " funcref)";
o << ' ';
printType(o, curr->type, currModule) << ')';
}
void visitTable(Table* curr) {
if (curr->imported()) {
Expand Down Expand Up @@ -2656,9 +2657,9 @@ struct PrintSExpression : public UnifiedExpressionVisitor<PrintSExpression> {
});
auto printElemType = [&]() {
if (allElementsRefFunc) {
TypeNamePrinter(o, currModule).print(HeapType::func);
o << "func";
} else {
TypeNamePrinter(o, currModule).print(Type::funcref);
printType(o, curr->type, currModule);
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to print the value type (e.g. anyref) or the heap type (e.g. any)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be a value type. My understanding is that the only heap type allowed here is func which is used in the abbreviated form that doesn't use expressions. In all other cases, it should be a value type and items should be expressions.

}
};

Expand All @@ -2671,7 +2672,6 @@ struct PrintSExpression : public UnifiedExpressionVisitor<PrintSExpression> {
}

if (curr->table.is()) {
// TODO(reference-types): check for old-style based on the complete spec
if (!allElementsRefFunc || currModule->tables.size() > 1) {
// tableuse
o << " (table ";
Expand Down
16 changes: 14 additions & 2 deletions src/shell-interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,13 @@ struct ShellExternalInterface : ModuleInstance::ExternalInterface {
wasm.memory.initial = 1;
wasm.memory.max = 2;
}

ModuleUtils::iterImportedTables(wasm, [&](Table* table) {
if (table->module == SPECTEST && table->base == TABLE) {
table->initial = 10;
table->max = 20;
Comment on lines +149 to +150
Copy link
Member

Choose a reason for hiding this comment

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

Where do these numbers come from?

Copy link
Contributor Author

@martianboy martianboy Apr 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

From the reference interpreter! This is an old mechanism for to link a few default global when running spec tests. More recent tests have (register ... instruction for this purpose, although we don't support it yet in wasm-shell.

I was trying to get more spec tests to pass, so I decided to dig into the differences between wasm-shell and the reference interpreter. Looks like table initialization from element segments is fully handled at module instantiation, including the bounds check. We check bounds at validation, which still makes this change useless. There are test cases like:

(module
  (import "spectest" "table" (table 0 funcref))
  (func $f)
  (elem (i32.const 0) $f)
)

that are a validation error in wasm-shell but pass in the reference interpreter, because the former (incorrectly) takes the table's initial to be 0, whereas the latter imports the table instance and sees that it has an initial of 10.

Again, to be clear, these difference haven't been fully addressed in this PR, so I may remove these from this PR as well if you think that's cleaner.

}
});
}

Literals callImport(Function* import, LiteralList& arguments) override {
Expand Down Expand Up @@ -234,8 +241,13 @@ struct ShellExternalInterface : ModuleInstance::ExternalInterface {
memory.set<std::array<uint8_t, 16>>(addr, value);
}

void tableStore(Name tableName, Address addr, Literal entry) override {
tables[tableName][addr] = entry;
void tableStore(Name tableName, Address addr, const Literal& entry) override {
auto& table = tables[tableName];
if (addr >= table.size()) {
trap("out of bounds table access");
} else {
table.emplace(table.begin() + addr, entry);
}
}

bool growMemory(Address /*oldSize*/, Address newSize) override {
Expand Down
45 changes: 32 additions & 13 deletions src/tools/fuzzing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,18 +426,31 @@ class TranslateToFuzzReader {

// TODO(reference-types): allow the fuzzer to create multiple tables
void setupTables() {
// Ensure an element segment, adding one or even adding a whole table as
// needed.
if (wasm.tables.empty()) {
auto table = builder.makeTable(
Names::getValidTableName(wasm, "fuzzing_table"), 0, 0);
table->hasExplicitName = true;
wasm.addTable(std::move(table));
}
if (wasm.elementSegments.empty()) {
// Ensure a funcref element segment and table exist. Segments with more
// specific function types may have a smaller chance of getting functions.
Table* table = nullptr;
auto iter =
std::find_if(wasm.tables.begin(), wasm.tables.end(), [&](auto& table) {
return table->type == Type::funcref;
});
if (iter != wasm.tables.end()) {
table = iter->get();
} else {
auto tablePtr = builder.makeTable(
Names::getValidTableName(wasm, "fuzzing_table"), Type::funcref, 0, 0);
tablePtr->hasExplicitName = true;
table = wasm.addTable(std::move(tablePtr));
}
bool hasFuncrefElemSegment = std::any_of(
wasm.elementSegments.begin(),
wasm.elementSegments.end(),
[&](auto& segment) {
return segment->table.is() && segment->type == Type::funcref;
});
if (!hasFuncrefElemSegment) {
// TODO: use a random table
auto segment = std::make_unique<ElementSegment>(
wasm.tables[0]->name, builder.makeConst(int32_t(0)));
table->name, builder.makeConst(int32_t(0)));
segment->setName(Names::getValidElementSegmentName(wasm, "elem$"), false);
wasm.addElementSegment(std::move(segment));
}
Expand Down Expand Up @@ -722,10 +735,16 @@ class TranslateToFuzzReader {
}
// add some to an elem segment
while (oneIn(3) && !finishedInput) {
auto& randomElem =
wasm.elementSegments[upTo(wasm.elementSegments.size())];
// FIXME: make the type NonNullable when we support it!
auto type = Type(HeapType(func->sig), Nullable);
std::vector<ElementSegment*> compatibleSegments;
ModuleUtils::iterActiveElementSegments(
wasm, [&](ElementSegment* segment) {
if (Type::isSubType(type, segment->type)) {
compatibleSegments.push_back(segment);
}
});
auto& randomElem = compatibleSegments[upTo(compatibleSegments.size())];
// FIXME: make the type NonNullable when we support it!
randomElem->data.push_back(builder.makeRefFunc(func->name, type));
}
numAddedFunctions++;
Expand Down
3 changes: 2 additions & 1 deletion src/tools/wasm-ctor-eval.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,8 @@ struct CtorEvalExternalInterface : EvallingModuleInstance::ExternalInterface {
}

// called during initialization, but we don't keep track of a table
void tableStore(Name tableName, Address addr, Literal value) override {}
void tableStore(Name tableName, Address addr, const Literal& value) override {
}

bool growMemory(Address /*oldSize*/, Address newSize) override {
throw FailToEvalException("grow memory");
Expand Down
7 changes: 5 additions & 2 deletions src/wasm-builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,13 @@ class Builder {
return func;
}

static std::unique_ptr<Table>
makeTable(Name name, Address initial = 0, Address max = Table::kMaxSize) {
static std::unique_ptr<Table> makeTable(Name name,
Type type = Type::funcref,
Address initial = 0,
Address max = Table::kMaxSize) {
auto table = std::make_unique<Table>();
table->name = name;
table->type = type;
table->initial = initial;
table->max = max;

Expand Down
3 changes: 2 additions & 1 deletion src/wasm-interpreter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2213,7 +2213,8 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
WASM_UNREACHABLE("unimp");
}

virtual void tableStore(Name tableName, Address addr, Literal entry) {
virtual void
tableStore(Name tableName, Address addr, const Literal& entry) {
WASM_UNREACHABLE("unimp");
}
};
Expand Down
13 changes: 9 additions & 4 deletions src/wasm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1678,13 +1678,17 @@ class ElementSegment : public Named {
public:
Name table;
Expression* offset;
Type type = Type::funcref;
std::vector<Expression*> data;

ElementSegment() = default;
ElementSegment(Name table, Expression* offset)
: table(table), offset(offset) {}
ElementSegment(Name table, Expression* offset, std::vector<Expression*>& init)
: table(table), offset(offset) {
ElementSegment(Name table, Expression* offset, Type type = Type::funcref)
: table(table), offset(offset), type(type) {}
ElementSegment(Name table,
Expression* offset,
Type type,
std::vector<Expression*>& init)
: table(table), offset(offset), type(type) {
data.swap(init);
}
};
Expand All @@ -1698,6 +1702,7 @@ class Table : public Importable {

Address initial = 0;
Address max = kMaxSize;
Type type = Type::funcref;

bool hasMax() { return max != kUnlimitedSize; }
void clear() {
Expand Down
Loading