| Summary: | WebAssembly: several tests added in r214504 crash when building with GCC | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | buildbot, jfbastien, keith_miller, mark.lam, msaboff, saam, ysuzuki | ||||||
| Priority: | P2 | Keywords: | Gtk | ||||||
| Version: | WebKit Local Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=169187 | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 170391 | ||||||||
| Attachments: |
|
||||||||
Created attachment 306054 [details]
Patch
That's not good! Would you want to prepare a patch to fix it? (In reply to JF Bastien from comment #2) > That's not good! Would you want to prepare a patch to fix it? Ha, we raced! I think it would be better to get `VM& vm` from plan, and then use it, instead of doing p->vm(). (In reply to JF Bastien from comment #3) > (In reply to JF Bastien from comment #2) > > That's not good! Would you want to prepare a patch to fix it? > > Ha, we raced! > > > I think it would be better to get `VM& vm` from plan, and then use it, > instead of doing p->vm(). Why? p and plan are actually the same, plan is just a Ref created from p (which is a reference, we are not doing p->vm() but p.vm()) to pass it to the lambda. Created attachment 306063 [details]
Updated patch
Make it simpler by using makeRefPtr in the lambda capture
Comment on attachment 306063 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=306063&action=review > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94 > + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable { Why not plan=WTFMove(plan)? (In reply to Saam Barati from comment #6) > Comment on attachment 306063 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306063&action=review > > > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94 > > + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable { > > Why not plan=WTFMove(plan)? Because I've removed the line that created the plan to simplify it. RefPtr<Plan> plan = makeRef(p); [plan = WTFMove(plan)] is equivalent to [plan = makeRefPtr(p)] I think this way is simpler, since we were only creating the plan to transfer it to the lambda capture (and to get the vm, but that is what caused the crash). Comment on attachment 306063 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=306063&action=review >>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94 >>> + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable { >> >> Why not plan=WTFMove(plan)? > > Because I've removed the line that created the plan to simplify it. > > RefPtr<Plan> plan = makeRef(p); > [plan = WTFMove(plan)] > > is equivalent to > > [plan = makeRefPtr(p)] > > I think this way is simpler, since we were only creating the plan to transfer it to the lambda capture (and to get the vm, but that is what caused the crash). Gotcha. I see what's going on. I still think you need a local variable for the plan, otherwise, won't the lambda capture the outermost plan? I think that'll lead to a reference cycle. Even if that assignment doesn't cause this to happen, I think it's harder to reason about than just having a local variable. Comment on attachment 306063 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=306063&action=review >>>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94 >>>> + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable { >>> >>> Why not plan=WTFMove(plan)? >> >> Because I've removed the line that created the plan to simplify it. >> >> RefPtr<Plan> plan = makeRef(p); >> [plan = WTFMove(plan)] >> >> is equivalent to >> >> [plan = makeRefPtr(p)] >> >> I think this way is simpler, since we were only creating the plan to transfer it to the lambda capture (and to get the vm, but that is what caused the crash). > > Gotcha. I see what's going on. > I still think you need a local variable for the plan, otherwise, won't the lambda capture the outermost plan? I think that'll lead to a reference cycle. > Even if that assignment doesn't cause this to happen, I think it's harder to reason about than just having a local variable. Seems like you could also keep the code as is, and just use p.vm() instead of plan->vm(). Also, I'm not sure why we make a RefPtr instead of a Ref here. (In reply to Saam Barati from comment #9) > Comment on attachment 306063 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306063&action=review > > >>>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94 > >>>> + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable { > >>> > >>> Why not plan=WTFMove(plan)? > >> > >> Because I've removed the line that created the plan to simplify it. > >> > >> RefPtr<Plan> plan = makeRef(p); > >> [plan = WTFMove(plan)] > >> > >> is equivalent to > >> > >> [plan = makeRefPtr(p)] > >> > >> I think this way is simpler, since we were only creating the plan to transfer it to the lambda capture (and to get the vm, but that is what caused the crash). > > > > Gotcha. I see what's going on. > > I still think you need a local variable for the plan, otherwise, won't the lambda capture the outermost plan? I think that'll lead to a reference cycle. > > Even if that assignment doesn't cause this to happen, I think it's harder to reason about than just having a local variable. > > Seems like you could also keep the code as is, and just use p.vm() instead > of plan->vm(). > Also, I'm not sure why we make a RefPtr instead of a Ref here. This weirdness is why I suggest getting VM, and then using it on this line. The entire problem is using plan two ways on one line of code: once to get to VM, once to lambda capture. Getting rid of one is an easy fix, lambda capture can't go, so VM on another line seems like the right fix. (In reply to Saam Barati from comment #8) > Comment on attachment 306063 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306063&action=review > > >>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94 > >>> + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable { > >> > >> Why not plan=WTFMove(plan)? > > > > Because I've removed the line that created the plan to simplify it. > > > > RefPtr<Plan> plan = makeRef(p); > > [plan = WTFMove(plan)] > > > > is equivalent to > > > > [plan = makeRefPtr(p)] > > > > I think this way is simpler, since we were only creating the plan to transfer it to the lambda capture (and to get the vm, but that is what caused the crash). > > Gotcha. I see what's going on. > I still think you need a local variable for the plan, otherwise, won't the > lambda capture the outermost plan? I think that'll lead to a reference cycle. I think it's the same, we are taking a reference of p and transferring it to the lambda. > Even if that assignment doesn't cause this to happen, I think it's harder to > reason about than just having a local variable. I don't mind leaving the local variable, I just thought it was simpler this way, in one step. (In reply to Saam Barati from comment #9) > Comment on attachment 306063 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306063&action=review > > >>>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94 > >>>> + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable { > >>> > >>> Why not plan=WTFMove(plan)? > >> > >> Because I've removed the line that created the plan to simplify it. > >> > >> RefPtr<Plan> plan = makeRef(p); > >> [plan = WTFMove(plan)] > >> > >> is equivalent to > >> > >> [plan = makeRefPtr(p)] > >> > >> I think this way is simpler, since we were only creating the plan to transfer it to the lambda capture (and to get the vm, but that is what caused the crash). > > > > Gotcha. I see what's going on. > > I still think you need a local variable for the plan, otherwise, won't the lambda capture the outermost plan? I think that'll lead to a reference cycle. > > Even if that assignment doesn't cause this to happen, I think it's harder to reason about than just having a local variable. > > Seems like you could also keep the code as is, and just use p.vm() instead > of plan->vm(). Yes, that's my first patch see comment #1 > Also, I'm not sure why we make a RefPtr instead of a Ref here. I think it's because then we end up doing Ref<> foo = Ref(); and Ref(const Ref& other) is deleted. (In reply to JF Bastien from comment #10) > (In reply to Saam Barati from comment #9) > > Comment on attachment 306063 [details] > > Updated patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=306063&action=review > > > > >>>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94 > > >>>> + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable { > > >>> > > >>> Why not plan=WTFMove(plan)? > > >> > > >> Because I've removed the line that created the plan to simplify it. > > >> > > >> RefPtr<Plan> plan = makeRef(p); > > >> [plan = WTFMove(plan)] > > >> > > >> is equivalent to > > >> > > >> [plan = makeRefPtr(p)] > > >> > > >> I think this way is simpler, since we were only creating the plan to transfer it to the lambda capture (and to get the vm, but that is what caused the crash). > > > > > > Gotcha. I see what's going on. > > > I still think you need a local variable for the plan, otherwise, won't the lambda capture the outermost plan? I think that'll lead to a reference cycle. > > > Even if that assignment doesn't cause this to happen, I think it's harder to reason about than just having a local variable. > > > > Seems like you could also keep the code as is, and just use p.vm() instead > > of plan->vm(). > > Also, I'm not sure why we make a RefPtr instead of a Ref here. > > This weirdness is why I suggest getting VM, and then using it on this line. > The entire problem is using plan two ways on one line of code: once to get > to VM, once to lambda capture. Getting rid of one is an easy fix, lambda > capture can't go, so VM on another line seems like the right fix. I don't see anything weird in the current patch, TBH. But I'm fine with any other solution, either my first patch or adding another local variable for the VM or a combination of both using a local variable for VM, but using makeRefPtr directly in the lambda capture. All of them should work. (In reply to Carlos Garcia Campos from comment #12) > (In reply to Saam Barati from comment #9) > > Comment on attachment 306063 [details] > > Updated patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=306063&action=review > > > > >>>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94 > > >>>> + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable { > > >>> > > >>> Why not plan=WTFMove(plan)? > > >> > > >> Because I've removed the line that created the plan to simplify it. > > >> > > >> RefPtr<Plan> plan = makeRef(p); > > >> [plan = WTFMove(plan)] > > >> > > >> is equivalent to > > >> > > >> [plan = makeRefPtr(p)] > > >> > > >> I think this way is simpler, since we were only creating the plan to transfer it to the lambda capture (and to get the vm, but that is what caused the crash). > > > > > > Gotcha. I see what's going on. > > > I still think you need a local variable for the plan, otherwise, won't the lambda capture the outermost plan? I think that'll lead to a reference cycle. > > > Even if that assignment doesn't cause this to happen, I think it's harder to reason about than just having a local variable. > > > > Seems like you could also keep the code as is, and just use p.vm() instead > > of plan->vm(). > > Yes, that's my first patch see comment #1 I like your first patch. I'm not sure what the semantics are here of your capture of plan, since you don't define another variable named "plan". Anyways, I'm worried about a reference cycle, which is why I like an explicit local variable. See the comment in WasmPlan.h. > > > Also, I'm not sure why we make a RefPtr instead of a Ref here. > > I think it's because then we end up doing Ref<> foo = Ref(); and Ref(const > Ref& other) is deleted. Why not just do: Ref<Plan> plan(p); ? (In reply to Saam Barati from comment #14) > (In reply to Carlos Garcia Campos from comment #12) > > (In reply to Saam Barati from comment #9) > > > Comment on attachment 306063 [details] > > > Updated patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=306063&action=review > > > > > > >>>> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:94 > > > >>>> + p.vm().promiseDeferredTimer->scheduleWorkSoon(promise, [source, promise, globalObject, plan = makeRefPtr(p)] () mutable { > > > >>> > > > >>> Why not plan=WTFMove(plan)? > > > >> > > > >> Because I've removed the line that created the plan to simplify it. > > > >> > > > >> RefPtr<Plan> plan = makeRef(p); > > > >> [plan = WTFMove(plan)] > > > >> > > > >> is equivalent to > > > >> > > > >> [plan = makeRefPtr(p)] > > > >> > > > >> I think this way is simpler, since we were only creating the plan to transfer it to the lambda capture (and to get the vm, but that is what caused the crash). > > > > > > > > Gotcha. I see what's going on. > > > > I still think you need a local variable for the plan, otherwise, won't the lambda capture the outermost plan? I think that'll lead to a reference cycle. > > > > Even if that assignment doesn't cause this to happen, I think it's harder to reason about than just having a local variable. > > > > > > Seems like you could also keep the code as is, and just use p.vm() instead > > > of plan->vm(). > > > > Yes, that's my first patch see comment #1 > I like your first patch. I'm not sure what the semantics are here of your > capture of plan, since you don't define another variable named "plan". > Anyways, I'm worried about a reference cycle, which is why I like an > explicit local variable. See the comment in WasmPlan.h. But this lambda is not the completion task one. That one is kept as a member of Plan, that's why it can cause a cycle. In this case we are keeping a ref in the lambda even if we use a local variable, because create the ref and then move to the lambda is the same as creating the ref in the lambda capture directly. > > > > > Also, I'm not sure why we make a RefPtr instead of a Ref here. > > > > I think it's because then we end up doing Ref<> foo = Ref(); and Ref(const > > Ref& other) is deleted. > Why not just do: > Ref<Plan> plan(p); > ? JSWebAssemblyModule::createStub() expects a RefPtr<>&& anyway, so it's simpler this way. Saam, anything else you want me to change in the patch? Comment on attachment 306054 [details]
Patch
r=me
Committed r214935: <http://trac.webkit.org/changeset/214935> |
The pattern foo->bar([f = WTFMove(foo)]); crashes when building with GCC, I assume the move happens before the foo is used to invoke the function. Thread 1 (Thread 0x7ff8237ff700 (LWP 9200)): #0 0x00007ffa7b5071e6 in std::_Function_handler<void (JSC::Wasm::Plan&), JSC::compileAndInstantiate(JSC::VM&, JSC::ExecState*, JSC::JSPromiseDeferred*, JSC::JSValue, JSC::JSObject*)::{lambda(JSC::Wasm::Plan&)#1}>::_M_invoke(std::_Any_data const&, JSC::Wasm::Plan&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #1 0x00007ffa7b4d101b in JSC::Wasm::Plan::complete(WTF::AbstractLocker const&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #2 0x00007ffa7b4d3e0b in JSC::Wasm::Plan::parseAndValidateModule() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #3 0x00007ffa7b4f8098 in JSC::Wasm::Worklist::Thread::work() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #4 0x00007ffa7b5760ef in std::_Function_handler<void (), WTF::AutomaticThread::start(WTF::AbstractLocker const&)::{lambda()#1}>::_M_invoke(std::_Any_data const&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #5 0x00007ffa7b589705 in WTF::threadEntryPoint(void*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #6 0x00007ffa7b5bb73a in WTF::wtfThreadEntryPoint(void*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #7 0x00007ffa77c0e424 in start_thread (arg=0x7ff8237ff700) at pthread_create.c:333 #8 0x00007ffa76a209bf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105