Bug 128549 – ENABLED(JIT) && !ENABLED(DFG_JIT) breaks the build

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

[Allison Lortie (desrt)]

Building on FreeBSD results in JIT being enabled, but not DFG_JIT.  This is a result of this check:

#if !defined(ENABLE_DFG_JIT) && ENABLE(JIT) && !COMPILER(MSVC)
/* Enable the DFG JIT on X86 and X86_64.  Only tested on Mac and GNU/Linux. */
#if (CPU(X86) || CPU(X86_64)) && (OS(DARWIN) || OS(LINUX))
#define ENABLE_DFG_JIT 1
#endif


As a result, some files in JavascriptScore fail to compile in a fairly simple (presumably as a result of nobody testing this combination).

Source/JavaScriptCore/bytecode/CallLinkStatus.cpp:125:24: error: no member named 'hasExitSite' in 'JSC::CodeBlock'
    if (profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadCache))
        ~~~~~~~~~~~~~  ^
Source/JavaScriptCore/bytecode/CallLinkStatus.cpp:125:49: error: no member named 'FrequentExitSite' in namespace
      'JSC::DFG'
    if (profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadCache))
                                           ~~~~~^
Source/JavaScriptCore/bytecode/CallLinkStatus.cpp:126:27: error: no member named 'hasExitSite' in 'JSC::CodeBlock'
        || profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadCacheWatchpoint))
           ~~~~~~~~~~~~~  ^
Source/JavaScriptCore/bytecode/CallLinkStatus.cpp:126:52:Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:40:27: error: no member named 'hasExitSite' in 'JSC::CodeBlock'
    return profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadCache, jitType))
           ~~~~~~~~~~~~~  ^
 error: no member named 'FrequentExitSite' in namespace
      'JSC::DFG'
        || profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadCacheWatchpoint))
                                              ~~~~~^
Source/JavaScriptCore/bytecode/CallLinkStatus.cpp:127:27: error: no member named 'hasExitSite' in 'JSC::CodeBlock'
        || profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadExecutable)))
           ~~~~~~~~~~~~~  ^
Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:40:52: error: no member named 'FrequentExitSite' in namespace
      'JSC::DFG'
    return profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadCache, jitType))
                                              ~~~~~^
Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:41:27: error: no member named 'hasExitSite' in 'JSC::CodeBlock'
        || profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadCacheWatchpoint, jitType))
           ~~~~~~~~~~~~~  ^
Source/JavaScriptCore/bytecode/CallLinkStatus.cpp:127:52: error: no member named 'FrequentExitSite' in namespace
      'JSC::DFG'
        || profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadExecutable)))
                                              ~~~~~^
Source/JavaScriptCore/bytecode/CallLinkStatus.cpp:142:24: error: no member named 'hasExitSite' in 'JSC::CodeBlock'
    if (profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadFunction)))
        ~~~~~~~~~~~~~  ^
Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:41:52: error: no member named 'FrequentExitSite' in namespace
      'JSC::DFG'
        || profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadCacheWatchpoint, jitType))
                                              ~~~~~^
Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:42:27: error: no member named 'hasExitSite' in 'JSC::CodeBlock'
        || profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadWeakConstantCache, jitType))
           ~~~~~~~~~~~~~  ^
Source/JavaScriptCore/bytecode/CallLinkStatus.cpp:142:49: error: no member named 'FrequentExitSite' in namespace
      'JSC::DFG'
    if (profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadFunction)))
                                           ~~~~~^
Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:42:52: error: no member named 'FrequentExitSite' in namespace
      'JSC::DFG'
        || profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadWeakConstantCache, jitType))
                                              ~~~~~^
Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:43:27: error: no member named 'hasExitSite' in 'JSC::CodeBlock'
        || profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadWeakConstantCacheWatchpoin...
           ~~~~~~~~~~~~~  ^
  CXX      Source/JavaScriptCore/bytecode/libjavascriptcoregtk_3_0_la-LazyOperandValueProfile.lo
Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:43:52: error: no member named 'FrequentExitSite' in namespace
      'JSC::DFG'
        || profiledBlock->hasExitSite(locker, DFG::FrequentExitSite(bytecodeIndex, BadWeakConstantCacheWatchpoin...
                                              ~~~~~^
  CXX      Source/JavaScriptCore/bytecode/libjavascriptcoregtk_3_0_la-MethodOfGettingAValueProfile.lo
8 errors generated.

The first of these errors, at CallLinkStatus.cpp:125:24, is caused by a use of DFG::FrequentExitSite guarded by only ENABLE(JIT) whereas its site of definition is guarded by ENABLE(DFG_JIT):

#ifndef DFGExitProfile_h
#define DFGExitProfile_h

#if ENABLE(DFG_JIT)

...

namespace JSC { namespace DFG {

class FrequentExitSite {

...


A earlier use of DFG::FrequentExitSite in the same file (line 94) is guarded by ENABLE(DFG_JIT) and there are no problems there.

The fix to this bug is probably to change some cases of ENABLE(JIT) to ENABLE(DFG_JIT) but ultimately it seems like it might also be a good idea to enable DFG_JIT on FreeBSD.

[Alberto Garcia]

(In reply to comment #0)
> The fix to this bug is probably to change some cases of ENABLE(JIT)
> to ENABLE(DFG_JIT)

Yes, probably, it seems that this was introduced when the jsCStack was
merged, adding Michael Saboff to Cc.

> but ultimately it seems like it might also be a good idea to enable
> DFG_JIT on FreeBSD.

I agree, can you test if it works or it doesn't build yet? :)

Thanks!

[Allison Lortie (desrt)]

It still doesn't build because of the 'too long argument list' to the linker problem.

I can verify that adding OS(FREEBSD) to the list does get it past the build problem described here, though.

[Allison Lortie (desrt)]

Okay.  I managed to fix a couple of issues to get a complete build.  I can verify that forcing DFG_JIT to be enabled on FreeBSD results in a working browser.  It scores 100/100 on Acid3, at least.  Is there any other 'more scientific' way that I can test that?

[Allison Lortie (desrt)]

Created attachment 223847 [details]
Enable DFG_JIT on FreeBSD

[Alberto Garcia]

(In reply to comment #4)
> Enable DFG_JIT on FreeBSD

It's probably fine to enable this on FreeBSD, but there's still the
original problem of why it no longer builds without it.

[Zan Dobersek]

(In reply to comment #5)
> (In reply to comment #4)
> > Enable DFG_JIT on FreeBSD
> 
> It's probably fine to enable this on FreeBSD, but there's still the
> original problem of why it no longer builds without it.

The reasons are probably the same as those outlined here:
https://bugs.webkit.org/show_bug.cgi?id=126502#c4

[Alberto Garcia]

(In reply to comment #6)
> > It's probably fine to enable this on FreeBSD, but there's still
> > the original problem of why it no longer builds without it.

> The reasons are probably the same as those outlined here:
> https://bugs.webkit.org/show_bug.cgi?id=126502#c4

I was thinking the same, but then we should probably remove the
#ifdefs altogether.

[Allison Lortie (desrt)]

With DFG_JIT on FreeBSD, stress tests pass 8212/8212.  The mozilla tests have 45 failures out of 1127, but apparently this is normal (and indeed, many of the failures have links to already-open bugs).

[Dániel Bátyai]

Created attachment 224057 [details]
Build fix for enabled JIT with disabled DFG_JIT

[Alberto Garcia]

(In reply to comment #9)
> Created an attachment (id=224057) [details]
> Build fix for enabled JIT with disabled DFG_JIT

Thanks for the patch, although I haven't had the chance to test it
yet.

Even if we fix the build we can also enable DFG_JIT on FreeBSD, I just
filed bug 128898 for that.

[Csaba Osztrogonác]

Comment on attachment 224057 [details]
Build fix for enabled JIT with disabled DFG_JIT

CallLinkStatus.cpp, GetByIdStatus.cpp and PutByIdStatus.cpp were already fixed by https://trac.webkit.org/changeset/164164

[Csaba Osztrogonác]

Comment on attachment 224057 [details]
Build fix for enabled JIT with disabled DFG_JIT

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

r-, because it can't be applied now.

> Source/JavaScriptCore/jit/Repatch.cpp:825
> +#if ENABLE(DFG_JIT)
>      MacroAssembler::Jump definitelyNotMarked = DFG::SpeculativeJIT::genericWriteBarrier(jit, owner, scratch1, scratch2);
> -    MacroAssembler::Call call = storeToWriteBarrierBuffer(jit, owner, scratch1, scratch2, allocator);
>      definitelyNotMarked.link(&jit);
> +#endif // ENABLE(DFG_JIT)
> +
> +    MacroAssembler::Call call = storeToWriteBarrierBuffer(jit, owner, scratch1, scratch2, allocator);

Are you sure if this is the proper fix? I don't understand this code, but these lines looks to me as they can't be separated.

[Csaba Osztrogonác]

I tried the proposed fix for Repatch.cpp, it builds but I got some test failures:

stress/tricky-array-bounds-checks.js.always-trigger-copy-phase: Exception: Error: bad result for trick: ,,,false
stress/tricky-array-bounds-checks.js.default: Exception: Error: bad result for trick: ,,,false
stress/tricky-array-bounds-checks.js.no-llint: Exception: Error: bad result for trick: ,,,false
stress/tricky-array-bounds-checks.js.no-cjit-validate-phases: Exception: Error: bad result for trick: ,,,false
stress/tricky-array-bounds-checks.js.default: ERROR: Unexpected exit code: 3
stress/tricky-array-bounds-checks.js.always-trigger-copy-phase: ERROR: Unexpected exit code: 3
stress/tricky-array-bounds-checks.js.no-llint: ERROR: Unexpected exit code: 3
stress/tricky-array-bounds-checks.js.dfg-eager: Exception: Error: bad result for trick: ,,,false
stress/tricky-array-bounds-checks.js.dfg-eager-no-cjit-validate: Exception: Error: bad result for trick: ,,,false
stress/tricky-array-bounds-checks.js.no-cjit-validate-phases: ERROR: Unexpected exit code: 3
stress/tricky-array-bounds-checks.js.dfg-eager: ERROR: Unexpected exit code: 3
stress/tricky-array-bounds-checks.js.dfg-eager-no-cjit-validate: ERROR: Unexpected exit code: 3
8352/8352 (failed 6)                         

** The following JSC stress test failures have been introduced:
	stress/tricky-array-bounds-checks.js.default
	stress/tricky-array-bounds-checks.js.no-llint
	stress/tricky-array-bounds-checks.js.always-trigger-copy-phase
	stress/tricky-array-bounds-checks.js.no-cjit-validate-phases
	stress/tricky-array-bounds-checks.js.dfg-eager
	stress/tricky-array-bounds-checks.js.dfg-eager-no-cjit-validate

Results for JSC stress tests:
    6 failures found.


I'm not sure if they were caused by this change or other bug.

[Csaba Osztrogonác]

I checked the failures, they aren't related to this fix,
they fail on trunk with runtime dissabled DFG too.

[Csaba Osztrogonác]

(In reply to comment #14)
> I checked the failures, they aren't related to this fix,
> they fail on trunk with runtime dissabled DFG too.
New bug report for this failire: https://bugs.webkit.org/show_bug.cgi?id=128910

[Allison Lortie (desrt)]

(In reply to comment #14)
> I checked the failures, they aren't related to this fix,
> they fail on trunk with runtime dissabled DFG too.

You still have a review- flag on this bug.

[Csaba Osztrogonác]

(In reply to comment #16)
> (In reply to comment #14)
> > I checked the failures, they aren't related to this fix,
> > they fail on trunk with runtime dissabled DFG too.
> 
> You still have a review- flag on this bug.

I r- -ed it, because it is obsolete now, see https://bugs.webkit.org/show_bug.cgi?id=128549#c11 for details.

Maybe the fix in Source/JavaScriptCore/jit/Repatch.cpp is good.
It worked for me without new regressions. But I'm not sure if it 
is the proper fix. Comment about it from a JSC expert would be welcome.

[Dániel Bátyai]

Created attachment 225780 [details]
Proposed fix v2

[Mark Lam]

Comment on attachment 225780 [details]
Proposed fix v2

Thanks for looking into this.  However, I think this issue has been solved in a more appropriate refactoring in https://bugs.webkit.org/show_bug.cgi?id=129760.  Please re-test after that lands (or try the patch in that bug) to see if there are still other outstanding issue for the !ENABLE(DFG_JIT) build.

[Dániel Bátyai]

(In reply to comment #19)
> (From update of attachment 225780 [details])
> Thanks for looking into this.  However, I think this issue has been solved in a more appropriate refactoring in https://bugs.webkit.org/show_bug.cgi?id=129760.  Please re-test after that lands (or try the patch in that bug) to see if there are still other outstanding issue for the !ENABLE(DFG_JIT) build.

Thanks for the heads up. I tried the patch and managed to build with disabled DFG JIT without any problems.

[Csaba Osztrogonác]

ENABLE(JIT) && !ENABLE(DFG_JIT) build works fine on ToT.