Bug 183263 – Meta-program setupArguments and callOperation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Meta-program setupArguments and callOperation

[Keith Miller]

Created attachment 334853 [details]
WIP

[Keith Miller]

Created attachment 334929 [details]
WIP needs 32-bit and Baseline

[Filip Pizlo]

Comment on attachment 334929 [details]
WIP needs 32-bit and Baseline

looks pretty good so far

[Saam Barati]

Comment on attachment 334929 [details]
WIP needs 32-bit and Baseline

So much red 👌🏽

[JF Bastien]

Comment on attachment 334929 [details]
WIP needs 32-bit and Baseline

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

I like how much red is in the patch.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:937
> +    callOperation(OperationType operation, ResultRegType result, Args... args)

Args&&... args
and below
std::forward<Args>(args)...

> Source/JavaScriptCore/jit/CCallHelpers.h:413
> +        || std::is_convertible<Arg, TrustedImm>::value> // We have this since Speculative JIT has it's own implementation of TrustedImmPtr

"its"

> Source/JavaScriptCore/jit/FPRInfo.h:309
> +// We using this hack for getting the FPRInfo from the FPRReg type in templates because our code is bad and we should feel bad..

We using?

> Source/WTF/wtf/FunctionTraits.h:36
> +struct FunctionTraits<Result(Args...)> {

mlam and I were just checking out std::result_of versus std::invoke_result yesterday. Context turns out to be http://wg21.link/P0604

[Keith Miller]

Created attachment 335218 [details]
Patch

[Keith Miller]

Created attachment 335220 [details]
Patch

[Filip Pizlo]

Comment on attachment 335220 [details]
Patch

rs=me

[Keith Miller]

Created attachment 335225 [details]
Patch

[Keith Miller]

Created attachment 335227 [details]
Patch

[Keith Miller]

Just as a heads up this is definitely going to break the ARM32 JITs.

[Keith Miller]

(In reply to Keith Miller from comment #11)
> Just as a heads up this is definitely going to break the ARM32 JITs.

Also, probably MIPS.

[Saam Barati]

Comment on attachment 335227 [details]
Patch

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

LGTM too

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:2248
> +        // FIXME: This is kinda a hack since we don't use xmm7 as a temp.

Bug #?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4317
> +//        m_jit.probe([=] (Probe::Context& context) {

Remove

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10145
> +            operationSwitchString, string, size_t(data->switchTableIndex), string);

Static cast

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10182
> +    callOperation(operationSwitchString, string, size_t(data->switchTableIndex), string);

Ditto

> Source/JavaScriptCore/jit/FPRInfo.h:309
> +// We using this hack for getting the FPRInfo from the FPRReg type in templates because our code is bad and we should feel bad..

Is this actually true? Doesn’t seem like this does anything

> Source/JavaScriptCore/jit/FPRInfo.h:310
> +constexpr FPRInfo toInfoFromReg(FPRReg) { return FPRInfo(); }

Also, “we using” in above comment

> Source/JavaScriptCore/jit/GPRInfo.h:811
> +// We using this hack for getting the GPRInfo from the GPRReg type in templates because our code is bad and we should feel bad..

Ditto

[Michael Catanzaro]

Thanks for the heads-up!

[Keith Miller]

Created attachment 335236 [details]
Patch

[Keith Miller]

Comment on attachment 335227 [details]
Patch

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

uuse

>> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:2248
>> +        // FIXME: This is kinda a hack since we don't use xmm7 as a temp.
> 
> Bug #?

I'm gonna make this not a FIXME since I don't think there's an alternative...

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4317
>> +//        m_jit.probe([=] (Probe::Context& context) {
> 
> Remove

Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10145
>> +            operationSwitchString, string, size_t(data->switchTableIndex), string);
> 
> Static cast

changed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10182
>> +    callOperation(operationSwitchString, string, size_t(data->switchTableIndex), string);
> 
> Ditto

changed.

>> Source/JavaScriptCore/jit/FPRInfo.h:309
>> +// We using this hack for getting the FPRInfo from the FPRReg type in templates because our code is bad and we should feel bad..
> 
> Is this actually true? Doesn’t seem like this does anything

What do you mean? If you have a function:

template<typename RegType>
void foo(RegType reg)
{
    using RegInfo = decltype(toInfoFromReg(reg));
{

How do you get the RegType's Info class without something like this?

>> Source/JavaScriptCore/jit/FPRInfo.h:310
>> +constexpr FPRInfo toInfoFromReg(FPRReg) { return FPRInfo(); }
> 
> Also, “we using” in above comment

Fixed.

>> Source/JavaScriptCore/jit/GPRInfo.h:811
>> +// We using this hack for getting the GPRInfo from the GPRReg type in templates because our code is bad and we should feel bad..
> 
> Ditto

Fixed.

[EWS Watchlist]

Comment on attachment 335236 [details]
Patch

Attachment 335236 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6847337

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html
js/dom/dfg-custom-getter-throw-inlined.html
js/dom/dfg-inline-resolve.html
fast/workers/worker-cloneport.html
imported/w3c/web-platform-tests/WebCryptoAPI/generateKey/successes_AES-CBC.worker.html
js/dom/dfg-custom-getter-throw.html

[EWS Watchlist]

Created attachment 335247 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Keith Miller]

Windows looks like it failed to build for unrelated reasons (built through JSC). Fixed an issue where I wasn't setting vm.topCallFrame before calling into DOM getters (I guess they don't expect to need to set topCallFrame).

[Keith Miller]

Created attachment 335248 [details]
Patch

[Keith Miller]

Committed r229391: <https://trac.webkit.org/changeset/229391>

[Radar WebKit Bug Importer]

<rdar://problem/38246426>

[Yusuke Suzuki]

Really awesome!

[Ross Kirsling]

The Windows build break from this patch was fixed in https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed compile-time errors and not runtime ones.

Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying to launch Web Inspector as of this revision.

The first issue is here, where the array is supposedly uninitialized:
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/CCallHelpers.h#L262
A compiler warning for this was squelched in r229424/r229432 but I don't think that helps at all(?) because there's still a runtime error either way.

If I add braces to the std::array declaration, the next error is here:
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/interpreter/Register.h#L203
With the following call stack:
> JavaScriptCore.dll!JSC::Register::pointer() Line 203
> JavaScriptCore.dll!JSC::ExecState::callee() Line 108
> JavaScriptCore.dll!JSC::ExecState::vm() Line 129
> JavaScriptCore.dll!operationOptimize(JSC::ExecState * exec, unsigned int bytecodeIndex) Line 1302
> [External Code]

[Keith Miller]

(In reply to Ross Kirsling from comment #24)
> The Windows build break from this patch was fixed in
> https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed
> compile-time errors and not runtime ones.
> 
> Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying
> to launch Web Inspector as of this revision.

AFAICT, the AppleWin bots don't have any new failing tests from this patch.

> 
> The first issue is here, where the array is supposedly uninitialized:
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/
> CCallHelpers.h#L262
> A compiler warning for this was squelched in r229424/r229432 but I don't
> think that helps at all(?) because there's still a runtime error either way.

Oh? I thought all of the following were equivalent in C++ (at least since C++11):

T v;
T v();
T v { };

If so, all that would happen is that the std::array would contain indeterminate values until they were initialized in the loop below. Maybe I'm wrong though?

> 
> If I add braces to the std::array declaration, the next error is here:
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/
> interpreter/Register.h#L203
> With the following call stack:
> > JavaScriptCore.dll!JSC::Register::pointer() Line 203
> > JavaScriptCore.dll!JSC::ExecState::callee() Line 108
> > JavaScriptCore.dll!JSC::ExecState::vm() Line 129
> > JavaScriptCore.dll!operationOptimize(JSC::ExecState * exec, unsigned int bytecodeIndex) Line 1302
> > [External Code]

Seems weird that there would be a crash there since that's just a cast. Is this from a release build?

[Ross Kirsling]

(In reply to Keith Miller from comment #25)
> (In reply to Ross Kirsling from comment #24)
> > The Windows build break from this patch was fixed in
> > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed
> > compile-time errors and not runtime ones.
> > 
> > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying
> > to launch Web Inspector as of this revision.
> 
> AFAICT, the AppleWin bots don't have any new failing tests from this patch.

Damn, I was hoping our destinies would align. :P

> > 
> > The first issue is here, where the array is supposedly uninitialized:
> > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/
> > CCallHelpers.h#L262
> > A compiler warning for this was squelched in r229424/r229432 but I don't
> > think that helps at all(?) because there's still a runtime error either way.
> 
> Oh? I thought all of the following were equivalent in C++ (at least since
> C++11):
> 
> T v;
> T v();
> T v { };
> 
> If so, all that would happen is that the std::array would contain
> indeterminate values until they were initialized in the loop below. Maybe
> I'm wrong though?

While `T v();` would be taken as a function declaration (the so-called "most vexing parse"), my understanding too is that adding braces shouldn't help, so I was surprised when they did. Perhaps this is disguising a different issue -- is it problematic that TargetSize is 0?

> > 
> > If I add braces to the std::array declaration, the next error is here:
> > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/
> > interpreter/Register.h#L203
> > With the following call stack:
> > > JavaScriptCore.dll!JSC::Register::pointer() Line 203
> > > JavaScriptCore.dll!JSC::ExecState::callee() Line 108
> > > JavaScriptCore.dll!JSC::ExecState::vm() Line 129
> > > JavaScriptCore.dll!operationOptimize(JSC::ExecState * exec, unsigned int bytecodeIndex) Line 1302
> > > [External Code]
> 
> Seems weird that there would be a crash there since that's just a cast. Is
> this from a release build?

Whoops, I stupidly forgot to say what the error is -- that `this` is null.
It's from a Debug build, as I was getting awfully confused trying to debug in Release mode, hehe.

[Christopher Reid]

(In reply to Ross Kirsling from comment #26)
> (In reply to Keith Miller from comment #25)
> > (In reply to Ross Kirsling from comment #24)
> > > The Windows build break from this patch was fixed in
> > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed
> > > compile-time errors and not runtime ones.
> > > 
> > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying
> > > to launch Web Inspector as of this revision.
> > 
> > AFAICT, the AppleWin bots don't have any new failing tests from this patch.
> 
> Damn, I was hoping our destinies would align. :P

It looks like AppleWin is at least affected by the same uninitialized Run-time check failure. I downloaded the latest AppleWin artifacts (https://s3-us-west-2.amazonaws.com/archives.webkit.org/win-i386-debug/229557.zip) and tested a simple looped function call.

function foo(a){
	a += 1;
};
for (i=0; i<1000; i++)
	foo(1);
print('done');

'done' is not getting printed unless I lower the maximum iterations to around 10. I attached a debugger to confirm that it was hitting that same uninitialized Run-time check failure.

I'm not sure if I can easily check to see if AppleWin hitting that null crash too without getting a build going.

[Keith Miller]

(In reply to Christopher Reid from comment #27)
> (In reply to Ross Kirsling from comment #26)
> > (In reply to Keith Miller from comment #25)
> > > (In reply to Ross Kirsling from comment #24)
> > > > The Windows build break from this patch was fixed in
> > > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed
> > > > compile-time errors and not runtime ones.
> > > > 
> > > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying
> > > > to launch Web Inspector as of this revision.
> > > 
> > > AFAICT, the AppleWin bots don't have any new failing tests from this patch.
> > 
> > Damn, I was hoping our destinies would align. :P
> 
> It looks like AppleWin is at least affected by the same uninitialized
> Run-time check failure. I downloaded the latest AppleWin artifacts
> (https://s3-us-west-2.amazonaws.com/archives.webkit.org/win-i386-debug/
> 229557.zip) and tested a simple looped function call.
> 
> function foo(a){
> 	a += 1;
> };
> for (i=0; i<1000; i++)
> 	foo(1);
> print('done');
> 
> 'done' is not getting printed unless I lower the maximum iterations to
> around 10. I attached a debugger to confirm that it was hitting that same
> uninitialized Run-time check failure.
> 
> I'm not sure if I can easily check to see if AppleWin hitting that null
> crash too without getting a build going.

Interesting... It's weird that the bots are not showing it? https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29?numbuilds=200 doesn't show any new regressions after the patch landed.

(In reply to Ross Kirsling from comment #26)
> (In reply to Keith Miller from comment #25)
> > (In reply to Ross Kirsling from comment #24)
> > > The Windows build break from this patch was fixed in
> > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed
> > > compile-time errors and not runtime ones.
> > > 
> > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying
> > > to launch Web Inspector as of this revision.
> > 
> > AFAICT, the AppleWin bots don't have any new failing tests from this patch.
> 
> Damn, I was hoping our destinies would align. :P
> 
> > > 
> > > The first issue is here, where the array is supposedly uninitialized:
> > > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/
> > > CCallHelpers.h#L262
> > > A compiler warning for this was squelched in r229424/r229432 but I don't
> > > think that helps at all(?) because there's still a runtime error either way.
> > 
> > Oh? I thought all of the following were equivalent in C++ (at least since
> > C++11):
> > 
> > T v;
> > T v();
> > T v { };
> > 
> > If so, all that would happen is that the std::array would contain
> > indeterminate values until they were initialized in the loop below. Maybe
> > I'm wrong though?
> 
> While `T v();` would be taken as a function declaration (the so-called "most
> vexing parse"), my understanding too is that adding braces shouldn't help,
> so I was surprised when they did. Perhaps this is disguising a different
> issue -- is it problematic that TargetSize is 0?

Right, C++ is such a beautiful language... 

I don't think it should be a problem if TargetSize is 0. We only ever loop on the iterator or indexed on the size of the array. If the size of the std::array is 0 then begin() == end() so the loops should never run. Maybe there's some weird MSVC bug? I don't have a windows machine right now but I might be able to find one and take a look. If you could run:

function foo(a){
	a += 1;
};
for (i=0; i<1000; i++)
	foo(1);
print('done');


again in jsc. With `--dumpDisassembly=1 --useDFGJIT=0` and send me the Baseline disassembly that might help me fix the issue too.

[Ross Kirsling]

Created attachment 335721 [details]
Baseline disassembly (WinCairo 64-bit Debug)

Here's the disassembly with the options you requested. I noticed that (on my machine at least) the crash occurs when the loop has 485+ iterations, but not with less than that.

[Christopher Reid]

(In reply to Keith Miller from comment #28)
> (In reply to Christopher Reid from comment #27)
> > (In reply to Ross Kirsling from comment #26)
> > > (In reply to Keith Miller from comment #25)
> > > > (In reply to Ross Kirsling from comment #24)
> > > > > The Windows build break from this patch was fixed in
> > > > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed
> > > > > compile-time errors and not runtime ones.
> > > > > 
> > > > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying
> > > > > to launch Web Inspector as of this revision.
> > > > 
> > > > AFAICT, the AppleWin bots don't have any new failing tests from this patch.
> > > 
> > > Damn, I was hoping our destinies would align. :P
> > 
> > It looks like AppleWin is at least affected by the same uninitialized
> > Run-time check failure. I downloaded the latest AppleWin artifacts
> > (https://s3-us-west-2.amazonaws.com/archives.webkit.org/win-i386-debug/
> > 229557.zip) and tested a simple looped function call.
> > 
> > function foo(a){
> > 	a += 1;
> > };
> > for (i=0; i<1000; i++)
> > 	foo(1);
> > print('done');
> > 
> > 'done' is not getting printed unless I lower the maximum iterations to
> > around 10. I attached a debugger to confirm that it was hitting that same
> > uninitialized Run-time check failure.
> > 
> > I'm not sure if I can easily check to see if AppleWin hitting that null
> > crash too without getting a build going.
> 
> Interesting... It's weird that the bots are not showing it?
> https://build.webkit.org/builders/
> Apple%20Win%207%20Release%20%28Tests%29?numbuilds=200 doesn't show any new
> regressions after the patch landed.

Ah, it looks like the RTC failure is only happening on debug builds and AppleWin tests are running on release. The crash looks Wincario specific as it crashes in Wincairo release but not on AppleWin release. I'm not sure what could be causing that, but it's starting to look like a separate issue.

I took a look at the MSVC disassembly https://godbolt.org/g/FMCe9b. For whatever reason MSVC is initializing the std::array to null causing _RTC_UninitUse to get hit. With braces, the _RTC_UninitUse call doesn't get compiled in. Adding braces is looking like the best fix here.

[Ross Kirsling]

(In reply to Christopher Reid from comment #30)
> (In reply to Keith Miller from comment #28)
> > (In reply to Christopher Reid from comment #27)
> > > (In reply to Ross Kirsling from comment #26)
> > > > (In reply to Keith Miller from comment #25)
> > > > > (In reply to Ross Kirsling from comment #24)
> > > > > > The Windows build break from this patch was fixed in
> > > > > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed
> > > > > > compile-time errors and not runtime ones.
> > > > > > 
> > > > > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying
> > > > > > to launch Web Inspector as of this revision.
> > > > > 
> > > > > AFAICT, the AppleWin bots don't have any new failing tests from this patch.
> > > > 
> > > > Damn, I was hoping our destinies would align. :P
> > > 
> > > It looks like AppleWin is at least affected by the same uninitialized
> > > Run-time check failure. I downloaded the latest AppleWin artifacts
> > > (https://s3-us-west-2.amazonaws.com/archives.webkit.org/win-i386-debug/
> > > 229557.zip) and tested a simple looped function call.
> > > 
> > > function foo(a){
> > > 	a += 1;
> > > };
> > > for (i=0; i<1000; i++)
> > > 	foo(1);
> > > print('done');
> > > 
> > > 'done' is not getting printed unless I lower the maximum iterations to
> > > around 10. I attached a debugger to confirm that it was hitting that same
> > > uninitialized Run-time check failure.
> > > 
> > > I'm not sure if I can easily check to see if AppleWin hitting that null
> > > crash too without getting a build going.
> > 
> > Interesting... It's weird that the bots are not showing it?
> > https://build.webkit.org/builders/
> > Apple%20Win%207%20Release%20%28Tests%29?numbuilds=200 doesn't show any new
> > regressions after the patch landed.
> 
> Ah, it looks like the RTC failure is only happening on debug builds and
> AppleWin tests are running on release. The crash looks Wincario specific as
> it crashes in Wincairo release but not on AppleWin release. I'm not sure
> what could be causing that, but it's starting to look like a separate issue.
> 
> I took a look at the MSVC disassembly https://godbolt.org/g/FMCe9b. For
> whatever reason MSVC is initializing the std::array to null causing
> _RTC_UninitUse to get hit. With braces, the _RTC_UninitUse call doesn't get
> compiled in. Adding braces is looking like the best fix here.

Seems like perhaps that's the only issue with the example script? But on Web Inspector launch, we still end up with no VM (?) here:
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/JITOperations.cpp#L1302

[Ross Kirsling]

(In reply to Ross Kirsling from comment #31)
> ... on Web Inspector launch, we still end up with no VM (?) here:
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/
> JITOperations.cpp#L1302

I'm still looking into this, but it appears that `callOperation(operationOptimize, m_bytecodeOffset);` @ JIT::emitEnterOptimizationCheck / JIT::emitSlow_op_loop_hint used to take us into:
JIT::appendCallWithExceptionCheckAndSlowPathReturnType
JIT::appendCallWithSlowPathReturnType
MacroAssemblerX86_64::callWithSlowPathReturnType

These functions are all #if'ed for 64-bit Windows and were not removed in the patch, yet they no longer appear to be used anywhere in JSC -- after this patch, we enter the non-SPRT call chain instead. Perhaps this is the cause of the issue?

[Mark Lam]

(In reply to Ross Kirsling from comment #32)
> (In reply to Ross Kirsling from comment #31)
> > ... on Web Inspector launch, we still end up with no VM (?) here:
> > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/
> > JITOperations.cpp#L1302
> 
> I'm still looking into this, but it appears that
> `callOperation(operationOptimize, m_bytecodeOffset);` @
> JIT::emitEnterOptimizationCheck / JIT::emitSlow_op_loop_hint used to take us
> into:
> JIT::appendCallWithExceptionCheckAndSlowPathReturnType
> JIT::appendCallWithSlowPathReturnType
> MacroAssemblerX86_64::callWithSlowPathReturnType
> 
> These functions are all #if'ed for 64-bit Windows and were not removed in
> the patch, yet they no longer appear to be used anywhere in JSC -- after
> this patch, we enter the non-SPRT call chain instead. Perhaps this is the
> cause of the issue?

Yes, I think this is a likely cause of crashes on Windows.  I just happened to discover this independently while analyzing the code.  I filed https://bugs.webkit.org/show_bug.cgi?id=183655 to track this.

[Ross Kirsling]

(In reply to Mark Lam from comment #33)
> Yes, I think this is a likely cause of crashes on Windows.  I just happened
> to discover this independently while analyzing the code.  I filed
> https://bugs.webkit.org/show_bug.cgi?id=183655 to track this.

Great! I've put up a patch for the braces issue in https://bugs.webkit.org/show_bug.cgi?id=183673.