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

Julien Brianceau
Reported 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).
Attachments
Flush constant pool for sh4 and arm (5.79 KB, patch)
2013-10-21 04:56 PDT, Julien Brianceau 		no flags
DetailsFormatted DiffDiff
Flush constant pool for sh4 and arm (with style fix) (5.79 KB, patch)
2013-10-21 05:06 PDT, Julien Brianceau 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Julien Brianceau
Comment 1 2013-10-21 04:56:13 PDT
Created attachment 214727 [details] Flush constant pool for sh4 and arm
WebKit Commit Bot
Comment 2 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.
Julien Brianceau
Comment 3 2013-10-21 05:06:00 PDT
Created attachment 214729 [details] Flush constant pool for sh4 and arm (with style fix)
Brian Holt
Comment 4 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'
Julien Brianceau
Comment 5 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
Julien Brianceau
Comment 6 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.
WebKit Commit Bot
Comment 7 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>
WebKit Commit Bot
Comment 8 2013-10-22 08:50:13 PDT
All reviewed patches have been landed. Closing bug.
ChangSeok Oh
Comment 9 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?
Filip Pizlo
Comment 10 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.
ChangSeok Oh
Comment 11 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
Note You need to log in before you can comment on or make changes to this bug.