Bug 34888 – Mac OS X: Use deployment target to determine whether memory tagging should be enabled

• Component: JavaScriptCore
• Status: CLOSED FIXED

[Kent Hansen]

When building on 10.5+ but targeting 10.4, tagging should be disabled, otherwise mmap() fails (and consequently, WebKit crashes).

[Kent Hansen]

Created attachment 48641 [details]
Proposed patch

[Eric Seidel (no email)]

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

[Mark Mentovai]

Comment on attachment 48641 [details]
Proposed patch

Seems right to me.

[Eric Seidel (no email)]

Comment on attachment 48641 [details]
Proposed patch

OK.  I think I understand and this looks sane.

[WebKit Commit Bot]

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

[Kent Hansen]

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

[Tor Arne Vestbø]

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

[Tor Arne Vestbø]

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

[WebKit Commit Bot]

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

Clearing flags on attachment: 50201

Committed r55889: <http://trac.webkit.org/changeset/55889>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Rolled out rr55889 in r56062 as it caused crashes on Leopard

[Kent Hansen]

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?).

[Kent Hansen]

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.

[Kent Hansen]

(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.

[Eric Seidel (no email)]

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.

[Kent Hansen]

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

[Kent Hansen]

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

[Geoffrey Garen]

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

[Kent Hansen]

(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.

[Maciej Stachowiak]

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

r=me

[Geoffrey Garen]

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

[Kent Hansen]

(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.

[Kent Hansen]

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

[Kent Hansen]

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.

[Kent Hansen]

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?

[Geoffrey Garen]

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

r=me

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

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