Bug 169611

Created attachment 304587 [details] patch

FWIW I tested this on x86-64 and ARM64. This will break platforms which build WebAssemnbly but don't support fast TLS (e.g. Linux). Fixing it should be easy with access to a Linux box.

Created attachment 304589 [details] patch Rebase.

Created attachment 304590 [details] patch Improve changelog as suggested by Keith.

Comment on attachment 304590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=304590&action=review > Source/JavaScriptCore/ChangeLog:3 > + WebAssembly: store state in TLS instead of on VM This has nothing to do with Transport Layer Security. Do we really need another thing associated with web technology named "TLS"?

I believe WasmTop:: slot should be counted as one of GC root.

(In reply to comment #6) > I believe WasmTop:: slot should be counted as one of GC root. I don't think so: WasmTop is only set when there's a wasm instance on the call stack. That instance therefore has to already be live somehow, otherwise it couldn't be on the call stack. Calling into wasm sets it, calling out resets the previous value (it can be nested). (In reply to comment #5) > Comment on attachment 304590 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304590&action=review > > > Source/JavaScriptCore/ChangeLog:3 > > + WebAssembly: store state in TLS instead of on VM > > This has nothing to do with Transport Layer Security. Do we really need > another thing associated with web technology named "TLS"? That's the term of art. One predates the other :) For reference, the Linux handling of TLS seems very similar to what we do: https://www.akkadia.org/drepper/tls.pdf

Comment on attachment 304590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=304590&action=review Functionally, I think it looks pretty good, but there are some style issues. >>> Source/JavaScriptCore/ChangeLog:3 >>> + WebAssembly: store state in TLS instead of on VM >> >> This has nothing to do with Transport Layer Security. Do we really need another thing associated with web technology named "TLS"? > > That's the term of art. One predates the other :) > > > > For reference, the Linux handling of TLS seems very similar to what we do: > https://www.akkadia.org/drepper/tls.pdf TLS is a term of art for thread-local storage. In the OS literature, it's common to use the term TLS particularly when referring to the low-level machinery. The high-level APIs use other terms, like ThreadSpecific (and pthread_set/getspecific). This makes the term TLS particularly appropriate here, since we're using the OS's low-level TLS ABI rather than some high-level API function. > Source/JavaScriptCore/ChangeLog:67 > + (JSC::WasmTop::load): > + (JSC::WasmTop::store): Really don't like the name WasmTop. It tells me nothing about what it is. It's a particularly risky term because we use the term "Top" in our system to refer to the tautology, or the set of all things. We will probably at some point have a Wasm concept of Top in some static analysis. I would call this WasmContext. That tells me everything I need to know about what it does. > Source/JavaScriptCore/jit/Repatch.cpp:596 > + return WasmTop::load()->module(); WasmContext::get() And it should return WasmContext& > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:274 > + m_instanceValue = WasmTop::load(m_proc, m_currentBlock); Don't put B3 code generation things outside the lowering code. This code should emit the IR for getting the WasmContext. If you need a helper for this, then put the helper in B3IRGenerator. Also, can we get rid of m_instanceValue and just load this lazily? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:286 > + static_assert(sizeof(decltype(WasmTop::load()->memory()->memory().memory())) == sizeof(void*), "codegen relies on this size"); memory memory memory Can we do something about that? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:297 > + WasmTop::store(proc, block, instance); Inline the IR, or put the helper in B3IRGenerator > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1168 > + WasmTop::load(jit, baseMemory); Put this helper in Assemblyhelpers. > Source/JavaScriptCore/wasm/WasmBinding.cpp:49 > + WasmTop::load(jit, result); Put this helper in AssemblyHelpers. > Source/JavaScriptCore/wasm/WasmTop.cpp:69 > + { Unindent. > Source/JavaScriptCore/wasm/WasmTop.cpp:88 > +#if ENABLE(FAST_TLS_JIT) > + jit.loadFromTLSPtr(fastTLSOffsetForKey(WTF_WEBASSEMBLY_KEY), dst); > +#else > +#error WebAssembly without fast TLS isn't implemented yet. > +#endif This is super risky. We would be undoing the work to enable Wasm on Linux. I thought that Yusuke had done that. If he has done it, then I think we should come up with a way to do this patch that doesn't disable wasm on Linux. > Source/JavaScriptCore/wasm/WasmTop.cpp:109 > + PatchpointValue* patchpoint = block->appendNew<PatchpointValue>(proc, pointerType(), Origin()); > + patchpoint->clobber(RegisterSet::macroScratchRegisters()); > + patchpoint->setGenerator( > + [&] (CCallHelpers& jit, const StackmapGenerationParams& params) { > + AllowMacroScratchRegisterUsage allowScratch(jit); > + WasmTop::load(jit, params[0].gpr()); > + }); You could be clever. We don't need scratch on x86. So, I would conditionalize that part of the patch. > Source/JavaScriptCore/wasm/WasmTop.cpp:123 > + PatchpointValue* patchpoint = block->appendNew<PatchpointValue>(proc, Void, Origin()); > + patchpoint->clobber(RegisterSet::macroScratchRegisters()); > + patchpoint->append(ConstrainedValue(arg, ValueRep::SomeRegister)); > + patchpoint->setGenerator( > + [&] (CCallHelpers& jit, const StackmapGenerationParams& params) { > + AllowMacroScratchRegisterUsage allowScratch(jit); > + WasmTop::store(jit, params[0].gpr()); > + }); Ditto. > Source/JavaScriptCore/wasm/WasmTop.h:43 > +class WasmTop { Should be called WasmContext. > Source/JavaScriptCore/wasm/WasmTop.h:53 > + static void load(CCallHelpers&, GPRReg); > + static void store(CCallHelpers&, GPRReg); > + Put this in AssemblyHelpers or inline it. > Source/JavaScriptCore/wasm/WasmTop.h:55 > + static B3::Value* load(B3::Procedure&, B3::BasicBlock*); > + static void store(B3::Procedure&, B3::BasicBlock*, B3::Value*); Put this in B3IRGenerator or inline it.

Comment on attachment 304590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=304590&action=review >> Source/JavaScriptCore/wasm/WasmTop.cpp:88 >> +#endif > > This is super risky. We would be undoing the work to enable Wasm on Linux. I thought that Yusuke had done that. If he has done it, then I think we should come up with a way to do this patch that doesn't disable wasm on Linux. In Linux, we don't have any system reserved TLS... That's super unfortunate! If we use "initial-exec" TLS model in Linux binary, it means that we cannot dlopen the shared library (or introducing additional restrictions). I think we have several options in Linux, 1. Implement TLS just like "dynamic-global" model Let's call the function, and returns the value with pthread_getspecific. It's simple. WasmTop::load in B3 should be implemented in Patchpoint, it calls pthread_getspecific one. WasmTop::load(CCallHelpers& jit, GPRReg dst) becomes a bit complicated: We need to store all the registers! That's not good....... 2. Copy B3 code when passing it to the other thread, and fill the patchable value for that I think it is not so good. We need to have multiple copies of Wasm code in multiple threads. What do you think of?

Comment on attachment 304590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=304590&action=review >>> Source/JavaScriptCore/wasm/WasmTop.cpp:88 >>> +#endif >> >> This is super risky. We would be undoing the work to enable Wasm on Linux. I thought that Yusuke had done that. If he has done it, then I think we should come up with a way to do this patch that doesn't disable wasm on Linux. > > In Linux, we don't have any system reserved TLS... That's super unfortunate! > If we use "initial-exec" TLS model in Linux binary, it means that we cannot dlopen the shared library (or introducing additional restrictions). > > I think we have several options in Linux, > > 1. Implement TLS just like "dynamic-global" model > > Let's call the function, and returns the value with pthread_getspecific. It's simple. > WasmTop::load in B3 should be implemented in Patchpoint, it calls pthread_getspecific one. > WasmTop::load(CCallHelpers& jit, GPRReg dst) becomes a bit complicated: We need to store all the registers! That's not good....... > > 2. Copy B3 code when passing it to the other thread, and fill the patchable value for that > > I think it is not so good. We need to have multiple copies of Wasm code in multiple threads. > > What do you think of? Oh, wait. In WebKit2 model, we don't have any case like dlopening the library which includes JSC, right?

Comment on attachment 304590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=304590&action=review >>>> Source/JavaScriptCore/wasm/WasmTop.cpp:88 >>>> +#endif >>> >>> This is super risky. We would be undoing the work to enable Wasm on Linux. I thought that Yusuke had done that. If he has done it, then I think we should come up with a way to do this patch that doesn't disable wasm on Linux. >> >> In Linux, we don't have any system reserved TLS... That's super unfortunate! >> If we use "initial-exec" TLS model in Linux binary, it means that we cannot dlopen the shared library (or introducing additional restrictions). >> >> I think we have several options in Linux, >> >> 1. Implement TLS just like "dynamic-global" model >> >> Let's call the function, and returns the value with pthread_getspecific. It's simple. >> WasmTop::load in B3 should be implemented in Patchpoint, it calls pthread_getspecific one. >> WasmTop::load(CCallHelpers& jit, GPRReg dst) becomes a bit complicated: We need to store all the registers! That's not good....... >> >> 2. Copy B3 code when passing it to the other thread, and fill the patchable value for that >> >> I think it is not so good. We need to have multiple copies of Wasm code in multiple threads. >> >> What do you think of? > > Oh, wait. In WebKit2 model, we don't have any case like dlopening the library which includes JSC, right? Ah, but maybe, it is still broken when using libjavascriptcoregtk.so. So I think we should have a fallback way. I think the (1) one should be simple enough for the first implementation.

(In reply to comment #11) > Comment on attachment 304590 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304590&action=review > > >>>> Source/JavaScriptCore/wasm/WasmTop.cpp:88 > >>>> +#endif > >>> > >>> This is super risky. We would be undoing the work to enable Wasm on Linux. I thought that Yusuke had done that. If he has done it, then I think we should come up with a way to do this patch that doesn't disable wasm on Linux. > >> > >> In Linux, we don't have any system reserved TLS... That's super unfortunate! > >> If we use "initial-exec" TLS model in Linux binary, it means that we cannot dlopen the shared library (or introducing additional restrictions). > >> > >> I think we have several options in Linux, > >> > >> 1. Implement TLS just like "dynamic-global" model > >> > >> Let's call the function, and returns the value with pthread_getspecific. It's simple. > >> WasmTop::load in B3 should be implemented in Patchpoint, it calls pthread_getspecific one. > >> WasmTop::load(CCallHelpers& jit, GPRReg dst) becomes a bit complicated: We need to store all the registers! That's not good....... > >> > >> 2. Copy B3 code when passing it to the other thread, and fill the patchable value for that > >> > >> I think it is not so good. We need to have multiple copies of Wasm code in multiple threads. > >> > >> What do you think of? > > > > Oh, wait. In WebKit2 model, we don't have any case like dlopening the library which includes JSC, right? > > Ah, but maybe, it is still broken when using libjavascriptcoregtk.so. So I > think we should have a fallback way. > I think the (1) one should be simple enough for the first implementation. I have an idea: we should pin a register for the wasm TLS on Linux. This is what I always did before I worked on WebKit. Jikes RVM did it this way, and it worked great. I think that it would be best for this patch to fall back to a global variable on Linux for now, with a giant FIXME - and then we should add support for the pinned register. We may then find that the pinned register outperforms the TLS slot. I suspect that it might on ARM64, since we have so many registers there.

Comment on attachment 304590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=304590&action=review >>>>>> Source/JavaScriptCore/wasm/WasmTop.cpp:88 >>>>>> +#endif >>>>> >>>>> This is super risky. We would be undoing the work to enable Wasm on Linux. I thought that Yusuke had done that. If he has done it, then I think we should come up with a way to do this patch that doesn't disable wasm on Linux. >>>> >>>> In Linux, we don't have any system reserved TLS... That's super unfortunate! >>>> If we use "initial-exec" TLS model in Linux binary, it means that we cannot dlopen the shared library (or introducing additional restrictions). >>>> >>>> I think we have several options in Linux, >>>> >>>> 1. Implement TLS just like "dynamic-global" model >>>> >>>> Let's call the function, and returns the value with pthread_getspecific. It's simple. >>>> WasmTop::load in B3 should be implemented in Patchpoint, it calls pthread_getspecific one. >>>> WasmTop::load(CCallHelpers& jit, GPRReg dst) becomes a bit complicated: We need to store all the registers! That's not good....... >>>> >>>> 2. Copy B3 code when passing it to the other thread, and fill the patchable value for that >>>> >>>> I think it is not so good. We need to have multiple copies of Wasm code in multiple threads. >>>> >>>> What do you think of? >>> >>> Oh, wait. In WebKit2 model, we don't have any case like dlopening the library which includes JSC, right? >> >> Ah, but maybe, it is still broken when using libjavascriptcoregtk.so. So I think we should have a fallback way. >> I think the (1) one should be simple enough for the first implementation. > > I have an idea: we should pin a register for the wasm TLS on Linux. This is what I always did before I worked on WebKit. Jikes RVM did it this way, and it worked great. > > I think that it would be best for this patch to fall back to a global variable on Linux for now, with a giant FIXME - and then we should add support for the pinned register. > > We may then find that the pinned register outperforms the TLS slot. I suspect that it might on ARM64, since we have so many registers there. Using pinned register sounds fine! Until we start sharing Wasm code in multiple threads, global variable just works.

I'll update the uploaded patch with FIXME. Since now I have a Linux box, it should be easier than repeating "attempt to fix" in EWS.

Created attachment 304665 [details] Update for Linux

I don't think the global works because wasm can be used with workers. I'll revert back to VM.topJSWebAssemblyInstance if there's no fast TLS, and later we can address the FIXME to use a pinned register.

Created attachment 304795 [details] patch Address all comments (or punt and file bug).

Created attachment 304825 [details] patch Fix build.

Comment on attachment 304825 [details] patch Attachment 304825 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3352050 New failing tests: wasm.yaml/wasm/spec-tests/left-to-right.wast.js.default-wasm wasm.yaml/wasm/function-tests/table-basic.js.default-wasm wasm.yaml/wasm/spec-tests/call_indirect.wast.js.default-wasm wasm.yaml/wasm/function-tests/grow-memory.js.default-wasm wasm.yaml/wasm/spec-tests/resizing.wast.js.default-wasm wasm.yaml/wasm/function-tests/exceptions.js.default-wasm wasm.yaml/wasm/function-tests/many-arguments-to-function.js.default-wasm wasm.yaml/wasm/function-tests/function-import-return-value.js.default-wasm wasm.yaml/wasm/spec-tests/names.wast.js.default-wasm wasm.yaml/wasm/function-tests/trap-store-2.js.default-wasm wasm.yaml/wasm/js-api/test_Instance.js.default-wasm wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.default-wasm wasm.yaml/wasm/fuzz/export-function.js.default-wasm wasm.yaml/wasm/js-api/call-indirect.js.default-wasm wasm.yaml/wasm/spec-tests/memory_trap.wast.js.default-wasm wasm.yaml/wasm/spec-tests/start.wast.js.default-wasm wasm.yaml/wasm/js-api/wrapper-function.js.default-wasm wasm.yaml/wasm/function-tests/trap-load-2.js.default-wasm wasm.yaml/wasm/js-api/wasm-to-wasm.js.default-wasm wasm.yaml/wasm/spec-tests/imports.wast.js.default-wasm wasm.yaml/wasm/function-tests/table-basic-2.js.default-wasm wasm.yaml/wasm/js-api/unique-signature.js.default-wasm wasm.yaml/wasm/function-tests/i64-from-js-exceptions.js.default-wasm wasm.yaml/wasm/spec-tests/func.wast.js.default-wasm wasm.yaml/wasm/js-api/test_Start.js.default-wasm wasm.yaml/wasm/spec-tests/address.wast.js.default-wasm wasm.yaml/wasm/js-api/export-arity.js.default-wasm wasm.yaml/wasm/function-tests/grow-memory-2.js.default-wasm wasm.yaml/wasm/spec-tests/nop.wast.js.default-wasm wasm.yaml/wasm/js-api/test_Data.js.default-wasm wasm.yaml/wasm/function-tests/grow-memory-4.js.default-wasm

(In reply to comment #19) > Comment on attachment 304825 [details] > patch > > Attachment 304825 [details] did not pass jsc-ews (mac): > Output: http://webkit-queues.webkit.org/results/3352050 Hmm, these failures are weird. I can't repro them locally with: make release && ./Tools/Scripts/run-javascriptcore-tests --filter wasm --no-build --no-testapi --root=./WebKitBuild/Release/ I'll figure out why...

Created attachment 304931 [details] patch Still can't repro locally after rebase, rebuild, and: ./Tools/Scripts/run-javascriptcore-tests --no-fail-fast --release --filter wasm Shot in the dark re-run on the bots. If it doesn't work i'll need to add logging.

Comment on attachment 304931 [details] patch Attachment 304931 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3369762 New failing tests: wasm.yaml/wasm/spec-tests/left-to-right.wast.js.default-wasm wasm.yaml/wasm/function-tests/table-basic.js.default-wasm wasm.yaml/wasm/spec-tests/call_indirect.wast.js.default-wasm wasm.yaml/wasm/function-tests/grow-memory.js.default-wasm wasm.yaml/wasm/spec-tests/resizing.wast.js.default-wasm wasm.yaml/wasm/function-tests/exceptions.js.default-wasm wasm.yaml/wasm/function-tests/many-arguments-to-function.js.default-wasm wasm.yaml/wasm/function-tests/function-import-return-value.js.default-wasm wasm.yaml/wasm/spec-tests/names.wast.js.default-wasm wasm.yaml/wasm/function-tests/trap-store-2.js.default-wasm wasm.yaml/wasm/js-api/test_Instance.js.default-wasm wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.default-wasm wasm.yaml/wasm/fuzz/export-function.js.default-wasm wasm.yaml/wasm/js-api/call-indirect.js.default-wasm wasm.yaml/wasm/spec-tests/memory_trap.wast.js.default-wasm wasm.yaml/wasm/spec-tests/start.wast.js.default-wasm wasm.yaml/wasm/js-api/wrapper-function.js.default-wasm wasm.yaml/wasm/function-tests/trap-load-2.js.default-wasm wasm.yaml/wasm/js-api/wasm-to-wasm.js.default-wasm wasm.yaml/wasm/spec-tests/imports.wast.js.default-wasm wasm.yaml/wasm/function-tests/table-basic-2.js.default-wasm wasm.yaml/wasm/js-api/unique-signature.js.default-wasm wasm.yaml/wasm/function-tests/i64-from-js-exceptions.js.default-wasm wasm.yaml/wasm/spec-tests/func.wast.js.default-wasm wasm.yaml/wasm/js-api/test_Start.js.default-wasm wasm.yaml/wasm/spec-tests/address.wast.js.default-wasm wasm.yaml/wasm/js-api/export-arity.js.default-wasm wasm.yaml/wasm/function-tests/grow-memory-2.js.default-wasm wasm.yaml/wasm/spec-tests/nop.wast.js.default-wasm wasm.yaml/wasm/js-api/test_Data.js.default-wasm wasm.yaml/wasm/function-tests/grow-memory-4.js.default-wasm

The crash only occurs on a bot with an older OS release (Michael also can't reproduce, I can't on x86-64 debug/release, neither can I on A64). ap@ sent me the full log and crash for this (below). It looks like we assert out in a patchpoint. I'll look at the code, and I'm also installing that OS version on a random machine in case I don't get enlightened by the code. Exception Type: EXC_BREAKPOINT (SIGTRAP) Exception Codes: 0x0000000000000002, 0x0000000000000000 Exception Note: EXC_CORPSE_NOTIFY Thread 7 Crashed:: jsc.wasm-b3-compilation.thread 0 com.apple.JavaScriptCore 0x000000010b61b3e9 0x10b335000 + 3040233 1 com.apple.JavaScriptCore 0x000000010b694dbb JSC::B3::PatchpointSpecial::generate(JSC::B3::Air::Inst&, JSC::CCallHelpers&, JSC::B3::Air::GenerationContext&) + 1371 (Vector.h:316) 2 com.apple.JavaScriptCore 0x000000010b4888f3 JSC::B3::Air::generate(JSC::B3::Air::Code&, JSC::CCallHelpers&) + 1955 (AirGenerate.cpp:220) 3 com.apple.JavaScriptCore 0x000000010bf374a8 JSC::Wasm::parseAndCompile(JSC::VM&, JSC::Wasm::CompilationContext&, unsigned char const*, unsigned long, JSC::Wasm::Signature const*, WTF::Vector<JSC::Wasm::UnlinkedWasmToWasmCall, 0ul, WTF::CrashOnOverflow, 16ul>&, JSC::Wasm::ModuleInformation const&, WTF::Vector<unsigned int, 0ul, WTF::CrashOnOverflow, 16ul> const&, unsigned int) + 1384 (memory:2705) 4 com.apple.JavaScriptCore 0x000000010bf99a59 JSC::Wasm::Plan::run(std::optional<JSC::Wasm::Memory::Mode>)::$_1::operator()() const + 377 (WasmPlan.cpp:182) 5 com.apple.JavaScriptCore 0x000000010c1e06c2 WTF::threadEntryPoint(void*) + 178 (functional:1766) 6 com.apple.JavaScriptCore 0x000000010c1e0b1f WTF::wtfThreadEntryPoint(void*) + 15 (memory:2714) 7 libsystem_pthread.dylib 0x00007fff8ac1f99d _pthread_body + 131 8 libsystem_pthread.dylib 0x00007fff8ac1f91a _pthread_start + 168 9 libsystem_pthread.dylib 0x00007fff8ac1d351 thread_start + 13 Thread 7 crashed with X86 Thread State (64-bit): rax: 0x0000000000000000 rbx: 0x00007ff3d1f00710 rcx: 0x000000000000000d rdx: 0x000000010ce06630 rdi: 0x00007ff3d1f00710 rsi: 0x000000000000000d rbp: 0x0000700000322900 rsp: 0x00007000003228e0 r8: 0x0000000000000000 r9: 0x0000000000000010 r10: 0x000000010ccaf800 r11: 0x000000010cc57e80 r12: 0x0000700000322a70 r13: 0x00000001103add68 r14: 0x000000000000000d r15: 0x000000010ce06630 rip: 0x000000010b61b3e9 rfl: 0x0000000000000297 cr2: 0x000000010b78e060 Binary Images: 0x10b335000 - 0x10c287ff7 com.apple.JavaScriptCore (604+ - 604.1.13+) <03220EF9-A29C-3F87-A21B-3127998F140F> /Volumes/VOLUME/*/JavaScriptCore.framework/Versions/A/JavaScriptCore

The repro I tried on a machine with a similar OS version didn't work. I'll look more with ap@ this afternoon. Maybe it's related to specific hardware.

Comment on attachment 304931 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=304931&action=review > Source/JavaScriptCore/jit/AssemblyHelpers.h:1664 > + return false; This needs a scratch register. > Source/JavaScriptCore/jit/AssemblyHelpers.h:1673 > + return false; ditto

Saam's psychic debugging found the issue! Here's Platform.h: #if __has_include(<System/pthread_machdep.h>) #define HAVE_FAST_TLS 1 #endif #if (CPU(X86_64) || CPU(ARM64)) && HAVE(FAST_TLS) #define ENABLE_FAST_TLS_JIT 1 #endif The bot uses ye olden version of MacOS for which __has_include(<System/pthread_machdep.h>) is false. It then fails here: 48 X86Registers::RegisterID scratchRegister() 49 { -> 50 RELEASE_ASSERT(m_allowScratchRegister); 51 return s_scratchRegister; 52 } It's a silly bug, easy fix as Saam suggested... but!!! I'm now worried that the bots aren't exercising the codepath I'm adding. Do we even build this new code that I added? I'll ask around, I don't understand our build. Also, I'll add a runtime option: if ENABLE(FAST_TLS_JIT) then it should be possible at runtime to "downgrade" back to no fast TLS.

Created attachment 305251 [details] patch Speculatively, this should work, let's see if the bot is happy. I need to answer the above questions though. And update ChangeLog.

It looks like _pthread_setspecific_direct is actually declared and defined in libpthread's tsd_private.h, not pthread_machdep.h. However, pthread_machdep.h is just a symlink to tsd_private.h. Other files have been relying on pthread_machdep.h since 2014: - https://bugs.webkit.org/show_bug.cgi?id=130320 - https://bugs.webkit.org/show_bug.cgi?id=131170 I assume these worked back then. I can't find any evidence that the symlink wasn't followed by clang...

Created attachment 305293 [details] experiment Let's try something out: #error out if __has_include(<System/pthread_machdep.h>) is false. Obviously this isn't to be committed, I just want to check that my understanding of the bot behavior is correct.

Attachment 305293 [details] did not pass style-queue: ERROR: Source/WTF/wtf/Platform.h:768: Extra space for operator ! [whitespace/operators] [4] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 305296 [details] experiment (fixed)

My experiment gives a troubling result: fast TLS isn't supported on any of our EWS test builds. I can just commit this patch, it works locally on x86 and A64, but I'm very uncomfortable with how untested this is. As mentioned above, other files have been relying on pthread_machdep.h since 2014: - https://bugs.webkit.org/show_bug.cgi?id=130320 - https://bugs.webkit.org/show_bug.cgi?id=131170 Their codepath is also untested AFAICT. In a hallway chat ap@ mentioned another way to detect that the build has the required headers. I'll ping him again.

Created attachment 305326 [details] patch I talked with ggaren@ since bmalloc has the same issue. This is an acceptable limitation of EWS because we also have internal bots which perform the build and tests with the machdep.h header available. The downside is that this code isn't tested by EWS and isn't available to open source contributors. They exercise a different, slower, code path. We do have options to "fix" the EWS situation: - Use the internal SDK on EWS. This means anyone with bot access can leak the content, we can then restrict it but... eh. - Copy the relevant bits to a fallback header in WebKit. This could diverge, and copying code is ew. - Copy the entire header from libc releases such as [0]. This is tricky because the license is different and we'd need to talk to a lawyer. I'm not sure either option is worth it. For now ggaren@ agreed to just go with the status quo that we already have. [0]: https://opensource.apple.com/source/Libc/Libc-583/pthreads/pthread_machdep.h

(In reply to JF Bastien from comment #33) > Created attachment 305326 [details] > patch > > I talked with ggaren@ since bmalloc has the same issue. This is an > acceptable limitation of EWS because we also have internal bots which > perform the build and tests with the machdep.h header available. > > The downside is that this code isn't tested by EWS and isn't available to > open source contributors. They exercise a different, slower, code path. > > We do have options to "fix" the EWS situation: > - Use the internal SDK on EWS. This means anyone with bot access can leak > the content, we can then restrict it but... eh. > - Copy the relevant bits to a fallback header in WebKit. This could > diverge, and copying code is ew. > - Copy the entire header from libc releases such as [0]. This is tricky > because the license is different and we'd need to talk to a lawyer. > > I'm not sure either option is worth it. For now ggaren@ agreed to just go > with the status quo that we already have. > > [0]: > https://opensource.apple.com/source/Libc/Libc-583/pthreads/ Please don't block landing this on EWS. Please land this soon since we have other things blocked on it.

> Please don't block landing this on EWS. Please land this soon since we have > other things blocked on it. Right: we're going with status quo. As soon as I have r+ I'll land this.

Comment on attachment 305326 [details] patch LGTM too

Comment on attachment 305326 [details] patch Clearing flags on attachment: 305326 Committed r214384: <http://trac.webkit.org/changeset/214384>

All reviewed patches have been landed. Closing bug.

This causes crashes in WasmBench when JSC is built with build-jsc --release. JF: how did you build and test this patch? (lldb) bt * thread #1: tid = 0x707a420, 0x00000001002c81b8 JavaScriptCore`JSC::MacroAssemblerX86_64::store64(JSC::X86Registers::RegisterID, void*) [inlined] JSC::MacroAssemblerX86Common::scratchRegister() at MacroAssemblerX86Common.h:50, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0) * frame #0: 0x00000001002c81b8 JavaScriptCore`JSC::MacroAssemblerX86_64::store64(JSC::X86Registers::RegisterID, void*) [inlined] JSC::MacroAssemblerX86Common::scratchRegister() at MacroAssemblerX86Common.h:50 [opt] frame #1: 0x00000001002c81b8 JavaScriptCore`JSC::MacroAssemblerX86_64::store64(this=<unavailable>, src=<unavailable>, address=<unavailable>) + 328 at MacroAssemblerX86_64.h:852 [opt] frame #2: 0x000000030835a300 frame #3: 0x0000000100ba77ef JavaScriptCore`WTF::SharedTaskFunctor<void (JSC::CCallHelpers&, JSC::B3::StackmapGenerationParams const&), JSC::Wasm::storeWasmContext(JSC::B3::Procedure&, JSC::B3::BasicBlock*, JSC::B3::Value*)::$_43>::run(JSC::CCallHelpers&, JSC::B3::StackmapGenerationParams const&) [inlined] JSC::MacroAssembler::storePtr(address=<unavailable>) + 271 at MacroAssembler.h:1007 [opt] frame #4: 0x0000000100ba77e4 JavaScriptCore`WTF::SharedTaskFunctor<void (JSC::CCallHelpers&, JSC::B3::StackmapGenerationParams const&), JSC::Wasm::storeWasmContext(JSC::B3::Procedure&, JSC::B3::BasicBlock*, JSC::B3::Value*)::$_43>::run(JSC::CCallHelpers&, JSC::B3::StackmapGenerationParams const&) [inlined] JSC::AssemblyHelpers::storeWasmContext(JSC::X86Registers::RegisterID) + 189 at AssemblyHelpers.h:1658 [opt] frame #5: 0x0000000100ba7727 JavaScriptCore`WTF::SharedTaskFunctor<void (JSC::CCallHelpers&, JSC::B3::StackmapGenerationParams const&), JSC::Wasm::storeWasmContext(JSC::B3::Procedure&, JSC::B3::BasicBlock*, JSC::B3::Value*)::$_43>::run(JSC::CCallHelpers&, JSC::B3::StackmapGenerationParams const&) [inlined] JSC::Wasm::storeWasmContext(JSC::B3::Procedure&, JSC::B3::BasicBlock*, JSC::B3::Value*)::$_43::operator()(JSC::CCallHelpers&, JSC::B3::StackmapGenerationParams const&) const + 47 at WasmB3IRGenerator.cpp:257 [opt] frame #6: 0x0000000100ba76f8 JavaScriptCore`WTF::SharedTaskFunctor<void (JSC::CCallHelpers&, JSC::B3::StackmapGenerationParams const&), JSC::Wasm::storeWasmContext(JSC::B3::Procedure&, JSC::B3::BasicBlock*, JSC::B3::Value*)::$_43>::run(this=<unavailable>, arguments=<unavailable>, arguments=<unavailable>) + 24 at SharedTask.h:90 [opt] frame #7: 0x000000010033facb JavaScriptCore`JSC::B3::PatchpointSpecial::generate(this=<unavailable>, inst=<unavailable>, jit=<unavailable>, context=<unavailable>) + 1355 at B3PatchpointSpecial.cpp:153 [opt] frame #8: 0x00000001001d7353 JavaScriptCore`JSC::B3::Air::generate(code=<unavailable>, jit=0x000000010150adf0) + 1955 at AirGenerate.cpp:225 [opt] frame #9: 0x0000000100ba06e7 JavaScriptCore`JSC::Wasm::parseAndCompile(vm=<unavailable>, compilationContext=0x00000001041fa540, functionStart="\x02\x03\x7f\x01|\x02\x7f#\x06!\x01#\x06A\x10j$\x06 \x01!\x03 \x01A\bj\"\x02A", functionLength=159, signature=0x00000001021f9cd8, unlinkedWasmToWasmCalls=<unavailable>, info=<unavailable>, moduleSignatureIndicesToUniquedSignatureIndices=<unavailable>, optLevel=1) + 1415 at WasmB3IRGenerator.cpp:1253 [opt] frame #10: 0x0000000100bf8559 JavaScriptCore`JSC::Wasm::Plan::run(this=<unavailable>)::$_1::operator()() const + 377 at WasmPlan.cpp:180 [opt] frame #11: 0x0000000100bf751a JavaScriptCore`JSC::Wasm::Plan::run(this=<unavailable>, recompileMode=<unavailable>) + 2170 at WasmPlan.cpp:209 [opt] frame #12: 0x00000001009550f2 JavaScriptCore`JSC::JSWebAssemblyModule::buildCodeBlock(this=0x0000000103513f60, vm=0x0000000102200000, exec=0x00007fff5fbfe320, plan=0x00007fff5fbfe108, mode=<unavailable>) + 82 at JSWebAssemblyModule.cpp:49 [opt] frame #13: 0x0000000100955392 JavaScriptCore`JSC::JSWebAssemblyModule::finishCreation(this=0x0000000103513f60, vm=0x0000000102200000, exec=<unavailable>, source="", byteSize=217177) + 98 at JSWebAssemblyModule.cpp:122 [opt] frame #14: 0x0000000100955309 JavaScriptCore`JSC::JSWebAssemblyModule::create(vm=0x0000000102200000, exec=0x00007fff5fbfe320, structure=0x00000001035440a0, source="", byteSize=217177) + 185 at JSWebAssemblyModule.cpp:72 [opt] frame #15: 0x0000000100c19b06 JavaScriptCore`JSC::WebAssemblyModuleConstructor::createModule(exec=<unavailable>, buffer=<unavailable>, structure=0x00000001035440a0) + 646 at WebAssemblyModuleConstructor.cpp:84 [opt] frame #16: 0x000000010095329a JavaScriptCore`JSC::webAssemblyInstantiateFunc(exec=<unavailable>) + 442 at JSWebAssembly.cpp:102 [opt] frame #17: 0x0000453f1aa01028 frame #18: 0x00000001009bc9bb JavaScriptCore`llint_entry + 26701 at LowLevelInterpreter.asm:795 frame #19: 0x00000001009bc9bb JavaScriptCore`llint_entry + 26701 at LowLevelInterpreter.asm:795 frame #20: 0x00000001009bc9bb JavaScriptCore`llint_entry + 26701 at LowLevelInterpreter.asm:795 frame #21: 0x00000001009b5f8b JavaScriptCore`llintPCRangeStart + 299 at LowLevelInterpreter64.asm:254 frame #22: 0x0000000100817b3f JavaScriptCore`JSC::JITCode::execute(this=<unavailable>, vm=<unavailable>, protoCallFrame=<unavailable>) + 127 at JITCode.cpp:81 [opt] frame #23: 0x00000001007ddbb0 JavaScriptCore`JSC::Interpreter::executeProgram(this=<unavailable>, source=<unavailable>, callFrame=0x00000001035d8b78, thisObj=0x00000001035c86e0) + 13888 at Interpreter.cpp:888 [opt] frame #24: 0x00000001004175ce JavaScriptCore`JSC::evaluate(exec=0x00000001035d8b78, source=0x00007fff5fbff048, thisValue=<unavailable>, returnedException=0x00007fff5fbff060) + 302 at Completion.cpp:102 [opt] frame #25: 0x000000010000beb6 jsc`functionLoad(exec=0x00007fff5fbff0e0) + 630 at jsc.cpp:2169 [opt] frame #26: 0x0000453f1aa01028 frame #27: 0x00000001009bc9bb JavaScriptCore`llint_entry + 26701 at LowLevelInterpreter.asm:795 frame #28: 0x00000001009bc9bb JavaScriptCore`llint_entry + 26701 at LowLevelInterpreter.asm:795 frame #29: 0x00000001009bc9bb JavaScriptCore`llint_entry + 26701 at LowLevelInterpreter.asm:795 frame #30: 0x00000001009b5f8b JavaScriptCore`llintPCRangeStart + 299 at LowLevelInterpreter64.asm:254 frame #31: 0x0000000100817b3f JavaScriptCore`JSC::JITCode::execute(this=<unavailable>, vm=<unavailable>, protoCallFrame=<unavailable>) + 127 at JITCode.cpp:81 [opt] frame #32: 0x00000001007ddf86 JavaScriptCore`JSC::Interpreter::executeCall(this=<unavailable>, callFrame=<unavailable>, function=<unavailable>, callType=69167880, callData=<unavailable>, thisValue=JSValue @ 0x00007fff5fbff370, args=<unavailable>) + 470 at Interpreter.cpp:947 [opt] frame #33: 0x00000001003b8b32 JavaScriptCore`JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) [inlined] JSC::call(functionObject=<unavailable>, callType=JS, callData=0x00007fff5fbff4f0, thisValue=JSValue @ 0x00007fff5fbff430, args=<unavailable>) + 54 at CallData.cpp:39 [opt] frame #34: 0x00000001003b8afc JavaScriptCore`JSC::profiledCall(exec=0x00000001035d80e8, reason=Microtask, functionObject=JSValue @ r14, callType=JS, callData=0x00007fff5fbff4f0, thisValue=JSValue @ 0x00007fff5fbff430, args=<unavailable>) + 124 at CallData.cpp:59 [opt] frame #35: 0x00000001008bfa16 JavaScriptCore`JSC::JSJobMicrotask::run(this=<unavailable>, exec=<unavailable>) + 438 at JSJob.cpp:75 [opt] frame #36: 0x0000000100b96b15 JavaScriptCore`JSC::VM::drainMicrotasks() [inlined] JSC::QueuedTask::run(this=0x00000001021f6eb0) + 277 at VM.cpp:876 [opt] frame #37: 0x0000000100b96aec JavaScriptCore`JSC::VM::drainMicrotasks(this=<unavailable>) + 236 at VM.cpp:871 [opt] frame #38: 0x000000010000414f jsc`jscmain(int, char**) [inlined] int runJSC<jscmain(int, char**)::$_9>(options=<unavailable>)::$_9 const&) + 109 at jsc.cpp:3753 [opt] frame #39: 0x00000001000040e2 jsc`jscmain(argc=<unavailable>, argv=<unavailable>) + 4738 at jsc.cpp:3824 [opt] frame #40: 0x0000000100002e4b jsc`main(argc=<unavailable>, argv=<unavailable>) + 27 at jsc.cpp:3306 [opt] frame #41: 0x00007fff9e7f8255 libdyld.dylib`start + 1

The bug is trivially fixable. The problem was this code: void storeWasmContext(GPRReg src) { #if ENABLE(FAST_TLS_JIT) if (Options::useWebAssemblyFastTLS()) storeToTLSPtr(src, fastTLSOffsetForKey(WTF_WASM_CONTEXT_KEY)); #endif // FIXME: Save this state elsewhere to allow PIC. https://bugs.webkit.org/show_bug.cgi?id=169773 storePtr(src, &m_vm->wasmContext); } Since there was no return after storeToTLSPtr, we always fall through, and then barf because of scratch register.

Fixed in r214402.

(In reply to Filip Pizlo from comment #39) > This causes crashes in WasmBench when JSC is built with build-jsc --release. > > JF: how did you build and test this patch? Ugh apologies! I thought I'd re-run the tests in debug on x86, but evidently that's not what I did. I had run x86 debug+release as well as A64 prior to the breaking change. I must have run the tests in one of the other workspaces I had going :( Thanks for fixing!

To be clear, the issue is not just EWS, it's all open source builds, either on bots or one's local machine. > - Copy the relevant bits to a fallback header in WebKit. This could diverge, and copying code is ew. We have some techniques that mitigate the risk (see any *SPI.h headers).