Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2204703002:
[wasm] Get rid of extra wrappers when import another wasm export (Closed)
DescriptionAdd signature checking when directly import a foreign function
Committed: https://crrev.com/dfd8db8bec891aa1f82b9ec728c09ccdcaebe29d
Cr-Commit-Position: refs/heads/master@{#38349}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Pass the arity to the internal field instead of the whole sharedFuncInfo, as well as fixing some ot… #Patch Set 3 : Add enum for exported JS Function internal fields, also make the code more clean in CompileWrappers… #Patch Set 4 : Change the interface of WrapExportCodeAsJSFunction in case of that there is no signature to pass in… #
Total comments: 1
Patch Set 5 : Adding test cases #
Total comments: 12
Patch Set 6 : Made code more clean according to code reviews #
Total comments: 6
Patch Set 7 : Modified runtime test functions and added more test cases #
Total comments: 12
Patch Set 8 : Make test case code more clean #
Messages
Total messages: 51 (36 generated)
clarkchenwang@google.com changed reviewers: + mtrofin@chromium.org
Hi Mircea, I would like you to review my code changes. Thanks!
The CQ bit was checked by clarkchenwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10087) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6061)
This is the right idea. See comments. https://codereview.chromium.org/2204703002/diff/1/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2204703002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:132: function->SetInternalField(2, *signature); This might fail, because we only specified a grand total of 2 internal fields to the wasm_function_map (as per your change in wasm-js.cc) https://codereview.chromium.org/2204703002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:687: // check whether two functions have same param count You should get the parameter count from the wasm function signature (we save the arity earlier on - see kExportArity). Maybe you can just point to the entire WasmExportMetadata, rather than just the ByteArray with the signature? SharedFunctionInfo is a JS concept, while the wasm function signature is a WASM concept. https://codereview.chromium.org/2204703002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:690: Handle<ByteArray>(ByteArray::cast(func->GetInternalField(2))); why continue computing exportedSig if isMatch=false? https://codereview.chromium.org/2204703002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:711: wasm_count++; ++wasm_count https://codereview.chromium.org/2204703002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:715: CHECK(wasm_count == 1); DCHECK
I have made patch 2, pass arity to the internal field instead of sharedFuncInfo.
The CQ bit was checked by clarkchenwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10091) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/10031) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6065)
The CQ bit was checked by clarkchenwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
The CQ bit was checked by clarkchenwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patch 4. Fixed the building errors and runtime errors.
clarkchenwang@google.com changed reviewers: + bradnelson@chromium.org
Hi, Mircea and Brad, I have made four test cases. The first one has exact matched signatures for the imported and exported functions, so the wrappers are got rid of. I check the Relocinfo in the runtime test. Other test cases have unmatched signatures(different parameter numbers or different data types). The wrappers remain but no exceptions are thrown out. I am not sure that I have ideas for making a test case, in which case the code can pass the compilation check but throws exception in the runtime. Thanks!
The CQ bit was checked by clarkchenwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice - see comments. https://codereview.chromium.org/2204703002/diff/60001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2204703002/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:28: enum JSFunctionExportInternalField { You could move this in the anonymous namespace below. https://codereview.chromium.org/2204703002/diff/80001/src/runtime/runtime-tes... File src/runtime/runtime-test.cc (right): https://codereview.chromium.org/2204703002/diff/80001/src/runtime/runtime-tes... src/runtime/runtime-test.cc:285: RUNTIME_FUNCTION(Runtime_CheckImportExportFunction) { add a comment saying that this only supports the case where the function being exported calls exactly one import. https://codereview.chromium.org/2204703002/diff/80001/src/runtime/runtime-tes... src/runtime/runtime-test.cc:310: Handle<Code> increment_fct; The notion of "increment" is probably tied to the example we used. Maybe call it "wrapped_function"? https://codereview.chromium.org/2204703002/diff/80001/src/runtime/runtime-tes... src/runtime/runtime-test.cc:321: DCHECK(increment_fct->kind() == Code::WASM_FUNCTION && count == 1); you probably want to check here that increment_fct was found. In fact, from how the code is written, all you care about checking is count == 1 - nonzero count implies you found the function. https://codereview.chromium.org/2204703002/diff/80001/src/runtime/runtime-tes... src/runtime/runtime-test.cc:331: if (type->value() == 0) you need {} https://codereview.chromium.org/2204703002/diff/80001/src/runtime/runtime-tes... src/runtime/runtime-test.cc:331: if (type->value() == 0) move out of the for loop the target_kind assignment, since "type" doesn't change. https://codereview.chromium.org/2204703002/diff/80001/src/runtime/runtime-tes... src/runtime/runtime-test.cc:334: target_kind = Code::WASM_TO_JS_FUNCTION; {} also - because the assignment is on the second line https://codereview.chromium.org/2204703002/diff/80001/src/runtime/runtime.h File src/runtime/runtime.h (right): https://codereview.chromium.org/2204703002/diff/80001/src/runtime/runtime.h#n... src/runtime/runtime.h:848: F(CheckImportExportFunction, 2, 1) \ how about CheckWasmWrapperElision https://codereview.chromium.org/2204703002/diff/80001/test/mjsunit/wasm/test-... File test/mjsunit/wasm/test-import-export-wrapper.js (right): https://codereview.chromium.org/2204703002/diff/80001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-import-export-wrapper.js:13: second_module.addImportWithModule("js", "thePrint", sig_index); can you change the names, instead of "thePrint" and "blah", "nothing", etc, to something more suggestive (e.g. export_1, export_2, etc) also, mind adding in comments what the functions do? makes it easy to read. https://codereview.chromium.org/2204703002/diff/80001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-import-export-wrapper.js:38: var f = second_module.instantiate({js:{thePrint:print}}).exports.second_export; 80 char limit - git cl format, unfortunately, doesn't help for js files. https://codereview.chromium.org/2204703002/diff/80001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-import-export-wrapper.js:41: the_export(2); want to check the result? https://codereview.chromium.org/2204703002/diff/80001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-import-export-wrapper.js:42: %CheckImportExportFunction(the_export, 0); maybe add somewhere a var expect_no_elision = 1 and var expect_elision = 0 https://codereview.chromium.org/2204703002/diff/80001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-import-export-wrapper.js:77: the_export(4, 3); check result? (same for the remainder) should the call fail here?
The CQ bit was checked by clarkchenwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Mircea, Changes have been made due to your comments! Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2204703002/diff/100001/src/runtime/runtime-te... File src/runtime/runtime-test.cc (right): https://codereview.chromium.org/2204703002/diff/100001/src/runtime/runtime-te... src/runtime/runtime-test.cc:285: RUNTIME_FUNCTION(Runtime_CheckWasmWrapperElison) { typo (...Elison => ...Elision) https://codereview.chromium.org/2204703002/diff/100001/src/runtime/runtime-te... src/runtime/runtime-test.cc:344: return isolate->heap()->undefined_value(); return true/false, otherwise you'll bring down the whole vm (even if you use CHECKs), and I think that'll skip running the rest of the tests in a file. https://codereview.chromium.org/2204703002/diff/100001/test/mjsunit/wasm/test... File test/mjsunit/wasm/test-import-export-wrapper.js (right): https://codereview.chromium.org/2204703002/diff/100001/test/mjsunit/wasm/test... test/mjsunit/wasm/test-import-export-wrapper.js:46: var the_export = first_module.instantiate({import_module_1:{import_name_1:f}}) don't you want to still call the_export, to make sure code is executed as expected (same comment for below)
The CQ bit was checked by clarkchenwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Mircea and Brad, Runtime test function has been changed to return boolean and I added some test cases to test the functionality of the exported and imported functions. Best, Chen https://codereview.chromium.org/2204703002/diff/100001/src/runtime/runtime-te... File src/runtime/runtime-test.cc (right): https://codereview.chromium.org/2204703002/diff/100001/src/runtime/runtime-te... src/runtime/runtime-test.cc:285: RUNTIME_FUNCTION(Runtime_CheckWasmWrapperElison) { On 2016/08/04 00:51:28, Mircea Trofin wrote: > typo (...Elison => ...Elision) Done. https://codereview.chromium.org/2204703002/diff/100001/src/runtime/runtime-te... src/runtime/runtime-test.cc:344: return isolate->heap()->undefined_value(); On 2016/08/04 00:51:28, Mircea Trofin wrote: > return true/false, otherwise you'll bring down the whole vm (even if you use > CHECKs), and I think that'll skip running the rest of the tests in a file. Done. https://codereview.chromium.org/2204703002/diff/100001/test/mjsunit/wasm/test... File test/mjsunit/wasm/test-import-export-wrapper.js (right): https://codereview.chromium.org/2204703002/diff/100001/test/mjsunit/wasm/test... test/mjsunit/wasm/test-import-export-wrapper.js:46: var the_export = first_module.instantiate({import_module_1:{import_name_1:f}}) On 2016/08/04 00:51:28, Mircea Trofin wrote: > don't you want to still call the_export, to make sure code is executed as > expected (same comment for below) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... File src/runtime/runtime-test.cc (right): https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... src/runtime/runtime-test.cc:296: if (export_code->kind() != Code::JS_TO_WASM_FUNCTION) { actually, this is a fine place to fail (CHECK). The contract of the CheckWasmWrapperElision function is that the caller must pass in a module export. If that's invalidated, it's a bug in the caller's code, and failfast is fine. https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... src/runtime/runtime-test.cc:312: if (count != 1) { This is also a CHECK - an export is expected to wrap a wasm function. https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... src/runtime/runtime-test.cc:315: // check the type of the $increment $increment? https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... src/runtime/runtime-test.cc:327: if (count != 1) { also a CHECK https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... src/runtime/runtime-test.cc:348: } CHECK_LE(count, 1) https://codereview.chromium.org/2204703002/diff/120001/test/mjsunit/wasm/test... File test/mjsunit/wasm/test-import-export-wrapper.js (right): https://codereview.chromium.org/2204703002/diff/120001/test/mjsunit/wasm/test... test/mjsunit/wasm/test-import-export-wrapper.js:23: second_module.addImportWithModule("import_module_2", "import_name_2", sig_index); check 80 char limit
The CQ bit was checked by clarkchenwang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Mircea, Done with all the comments, Thanks! Best, Chen https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... File src/runtime/runtime-test.cc (right): https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... src/runtime/runtime-test.cc:296: if (export_code->kind() != Code::JS_TO_WASM_FUNCTION) { On 2016/08/04 18:21:08, Mircea Trofin wrote: > actually, this is a fine place to fail (CHECK). The contract of the > CheckWasmWrapperElision function is that the caller must pass in a module > export. If that's invalidated, it's a bug in the caller's code, and failfast is > fine. Done. https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... src/runtime/runtime-test.cc:312: if (count != 1) { On 2016/08/04 18:21:08, Mircea Trofin wrote: > This is also a CHECK - an export is expected to wrap a wasm function. Done. https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... src/runtime/runtime-test.cc:315: // check the type of the $increment On 2016/08/04 18:21:08, Mircea Trofin wrote: > $increment? Done. https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... src/runtime/runtime-test.cc:327: if (count != 1) { On 2016/08/04 18:21:08, Mircea Trofin wrote: > also a CHECK Done. https://codereview.chromium.org/2204703002/diff/120001/src/runtime/runtime-te... src/runtime/runtime-test.cc:348: } On 2016/08/04 18:21:08, Mircea Trofin wrote: > CHECK_LE(count, 1) Done. https://codereview.chromium.org/2204703002/diff/120001/test/mjsunit/wasm/test... File test/mjsunit/wasm/test-import-export-wrapper.js (right): https://codereview.chromium.org/2204703002/diff/120001/test/mjsunit/wasm/test... test/mjsunit/wasm/test-import-export-wrapper.js:23: second_module.addImportWithModule("import_module_2", "import_name_2", sig_index); On 2016/08/04 18:21:08, Mircea Trofin wrote: > check 80 char limit Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clarkchenwang@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add signature checking when directly import a foreign function ========== to ========== Add signature checking when directly import a foreign function Committed: https://crrev.com/dfd8db8bec891aa1f82b9ec728c09ccdcaebe29d Cr-Commit-Position: refs/heads/master@{#38349} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/dfd8db8bec891aa1f82b9ec728c09ccdcaebe29d Cr-Commit-Position: refs/heads/master@{#38349} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
