Bug 123092 – REGRESSION(r157690, r157699) Broken architectures using AssemblerBufferWithConstantPool

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

[Julien Brianceau]

Architectures using AssemblerBufferWithConstantPool are broken since r157690 (and not fixed in r157699) because constant pools are not flushed before linking.

Impacted architecures are CPU(SH4) and CPU(ARM_TRADITIONAL).

[Julien Brianceau]

Created attachment 214727 [details]
Flush constant pool for sh4 and arm

[WebKit Commit Bot]

Attachment 214727 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/assembler/ARMAssembler.h', u'Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h', u'Source/JavaScriptCore/assembler/LinkBuffer.cpp', u'Source/JavaScriptCore/assembler/SH4Assembler.h']" exit_code: 1
Source/JavaScriptCore/assembler/AssemblerBufferWithConstantPool.h:228:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Brianceau]

Created attachment 214729 [details]
Flush constant pool for sh4 and arm (with style fix)

[Brian Holt]

I applied the patch and built with ARM_TRADITIONAL:

../../Source/JavaScriptCore/assembler/ARMAssembler.cpp: In member function 'WTF::PassRefPtr<WTF::MetaAllocatorHandle> JSC::ARMAssembler::executableCopy(JSC::VM&, void*, JSC::JITCompilationEffort)':
../../Source/JavaScriptCore/assembler/ARMAssembler.cpp:401:54: error: 'JSC::ARMAssembler::ARMBuffer' has no member named 'executableCopy'

[Julien Brianceau]

(In reply to comment #4)
> I applied the patch and built with ARM_TRADITIONAL:
> 
> ../../Source/JavaScriptCore/assembler/ARMAssembler.cpp: In member function 'WTF::PassRefPtr<WTF::MetaAllocatorHandle> JSC::ARMAssembler::executableCopy(JSC::VM&, void*, JSC::JITCompilationEffort)':
> ../../Source/JavaScriptCore/assembler/ARMAssembler.cpp:401:54: error: 'JSC::ARMAssembler::ARMBuffer' has no member named 'executableCopy'

PassRefPtr<ExecutableMemoryHandle> executableCopy(VM&, void* ownerUID, JITCompilationEffort) prototype declaration and its implementation should be removed from ARMAssembler.h and ARMAssembler.cpp because of r157690

[Julien Brianceau]

Mmmm, I didn't see that ARMAssembler::executableCopy does a lot of things compared to what SH4Assembler::executableCopy used to do(see http://trac.webkit.org/changeset/157699#file2).

So this patch fixes crashes with SH4, but I think more actions are needed to fix ARM_TRADITIONAL.

[WebKit Commit Bot]

Comment on attachment 214729 [details]
Flush constant pool for sh4 and arm (with style fix)

Clearing flags on attachment: 214729

Committed r157796: <http://trac.webkit.org/changeset/157796>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[ChangSeok Oh]

(In reply to comment #6)
> Mmmm, I didn't see that ARMAssembler::executableCopy does a lot of things compared to what SH4Assembler::executableCopy used to do(see http://trac.webkit.org/changeset/157699#file2).
> 
> So this patch fixes crashes with SH4, but I think more actions are needed to fix ARM_TRADITIONAL.

Right. I'm facing the exactly same problem you mentioned with old ARM. Somebody cares about this?

[Filip Pizlo]

(In reply to comment #9)
> (In reply to comment #6)
> > Mmmm, I didn't see that ARMAssembler::executableCopy does a lot of things compared to what SH4Assembler::executableCopy used to do(see http://trac.webkit.org/changeset/157699#file2).
> > 
> > So this patch fixes crashes with SH4, but I think more actions are needed to fix ARM_TRADITIONAL.
> 
> Right. I'm facing the exactly same problem you mentioned with old ARM. Somebody cares about this?

Good question. We should decide if we care. If we don't care then we should remove it.

[ChangSeok Oh]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #6)
> > > Mmmm, I didn't see that ARMAssembler::executableCopy does a lot of things compared to what SH4Assembler::executableCopy used to do(see http://trac.webkit.org/changeset/157699#file2).
> > > 
> > > So this patch fixes crashes with SH4, but I think more actions are needed to fix ARM_TRADITIONAL.
> > 
> > Right. I'm facing the exactly same problem you mentioned with old ARM. Somebody cares about this?
> 
> Good question. We should decide if we care. If we don't care then we should remove it.

Well. I think it's not the time to stop caring this. I opened a bug to discuss further.
https://bugs.webkit.org/show_bug.cgi?id=123247