Initial commit to log functions in Wasm. #12764
Conversation
# Conflicts: # src/settings.js
There was a problem hiding this comment.
Nice and simple. Obviously would want some kind of test too, once the binaryen side lands.
I wonder if it should be called something a little more specific. INSTRUMENT_FUNCTION_CALLS? The name of the callback should probably more specific too to avoid colliding with user symbols. Perhaps it should even be configurable? -s INSTRUMENT_FUNCTION_CALLS=myfunc ?
| @@ -650,6 +650,9 @@ def check_human_readable_list(items): | |||
| extras = shared.Settings.BINARYEN_EXTRA_PASSES.split(',') | |||
| passes += [('--' + p) if p[0] != '-' else p for p in extras if p] | |||
|
|
|||
| if shared.Settings.INSTRUMENT: | |||
There was a problem hiding this comment.
I think this BINARYEN_EXTRA_PASSES should comes last go maybe move this up a few lines
There was a problem hiding this comment.
From our discussion last week, I believe the logging function will be hard-coded in Wasm so this will no longer be necessary. The instrumentation phase will also be executed from wasm-split so I think the code will be something like:
if shared.Settings.INSTRUMENT:
wasm-split instrument a.wasm ...
if shared.Settings.INSTRUMENT:
wasm-split split a.wasm ...
And the wasm-split tool should preferably be run at the end so that both instrumentation and splitting will run on the same binary?
| @@ -661,6 +661,9 @@ def add_standard_wasm_imports(send_items_map): | |||
| return value; | |||
| }''' | |||
|
|
|||
| if shared.Settings.INSTRUMENT: | |||
| send_items_map['log_function'] = '''log_function''' | |||
There was a problem hiding this comment.
No need for triple quotes here.
There was a problem hiding this comment.
Where is log_function getting defined? Can we avoid needing to specify a special/dedicated mechanism to import this, but could instead use a regular JS library function for that?
I would be interested in overriding this log_function for my own debugging/tooling, and also perhaps the ones that AUTODEBUG provides, but it looks like overriding the functionality is not currently possible.
JS libraries have the convenience that if one specifies a --js-library file on the command line, it can override system JS library defined symbols, so one can override default behavior for the "I know what I'm doing" scenarios. That would be cool to support for this feature!
There was a problem hiding this comment.
A simpler option might be to have the user do -s BINARYEN_EXTRA_PASSES=--log-function. That would use an existing setting instead of adding a new one.
That leaves getting the asmLibraryArg import added. I think we can add a new option for that in a generic way (we already have various other options in this area, just not that specifically), EXTRA_WASM_IMPORTS perhaps?
|
I'm not sure how useful |
Yes, that's what I'm thinking. I'll start a PR. |
We would want to instrument at the block level in the future so I guess |
|
Given the activity in @tlively 's work, do we still need this PR? |
|
I think we probably don't need this specific PR any more, but yes, I think we should still add Emscripten options to make wasm-split easier to use for Emscripten users and to automatically load the second module in the Emscripten runtime code. @sbc100, do you still want to do that integration work? |
|
This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant. |
Initial code to allow the user to log functions in Wasm. He will have to set
-s INSTRUMENT=1and provide a Javascriptlog_functionto implement the logging. FYI @sbc100 and @tlively .