Bug 204126 – Support or16(TrustedImm32, AbsoluteAddress) in the MIPS MacroAssembler

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Robin Morisset]

My patch at https://bugs.webkit.org/show_bug.cgi?id=202832 was rolled out, because it was using or32 on a now-16bits field.
I am currently fixing it, which includes implementing or16 in the 32 and 64-bits x86 and ARM MacroAssembler backends.
I cannot fix the MIPS backend as I have no way to test it and don't even know which processors it is supposed to work on; so once I am done and lands the patch again it will probably break the MIPS port.
The solution is simply to implement or16(TrustedImm32, AbsoluteAddress) in MacroAssemblerMIPS.h. There is already a 32-bit version, so it should not be too difficult.

[Caio Lima]

Created attachment 383417 [details]
WIP - Patch

This seems to be the proper implementation. I'm running tests to validate it.

[Caio Lima]

Created attachment 383476 [details]
WIP - Patch

Diff from current https://bugs.webkit.org/show_bug.cgi?id=202832

[Robin Morisset]

Comment on attachment 383476 [details]
WIP - Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:-491
> -            m_assembler.lw(immTempRegister, addrTempRegister, adr & 0xffff);

This patch appears to be altering the implementation of or32, instead of adding or16?

[Caio Lima]

Comment on attachment 383476 [details]
WIP - Patch

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

>> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:-491
>> -            m_assembler.lw(immTempRegister, addrTempRegister, adr & 0xffff);
> 
> This patch appears to be altering the implementation of or32, instead of adding or16?

Actually, this is a diff from your current patch that already landed. I'm uploading a Patch to this soon.

[Caio Lima]

Created attachment 383533 [details]
Patch

[Mark Lam]

Comment on attachment 383533 [details]
Patch

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

> JSTests/stress/regress-57020338.js:6
> +  for (let j = 0; j < 10000; j++) {

I’m not sure this is a valid is an appropriate change to make.  Robin, is 10000 sufficient to repro the problem in x86_64?  If not,, then the timeout issue on slow ports should be handled in a different way.

[Caio Lima]

Created attachment 383552 [details]
Patch

[Caio Lima]

(In reply to Mark Lam from comment #6)
> Comment on attachment 383533 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383533&action=review
> 
> > JSTests/stress/regress-57020338.js:6
> > +  for (let j = 0; j < 10000; j++) {
> 
> I’m not sure this is a valid is an appropriate change to make.  Robin, is
> 10000 sufficient to repro the problem in x86_64?  If not,, then the timeout
> issue on slow ports should be handled in a different way.

To reproduce the issue, I changed `ArithProfile<BitfieldType>::emitUnconditionalSet` to use `or32` instead of `or16`.

I was able to see crashes in x86_64, but not every time. Now, I changed another place and I was able to reproduce segfault 100/100 tries in my x86_64 macOS build. I also tested this on jsc-only and reproduced the crash.

[Caio Lima]

ping

[Mark Lam]

Comment on attachment 383552 [details]
Patch

rs=me

[Caio Lima]

For the record, both issues of stress/ensure-code-block-is-not-precise-allocation.js.dfg-eager (MIPS) and stress/toctou-having-a-bad-time-new-array.js (ARMv7) are not related with this Patch. The former is due the broken OSR to LLInt into MIPS and later is a OOM on ToT (see one report here https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/9979/steps/jscore-test/logs/stdio)

[Caio Lima]

Comment on attachment 383552 [details]
Patch

Thank you very much for the review!

[WebKit Commit Bot]

Comment on attachment 383552 [details]
Patch

Clearing flags on attachment: 383552

Committed r252463: <https://trac.webkit.org/changeset/252463>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57200550>