This repository was archived by the owner on Nov 3, 2021. It is now read-only.
Repository navigation
This repository was archived by the owner on Nov 3, 2021. It is now read-only.
Inadvertent exporting of Wasm functions #31
Description
The anyref/anyfunc allows a function to exported from a WASM module dynamically - as part of the returned value of another exported function. Similarly for imports.
This effectively blows apart any discipline for what is exported from a WASM module. It also complicates implementation because potentially every WASM function may become accessible from JS.
The table.set operator in conjunction with indirect calling of functions from any table allows monkey-patching of functions.
Activity
lukewagner commentedon Feb 14, 2019
Technically it's not any function; just functions that are statically referrenced by
ref.func. Tables are already readable/writable from JS (viaTable.prototype.getandset), so once aref.funcd function is stored in a table, it's no different than if it was added to the table viaelemsegment.Maybe the issue you're getting at, though, which I do think we should consider, is that, in the MVP, we know all the external (exported or stored in an external table) functions before the code section starts, whereas with
ref.func, we'll only know this by the end of the code section. This could negatively impact streaming parallel compilation. Given that we explicitly moved theelemsection from after to before thecodesection (way back when) specifically so that it was known before the code section, we might want to consider adding some sort of "aliasable" section which declares those functions (and later, other kinds of definitions too!) which were allowed to beref.funcd.titzer commentedon Feb 15, 2019
I agree with @lukewagner here. It'd be nice to know up front which functions are used in a first-class way, before the code section.
rossberg commentedon Feb 19, 2019
So, if I was a lazy producer that simply put all their functions into this section, what would be the penalty?
titzer commentedon Feb 19, 2019
On V8, the penalty is that we generate wrapper code per signature (essentially a machine code stub whose complexity is proportional to the number of arguments) plus a JavaScript function per Wasm function (minimum two objects--a JSFunction and a SharedFunctionInfo). We're working to make the generation of those wrappers cheaper (and happen in parallel), but the JSFunctions we'd need to do lazily to avoid the upfront cost.
rossberg commentedon Feb 21, 2019
Hm, how difficult would a lazy approach be for an implementation?
An extra section adds somewhat non-trivial language complexity in this case: we would need to decode that section, validate it, extend static function contexts to track the information whether a func is "first-class", employ that to validate uses of func.ref, etc. All doable, but somewhat ugly and ad-hoc.
Shame, I suppose a custom section for this sort of (primarily JS-relevant?) optimisation hint would not cut it, since you cannot rely on its being accurate.
lukewagner commentedon Mar 12, 2019
Indirect callability could end up influencing codegen for streaming non-JS hosts as well. I would think, spec-wise, this could be handled as a purely binary encoding detail, similar to how we did with the data segments up-front size declaration.
rossberg commentedon Mar 14, 2019
Yeah, I'm not sure. The "binary encoding detail" approach already became slightly hacky for the simple data count section. I suspect it would be borderline for something like this, which requires more involved validation. I'll think about it.
[-]Inadvertent exporting of WASM functions[/-][+]Inadvertent exporting of Wasm functions[/+]lukewagner commentedon Apr 24, 2019
I'm about to implement this; which means either harvesting
ref.funcd func-indices after-the-fact or adding a new declarative section, so it'd be nice to figure this out.I still think we should maintain the general property that all aliased functions are known before the code section. I realized that aliased functions are not just externally callable (requiring entry stubs, which should be lazily generated for a couple of reasons) but also indirectly callable (since any
funcrefcan be put into a table). For some impls, indirect calls may have a slightly different ABI with a second prologue that immediately precedes the normal prologue of indirectly-callable functions. Without an up-front declaration, streaming/parallel compilation would need to conservatively give every function an indirect call prologue, increasing codegen size. It's a modestly-compelling example, but given that we've already ensured this property in the MVP, I think it makes sense to continue to ensure it when adding features.Any new thoughts @titzer @rossberg ?
rossberg commentedon Apr 25, 2019
Ah, thanks for the reminder. I have been throwing this around in my head for a while and so far think that a clean and forward-looking solution involves the following:
Adding a "reference section" to the binary format. Its structure follows that of the export section, in that it can (potentially) list entities of different sort. For now we would only allow functions.
Extending the abstract syntax with "reference declarations", again similar to export declarations.
Validation would check that (a) all reference declarations refer to valid indices, and (b) that every use of
ref.func(or similar instructions in the future) only mentions indices declared as references. To that end, validation would require to track the set of declared reference indices as an additional context component.There would probably be a bunch of ad-hoc exceptions for uses of
ref.funcoutside function bodies, i.e., in element segments, which don't require a priori declaration?Since they become part of the AST, reference declarations would also need to occur in the text format, which, among other things, allows writing proper tests for this feature. We could add the same shortcuts as for exports.
I also have considered the idea of only making it a detail of the binary format. But it seems that would be stretching it too far in this case, since the validation conditions are substantially more extensive than just comparing two numbers.
WDYT?
rossberg commentedon Apr 25, 2019
FWIW, another option might be to piggyback on (and abuse) segments and the validation exception that we'll probably need to introduce for them anyway. That is:
Validation requires that ref.func only mentions functions that are also referenced inside an element segment. (For ref.func occurring in segments this condition holds vacuously.)
To avoid redundant allocation for auxiliary forward declaration segments, introduce a third kind of segment, unused (or declared), which is like a passive segment in that it does not perform any initialisation, but like an active one in that it is dropped implicitly.
This solution is a bit more hacky, but would require fewer extensions to AST, binary, and text format (only adding the new segment kind).
lars-t-hansen commentedon Apr 25, 2019
I think using element segments came up before but was shot down as too hacky. The declared bit (expressible more to the point?) reduces the hackiness by quite a lot, I think. If other kinds of values require a reference section in the future so be it, but until then we keep the complexity down.
rossberg commentedon Apr 25, 2019
Well, I wouldn't want to end up in a future where we have both. If we go for the segment approach, then we should design it such that the new segment kind is extensible to other reference kinds later.
lukewagner commentedon Apr 25, 2019
@rossberg Your "reference section" idea is exactly what I was imagining.
Regarding binary-encoding vs. validation: I think it'd be kindof a pain to have to forward-declare references in the text format, but if we have them implicitly declared/unified (as we do with function types), then we won't be able to write negative tests. If the distinction is not present in the text format, I'd be inclined to keep it in the binary format as well, but no strong opinion here.
Regarding the elem section idea: scanning the elem section encoding just now, it's already a bit hairy, so I'd be reticent to add another case. Plus, it raises some questions without very satisfying answers:
rossberg commentedon Apr 25, 2019
@lukewagner, wasn't the text format meant to be a faithful representation of the AST, plus some sugar? That implies that everything expressible in the AST should be expressible in the text format. Making ref decls implicit would violate that (unlike implicit function types, which are just a shorthand).
Analogous to
export, I'd imagine a declarationplus the shorthand
Regarding elem sections, I think question (1) may have to be answered either way: does the ref section have to repeat indices already mentioned in a segment? If no then the segment approach would actually make that more uniform. For (2), I was imagining something like
externkind vec(idx), closer to the existing format of active segments.I don't understand the distinction you are making between "stored in table" and "first-class". Under this proposal, you can always emulate
ref.funcvia elem section +table.get, so how could an engine treat these cases differently? (Without analysing all the code, because then you wouldn't need the forward declaration in the first place.)But in general, I agree that a proper reference section would be cleaner.
20 remaining items
binji commentedon May 13, 2019
I don't think it is --
table.initalso simplifies to a single bounds-check without requiring VM optimization.I can see
ref.nullbeing used this way.OK, but we could do that for bulk-memory anyway, since MVP only allows 1 memory, 1 table. TBH, I'd be OK with this change.
lukewagner commentedon May 13, 2019
Yes, but using the same manner of argument as with size: if it's removing branches we're optimizing for, performance would be further improved by removing the branching on the definition kind from the inner loop of
table.initby grouping indices by definition kind. Practically speaking though, I assume that, as a run-once, load-time op,table.initisn't being optimized and this distinction won't be meaningful to overall program performance (especially oncetable.set+ref.fucare properly inlined, which, as ops intended to be used on hot import/export call paths, they should be).But frequently interleaved throughout an otherwise contiguous segment? Note that this isn't even possible in the MVP's elem segment. If nothing else, this seems like a waste of binary size.
binji commentedon May 13, 2019
Let's discuss this tomorrow in the CG meeting: WebAssembly/meetings#405
lukewagner commentedon Aug 6, 2019
Just to confirm, is the consensus above that for a
ref.func $fto be valid,$fmust be used or declared in some elem segment (active/passive/declared) and, in particular, this does not include functions used in the exports section or the start section?rossberg commentedon Aug 8, 2019
Yes,
$fmust occur in an elem segment. It doesn't count to (only) have it occur as an export or start function.Bug 1546138 - Wasm: Implement 'ref.func'. r=bbouvier
Bug 1546138 - Wasm: Implement 'ref.func'. r=bbouvier
Bug 1546138 - Wasm: Implement 'ref.func'. r=bbouvier
Bug 1546138 - Wasm: Implement 'ref.func'. r=bbouvier
Bug 1546138 - Wasm: Implement 'ref.func'. r=bbouvier
lars-t-hansen commentedon Nov 15, 2019
@rossberg, we can close this. Even though @fgmccabe opened it to discuss something else, it is all about the declared element segments, and those have landed.
rossberg commentedon Nov 15, 2019
Actually, declared segments haven't landed yet, neither in the interpreter nor in the spec nor are there tests. I have an earlier version of them on a branch, but need to rebase on the bulk proposal, since that significantly changed the segment representation more recently. Discovered various holes in the spec & implementation of that proposal on the way that had to be fixed first.
I'll try to get to it next week.
Don't statically validate whether the segment index immediates of `me…
rossberg commentedon Jan 16, 2020
Fixed via #73.