Bug 34888

Created attachment 48641 [details] Proposed patch

I don't even know what memory tagging is, so I don't think I'm the right reviewer here. :)

Comment on attachment 48641 [details] Proposed patch Seems right to me.

Comment on attachment 48641 [details] Proposed patch OK. I think I understand and this looks sane.

Comment on attachment 48641 [details] Proposed patch Rejecting patch 48641 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel', '--force']" exit_code: 1 patching file JavaScriptCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file JavaScriptCore/wtf/VMTags.h Hunk #2 FAILED at 44. 1 out of 2 hunks FAILED -- saving rejects to file JavaScriptCore/wtf/VMTags.h.rej Full output: http://webkit-commit-queue.appspot.com/results/343092

Created attachment 50201 [details] Updated patch (rebased against ToT)

*** Bug 29538 has been marked as a duplicate of this bug. ***

*** Bug 35639 has been marked as a duplicate of this bug. ***

Comment on attachment 50201 [details] Updated patch (rebased against ToT) Clearing flags on attachment: 50201 Committed r55889: <http://trac.webkit.org/changeset/55889>

All reviewed patches have been landed. Closing bug.

Rolled out rr55889 in r56062 as it caused crashes on Leopard

My comment copied from https://bugs.webkit.org/show_bug.cgi?id=36118#c11: It looks like we are targeting Tiger (i.e. TARGETING_TIGER will be defined) even when building on Leopard and above. This means it's going to use the vm_map tags that work with Tiger, but unfortunately they _don't_ work with Leopard (and vice versa). But not crashing on Leopard (and crashing on Tiger) when you built it on Leopard is better than it not crashing on Tiger, I guess. ;D The way it is now (or with the patch rolled out, that is), I think there should be a configure/compile-time failure when this is detected, since we claim to target Tiger but the binary is going to crash on Tiger. Or, we bump the default macosx-version-min-required to 10.5 so that it will work for us (but one can still override it one needs to target 10.4, which some of our customers still do). I'll also investigate whether there is a set of tags that work on both 10.4 and 10.5, even if that means disabling the features that the tagging provides (AFAIK it's only for debugging/profiling?).

Created attachment 51158 [details] Revised patch (don't crash on Leopard when targeting Tiger!) New attempt, this time tested on Tiger/Safari build, Leopard+SnowLeopard/Qt build. Executive summary: - vm_{map,allocate} on (Snow) Leopard objects to being passed tag == -1. - mmap on Tiger objects to being passed tag != -1. - vm_{map,allocate} on Tiger doesn't mind being passed tag != -1. - mmap on (Snow) Leopard doesn't mind being passed tag == -1. The patch follows from that. It would be nice if Geoffrey Garen (and/or Mark Rowe) could take a look at this, since he did the original vm-tags Tiger crash fix.

(In reply to comment #13) > Created an attachment (id=51158) [details] > Revised patch (don't crash on Leopard when targeting Tiger!) > > New attempt, this time tested on Tiger/Safari build, Leopard+SnowLeopard/Qt > build. Tested Leopard/Safari build now, works.

Comment on attachment 48641 [details] Proposed patch Cleared Eric Seidel's review+ from obsolete attachment 48641 [details] so that this bug does not appear in http://webkit.org/pending-commit.

CC'ing Geoffrey, please slap me or poke the appropriate Mac dude. :)

*** Bug 29542 has been marked as a duplicate of this bug. ***

Did you verify that mapped memory is still tagged on Leopard and SnowLeopard?

(In reply to comment #18) > Did you verify that mapped memory is still tagged on Leopard and SnowLeopard? No, not beyond some printfs to try to verify that the defines seem sensible after the change. Is there a tool I can use to gather the information? Then I could do a before/after comparison.

Comment on attachment 51158 [details] Revised patch (don't crash on Leopard when targeting Tiger!) r=me

printfs are prolly sufficient. You can test the tagging by running 'vmmap Safari' and looking for the fast malloc tag.

(In reply to comment #21) > printfs are prolly sufficient. You can test the tagging by running 'vmmap > Safari' and looking for the fast malloc tag. Thanks, that's useful. On Leopard: Before patch: REGION TYPE [ VIRTUAL] =========== [ =======] ATS (font support) [ 32.7M] CG backing stores [ 3508K] CG raster data [ 3744K] CG shared images [ 3208K] Carbon [ 1096K] CoreGraphics [ 136K] IOKit [ 256.0M] MALLOC [ 22.2M] Memory tag=63 [ 768K] Memory tag=65 [ 4160K] STACK GUARD [ 56.1M] Stack [ 11.1M] VM_ALLOCATE ? [ 13.3M] __DATA [ 5068K] __IMAGE [ 1240K] __LINKEDIT [ 19.0M] __OBJC [ 2132K] __PAGEZERO [ 4K] __TEXT [ 100.2M] __UNICODE [ 532K] mapped file [ 24.0M] shared memory [ 16.0M] After patch (and a pull from latest trunk): REGION TYPE [ VIRTUAL] =========== [ =======] ATS (font support) [ 32.7M] CG backing stores [ 3508K] CG raster data [ 3684K] CG shared images [ 3208K] Carbon [ 1096K] CoreGraphics [ 136K] IOKit [ 256.0M] MALLOC [ 22.2M] Memory tag=63 [ 768K] Memory tag=65 [ 4160K] STACK GUARD [ 56.1M] Stack [ 11.1M] VM_ALLOCATE ? [ 13.5M] __DATA [ 5028K] __IMAGE [ 1240K] __LINKEDIT [ 19.1M] __OBJC [ 2132K] __PAGEZERO [ 4K] __TEXT [ 100.6M] __UNICODE [ 532K] mapped file [ 28.5M] shared memory [ 16.0M] (63 is collector memory; 65 is registerfile; 64 isn't used) Looks encouraging.

With or without the patch, my Qt/Mac build of r57269 with Qt/4.7 from March 15, on Leopard-PPC, crashes with JSC-related backtrace (on e.g. qt.nokia.com). The Safari build is fine. No rush to set the cq+, in other words; I'll wait until I have a non-crashing build without patch, to decrease the chance of this patch being blamed for the crash. :) Exception Type: EXC_BAD_ACCESS (SIGBUS) Exception Codes: KERN_PROTECTION_FAILURE at 0x00000000000000a3 Crashed Thread: 0 Thread 0 Crashed: 0 QtWebKit 0x02ab1b60 JSC::BytecodeGenerator::addConstant(JSC::Identifier const&) + 32 1 QtWebKit 0x02ab28e0 JSC::BytecodeGenerator::emitGetById(JSC::RegisterID*, JSC::RegisterID*, JSC::Identifier const&) + 384 2 QtWebKit 0x02ac78c4 JSC::PostfixDotNode::emitBytecode(JSC::BytecodeGenerator&, JSC::RegisterID*) + 708 3 QtWebKit 0x02ad4f24 JSC::ExprStatementNode::emitBytecode(JSC::BytecodeGenerator&, JSC::RegisterID*) + 324 4 QtWebKit 0x02ac9834 JSC::BlockNode::emitBytecode(JSC::BytecodeGenerator&, JSC::RegisterID*) + 340 5 QtWebKit 0x02ad53b4 JSC::IfNode::emitBytecode(JSC::BytecodeGenerator&, JSC::RegisterID*) + 1124 6 QtWebKit 0x02ac9834 JSC::BlockNode::emitBytecode(JSC::BytecodeGenerator&, JSC::RegisterID*) + 340 7 QtWebKit 0x02ac2464 JSC::FunctionBodyNode::emitBytecode(JSC::BytecodeGenerator&, JSC::RegisterID*) + 372 8 QtWebKit 0x02aaee1c JSC::BytecodeGenerator::generate() + 60 9 QtWebKit 0x02b4da78 JSC::FunctionExecutable::compile(JSC::ExecState*, JSC::ScopeChainNode*) + 1144 10 QtWebKit 0x02af62b8 JSC::Interpreter::privateExecute(JSC::Interpreter::ExecutionFlag, JSC::RegisterFile*, JSC::ExecState*, JSC::JSValue*) + 59048 11 QtWebKit 0x02afd608 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*, JSC::JSValue*) + 744 12 QtWebKit 0x02b353b8 JSC::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue) + 488 13 QtWebKit 0x0213b1f8 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*) + 664 14 QtWebKit 0x0213be10 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) + 48 15 QtWebKit 0x0215aad8 WebCore::ScriptController::executeScript(WebCore::ScriptSourceCode const&) + 600 16 QtWebKit 0x02455ea4 WebCore::HTMLTokenizer::scriptExecution(WebCore::ScriptSourceCode const&, WebCore::HTMLTokenizer::State) + 84 17 QtWebKit 0x02456cf0 WebCore::HTMLTokenizer::executeExternalScriptsIfReady() + 2272 18 QtWebKit 0x024cebf4 WebCore::CachedScript::checkNotify() + 100 19 QtWebKit 0x025248a4 WebCore::Loader::Host::didFinishLoading(WebCore::SubresourceLoader*) + 308 20 QtWebKit 0x02545060 WebCore::SubresourceLoader::didFinishLoading() + 96 21 QtWebKit 0x0279d43c WebCore::QNetworkReplyHandler::finish() + 204

Alright, updated to r57338 and no more crashes. Here's vmmap output after building QtWebKit with the patch on Leopard: REGION TYPE [ VIRTUAL] =========== [ =======] ATS (font support) [ 33.7M] CG backing stores [ 2092K] CG raster data [ 676K] CG shared images [ 3208K] Carbon [ 1096K] CoreGraphics [ 136K] IOKit [ 256.0M] MALLOC [ 31.8M] Memory tag=63 [ 768K] STACK GUARD [ 67.8M] Stack [ 13.1M] VM_ALLOCATE ? [ 36.1M] __DATA [ 11.8M] __IMAGE [ 1240K] __LINKEDIT [ 16.1M] __OBJC [ 1728K] __PAGEZERO [ 4K] __TEXT [ 152.8M] __UNICODE [ 532K] mapped file [ 30.5M] shared memory [ 16.0M] Since we are targeting Tiger, the line for "Memory tag=65" that was in the Safari build (not targeting Tiger) is gone. This is because we fall back to -1 for VM_TAG_FOR_REGISTERFILE_MEMORY when targeting Tiger. Note that "Memory tag=63" is present since that one works on Tiger (previously the tag was defined as -1). Testing QtWebKit on Tiger now.

vmmap output for Qt build on Tiger: REGION TYPE [ VIRTUAL] =========== [ =======] ATS (font support) [ 33756K] Carbon [ 1036K] CoreGraphics [ 14648K] IOKit [ 262144K] MALLOC [ 30640K] Memory tag=63 [ 512K] STACK GUARD [ 8K] Stack [ 8764K] VM_ALLOCATE ? [ 14768K] __DATA [ 8164K] __IMAGE [ 1088K] __LINKEDIT [ 37200K] __OBJC [ 852K] __PAGEZERO [ 4K] __TEXT [ 104392K] mapped file [ 109944K] shared memory [ 16376K] system [ 24K] Yay, collector memory successfully tagged. cq+ anyone?

Comment on attachment 51158 [details] Revised patch (don't crash on Leopard when targeting Tiger!) r=me

Comment on attachment 51158 [details] Revised patch (don't crash on Leopard when targeting Tiger!) Clearing flags on attachment: 51158 Committed r57583: <http://trac.webkit.org/changeset/57583>

All reviewed patches have been landed. Closing bug.

Revision r57583 cherry-picked into qtwebkit-2.0 with commit e806765e627839058e343187dde930ff8e2d3f20