[Qt][ARM][MIPS]REGRESSION(r133953): It broke the build

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

It broke the Qt ARMv7 (thumb2) build with the following error:
/mnt/raptor2/slaves/qt5-linux-armv7-release/build/WebKitBuild/Release/lib/libJavaScriptCore.so: undefined reference to `JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::JumpList JSC::JIT::emitGenericContiguousPutByVal<(unsigned char)20>(JSC::Instruction*, JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::PatchableJump&)'
/mnt/raptor2/slaves/qt5-linux-armv7-release/build/WebKitBuild/Release/lib/libJavaScriptCore.so: undefined reference to `JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::JumpList JSC::JIT::emitGenericContiguousPutByVal<(unsigned char)22>(JSC::Instruction*, JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::PatchableJump&)'
collect2: error: ld returned 1 exit status
make[3]: *** [../../bin/jsc] Error 1

[Peter Gal]

this is related to the MIPS one: https://bugs.webkit.org/show_bug.cgi?id=101704
and the real culprit is this I think: http://trac.webkit.org/changeset/133953

[Peter Gal]

Created attachment 173294 [details]
proposed fix.

[Simon Hausmann]

Comment on attachment 173294 [details]
proposed fix.

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

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:355
> -template<IndexingType indexingShape>
> -JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType)
> +JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType, IndexingType indexingShape)

I think the goal if it being a template is to generate a faster implementation, i.e. indexingShape is a compile time constant instead of a variable at run-time that's on the stack.

So I don't think it should become a regular method.

[Peter Gal]

(In reply to comment #3)
> (From update of attachment 173294 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173294&action=review
> 
> > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:355
> > -template<IndexingType indexingShape>
> > -JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType)
> > +JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType, IndexingType indexingShape)
> 
> I think the goal if it being a template is to generate a faster implementation, i.e. indexingShape is a compile time constant instead of a variable at run-time that's on the stack.
> 
> So I don't think it should become a regular method.

I know the reason, but: 
"""Because templates are compiled when required, this forces a restriction for multi-file projects: the implementation (definition) of a template class or function must be in the same file as its declaration. That means that we cannot separate the interface in a separate header file, and that we must include both interface and implementation in any file that uses the templates."""

and also the GetByVal counterpart is implemented almost in the same way (and not with templates).

[Simon Hausmann]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 173294 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=173294&action=review
> > 
> > > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:355
> > > -template<IndexingType indexingShape>
> > > -JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType)
> > > +JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType, IndexingType indexingShape)
> > 
> > I think the goal if it being a template is to generate a faster implementation, i.e. indexingShape is a compile time constant instead of a variable at run-time that's on the stack.
> > 
> > So I don't think it should become a regular method.
> 
> I know the reason, but: 
> """Because templates are compiled when required, this forces a restriction for multi-file projects: the implementation (definition) of a template class or function must be in the same file as its declaration. That means that we cannot separate the interface in a separate header file, and that we must include both interface and implementation in any file that uses the templates."""
> 
> and also the GetByVal counterpart is implemented almost in the same way (and not with templates).

Hmm, isn't the template function called from JITPropertyAccess.cpp and also defined there?

[Peter Gal]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 173294 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=173294&action=review
> > > 
> > > > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:355
> > > > -template<IndexingType indexingShape>
> > > > -JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType)
> > > > +JIT::JumpList JIT::emitGenericContiguousPutByVal(Instruction* currentInstruction, PatchableJump& badType, IndexingType indexingShape)
> > > 
> > > I think the goal if it being a template is to generate a faster implementation, i.e. indexingShape is a compile time constant instead of a variable at run-time that's on the stack.
> > > 
> > > So I don't think it should become a regular method.
> > 
> > I know the reason, but: 
> > """Because templates are compiled when required, this forces a restriction for multi-file projects: the implementation (definition) of a template class or function must be in the same file as its declaration. That means that we cannot separate the interface in a separate header file, and that we must include both interface and implementation in any file that uses the templates."""
> > 
> > and also the GetByVal counterpart is implemented almost in the same way (and not with templates).
> 
> Hmm, isn't the template function called from JITPropertyAccess.cpp and also defined there?


The implementation of emitGenericContiguousPutByVal is in the JITPropertyAccess.cpp/JITPropertyAccess64_32.cpp but it is used in the JIT.h. Maybe this leads to a problem when it is included from somewhere else.

[Peter Gal]

hmmm... maybe if I move the implementation of the emit{Int32,Double,..}PutByVal to the corresponding cpp it would be ok with the templates, but then it should be in both cpps.

[Peter Gal]

(In reply to comment #7)
> hmmm... maybe if I move the implementation of the emit{Int32,Double,..}PutByVal to the corresponding cpp it would be ok with the templates, but then it should be in both cpps.

I tried moving the implementation of these methods to the general parot of the JITPropertyAccess.cpp file, but The error message remained. After thinking it through the same situation occured.

So I think there is only 2 possible solution:
 1) use the my first patch which removes the template or
 2) move the emit{Int32,..}PutByVal implementation to both JITPropertyAccess files and there will be code duplication.

[Balazs Kilvady]

(In reply to comment #8)
> (In reply to comment #7)
> > hmmm... maybe if I move the implementation of the emit{Int32,Double,..}PutByVal to the corresponding cpp it would be ok with the templates, but then it should be in both cpps.
> 
> I tried moving the implementation of these methods to the general parot of the JITPropertyAccess.cpp file, but The error message remained. After thinking it through the same situation occured.
> 
> So I think there is only 2 possible solution:
>  1) use the my first patch which removes the template or
>  2) move the emit{Int32,..}PutByVal implementation to both JITPropertyAccess files and there will be code duplication.

The problem is that on mips VALUE_PROFILER not enabled currently and by:
inline JITArrayMode JIT::chooseArrayMode(ArrayProfile* profile)
{
#if ENABLE(VALUE_PROFILER)
    profile->computeUpdatedPrediction(m_codeBlock);
    ArrayModes arrayModes = profile->observedArrayModes();
    if (arrayProfileSaw(arrayModes, DoubleShape))
        return JITDouble;
    if (arrayProfileSaw(arrayModes, Int32Shape))
        return JITInt32;
    if (arrayProfileSaw(arrayModes, ArrayStorageShape))
        return JITArrayStorage;
    return JITContiguous;
#else
    UNUSED_PARAM(profile);
    return JITContiguous;
#endif
}

And in jit/JITPropertyAccess32_64.cpp the body of the template function emitGenericContiguousPutByVal<> not instantiated/compiled in for Int32 and Double types. but they are declared in the jit.h header so JITPropertyAccess.cpp can reference to them. I don't know where to put the implementation of template function. Maybe in the header? Or just don't use shorthands like emitInt32PutByVal but template calls as emitGenericContiguousPutByVal<Int32Shape>?

[Balazs Kilvady]

Created attachment 173323 [details]
Keep template fix

Template implementation moved from JITPropertyAccess32_64.cpp to JITPropertyAccess.cpp.

[Balazs Kilvady]

(In reply to comment #8)
> (In reply to comment #7)
> > hmmm... maybe if I move the implementation of the emit{Int32,Double,..}PutByVal to the corresponding cpp it would be ok with the templates, but then it should be in both cpps.
> 
> I tried moving the implementation of these methods to the general parot of the JITPropertyAccess.cpp file, but The error message remained. After thinking it through the same situation occured.
> 
> So I think there is only 2 possible solution:
>  1) use the my first patch which removes the template or
>  2) move the emit{Int32,..}PutByVal implementation to both JITPropertyAccess files and there will be code duplication.

In JITPropertyAccess.cpp there is a emitGenericContiguousPutByVal implementation for JSVALUE64 case. When I move the 32 bit implementation of the template function from JITPropertyAccess32_64.cpp to JITPropertyAccess.cpp then the Int32 and Double typed version of the template won't be optimized out and tests are successful with the compiled jsc.

[Csaba Osztrogonác]

ping?

[Csaba Osztrogonác]

ping again?

[Csaba Osztrogonác]

*** Bug 101704 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

ping?

[Peter Gal]

(In reply to comment #10)
> Created an attachment (id=173323) [details]
> Keep template fix
> 
> Template implementation moved from JITPropertyAccess32_64.cpp to JITPropertyAccess.cpp.

I don't really like this, because you move a JSVALUE32_64 specific code to the general JSPropertyAccess.cpp. A developer could miss it if he/she only checks the 32_64 file. (I know it's not a big reason, but then why does the 32_64.cpp file exists...)

If this is not a solid reason, then ok. go with it.

[Csaba Osztrogonác]

The JSVALUE64 implementation of emitGenericContiguousPutByVal is in JITPropertyAccess.cpp now. It isn't so nice, but I don't have better idea.

[Csaba Osztrogonác]

Comment on attachment 173323 [details]
Keep template fix

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

I think it isn't the best way to fix this bug ... but now I don't have better idea.

And it is a pending bug fix a valid _build_fail_ after 5 days. I don't think if
we should wait more 5 days to fix the build, so r=me with the typo fix I mentioned.

> Source/JavaScriptCore/ChangeLog:8
> +        Templete function body moved to fix VALUE_PROFILER disabled case.

s/Templete/Template

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/134599 with the changelog fix and removed ugly whitespace lines.

[WebKit Review Bot]

Re-opened since this is blocked by bug 102225

[Csaba Osztrogonác]

Rolled out by http://trac.webkit.org/changeset/134602,
because it broke the 32 bit EFL build:

Linking CXX executable ../../../bin/jsc
../../../lib/libjavascriptcore_efl.so.0.1.0: undefined reference to `JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList JSC::JIT::emitGenericContiguousPutByVal<(unsigned char)26>(JSC::Instruction*, JSC::AbstractMacroAssembler<JSC::X86Assembler>::PatchableJump&)'
../../../lib/libjavascriptcore_efl.so.0.1.0: undefined reference to `JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList JSC::JIT::emitGenericContiguousPutByVal<(unsigned char)20>(JSC::Instruction*, JSC::AbstractMacroAssembler<JSC::X86Assembler>::PatchableJump&)'
collect2: ld returned 1 exit status
make[2]: *** [bin/jsc] Error 1

[Csaba Osztrogonác]

Any idea?

[Peter Gal]

(In reply to comment #22)
> Any idea?

use my patch? :)

[Balazs Kilvady]

(In reply to comment #23)
> (In reply to comment #22)
> > Any idea?
> 
> use my patch? :)

(In reply to comment #22)
> Any idea?

Could you add any of the patches to commit queue or should I resend/rebase mine? Other patch is ok for me, too just let's go on somehow.

[Csaba Osztrogonác]

We tried your patch landing ... but we had to revert, because it broke the 32 bit EFL build ... :-/ That's why I asked: Any idea?

[Csaba Osztrogonác]

Comment on attachment 173294 [details]
proposed fix.

Let's try this one. :) Working build is more important than performance.

[Csaba Osztrogonác]

Comment on attachment 173294 [details]
proposed fix.

Clearing flags on attachment: 173294

Committed r134612: <http://trac.webkit.org/changeset/134612>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Balazs Kilvady]

(In reply to comment #22)
> Any idea?

It's a very hacky solution but if with keeping templates much performance gain can be achieved then:
By the error messages the problem comes up when the body of the 3 inline functions from the JIT.h header should be linked. We could force the compiler to compile in the template codes instantiated with all the 3 type parameters with a simple function in JITPropertyAccess32_64.cpp:

void forceTemplateCompilation()
{
    emitInt32PutByVal(currentInstruction, badType); // emitGenericContiguousPutByVal<(unsigned char)20>
    emitDoublePutByVal(currentInstruction, badType);
    emitContiguousPutByVal(currentInstruction, badType);
}

This function shouldn't be called ever but as an exportable one the compiler cannot optimize out the 3 version of the template whichever ENABLE_/USE_ macro combination are used. This function would be "Objective-C like private: not mentioned in the header).

As I tested the compilation the problems come from that that the above listed 3 functions declared in JIT.h so for <(unsigned char)20>, <(unsigned char)22>, <(unsigned char)26> the template codes of emitGenericContiguousPutByVal<> should be in the object file compiled from the cpp file which contains the template body. But with some ENABLE_ macro configurations the compiler sees that some of the 3 functions not used in JITPropertyAccess32_64.cpp so not compiled in.

I can't test it in EFL build env.