Bug 161710

Created attachment 289843 [details] Patch

Comment on attachment 289843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289843&action=review > Source/JavaScriptCore/testWASM.cpp:385 > + Vector<uint8_t> vector = { > + 0x00, 0x61, 0x73, 0x6d, 0x0c, 0x00, 0x00, 0x00, 0x01, 0x87, 0x80, 0x80, 0x80, 0x00, 0x01, 0x40, > + 0x03, 0x01, 0x01, 0x01, 0x00, 0x03, 0x82, 0x80, 0x80, 0x80, 0x00, 0x01, 0x00, 0x05, 0x83, 0x80, > + 0x80, 0x80, 0x00, 0x01, 0x01, 0x01, 0x07, 0x8f, 0x80, 0x80, 0x80, 0x00, 0x01, 0x0b, 0x77, 0x72, > + 0x69, 0x74, 0x65, 0x5f, 0x61, 0x72, 0x72, 0x61, 0x79, 0x00, 0x00, 0x0a, 0xaf, 0x80, 0x80, 0x80, > + 0x00, 0x01, 0xa9, 0x80, 0x80, 0x80, 0x00, 0x01, 0x01, 0x01, 0x10, 0x00, 0x15, 0x03, 0x01, 0x00, > + 0x02, 0x00, 0x14, 0x03, 0x14, 0x02, 0x56, 0x07, 0x01, 0x14, 0x01, 0x14, 0x03, 0x40, 0x14, 0x00, > + 0x2e, 0x00, 0x00, 0x10, 0x01, 0x14, 0x03, 0x40, 0x15, 0x03, 0x06, 0x00, 0x0f, 0x0f, 0x09, 0x0f > + }; > + Oh man, we should figure out a better way of writing these test cases! > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:344 > + // We need to make sure that we add the memoryOffset to the pointer with a 32bit add > + // so if it overflows we stay within our mapped memory. > + pointer = m_currentBlock->appendNew<Value>(m_proc, Add, Origin(), pointer, memoryOffset); > + > + PatchpointValue* patchpoint = m_currentBlock->appendNew<PatchpointValue>(m_proc, typeForLoad(op), Origin()); > + patchpoint->appendSomeRegister(pointer); > + patchpoint->numGPScratchRegisters++; > + patchpoint->setGenerator([=] (CCallHelpers& jit, const StackmapGenerationParams& params) { > + GPRReg scratch = params.gpScratch(0); > + GPRReg result = params[0].gpr(); > + GPRReg ptr = params[1].gpr(); > + > + jit.load32(bitwise_cast<char*>(&m_memory) + ModuleMemoryInformation::offsetOfSize(), scratch); > + CCallHelpers::Jump invalid = jit.branch32(CCallHelpers::AboveOrEqual, ptr, scratch); > + > + jit.move(CCallHelpers::ImmPtr(m_memory.memory()), scratch); > + loadForOp(jit, op, MacroAssembler::BaseIndex(scratch, ptr, MacroAssembler::Scale::TimesOne), result); > + CCallHelpers::Jump done = jit.jump(); > + > + invalid.link(&jit); > + // Right now we crash; In the future we should do the trapping thing. > + jit.breakpoint(); > + done.link(&jit); > + }); > + > + result = patchpoint; Whoa!!! Why not just use B3 32-bit load followed by B3 32->64 sign extend or zero extend? If this results in bad code, you could easily add B3 instruction selection rules for this.

Created attachment 289954 [details] Patch

Attachment 289954 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WASMOps.h:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/wasm/WASMOps.h:114: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 289954 [details] Patch Via offline discussion with pizlo I think we are going to change how this works.

Created attachment 290637 [details] WIP

Created attachment 290687 [details] Patch

Comment on attachment 290687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290687&action=review > Source/JavaScriptCore/CMakeLists.txt:861 > + wasm/WASMMMemory.cpp typo: WASMMMemory.cpp -> WASMMemory.cpp

Comment on attachment 290687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290687&action=review >> Source/JavaScriptCore/CMakeLists.txt:861 >> + wasm/WASMMMemory.cpp > > typo: WASMMMemory.cpp -> WASMMemory.cpp Whoops, fixed. Thanks.

Created attachment 290714 [details] Patch

Comment on attachment 290714 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290714&action=review > Source/JavaScriptCore/wasm/WASMMemory.cpp:42 > + // Try again with a different number. > + result = mmap(nullptr, maxPageNum, PROT_NONE, MAP_PRIVATE | MAP_ANON, 0, 0); Drive by comment: How is this different from the try above? The comment said to try with a different number, but it appears we're using the same numbers here.

Comment on attachment 290714 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290714&action=review >> Source/JavaScriptCore/wasm/WASMMemory.cpp:42 >> + result = mmap(nullptr, maxPageNum, PROT_NONE, MAP_PRIVATE | MAP_ANON, 0, 0); > > Drive by comment: How is this different from the try above? The comment said to try with a different number, but it appears we're using the same numbers here. Whoops, that should be maxSize.

Created attachment 290716 [details] Patch

Comment on attachment 290716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290716&action=review Haven't gone through everything, LMK when ready to review for real. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:310 > + break; This can be auto-generated from the JSON file's "return". > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:390 > + return 8; Ditto. > JSTests/stress/wasm/generate-wasmops-header.js:42 > + ...opcodeMacroizer(op => (op.category === "store")), (discussed on IRC) it's probably better to look at "return" to distinguish load / store instead of changing the category, because atomics like cmpxchg will make this weird.

Comment on attachment 290716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290716&action=review >> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:310 >> + break; > > This can be auto-generated from the JSON file's "return". I'm not sure I see how since the return field only states the WASM type of the return not the number of bytes being read from memory.

Comment on attachment 290716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290716&action=review >> JSTests/stress/wasm/generate-wasmops-header.js:42 >> + ...opcodeMacroizer(op => (op.category === "store")), > > (discussed on IRC) it's probably better to look at "return" to distinguish load / store instead of changing the category, because atomics like cmpxchg will make this weird. Fixed.

Comment on attachment 290716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290716&action=review >>> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:310 >>> + break; >> >> This can be auto-generated from the JSON file's "return". > > I'm not sure I see how since the return field only states the WASM type of the return not the number of bytes being read from memory. Oh right. We could add it to the file then? Later patch, I'm happy to do that :-) Seems like we'll want that info for tests, and for other ops like atomics.

Created attachment 290723 [details] Patch

Comment on attachment 290723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290723&action=review > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:404 > + FALLTHROUGH; Is i64 trunk + 32-bit value stored into 8-bit a b3-ism? Seems odd. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:657 > + // Return the result, if neeeded. typo > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:680 > + std::unique_ptr<FunctionCompilation> result = std::make_unique<FunctionCompilation>(); Just my style, but I prefer: auto result(std::make_unique<FunctionCompilation>()); > Source/JavaScriptCore/wasm/WASMFunctionParser.h:158 > + // TODO: What do I do with this? It's just a int which can be ignored. Could determine whether to split ops on ARM, doesn't seem useful on x86 (movdqa / movdqu isn't a thing anymore AFAIK). I assume that would just be taught to B3, passed from here. I think you need to validate the alignment though (power of 2, no more than natural). Note that alignment is on base+imm, not just base. This seems missing from the design repo... https://github.com/WebAssembly/spec/issues/217 and https://github.com/WebAssembly/spec/issues/302 I'll fix the design repo. > Source/JavaScriptCore/wasm/WASMFunctionParser.h:176 > + // TODO: What do I do with this? Same. > Source/JavaScriptCore/wasm/WASMMemory.cpp:39 > + void* result = mmap(nullptr, maxPageNum, PROT_NONE, MAP_PRIVATE | MAP_ANON, 0, 0); Shouldn't this be maxPageNum * pageSize? > Source/JavaScriptCore/wasm/WASMMemory.cpp:48 > + munmap(result, maxSize); maxSize may not have been what you allocated above. > Source/JavaScriptCore/wasm/WASMMemory.h:64 > + munmap(m_memory, m_maxSize); This is also wrong if you retried allocation.

Created attachment 291830 [details] Patch

Comment on attachment 291830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291830&action=review > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:306 > + RELEASE_ASSERT_NOT_REACHED(); This could be auto-generated from the JSON file. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:388 > + RELEASE_ASSERT_NOT_REACHED(); Same. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:646 > + for (unsigned i = 0; i < sizes.size(); ++i) Can you use a range-based for loop? > Source/JavaScriptCore/wasm/WASMCallingConvention.h:80 > + unsigned stackArgumentCount = arguments.size() < m_registerArguments.size() ? 0 : arguments.size() - m_registerArguments.size(); Vector::size returns a size_t, seems like a better fit here?

Comment on attachment 291830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291830&action=review r=me with a bunch of things to fix > Source/JavaScriptCore/ChangeLog:21 > + size. Currently, B3 only barely works with pinned values because > + we cannot change the size of memory. Is this still true? I think this has changed. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:218 > + // FIXME: We should support more than one size register. see: https://bugs.webkit.org/show_bug.cgi?id=162952 I don't think you need this comment here -- we have a record in bugzilla. You should add in buzilla commentary about how this is a proposed optimization. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:231 > + // FIXME: We should support more than one size register. see: https://bugs.webkit.org/show_bug.cgi?id=162952 Ditto. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:233 > + m_proc.pinRegister(m_memorySizeGPR); This should be a loop to pin all registers in the vector. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:238 > + // FIXME: Change this when we add trapping. See: https://bugs.webkit.org/show_bug.cgi?id=163351 I think this fixme is too vague since it doesn't say what we want to change to. I would say something about turning traps into exceptions rather than crashing. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:280 > + // FIXME: We should support more than one size register. see: https://bugs.webkit.org/show_bug.cgi?id=162952 Ditto. >> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:306 >> + default: >> + break; >> + } >> + RELEASE_ASSERT_NOT_REACHED(); > > This could be auto-generated from the JSON file. Does the JSON file indicate how many bytes per memory op? Maybe you're proposing adding that to the JSON file? It's better to leave the default case out because that way the compiler will do static checking for case exhaustion. If you want to leave the assert in, just in case, that's cool with me. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:309 > +// FIXME: B3 should support Int64 small loads and 16-bit unsigned loads. Same comment as below. >> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:388 >> + RELEASE_ASSERT_NOT_REACHED(); > > Same. It's better to leave the default case out because that way the compiler will do static checking for case exhaustion. If you want to leave the assert in, just in case, that's cool with me. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:392 > +// FIXME: We should just support an Int64 store8. I think this comment might be misleading because it suggests that something is wrong with this function and also that the change to Int64 store8 would be trivial. I would just remove this comment. If you added a bug URL, you could say "http://blah, we might be able to optimize this by adding an implicitly truncating store8 of an Int64." > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:607 > + // FIXME: Is this supposed to trap if less than expected number of arguments? Let's pick a behavior we like and raise an issue in the spec. I'd suggest throwing an exception. > Source/JavaScriptCore/wasm/WASMMemory.cpp:49 > + if (!result) > + return; mmap returns MAP_FAILED on error, no NULL! :P > Source/JavaScriptCore/wasm/WASMMemory.h:41 > +struct PinnedRegisterInfo { This is a good place to explain that you're planning on experimenting with holding different offsets. That would explain why this data type is a vector and not just a single entry. > Source/JavaScriptCore/wasm/WASMMemory.h:47 > +constexpr uint32_t maxPageNum = static_cast<uint32_t>((1ull << 32) / pageSize); Let's call this maxPageCount. We try to avoid abbreviation in WebKit style. > Source/JavaScriptCore/wasm/WASMMemory.h:74 > + bool growMemory(uint32_t newSize) Let's call this "grow". It already resides in a class name "memory". > Source/JavaScriptCore/wasm/WASMMemory.h:90 > + uint32_t m_maxBytes { 0 }; Let's call this m_capacity. > Source/JavaScriptCore/wasm/WASMMemory.h:91 > + uint64_t m_mappedBytes { 0 }; Let's call this m_mappedCapacity. > Source/JavaScriptCore/wasm/WASMModuleParser.cpp:180 > + max *= 64 * KB; > + size *= 64 * KB; This should use pageSize and the "size" and "capacity" naming scheme.

Created attachment 291886 [details] Patch for landing

Created attachment 291887 [details] Patch for landing

Comment on attachment 291887 [details] Patch for landing Rejecting attachment 291887 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ta/EWS/WebKit/Source/JavaScriptCore/yarr/YarrCanonicalizeUCS2.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/YarrCanonicalizeUCS2.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/WASMB3IRGenerator.o wasm/WASMB3IRGenerator.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: http://webkit-queues.webkit.org/results/2307387

Attachment 291887 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:394: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 291892 [details] Patch for landing

Created attachment 291895 [details] Patch for landing

Comment on attachment 291830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291830&action=review >> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:646 >> + for (unsigned i = 0; i < sizes.size(); ++i) > > Can you use a range-based for loop? Not really since I need the index to access sizeRegisters[i].

Comment on attachment 291895 [details] Patch for landing Clearing flags on attachment: 291895 Committed r207453: <http://trac.webkit.org/changeset/207453>

All reviewed patches have been landed. Closing bug.

Comment on attachment 291895 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=291895&action=review > Source/JavaScriptCore/wasm/WASMMemory.cpp:43 > + void* result = mmap(nullptr, m_mappedCapacity, PROT_NONE, MAP_PRIVATE | MAP_ANON, 0, 0); I thought mmap should pass -1 for file descriptor argument for these use cases? > Source/JavaScriptCore/wasm/WASMMemory.cpp:52 > + if (mprotect(result, startingSize, PROT_READ | PROT_WRITE)) { Nit: maybe it's worth asserting that m_mappedCapaxity < startingSize? > Source/JavaScriptCore/wasm/WASMMemory.h:42 > +// entry. Specifically an extry where the sizeOffset == 0. If we have more than one size register, Typo: extry => entry > Source/JavaScriptCore/wasm/WASMMemory.h:49 > +constexpr uint32_t pageSize = 64 * KB; Doesn't WTF expose this?

Comment on attachment 291895 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=291895&action=review Since this patch already landed I'm putting the changes in another bug https://bugs.webkit.org/show_bug.cgi?id=163601 >> Source/JavaScriptCore/wasm/WASMMemory.cpp:43 >> + void* result = mmap(nullptr, m_mappedCapacity, PROT_NONE, MAP_PRIVATE | MAP_ANON, 0, 0); > > I thought mmap should pass -1 for file descriptor argument for these use cases? I thought it just ignored the file descriptor. But it looks like for MAP_ANON mmap uses the file descriptor for Mach flags so we should use -1. I'll also add a FIXME for getting a VM tag too: https://bugs.webkit.org/show_bug.cgi?id=163600. >> Source/JavaScriptCore/wasm/WASMMemory.cpp:52 >> + if (mprotect(result, startingSize, PROT_READ | PROT_WRITE)) { > > Nit: maybe it's worth asserting that m_mappedCapaxity < startingSize? Sure, I'll change that. >> Source/JavaScriptCore/wasm/WASMMemory.h:42 >> +// entry. Specifically an extry where the sizeOffset == 0. If we have more than one size register, > > Typo: extry => entry Fixed. >> Source/JavaScriptCore/wasm/WASMMemory.h:49 >> +constexpr uint32_t pageSize = 64 * KB; > > Doesn't WTF expose this? Nope. It exposes the system page size, which is different than the Wasm page size.