Bug 34888 - Mac OS X: Use deployment target to determine whether memory tagging should be enabled
Summary: Mac OS X: Use deployment target to determine whether memory tagging should be...
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 29538 29542 35639 (view as bug list)
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-02-12 06:15 PST by Kent Hansen
Modified: 2010-04-26 03:29 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (1.69 KB, patch)
2010-02-12 06:19 PST, Kent Hansen
no flags Details | Formatted Diff | Diff
Updated patch (rebased against ToT) (1.61 KB, patch)
2010-03-08 03:10 PST, Kent Hansen
no flags Details | Formatted Diff | Diff
Revised patch (don't crash on Leopard when targeting Tiger!) (4.50 KB, patch)
2010-03-19 09:10 PDT, Kent Hansen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Hansen 2010-02-12 06:15:08 PST
When building on 10.5+ but targeting 10.4, tagging should be disabled, otherwise mmap() fails (and consequently, WebKit crashes).
Comment 1 Kent Hansen 2010-02-12 06:19:05 PST
Created attachment 48641 [details]
Proposed patch
Comment 2 Eric Seidel (no email) 2010-02-17 16:13:41 PST
I don't even know what memory tagging is, so I don't think  I'm the right reviewer here. :)
Comment 3 Mark Mentovai 2010-02-17 18:16:02 PST
Comment on attachment 48641 [details]
Proposed patch

Seems right to me.
Comment 4 Eric Seidel (no email) 2010-03-05 14:28:59 PST
Comment on attachment 48641 [details]
Proposed patch

OK.  I think I understand and this looks sane.
Comment 5 WebKit Commit Bot 2010-03-06 03:59:55 PST
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
Comment 6 Kent Hansen 2010-03-08 03:10:46 PST
Created attachment 50201 [details]
Updated patch (rebased against ToT)
Comment 7 Tor Arne Vestbø 2010-03-10 04:55:23 PST
*** Bug 29538 has been marked as a duplicate of this bug. ***
Comment 8 Tor Arne Vestbø 2010-03-10 04:55:34 PST
*** Bug 35639 has been marked as a duplicate of this bug. ***
Comment 9 WebKit Commit Bot 2010-03-11 22:46:46 PST
Comment on attachment 50201 [details]
Updated patch (rebased against ToT)

Clearing flags on attachment: 50201

Committed r55889: <http://trac.webkit.org/changeset/55889>
Comment 10 WebKit Commit Bot 2010-03-11 22:46:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Simon Hausmann 2010-03-16 08:39:53 PDT
Rolled out rr55889 in r56062 as it caused crashes on Leopard
Comment 12 Kent Hansen 2010-03-16 08:58:10 PDT
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?).
Comment 13 Kent Hansen 2010-03-19 09:10:02 PDT
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.
Comment 14 Kent Hansen 2010-03-22 04:20:11 PDT
(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 15 Eric Seidel (no email) 2010-03-24 14:31:32 PDT
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.
Comment 16 Kent Hansen 2010-03-25 02:38:24 PDT
CC'ing Geoffrey, please slap me or poke the appropriate Mac dude. :)
Comment 17 Kent Hansen 2010-04-06 05:55:03 PDT
*** Bug 29542 has been marked as a duplicate of this bug. ***
Comment 18 Geoffrey Garen 2010-04-07 10:01:30 PDT
Did you verify that mapped memory is still tagged on Leopard and SnowLeopard?
Comment 19 Kent Hansen 2010-04-07 10:25:09 PDT
(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 20 Maciej Stachowiak 2010-04-07 10:41:56 PDT
Comment on attachment 51158 [details]
Revised patch (don't crash on Leopard when targeting Tiger!)

r=me
Comment 21 Geoffrey Garen 2010-04-07 12:59:29 PDT
printfs are prolly sufficient. You can test the tagging by running 'vmmap Safari' and looking for the fast malloc tag.
Comment 22 Kent Hansen 2010-04-08 09:18:39 PDT
(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.
Comment 23 Kent Hansen 2010-04-09 06:24:43 PDT
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
Comment 24 Kent Hansen 2010-04-12 04:33:10 PDT
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.
Comment 25 Kent Hansen 2010-04-14 01:32:03 PDT
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 26 Geoffrey Garen 2010-04-14 09:03:24 PDT
Comment on attachment 51158 [details]
Revised patch (don't crash on Leopard when targeting Tiger!)

r=me
Comment 27 WebKit Commit Bot 2010-04-14 09:27:08 PDT
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>
Comment 28 WebKit Commit Bot 2010-04-14 09:27:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Simon Hausmann 2010-04-26 03:29:43 PDT
Revision r57583 cherry-picked into qtwebkit-2.0 with commit e806765e627839058e343187dde930ff8e2d3f20