-
Notifications
You must be signed in to change notification settings - Fork 830
Sourcemap support for Binaryen C/JS #1392
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.
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
Conversation
kripken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just the ABI issue concerns me.
| // Serialize a module into binary form including its source map. Uses the currently set | ||
| // global debugInfo option. | ||
| // @returns how many bytes were written. This will be less than or equal to outputSize | ||
| BinaryenBufferSizes BinaryenModuleWriteWithSourceMap(BinaryenModuleRef module, const char* url, char* output, size_t outputSize, char* sourceMap, size_t sourceMapSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returning a struct has some odd ABI issues in C that I'd rather avoid. instead, how about returning the output size as in BinaryenModuleWrite, and adding a parameter with a pointer to write the source map bytes into?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually exactly how output works (seen from the JS-side), just with two such buffers. What's done there is that the user pre-allocates a buffer or two on the JS side and provides their pointers including their maximum length. The function then returns the number of bytes written so that the JS side can trim the buffer, and in this specific case it has to return two (hence the struct).
There is a FIXME on the JS side because these buffers are currently capped to 1MB. I guess if we solve this one, the API might change anyway. From JS perspective, it would be ideal not to pre-allocate anything and just receive a properly sized buffer incl. its length from the function. With the ABI stuff, that'd look like
struct {
void* buffer;
size_t bufferSize;
}or something, and I don't currently have a better idea (can't use passing by reference from JS for example, except there's also an ABI for that?). Any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, if we could write just the source map without having to also write the binary, we could split that, making it two functions that return just size_t. Might require some changes to the binary writer, though, as source map generation is currently piggybacked on top of writing a binary (what about a specific sourcemap writer?). Wouldn't fix the hard-wired limit, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, an API variant that mallocs the buffer internally and returns it, and requires the caller to free it later, would be a good way to solve the 1MB limit. Alternatively the zlib API has a method that can upper bound the size needed for a buffer. That's less easy for us, but maybe there's a way to that, which would be better (avoid malloc, the user might have another preferred allocation method).
| if (!outputBuffer) outputBuffer = _malloc(MAX); | ||
| if (!sourceMapBuffer) sourceMapBuffer = _malloc(MAX); | ||
| return preserveStack(function() { | ||
| Module['_BinaryenModuleWriteWithSourceMap'](temp, module, strToStack(sourceMapUrl), outputBuffer, MAX, sourceMapBuffer, MAX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(looks like this depends on the ABI issues mentioned above, it assumes the output is pointed to by an extra param at the beginning? I'm actually surprised it's not at the end.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just reused what's done for BinaryenLiteral here, which is also a returned struct, above, and reused its temporary buffer. I actually don't know how exactly this works :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Yeah, we already do depend on those ABI details I guess... no big deal to add this, then.
|
Btw. there is one thing about source maps that made me wonder, but that's more about how Firefox handles them and not really regarding Binaryen. If I use "myModule.wasm.map" as the sourceMapURL instead of, let's say, "http://127.0.0.1:8080/myModule.wasm.map", Firefox issues a warning that the WASM's source map url isn't a valid URL. My assumption is that this is either still in the works or actually necessary for a specific reason / a result of using the fetch API? |
|
Hmm, I'm not sure. cc @yurydelendik who might know the answer to the above question? |
|
@dcodeIO There are two methods of WebAssembly module instantiation: via |
|
Thanks, yes, makes sense! |
This PR adds C- and JS-APIs for the source map functionality that is currently in the works.
C:
JS:
One thing that feels somewhat edgy to me is that a function must have been created already in order to set debug locations on its body expressions. Hence, a user must keep its own map of expressions to debug locations while generating function bodies, and apply them once the function has been added.