Use ImportResolver for table imports #8194
Conversation
stevenfontanella
force-pushed
the
table-import
branch
10 times, most recently
from
ebcd539 to
5c7736a
Compare
stevenfontanella
force-pushed
the
table-import
branch
10 times, most recently
from
34ac47d to
d465928
Compare
stevenfontanella
force-pushed
the
table-import
branch
from
d465928 to
f30f8c6
Compare
There was a problem hiding this comment.
Please make sure all the new files have the copyright header.
| virtual const Table* tableMeta() const = 0; | ||
| }; | ||
|
|
||
| class RealRuntimeTable : public RuntimeTable { |
There was a problem hiding this comment.
I'd prefer renaming RuntimeTable to RuntimeTableBase so we can name this one RuntimeTable.
| // Apply a reasonable limit on table size, 1GB, to avoid DOS on the | ||
| // interpreter. | ||
| if (newSize > 1024 * 1024 * 1024) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
We should probably carry this over to the new implementation.
| // of this instance and other instances. | ||
| std::map<Name, Literals*> allGlobals; | ||
|
|
||
| std::map<Name, RuntimeTable*> allTables; |
There was a problem hiding this comment.
Is there a reason these maps need to be ordered?
| // wasm spec only allows const and global.get there | ||
| WASM_UNREACHABLE("invalid expr type"); |
There was a problem hiding this comment.
This is no longer true. Let's add // TODO: Handle extended consts.
| // look in the proper range. if it instead gets a global, we rely on the | ||
| // fact that when not dynamically linking then the table is loaded at | ||
| // offset 0. |
There was a problem hiding this comment.
This is a very Emscripten-specific assumption. Let's add a TODO to make this safer, perhaps by adding an opt-in Emscripten mode.
| // No segment had a value for this. | ||
| return Literal::makeNull(HeapTypes::func); |
There was a problem hiding this comment.
Let's add a TODO to handle non-function tables.
| return table.initial; | ||
| } | ||
|
|
||
| virtual const Table* tableMeta() const override { return &table; } |
There was a problem hiding this comment.
Would it make sense to move this part into the base class? I can't imagine we would ever want to override this to do anything but return a pointer to an underlying Table.
|
|
||
| private: | ||
| Table table; | ||
| const bool& instanceInitialized; |
There was a problem hiding this comment.
Why take a reference to a bool rather than just copying it in?
| [this, &instanceInitialized](Literal initial, Table table) { | ||
| return std::make_unique<EvallingRuntimeTable>( | ||
| table, | ||
| instanceInitialized, | ||
| this->wasm, | ||
| [this](Name name, Type type) { return makeFuncData(name, type); }); | ||
| }) {} |
There was a problem hiding this comment.
😱
Is there a plan for avoiding callback hell here? It seems like pairing subclasses of ModuleRunnerBase with subclasses of RuntimeTable et. al. so that they all know about each other would let us avoid this indirection.
Part of #8180. We currently replicate the existing behavior. In the future, we can also change ctor-eval to really simulate tables, and only use the EvallingRuntimeTable in the case of table imports. Removes existing pointer chasing logic that we repeat for each table operation (e.g. tableLoad, tableStore).
Allows us to fix spec tests where a table import becomes valid by resizing it in #8222.