Bug 171088 - Update the MASM probe to only take 1 arg instead of 2 (in addition to the callback function).
Summary: Update the MASM probe to only take 1 arg instead of 2 (in addition to the cal...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-20 16:03 PDT by Mark Lam
Modified: 2017-04-20 17:21 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch. (36.56 KB, patch)
2017-04-20 16:43 PDT, Mark Lam
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-04-20 16:03:46 PDT
Experience shows that we never use the 2nd arg.  So, let's remove it to reduce the footprint at each probe site.

Also fix the MacroAssembler::print() function so that it is a no-op when !ENABLE(MASM_PROBE).  This will allow us to have print() statements in JIT code without a lot of #if ENABLE(MASM_PROBE)s later.
Comment 1 Mark Lam 2017-04-20 16:43:49 PDT
Created attachment 307660 [details]
proposed patch.
Comment 2 Mark Lam 2017-04-20 16:45:30 PDT
I've done builds for ARMv7, ARM64, x86, and x86_64 with this patch, and ran a smoke test of printing something using the MASM print.

I also did a build with !ENABLE(MASM_PROBE) to confirm that MASM print becomes a no-op in that case.
Comment 3 Michael Saboff 2017-04-20 16:51:37 PDT
Comment on attachment 307660 [details]
proposed patch.

r=me
Comment 4 Saam Barati 2017-04-20 16:52:05 PDT
r=me too
Comment 5 Mark Lam 2017-04-20 16:57:02 PDT
Thanks for the reviews.  Landed in r215592: <http://trac.webkit.org/r215592>.
Comment 6 Ryan Haddad 2017-04-20 17:11:31 PDT
(In reply to Mark Lam from comment #5)
> Thanks for the reviews.  Landed in r215592: <http://trac.webkit.org/r215592>.

This change appears to have broken the CLoop build:

https://build.webkit.org/builders/Apple%20Sierra%20LLINT%20CLoop%20%28BuildAndTest%29/builds/1009
Comment 7 Mark Lam 2017-04-20 17:21:40 PDT
(In reply to Ryan Haddad from comment #6)
> (In reply to Mark Lam from comment #5)
> > Thanks for the reviews.  Landed in r215592: <http://trac.webkit.org/r215592>.
> 
> This change appears to have broken the CLoop build:
> 
> https://build.webkit.org/builders/
> Apple%20Sierra%20LLINT%20CLoop%20%28BuildAndTest%29/builds/1009

Build fix for this landed in r215595: <http://trac.webkit.org/r215595>.