Bug 123092

Summary: REGRESSION(r157690, r157699) Broken architectures using AssemblerBufferWithConstantPool
Product: WebKit Reporter: Julien Brianceau <jbriance>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: brian.holt, changseok, commit-queue, fpizlo, msaboff, oliver, ossy, rgabor, yannick.poirier, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108645, 123247    
Attachments:
Description Flags
Flush constant pool for sh4 and arm
none
Flush constant pool for sh4 and arm (with style fix) none

Description Julien Brianceau 2013-10-21 04:51:11 PDT
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).
Comment 1 Julien Brianceau 2013-10-21 04:56:13 PDT
Created attachment 214727 [details]
Flush constant pool for sh4 and arm
Comment 2 WebKit Commit Bot 2013-10-21 04:58:31 PDT
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.
Comment 3 Julien Brianceau 2013-10-21 05:06:00 PDT
Created attachment 214729 [details]
Flush constant pool for sh4 and arm (with style fix)
Comment 4 Brian Holt 2013-10-21 06:29:09 PDT
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'
Comment 5 Julien Brianceau 2013-10-21 06:51:04 PDT
(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
Comment 6 Julien Brianceau 2013-10-21 07:23:33 PDT
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.
Comment 7 WebKit Commit Bot 2013-10-22 08:50:11 PDT
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>
Comment 8 WebKit Commit Bot 2013-10-22 08:50:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 ChangSeok Oh 2013-10-23 21:46:43 PDT
(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?
Comment 10 Filip Pizlo 2013-10-23 22:26:58 PDT
(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.
Comment 11 ChangSeok Oh 2013-10-23 22:45:28 PDT
(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