-
Notifications
You must be signed in to change notification settings - Fork 787
Update binaryen.js, fixes #1028, fixes #1029 #1030
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
@@ -200,7 +200,7 @@ extern "C" void EMSCRIPTEN_KEEPALIVE instantiate() { | |||
var source = Module['HEAP8'].subarray($1, $1 + $2); | |||
var target = new Int8Array(Module['asmExports']['memory']); | |||
target.set(source, $0); | |||
}, ConstantExpressionRunner(instance.globals).visit(segment.offset).value.geti32(), &segment.data[0], segment.data.size()); | |||
}, ConstantExpressionRunner<TrivialGlobalManager>(instance.globals).visit(segment.offset).value.geti32(), &segment.data[0], segment.data.size()); |
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.
FYI: It simply didn't compile without this change but I actually don't know if TrivialGlobalManager
is the right type here.
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, that looks right.
Daniel, could you please join the WebAssembly Community Group for this contribution? |
Just joined the group! |
@@ -299,12 +299,13 @@ BinaryenOp BinaryenCurrentMemory(void) { return CurrentMemory; } | |||
BinaryenOp BinaryenGrowMemory(void) { return GrowMemory; } | |||
BinaryenOp BinaryenHasFeature(void) { return HasFeature; } | |||
|
|||
BinaryenExpressionRef BinaryenBlock(BinaryenModuleRef module, const char* name, BinaryenExpressionRef* children, BinaryenIndex numChildren) { | |||
BinaryenExpressionRef BinaryenBlock(BinaryenModuleRef module, const char* name, BinaryenExpressionRef* children, BinaryenIndex numChildren, BinaryenType type) { |
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.
For reference, this is where the C API breaks while JS does not, which is unfortunate of course, Though, I require this to emulate what teeLocal
does for locals, but for globals. More precisely, I am using a typed block there that first performs a setGlobal
followed by a getGlobal
, which is the block's result, effectively doing the same (though not nearly thread-safe) as a teeGlobal
instruction, if it'd exist.
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 agree, this looks like a necessary C API change. (We should have done it when we changed how block types work.)
648f424
to
05fc024
Compare
src/binaryen-c.cpp
Outdated
auto* ret = ((Module*)module)->allocator.alloc<Block>(); | ||
if (name) ret->name = name; | ||
for (BinaryenIndex i = 0; i < numChildren; i++) { | ||
ret->list.push_back((Expression*)children[i]); | ||
} | ||
if (type) ret->type = WasmType(type); | ||
ret->finalize(); |
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 better to instead call finalize(type)
if a type is provided, as it is more efficient
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.
actually there is something ambiguous here. if type is 0, that means the none type, and we could also call finalize(none)
. but that means that there is no way for the user to say "figure out the type" (the old way) as opposed to "i am giving you the type". As the code is written right now, 0 means "figure out the type" instead of "none".
I guess we could add an api for making blocks without providing the type. but maybe it's fine to just change this to do finalize(type)
regardless of the value of type
. Thoughts?
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.
What do you think about using -1
to explicitly force the previous behavior (if that is possible at all)? The JS wrapper could then also check for undefined
as you suggested. If that's not possible, I'd of course be ok with dropping the old behavior - I actually didn't know about it, which is why I missed finalize
.
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 like the idea of using -1 for the old behavior. Seems low-risk for confusion.
src/binaryen-c.cpp
Outdated
// | ||
|
||
#ifdef EMSCRIPTEN | ||
int __cxa_thread_atexit(void(*)(char*), char*, char*) |
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.
did you figure out where this was needed? it's possibly a bug in emscripten that we should figure out
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 wasn't able to pinpoint it unfortunately. For what it's worth, I previously tried setting -s USE_PTHREADS=0
and -s NO_EXIT_RUNTIME=1
, but compilation still resulted in the same issue, if that helps and if these flags are connected to the issue at all
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.
Ok, I investigated that. Needed a little fix in emscripten, emscripten-core/emscripten#5261 . Nothing to block us here.
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, except to remove the definitions of __cxa_thread_atexit
. Which I guess we can only do once that other PR lands.
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.
To make sure, I also added TODO comments in my last commit referencing the PR.
src/js/binaryen.js-post.js
Outdated
@@ -171,10 +171,10 @@ | |||
}); | |||
}; | |||
|
|||
this['block'] = function(name, children) { | |||
this['block'] = function(name, children, type) { |
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.
for backwards compatibility, we could in theory see if type
is undefined, and set it to none
otherwise. this should happen anyhow since its coerced to an int, but i think it would be nicer to do so explicitly.
src/wasm-js.cpp
Outdated
@@ -561,3 +561,8 @@ extern "C" void EMSCRIPTEN_KEEPALIVE call_from_js(const char *target) { | |||
else if (ret.type == f64) EM_ASM_({ Module['tempReturn'] = $0 }, ret.getf64()); | |||
else abort(); | |||
} | |||
|
|||
extern "C" int __cxa_thread_atexit(void(*)(char*), char*, char*) |
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.
same question as before, and, is this needed twice?
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.
Ultimately decided to put it in the main API entrypoint of both. wasm.js seems to assume that it is compiled with Emscripten, while binaryen-c can be compiled both as a C library as well as with Emscripten. These files are not included in the respective other library / tool.
69336c8
to
efc8da8
Compare
src/binaryen-c.cpp
Outdated
@@ -94,6 +94,7 @@ BinaryenType BinaryenInt32(void) { return i32; } | |||
BinaryenType BinaryenInt64(void) { return i64; } | |||
BinaryenType BinaryenFloat32(void) { return f32; } | |||
BinaryenType BinaryenFloat64(void) { return f64; } | |||
BinaryenType BinaryenAuto(void) { return -1; } |
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.
How about BinaryenUndefined
?
src/binaryen-c.cpp
Outdated
@@ -305,8 +306,8 @@ BinaryenExpressionRef BinaryenBlock(BinaryenModuleRef module, const char* name, | |||
for (BinaryenIndex i = 0; i < numChildren; i++) { | |||
ret->list.push_back((Expression*)children[i]); | |||
} | |||
if (type) ret->type = WasmType(type); | |||
ret->finalize(); | |||
if (type != -1) ret->finalize(WasmType(type)); |
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 suggest replacing the -1
with a call to that function.
src/binaryen-c.h
Outdated
@@ -64,13 +64,14 @@ typedef uint32_t BinaryenIndex; | |||
// Core types (call to get the value of each; you can cache them, they | |||
// never change) | |||
|
|||
typedef uint32_t BinaryenType; | |||
typedef int32_t BinaryenType; |
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.
this should still be unsigned. might require writing uint32_t(-1)
if the compiler complains.
src/binaryen-c.h
Outdated
@@ -64,13 +64,14 @@ typedef uint32_t BinaryenIndex; | |||
// Core types (call to get the value of each; you can cache them, they | |||
// never change) |
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.
Please add a comment here about the new addition, that it is not a core type, but represents "no type defined / no type passed into the API" or something like that.
test/example/c-api-kitchen-sink.c
Outdated
@@ -182,7 +182,7 @@ void test_core() { | |||
makeBinary(module, BinaryenGtFloat64(), 4), | |||
makeBinary(module, BinaryenGeFloat32(), 3), | |||
// All the rest | |||
BinaryenBlock(module, NULL, NULL, 0, 0), // block with no name and no type | |||
BinaryenBlock(module, NULL, NULL, 0, -1), // block with no name and no type |
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.
instead of these -1
s, let's call the function
0a0421c
to
bcd67ad
Compare
src/binaryen-c.cpp
Outdated
@@ -321,7 +321,10 @@ BinaryenExpressionRef BinaryenBlock(BinaryenModuleRef module, const char* name, | |||
auto id = noteExpression(ret); | |||
std::cout << " expressions[" << id << "] = BinaryenBlock(the_module, "; | |||
traceNameOrNULL(name); | |||
std::cout << ", children, " << numChildren << ", " << type << ");\n"; | |||
std::cout << ", children, " << numChildren << ", "; | |||
if (type == BinaryenUndefined()) std::cout << "BinaryenUndefined()"; |
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.
For improved readability, it might be desirable to do this for all types in every traced function. That's some boring work, though, and I haven't yet managed to run the tests locally, so I decided not to touch this.
src/binaryen-c.h
Outdated
@@ -262,7 +266,7 @@ BinaryenOp BinaryenHasFeature(void); | |||
typedef void* BinaryenExpressionRef; | |||
|
|||
// Block: name can be NULL |
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.
please also add another comment about the use of BinaryenUndefined for the last value, and what that means
Ok, I think that's the last comment I have here, should be good to merge with that. |
I know this is bad timing, but I already started work on these additional functions. I also wonder if there isn't an optimization pass already that joins duplicate function signatures / eliminates unused ones? |
src/binaryen-c.cpp
Outdated
// ===== Emscripten support ===== | ||
// | ||
|
||
// TODO: remove this function once https://github.com/kripken/emscripten/pull/5261 lands |
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.
the emscripten PR fixing this landed, so this can be removed
src/wasm-js.cpp
Outdated
@@ -561,3 +561,9 @@ extern "C" void EMSCRIPTEN_KEEPALIVE call_from_js(const char *target) { | |||
else if (ret.type == f64) EM_ASM_({ Module['tempReturn'] = $0 }, ret.getf64()); | |||
else abort(); | |||
} | |||
|
|||
// TODO: remove this function once https://github.com/kripken/emscripten/pull/5261 lands |
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.
same, can be removed now
The It would also be a good idea, as you suggest, to remove duplicate function types. Not sure offhand if it's better to add a new pass or maybe rework the one mentioned before into |
src/binaryen-c.cpp
Outdated
@@ -163,6 +163,32 @@ BinaryenFunctionTypeRef BinaryenAddFunctionType(BinaryenModuleRef module, const | |||
return ret; | |||
} | |||
|
|||
BinaryenFunctionTypeRef BinaryenGetFunctionType(BinaryenModuleRef module, BinaryenType result, BinaryenType* paramTypes, BinaryenIndex numParams) { | |||
auto* wasm = (Module*)module; | |||
auto* ret = new FunctionType; |
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.
why not just do FunctionType ret
?
(also maybe rename to test
as it isn't returned)
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.
Just copied this from the respective add function to make sure that I don't accidentally break something. I'm not super experienced in C++, so I am just super cautious most of the time. Going to rename it.
src/binaryen-c.h
Outdated
@@ -103,6 +103,7 @@ typedef void* BinaryenFunctionTypeRef; | |||
// Add a new function type. This is thread-safe. | |||
// Note: name can be NULL, in which case we auto-generate a name | |||
BinaryenFunctionTypeRef BinaryenAddFunctionType(BinaryenModuleRef module, const char* name, BinaryenType result, BinaryenType* paramTypes, BinaryenIndex numParams); | |||
BinaryenFunctionTypeRef BinaryenGetFunctionType(BinaryenModuleRef module, BinaryenType result, BinaryenType* paramTypes, BinaryenIndex numParams); |
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 am not sure how I feel about adding this API call. One issue is that we may eventually add getters by name, so this one might collide with the natural name for that one. Perhaps BinaryenGetStructuralFunctionType
?
Another issue is that while this seems like a nice utility, it could be done by the user of the library that is creating the function types? Generally the binaryen API has been kept pretty minimal, and this feels like an "extra" - it's not strictly necessary, in other words. Another option to adding or not adding this might be to add it in a new section of the API (utilities? helpers?).
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.
My use case here is that I am compiling a couple of functions (i.e. malloc etc.) from C first, then loading the resulting module into binaryen.js for further processing (adding more functions). Without BinaryenGetStructuralFunctionType
(or how it'll be named finally, suggesting BinaryenGetFunctionTypeBySignature
), there is no way to obtain a reference to any of the existing signatures (even if I know their types exactly, or their names even when these become 0
, 1
, etc. after compilation without -g
), and I had to create a duplicate signature instead. This is somewhat related to the optimizer not yet merging / removing unused signatures.
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 see. So if we had an optimizer pass that does this, that would be good enough? Sounds like it would make all this simpler, avoiding adding a new API.
We need to add such an optimizer pass anyhow. Should be easy to write. I can do that unless you want to.
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.
Sure, but the problem would remain if the optimizer wasn't part of the plan, i.e. because the user actually intends to keep unused functions so these can be used in the next build step (i.e. after linking something else). That's related to the only options there are either use the optimizer with default options or don't use the optimizer at all when using binaryen.js.
Regarding the optimization pass: If you could add this that'd be great, as it will solve other issues I have here with signatures being still present after optimization.
I personally felt that adding GetFunctionType (or GetStructuralFunctionType, or GetFunctionTypeBySignature), using pretty much the same parameters as AddFunctionType, would make a good API addition especially because it doesn't require a shift in the way how to obtain an existing function type when a user already knows how to add one.
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.
We could add an API call to run a pass by name, yeah, right now it's pretty limited to optimize or not. Is that what you mean in the first paragraph?
Ok, I'll add that optimization.
I might be wrong, but to me it feels like such a getter could be implemented in JS, if we provided enough lower-level functionality, which feels more natural to me. Open to other opinions here 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.
My issue is just that I am already using this functionality, but you are right, it's mostly because of the lack of an alternative. What if we'd add this for now, with a name different than GetFunctionType
as you suggested, and I add a comment that it is subject to be removed again once a better alternative is in place? Unfortunately, I haven't yet obtained the understanding how to return actual data, instead of just a pointer which is what most of the code currently does, to JS. Otherwise, I'd of course love to contribute something better than that.
Regarding the first paragraph: Somewhat, I am fine with the default optimizations within my exact use case. It's just as you said, there is no better alternative yet.
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.
PR for that optimization: #1041
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, I'm ok with your suggestion: let's add your API for now, with a note that we'll remove it when there is a better way to do it from JS. BinaryenGetFunctionTypeBySignature
sounds good. Let's put it at the end of the API section in the header, under a header "utilities" or such.
Edit: Seems I got it all wrong and just have to update incoming instead of switching to master, sec... |
3fe483f
to
8ecd608
Compare
…bly#1029 This PR adds global variable support (addGlobal, getGlobal, setGlobal), host operations (currentMemory, growMemory), a few utility functions (removeImport, removeExport, getFunctionTypeBySignature with the latter being scheduled for removal once a better alternative is in place) and it introduces an additional argument to specify the result type in BinaryenBlock (effectively breaking the C-API but retaining previous behaviour by introducing the BinaryenUndefined() type for this purpose). Additionally, it enables compilation with exception support in build-js.sh as exceptions are thrown and caught when optimizing endless loops, intentionally resulting in an unreachable opcode. Affected test cases have been updated accordingly.
8ecd608
to
470af23
Compare
Rebased on master, updated the binaries and squashed the commits. Might be a good idea to recompile again once your changes to RemoveUnusedModuleElements have been merged. |
Great, thanks! And very nice work on AssemblyScript btw, great project. |
Thanks! FYI: I published the recent JS build to npm under the binaryen name so I and others can easily depend on it from our packages. If you'd like to take this over and/or have an idea how we could automate publishing to npm, just drop me a note! |
Good idea, I've been meaning to add it to npm but didn't find the time. Not sure how automation would best be done, but worth thinking about - especially if we start updating binaryen.js more frequently, which we should. |
Having it on NPM is a really good idea since it'll make it easier for people using JS to make tools with binaryen. Is there anything I could do to help with the effort of automating the publishes? |
Going to try building on Travis CI, but I have no idea how this will turn out due to time limits. Repo: https://github.com/dcodeIO/binaryen.js |
Would it be possible to add it to this repo as a PR instead? Then it could run whenever a git release passes |
As far as I am concerned, this can be moved to wherever it fits best, of course. Provided that I get it to work, that is. |
I think the question I'd have for @kripken is what the current process is for tagging releases on github. It might be that it'd be easy enough to get this thing on npm without having to set up any fancy travis scripts. You could add a package.json with Then anybody with access to npm could publish manually when a release is being made. I'm not seeing anything super fancy for tagging github releases, so I'm not sure if it's worth over-engineering the npm releases yet. |
I believe one issue here is that building binaryen.js differs from the other build targets quite a bit in that it requires an up-to-date installation of the Emscripten SDK (including a compatible install of LLVM/clang etc.) to be present. Of course compilation can be done manually, but if we can automate that, we probably should. |
Btw., it seems Travis is killing compilation because of memory exhaustion or something like that - as expected. One does not simply build LLVM/clang on Travis, I assume. |
Yeah, building LLVM/clang is pretty heavy. But the emsdk has a binary version for linux, which in theory could be used. However, it didn't just work, see emscripten-core/emscripten#5167 and emscripten-core/emscripten#5087 , something needs to be figured out there. |
Does travis allow docker images? It'd make sense to build a docker image with all the tools that are required and then re-use that as a basis for the image used to run tests or build the JS bundle. That way the image could be built on a dev machine instead of having travis do it. |
Seems that might be viable: https://docs.travis-ci.com/user/docker/ |
Somebody has already set up docker images with emscripten installed. Do these look like they'd work for this use-case? https://github.com/apiaryio/emscripten-docker |
Now simply tried installing emscripten-incoming from sources while using a clang nightly. As the issues linked above already noted, this then fails with:
I remember that I was able to fix that for another project by building with |
cc @juj, I think this came up earlier as well as a suggestion for building the emsdk for linux, but I'm not sure what was decided. |
I believe I got it working now (once ccache is saturated, a build takes about 10 minutes). One issue I have, though, is that build-js.sh only generates warnings like Further plans are to install a Travis cron job to build binaryen.js like once a day and publish it to npm under some special tag, like Edit: Added a simple test case loading the compiled file and performing a few operations. That should catch obviously broken builds at least. |
You can set |
This PR adds global variable support (addGlobal, getGlobal, setGlobal) and host operations (currentMemory, growMemory). Unfortunately, this only compiles with a 'unresolved symbol: __cxa_thread_atexit' warning in my setup (emscripten-incoming-64bit, Win10) resulting in an assertion error when trying to load binaryen.js. I can however confirm that the changes are working when I monkey-patch the resulting binaryen.js file to simply omit the assertion in question.
If there is anything I can change to fix this, please let me know.
Update: Additional commits add a
type
argument toBinaryenBlock
(this breaks the C-API, see below) to specify the block's return type, and add a parameter tobuild-js.sh
that enables exception catching because some parts of the optimizer seem to use exceptions to fall back to emit anunreachable
instruction. Going to squash once I'm done.