Bug 183655 – JIT callOperation() needs to support operations that return SlowPathReturnType differently on Windows.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Ross Kirsling]

Created attachment 335998 [details]
Patch

[Ross Kirsling]

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

[Ross Kirsling]

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.

[Yusuke Suzuki]

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)`.

[Yusuke Suzuki]

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);
}

[Ross Kirsling]

(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...?

[Keith Miller]

(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?

[Ross Kirsling]

(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.

[Ross Kirsling]

(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.

[Mark Lam]

(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.

[Ross Kirsling]

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.

[Yusuke Suzuki]

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?).

[Ross Kirsling]

(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.

[Ross Kirsling]

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).

[Ross Kirsling]

Created attachment 336331 [details]
Patch

[Ross Kirsling]

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. ;)

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ross Kirsling]

Test failure is unrelated flakiness:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=accessibility%2Fsmart-invert-reference.html

[Keith Miller]

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

[Ross Kirsling]

Created attachment 336542 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 336542 [details]
Patch for landing

Clearing flags on attachment: 336542

Committed r229989: <https://trac.webkit.org/changeset/229989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38883141>

[Per Arne Vollan]

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);
}

[Ross Kirsling]

(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).