Bug 183655 - JIT callOperation() needs to support operations that return SlowPathReturnType differently on Windows.
Summary: JIT callOperation() needs to support operations that return SlowPathReturnTyp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-15 00:18 PDT by Mark Lam
Modified: 2018-03-28 09:26 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.43 KB, patch)
2018-03-16 22:03 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Exhibit A (2.60 KB, patch)
2018-03-18 16:38 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Test script (628 bytes, text/javascript)
2018-03-21 18:22 PDT, Ross Kirsling
no flags Details
Patch (7.78 KB, patch)
2018-03-22 16:45 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.91 MB, application/zip)
2018-03-22 18:05 PDT, Build Bot
no flags Details
Patch for landing (7.83 KB, patch)
2018-03-26 14:09 PDT, Ross Kirsling
no flags 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-15 00:18:53 PDT
This was regressed in r229391 due to https://bugs.webkit.org/show_bug.cgi?id=183263.

See https://trac.webkit.org/r171005 for the reason why operations that return SlowPathReturnType need to be treated differently on Windows.
Comment 1 Ross Kirsling 2018-03-16 22:03:44 PDT
Created attachment 335998 [details]
Patch
Comment 2 Ross Kirsling 2018-03-16 22:04:58 PDT
Comment on attachment 335998 [details]
Patch

Here's the simple-and-ugly fix to restore the lost functionality. Feel free to reject and replace with a prettier solution. :P
Comment 3 Ross Kirsling 2018-03-16 22:30:48 PDT
Comment on attachment 335998 [details]
Patch

To clarify, I've confirmed that this addresses the WinCairo crash on Web Inspector launch. That said, in double-checking the additions r171005 made to CCallHelpers.h, it seems this patch may not cover all cases.
Comment 4 Yusuke Suzuki 2018-03-17 05:31:10 PDT
Comment on attachment 335998 [details]
Patch

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

> Source/JavaScriptCore/jit/CCallHelpers.h:483
> +#endif

This is unnecessary.

> Source/JavaScriptCore/jit/JIT.h:733
> +            setupArgumentsWithExecStateForCallWithSlowPathReturnType(TrustedImm32(op));

Let's just use `setupArguments<Sprt_JITOperation_EZ>(op)`.
Comment 5 Yusuke Suzuki 2018-03-17 05:39:44 PDT
Comment on attachment 335998 [details]
Patch

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

>> Source/JavaScriptCore/jit/JIT.h:733
>> +            setupArgumentsWithExecStateForCallWithSlowPathReturnType(TrustedImm32(op));
> 
> Let's just use `setupArguments<Sprt_JITOperation_EZ>(op)`.

I think the function should be like the following.

template<typename OperationType, typename... Args>
std::enable_if_t<std::is_same<FunctionTraits<OperationType>::ResultType, SlowPathReturnType>::value, MacroAssembler::Call>
callOperation(OperationType operation, Args... args)
{
    setupArguments<OperationType>(args...);
    return appendCallWithExceptionCheckAndSlowPathReturnType(operation);
}
Comment 6 Ross Kirsling 2018-03-17 14:11:37 PDT
(In reply to Yusuke Suzuki from comment #4)
> Comment on attachment 335998 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335998&action=review
> 
> > Source/JavaScriptCore/jit/CCallHelpers.h:483
> > +#endif
> 
> This is unnecessary.
> 
> > Source/JavaScriptCore/jit/JIT.h:733
> > +            setupArgumentsWithExecStateForCallWithSlowPathReturnType(TrustedImm32(op));
> 
> Let's just use `setupArguments<Sprt_JITOperation_EZ>(op)`.

I tried that at first, but it didn't resolve the crash. I assume it has to do with Windows needing argumentGPR2 and 1 instead of 1 and 0.

(In reply to Yusuke Suzuki from comment #5)
> I think the function should be like the following.
> 
> template<typename OperationType, typename... Args>
> std::enable_if_t<std::is_same<FunctionTraits<OperationType>::ResultType,
> SlowPathReturnType>::value, MacroAssembler::Call>
> callOperation(OperationType operation, Args... args)
> {
>     setupArguments<OperationType>(args...);
>     return appendCallWithExceptionCheckAndSlowPathReturnType(operation);
> }

Thanks for this -- I hadn't properly understood enable_if before. Unfortunately I'm seeing an error "'type': is not a member of 'std::enable_if<false,_Ty>'", which makes me wonder whether MSVC is handling SFINAE correctly...?
Comment 7 Keith Miller 2018-03-17 20:21:05 PDT
(In reply to Ross Kirsling from comment #6)
> (In reply to Yusuke Suzuki from comment #5)
> > I think the function should be like the following.
> > 
> > template<typename OperationType, typename... Args>
> > std::enable_if_t<std::is_same<FunctionTraits<OperationType>::ResultType,
> > SlowPathReturnType>::value, MacroAssembler::Call>
> > callOperation(OperationType operation, Args... args)
> > {
> >     setupArguments<OperationType>(args...);
> >     return appendCallWithExceptionCheckAndSlowPathReturnType(operation);
> > }
> 
> Thanks for this -- I hadn't properly understood enable_if before.
> Unfortunately I'm seeing an error "'type': is not a member of
> 'std::enable_if<false,_Ty>'", which makes me wonder whether MSVC is handling
> SFINAE correctly...?

Can you give more context? It's not clear what that error message is referring to as there's no identifier named 'type' in Yusuke's code. Maybe you can upload the broken patch?
Comment 8 Ross Kirsling 2018-03-17 21:03:17 PDT
(In reply to Keith Miller from comment #7)
> (In reply to Ross Kirsling from comment #6)
> > (In reply to Yusuke Suzuki from comment #5)
> > > I think the function should be like the following.
> > > 
> > > template<typename OperationType, typename... Args>
> > > std::enable_if_t<std::is_same<FunctionTraits<OperationType>::ResultType,
> > > SlowPathReturnType>::value, MacroAssembler::Call>
> > > callOperation(OperationType operation, Args... args)
> > > {
> > >     setupArguments<OperationType>(args...);
> > >     return appendCallWithExceptionCheckAndSlowPathReturnType(operation);
> > > }
> > 
> > Thanks for this -- I hadn't properly understood enable_if before.
> > Unfortunately I'm seeing an error "'type': is not a member of
> > 'std::enable_if<false,_Ty>'", which makes me wonder whether MSVC is handling
> > SFINAE correctly...?
> 
> Can you give more context? It's not clear what that error message is
> referring to as there's no identifier named 'type' in Yusuke's code. Maybe
> you can upload the broken patch?

Whoops, sorry for being unclear. It's the "type" arising from std::enable_if_t being an alias for typename std::enable_if...::type.
Comment 9 Ross Kirsling 2018-03-18 15:31:06 PDT
(In reply to Ross Kirsling from comment #8)
> Unfortunately I'm seeing an error "'type': is not a member of
> 'std::enable_if<false,_Ty>'", which makes me wonder whether MSVC is handling
> SFINAE correctly...?

Evidently, this error was referring to a missing `typename` keyword when accessing ResultType:

std::enable_if_t<std::is_same<typename FunctionTraits<OperationType>::ResultType, SlowPathReturnType>::value, MacroAssembler::Call>

Unfortunately, even after this, we still get an "ambiguous call to overloaded function" at `callOperation(operationOptimize, ...)`, since the new function's signature is basically the same as the existing one. Technically we could add the opposite enable_if_t there...

template<typename OperationType, typename... Args>
std::enable_if_t<!std::is_same<typename FunctionTraits<OperationType>::ResultType, SlowPathReturnType>::value, MacroAssembler::Call>
callOperation(OperationType operation, Args... args)
{
    setupArguments<OperationType>(args...);
    return appendCallWithExceptionCheck(operation);
}

...but that really doesn't seem ideal.
Comment 10 Mark Lam 2018-03-18 15:34:02 PDT
(In reply to Ross Kirsling from comment #9)
> (In reply to Ross Kirsling from comment #8)
> > Unfortunately I'm seeing an error "'type': is not a member of
> > 'std::enable_if<false,_Ty>'", which makes me wonder whether MSVC is handling
> > SFINAE correctly...?
> 
> Evidently, this error was referring to a missing `typename` keyword when
> accessing ResultType:
> 
> std::enable_if_t<std::is_same<typename
> FunctionTraits<OperationType>::ResultType, SlowPathReturnType>::value,
> MacroAssembler::Call>
> 
> Unfortunately, even after this, we still get an "ambiguous call to
> overloaded function" at `callOperation(operationOptimize, ...)`, since the
> new function's signature is basically the same as the existing one.
> Technically we could add the opposite enable_if_t there...
> 
> template<typename OperationType, typename... Args>
> std::enable_if_t<!std::is_same<typename
> FunctionTraits<OperationType>::ResultType, SlowPathReturnType>::value,
> MacroAssembler::Call>
> callOperation(OperationType operation, Args... args)
> {
>     setupArguments<OperationType>(args...);
>     return appendCallWithExceptionCheck(operation);
> }
> 
> ...but that really doesn't seem ideal.

Ross, please just upload the patch.  It'll be much easier for reviewers to see the code in context of surrounding code.  It also gives us the opportunity to comment on the lines where correction is needed, and maybe give you tips on how to resolve these issues.
Comment 11 Ross Kirsling 2018-03-18 16:38:33 PDT
Created attachment 336028 [details]
Exhibit A

This patch demonstrates Yusuke's approach with compiler errors addressed.

It does not constitute a fix for the crash.
Comment 12 Yusuke Suzuki 2018-03-18 18:14:33 PDT
Comment on attachment 336028 [details]
Exhibit A

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

> Source/JavaScriptCore/jit/JIT.h:728
>              setupArguments<OperationType>(args...);

Add `static_assert(sizeof(typename FunctionTraits<OperationType>::ResultType) <= 8, "...comment about Windows calling convension...");`. would be nice.

> Source/JavaScriptCore/jit/JIT.h:736
> +            setupArguments<OperationType>(args...);

Then, we can customize this call for SlowPathReturnType type.
I think the current patch is better than using Sprt_JITOperation_EZ directly, since it can catch all the operations that uses SlowPathReturnType for the result type.
In the previous approach, if we have another operation which has SlowPathReturnType for its result type, we need to add callOperation specialization each time.

Maybe, in the ideal world, we should specialize setupArguments for SlowPathReturnType, but since

1. SlowPathReturnType is the only result type that size is 128bit.
2. Only Windows needs this

Add some special call instead of setupArguments would be enough. At that time, we can meta-program it like,

1. If operation's first argument is ExecState*, we put GPRInfo::callFrameRegister for the first argument additionally to args.
2. After that, we additionally put GPRInfo::argumentGPR0 for the first argument. Then, succeeding arguments calculation does what we want (correct?).
Comment 13 Ross Kirsling 2018-03-19 22:10:28 PDT
(In reply to Yusuke Suzuki from comment #12)
> Maybe, in the ideal world, we should specialize setupArguments for
> SlowPathReturnType, but since
> 
> 1. SlowPathReturnType is the only result type that size is 128bit.
> 2. Only Windows needs this
> 
> Add some special call instead of setupArguments would be enough. At that
> time, we can meta-program it like,
> 
> 1. If operation's first argument is ExecState*, we put
> GPRInfo::callFrameRegister for the first argument additionally to args.
> 2. After that, we additionally put GPRInfo::argumentGPR0 for the first
> argument. Then, succeeding arguments calculation does what we want
> (correct?).

I spent the day wrestling with this, but I don't see a straightforward way to reuse the existing setupArgumentsImpl / marshallArgumentRegister logic to achieve the desired two `move`s, as this logic aims to ensure that the operation's arity is respected.

I'm also concerned about the deleted non-SPRT cases marked with the comment about "interlaced scalar and floating point arguments". For instance, it appears that if the third argument is floating-point, then Win64 expects argumentFPR3 (and not 0) to be used. Although this is separate from the crash that I'm noticing, I assume that this also constitutes lost functionality after r229391.
Comment 14 Ross Kirsling 2018-03-21 18:22:02 PDT
Created attachment 336252 [details]
Test script

(In reply to Ross Kirsling from comment #13)
> I'm also concerned about the deleted non-SPRT cases marked with the comment
> about "interlaced scalar and floating point arguments". For instance, it
> appears that if the third argument is floating-point, then Win64 expects
> argumentFPR3 (and not 0) to be used. Although this is separate from the
> crash that I'm noticing, I assume that this also constitutes lost
> functionality after r229391.

Here is a test script which hits all of the operation types that had Win64-specific codepaths prior to the patch. Having this not crash should hopefully suffice for verification.

Even with my "sloppy fix", the `doubleToString` case is currently failing (complaining of an invalid radix).
Comment 15 Ross Kirsling 2018-03-22 16:45:22 PDT
Created attachment 336331 [details]
Patch
Comment 16 Ross Kirsling 2018-03-22 16:58:52 PDT
Comment on attachment 336331 [details]
Patch

Confirmed that the test script does not crash as of this patch.
Feel free to tear my code to shreds. ;)
Comment 17 Build Bot 2018-03-22 18:05:56 PDT
Comment on attachment 336331 [details]
Patch

Attachment 336331 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7068996

New failing tests:
accessibility/smart-invert-reference.html
Comment 18 Build Bot 2018-03-22 18:05:57 PDT
Created attachment 336342 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 20 Keith Miller 2018-03-26 13:40:18 PDT
Comment on attachment 336331 [details]
Patch

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

r=me with some nits.

> Source/JavaScriptCore/jit/JIT.h:753
> +#else

Nit: please comment what #if you are ending e.g. // OS(WINDOWS) && CPU(X86_64)

> Source/JavaScriptCore/jit/JIT.h:760
> +#endif

Ditto.

> Source/JavaScriptCore/jit/JITOperations.h:263
> +typedef SlowPathReturnType (JIT_OPERATION *Sprt_JITOperation_EZ)(ExecState*, uint32_t);

This should be changed to: Sprt_JITOperation_EUi
Comment 21 Ross Kirsling 2018-03-26 14:09:40 PDT
Created attachment 336542 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2018-03-26 14:41:18 PDT
Comment on attachment 336542 [details]
Patch for landing

Clearing flags on attachment: 336542

Committed r229989: <https://trac.webkit.org/changeset/229989>
Comment 23 WebKit Commit Bot 2018-03-26 14:41:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2018-03-26 14:42:28 PDT
<rdar://problem/38883141>
Comment 25 Per Arne Vollan 2018-03-28 09:14:23 PDT
I wonder if we also need to make sure the following logic is maintained? 

ALWAYS_INLINE void setupArgumentsWithExecState(FPRReg arg1, TrustedImm32 arg2)
{
#if OS(WINDOWS) && CPU(X86_64)
    // On Windows, arguments map to designated registers based on the argument positions, even when there are interlaced scalar and floating point arguments.
    // See http://msdn.microsoft.com/en-us/library/zthk2dkh.aspx
    moveDouble(arg1, FPRInfo::argumentFPR1);
    move(arg2, GPRInfo::argumentGPR2);
#else
    moveDouble(arg1, FPRInfo::argumentFPR0);
    move(arg2, GPRInfo::argumentGPR1);
#endif
    move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
}
Comment 26 Ross Kirsling 2018-03-28 09:26:01 PDT
(In reply to Per Arne Vollan from comment #25)
> I wonder if we also need to make sure the following logic is maintained? 
> 
> ALWAYS_INLINE void setupArgumentsWithExecState(FPRReg arg1, TrustedImm32
> arg2)
> {
> #if OS(WINDOWS) && CPU(X86_64)
>     // On Windows, arguments map to designated registers based on the
> argument positions, even when there are interlaced scalar and floating point
> arguments.
>     // See http://msdn.microsoft.com/en-us/library/zthk2dkh.aspx
>     moveDouble(arg1, FPRInfo::argumentFPR1);
>     move(arg2, GPRInfo::argumentGPR2);
> #else
>     moveDouble(arg1, FPRInfo::argumentFPR0);
>     move(arg2, GPRInfo::argumentGPR1);
> #endif
>     move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> }

I've aimed to cover this by ensuring that FPRs and GPRs are not counted separately on Windows. That particular case is hit by `doubleToString` in my attached test script (and is precisely the case that was failing when I only addressed the SlowPathReturnType case).