Some tools such as leak detectors and such like to look at real pointers, and poisoned ones confuse them. Add a JSC option to disable poisoning, but log to the console when this is done.
<rdar://problem/36546265>
Created attachment 331392 [details] patch
Comment on attachment 331392 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=331392&action=review r=me. > Source/JavaScriptCore/ChangeLog:3 > + Allow dangerous disabling of poison I like to live my life on the edge... > Source/JavaScriptCore/runtime/Options.h:490 > + v(bool, dangerousDisablePoison, false, Normal, "if true, all poison will be initialized to 0 which defeats some Spectre and type confusion mitigations, but allows tools such as leak detectors to function better.") \ Nit: Can we put this by the other Spectre flags? Or move the other flags into their own section.
Created attachment 331395 [details] patch Move option.
Created attachment 331421 [details] patch Remove logging since it's not very clear and actionable.
Comment on attachment 331421 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=331421&action=review > Source/JavaScriptCore/runtime/Options.h:463 > + v(bool, dangerousDisablePoison, false, Normal, "if true, all poison will be initialized to 0 which defeats some Spectre and type confusion mitigations, but allows tools such as leak detectors to function better.") \ Why did we name it like this? I would've done: usePoisoning or something along those lines. Many of our options are dangerous. I don't think it needs to be called out.
Comment on attachment 331421 [details] patch Attachment 331421 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6098072 New failing tests: http/tests/misc/resource-timing-resolution.html
Created attachment 331435 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
(In reply to Saam Barati from comment #6) > Comment on attachment 331421 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331421&action=review > > > Source/JavaScriptCore/runtime/Options.h:463 > > + v(bool, dangerousDisablePoison, false, Normal, "if true, all poison will be initialized to 0 which defeats some Spectre and type confusion mitigations, but allows tools such as leak detectors to function better.") \ > > Why did we name it like this? I would've done: > usePoisoning > or something along those lines. > > Many of our options are dangerous. I don't think it needs to be called out. Because in this case we'll likely tell people to use this option if they want these tools to work, and it should be used with care. Is there a precedent for us telling people "set this option to get feature X working" when it's dangerous to do so?
Created attachment 331441 [details] patch Talked to Michael, moved to the name Saam suggested. Sending to CQ.
Comment on attachment 331441 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=331441&action=review > Source/JavaScriptCore/runtime/JSCPoison.cpp:44 > + if (Options::usePoisoning()) > + return; needs to be inverted.
Created attachment 331457 [details] patch (In reply to Saam Barati from comment #11) > Comment on attachment 331441 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331441&action=review > > > Source/JavaScriptCore/runtime/JSCPoison.cpp:44 > > + if (Options::usePoisoning()) > > + return; > > needs to be inverted. Outch, thanks for catching that! Coding while child mostly plays next to me is a bad idea...
The commit-queue encountered the following flaky tests while processing attachment 331457 [details]: imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-a-bitrate.html bug 181669 (authors: cdumez@apple.com and jer.noble@apple.com) The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 331457 [details]: imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-bitrate.html bug 181717 (authors: cdumez@apple.com and jer.noble@apple.com) imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-framerate.html bug 181718 (authors: cdumez@apple.com and jer.noble@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 331457 [details] patch Clearing flags on attachment: 331457 Committed r227047: <https://trac.webkit.org/changeset/227047>
All reviewed patches have been landed. Closing bug.