Bug 123092 - REGRESSION(r157690, r157699) Broken architectures using AssemblerBufferWithConstantPool
Summary: REGRESSION(r157690, r157699) Broken architectures using AssemblerBufferWithCo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 108645 123247
  Show dependency treegraph
 
Reported: 2013-10-21 04:51 PDT by Julien Brianceau
Modified: 2013-10-29 10:46 PDT (History)
10 users (show)

See Also:


Attachments
Flush constant pool for sh4 and arm (5.79 KB, patch)
2013-10-21 04:56 PDT, Julien Brianceau
no flags Details | Formatted Diff | Diff
Flush constant pool for sh4 and arm (with style fix) (5.79 KB, patch)
2013-10-21 05:06 PDT, Julien Brianceau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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