Bug 183573 - Make a NativeFunction into a class to support pointer profiling.
Summary: Make a NativeFunction into a class to support pointer profiling.
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: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-12 11:41 PDT by Mark Lam
Modified: 2018-03-13 08:06 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch. (208.31 KB, patch)
2018-03-12 12:44 PDT, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
proposed patch. (208.31 KB, patch)
2018-03-12 13:03 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (208.80 KB, patch)
2018-03-12 13:27 PDT, Mark Lam
fpizlo: 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 2018-03-12 11:41:21 PDT
We'll also introduce RawNativeFunction as the type of the raw pointer, and TaggedNativeFunction as the tagged version.
Comment 1 Radar WebKit Bug Importer 2018-03-12 11:52:05 PDT
<rdar://problem/38384697>
Comment 2 Mark Lam 2018-03-12 12:44:32 PDT
Created attachment 335626 [details]
proposed patch.
Comment 3 EWS Watchlist 2018-03-12 12:47:27 PDT
Attachment 335626 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:69:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:70:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Mark Lam 2018-03-12 13:03:38 PDT
Created attachment 335628 [details]
proposed patch.
Comment 5 EWS Watchlist 2018-03-12 13:06:07 PDT
Attachment 335628 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:69:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:70:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Mark Lam 2018-03-12 13:27:51 PDT
Created attachment 335632 [details]
proposed patch.
Comment 7 EWS Watchlist 2018-03-12 13:29:47 PDT
Attachment 335632 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:69:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/NativeFunction.h:70:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Filip Pizlo 2018-03-12 13:34:02 PDT
Comment on attachment 335632 [details]
proposed patch.

Nice!
Comment 9 Mark Lam 2018-03-12 14:08:14 PDT
Thanks for the review.  Landed in r229547: <http://trac.webkit.org/r229547>.
Comment 10 Ryan Haddad 2018-03-12 15:07:54 PDT
This change broke the Windows build:

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/8246

C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\jit\JITThunks.h(103): error C2220: warning treated as error - no 'object' file generated [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\LLIntOffsetsExtractor.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\jit\JITThunks.h(103): warning C4245: 'argument': conversion from 'int' to 'uintptr_t', signed/unsigned mismatch [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\LLIntOffsetsExtractor.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\jit\JITThunks.h(104): warning C4245: 'argument': conversion from 'int' to 'uintptr_t', signed/unsigned mismatch [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\LLIntOffsetsExtractor.vcxproj]
Comment 11 Mark Lam 2018-03-12 15:25:24 PDT
(In reply to Ryan Haddad from comment #10)
> This change broke the Windows build:
> ...

Speculative fix landed in r229557: <http://trac.webkit.org/r229557>.
Comment 12 Ross Kirsling 2018-03-12 19:25:08 PDT
WinCairo errors after r229557:
(https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/11559)

C:\WebKit-EWS\WebKit\Source\JavaScriptCore\jit\JITThunks.h(103): error C2440: '<function-style-cast>': cannot convert from 'int' to 'JSC::TaggedNativeFunction'
C:\WebKit-EWS\WebKit\Source\JavaScriptCore\jit\JITThunks.h(103): note: No constructor could take the source type, or constructor overload resolution was ambiguous
C:\WebKit-EWS\WebKit\Source\JavaScriptCore\jit\JITThunks.h(104): error C2440: '<function-style-cast>': cannot convert from 'int' to 'JSC::TaggedNativeFunction'
C:\WebKit-EWS\WebKit\Source\JavaScriptCore\jit\JITThunks.h(104): note: No constructor could take the source type, or constructor overload resolution was ambiguous
Comment 13 Mark Lam 2018-03-12 20:33:55 PDT
(In reply to Ross Kirsling from comment #12)
> WinCairo errors after r229557:
> (https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/11559)
> 
> C:\WebKit-EWS\WebKit\Source\JavaScriptCore\jit\JITThunks.h(103): error
> C2440: '<function-style-cast>': cannot convert from 'int' to
> 'JSC::TaggedNativeFunction'
> C:\WebKit-EWS\WebKit\Source\JavaScriptCore\jit\JITThunks.h(103): note: No
> constructor could take the source type, or constructor overload resolution
> was ambiguous
> C:\WebKit-EWS\WebKit\Source\JavaScriptCore\jit\JITThunks.h(104): error
> C2440: '<function-style-cast>': cannot convert from 'int' to
> 'JSC::TaggedNativeFunction'
> C:\WebKit-EWS\WebKit\Source\JavaScriptCore\jit\JITThunks.h(104): note: No
> constructor could take the source type, or constructor overload resolution
> was ambiguous

AppleWin bots shows that this issue has already been fixed (see build #8250).  The fix landed in r22955.  Are you seeing a build failure after that revision?
Comment 14 Mark Lam 2018-03-12 20:34:28 PDT
(In reply to Mark Lam from comment #13)
> AppleWin bots shows that this issue has already been fixed (see build
> #8250).  The fix landed in r22955.  Are you seeing a build failure after
> that revision?

Forgot the build bot url: https://build.webkit.org/builders/Apple%20Win%20Release%20(Build)?numbuilds=50
Comment 15 Ross Kirsling 2018-03-12 20:49:36 PDT
AppleWin went from red to green but WinCairo went from green to red. :)
https://build.webkit.org/builders/WinCairo%2064-Bit%20Release?numbuilds=50
Comment 16 Mark Lam 2018-03-12 23:11:07 PDT
(In reply to Ross Kirsling from comment #15)
> AppleWin went from red to green but WinCairo went from green to red. :)
> https://build.webkit.org/builders/WinCairo%2064-Bit%20Release?numbuilds=50

Speculative build fix for WinCairo landed in r229574: <http://trac.webkit.org/r229574>.
Comment 17 Ross Kirsling 2018-03-13 08:06:13 PDT
(In reply to Mark Lam from comment #16)
> (In reply to Ross Kirsling from comment #15)
> > AppleWin went from red to green but WinCairo went from green to red. :)
> > https://build.webkit.org/builders/WinCairo%2064-Bit%20Release?numbuilds=50
> 
> Speculative build fix for WinCairo landed in r229574:
> <http://trac.webkit.org/r229574>.

Thanks much!