Bug 158661 - [JSC] Move calling convention flags to WTF
Summary: [JSC] Move calling convention flags to WTF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-11 07:49 PDT by Yusuke Suzuki
Modified: 2016-06-15 09:43 PDT (History)
10 users (show)

See Also:

Attachments
Patch (5.29 KB, patch)
2016-06-11 07:54 PDT, Yusuke Suzuki 		keith_miller: review+
Details | Formatted Diff | Diff
Patch for landing (5.56 KB, patch)
2016-06-11 08:46 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch for landing (5.82 KB, patch)
2016-06-13 08:28 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Windows EWS checking (5.77 KB, patch)
2016-06-13 09:34 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Windows EWS checking (5.82 KB, patch)
2016-06-13 10:01 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Windows EWS checking (6.28 KB, patch)
2016-06-15 07:33 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Windows EWS checking (37.53 KB, patch)
2016-06-15 08:11 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Windows EWS checking (37.65 KB, patch)
2016-06-15 08:33 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-06-11 07:49:49 PDT 
[JSC] Move calling convension flags to WTF
Comment 1 Yusuke Suzuki 2016-06-11 07:54:50 PDT 
Created attachment 281100 [details]
Patch
Comment 2 Keith Miller 2016-06-11 08:25:09 PDT 
Comment on attachment 281100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281100&action=review

r=me with comments.

> Source/JavaScriptCore/ChangeLog:3
> +        [JSC] Move calling convension flags to WTF

typo: convension => convention

> Source/JavaScriptCore/ChangeLog:8
> +        Due to some calling convension flags and JIT_OPERATION falgs, MathCommon.h inclues MacroAssemblerCodeRef and JITOperations.h.

ditto.

> Source/JavaScriptCore/ChangeLog:10
> +        As the same to other calling convension flags like JSC_HOST_CALL, we move these flags to WTF.

I think this should be: 

As with other calling convention flags like JSC_HOST_CALL, these flags should be in WTF
Comment 3 Yusuke Suzuki 2016-06-11 08:42:02 PDT 
Comment on attachment 281100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281100&action=review

Thanks!!

>> Source/JavaScriptCore/ChangeLog:3
>> +        [JSC] Move calling convension flags to WTF
> 
> typo: convension => convention

Oooops, fixed.

>> Source/JavaScriptCore/ChangeLog:8
>> +        Due to some calling convension flags and JIT_OPERATION falgs, MathCommon.h inclues MacroAssemblerCodeRef and JITOperations.h.
> 
> ditto.

fixed.

>> Source/JavaScriptCore/ChangeLog:10
>> +        As the same to other calling convension flags like JSC_HOST_CALL, we move these flags to WTF.
> 
> I think this should be: 
> 
> As with other calling convention flags like JSC_HOST_CALL, these flags should be in WTF

Looks very nice! Changed.
Comment 4 Yusuke Suzuki 2016-06-11 08:46:12 PDT 
Created attachment 281101 [details]
Patch for landing

Check Windows port EWS
Comment 5 Mark Lam 2016-06-11 10:47:58 PDT 
Comment on attachment 281100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281100&action=review

>>> Source/JavaScriptCore/ChangeLog:8
>>> +        Due to some calling convension flags and JIT_OPERATION falgs, MathCommon.h inclues MacroAssemblerCodeRef and JITOperations.h.
>> 
>> ditto.
> 
> fixed.

Another typo: /falgs/flags/.
Comment 6 Yusuke Suzuki 2016-06-13 08:28:20 PDT 
Created attachment 281174 [details]
Patch for landing

Check Windows port EWS
Comment 7 Yusuke Suzuki 2016-06-13 09:34:51 PDT 
Created attachment 281177 [details]
Windows EWS checking

Check Windows port EWS, part 2...
Comment 8 Yusuke Suzuki 2016-06-13 10:01:21 PDT 
Created attachment 281179 [details]
Windows EWS checking

Check Windows port EWS, part 3...
Comment 9 Yusuke Suzuki 2016-06-15 07:33:57 PDT 
Created attachment 281360 [details]
Windows EWS checking

Check Windows port EWS, part 4...
Comment 10 Yusuke Suzuki 2016-06-15 08:11:54 PDT 
Created attachment 281361 [details]
Windows EWS checking

Check Windows port EWS, part 5...
Comment 11 Yusuke Suzuki 2016-06-15 08:14:27 PDT 
Ah OK, it seems that I've just found the existing bug in Windows port.
In Windows port, previously, we typedef the JIT operations as follows,

typedef EncodeJSValue JIT_OPERATION (*J_JITOperation_E)(ExecState*);

And we assumed that JIT_OPERATION is CDECL (And CDECL is __cdecl in Windows port).
But it was not correct, since 

typedef EncodeJSValue __cdecl (*J_JITOperation_E(ExecState*);

is syntax incorrect.


In previous Windows port, we accidentally define JIT_OPERATION here as "". (Maybe, CALLING_CONVENTION_IS_STDCALL is not defined accidentally in JITOperation.h)
So, the above typedef is recognized as

typedef EncodeJSValue  (*J_JITOperation_E)(ExecState*);

I checked it by manually typedefining `typedef EncodeJSValue __cdecl (*J_JITOperation_E)(ExecState*);` and seeing the build failure on Windows EWS.



To correct this, we need to define it as follows in Windows MSVC environment.

typedef EncodeJSValue  (__cdecl *J_JITOperation_E)(ExecState*);

So, we need to typedef the function pointer as follows,

typedef EncodeJSValue (JIT_OPERATION *J_JITOperation_E)(ExecState*);
Comment 12 Yusuke Suzuki 2016-06-15 08:33:56 PDT 
Created attachment 281362 [details]
Windows EWS checking

Check Windows port EWS, part 6... Maybe, the last check.
Comment 13 Yusuke Suzuki 2016-06-15 09:27:16 PDT 
Comment on attachment 281100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281100&action=review

>>>> Source/JavaScriptCore/ChangeLog:8
>>>> +        Due to some calling convension flags and JIT_OPERATION falgs, MathCommon.h inclues MacroAssemblerCodeRef and JITOperations.h.
>>> 
>>> ditto.
>> 
>> fixed.
> 
> Another typo: /falgs/flags/.

Thanks! Fixed.
Comment 14 Yusuke Suzuki 2016-06-15 09:28:26 PDT 
Committed r202092: <http://trac.webkit.org/changeset/202092>
Comment 15 Yusuke Suzuki 2016-06-15 09:43:58 PDT 
Committed r202094: <http://trac.webkit.org/changeset/202094>