Bug 183655

Created attachment 335998 [details] Patch

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

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

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

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

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

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

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

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

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

Created attachment 336331 [details] Patch

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

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

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

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

Created attachment 336542 [details] Patch for landing

Comment on attachment 336542 [details] Patch for landing Clearing flags on attachment: 336542 Committed r229989: <https://trac.webkit.org/changeset/229989>

All reviewed patches have been landed. Closing bug.

<rdar://problem/38883141>

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

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