Bug 95749 – [Qt] Enable the LLInt on Linux

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

Once we will need to enable LLInt.
( GTK did it previously - https://bugs.webkit.org/show_bug.cgi?id=88315 )

[Csaba Osztrogonác]

to help fixing this bug, there are some useful commit:
- LLInt introduced in https://trac.webkit.org/changeset/108444
- enabled on GTK - https://trac.webkit.org/changeset/119593
- LLInt C loop backand landed in https://trac.webkit.org/changeset/127374

[Csaba Osztrogonác]

I managed to build JSC with LLInt enabled manually. 
But inegrating it to the build system needs more work.

As far as I see we have to do the following things:
- generate LLIntDesiredOffsets.h: ruby offlineasm/generate_offset_extractor.rb llint/LowLevelInterpreter.asm generated/LLIntDesiredOffsets.h
- build LLIntOffsetsExtractor binary (It depends on LLIntOffsetsExtractor.cpp,h and the new generated LLIntDesiredOffsets.h)
- generate LLIntAssembly.h: ruby offlineasm/asm.rb llint/LowLevelInterpreter.asm LLIntOffsetsExtractor LLIntAssembly.h (It depends on the binary LLIntOffsetsExtractor)

Am I correct?

[Filip Pizlo]

(In reply to comment #2)
> I managed to build JSC with LLInt enabled manually. 
> But inegrating it to the build system needs more work.
> 
> As far as I see we have to do the following things:
> - generate LLIntDesiredOffsets.h: ruby offlineasm/generate_offset_extractor.rb llint/LowLevelInterpreter.asm generated/LLIntDesiredOffsets.h
> - build LLIntOffsetsExtractor binary (It depends on LLIntOffsetsExtractor.cpp,h and the new generated LLIntDesiredOffsets.h)
> - generate LLIntAssembly.h: ruby offlineasm/asm.rb llint/LowLevelInterpreter.asm LLIntOffsetsExtractor LLIntAssembly.h (It depends on the binary LLIntOffsetsExtractor)
> 
> Am I correct?

Yup, that's right!

[Andras Becsi]

Note that the old interpreter will be removed on Sept 24th:
http://lists.webkit.org/pipermail/webkit-dev/2012-September/022157.html

Ossy, is someone working on this on your side?

[Csaba Osztrogonác]

(In reply to comment #4)
> Note that the old interpreter will be removed on Sept 24th:
> http://lists.webkit.org/pipermail/webkit-dev/2012-September/022157.html
> 
> Ossy, is someone working on this on your side?

Yes, I read this mail, so the timing is the best. :) I started working on it 
and I think Gábor Ballabás will join us with the ARM related fixes soon.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Created attachment 162778 [details]
Patch

Preliminary patch for enabling LLInt (not C loop now). Now I added the generated LLIntAssembly.h for x86_64. Of course, I'll make the build system to generate it automatically soon.

[Csaba Osztrogonác]

Created attachment 162783 [details]
LLInt C loop

I'm fighting with LLInt C loop too. I generated the 
LLIntAssembly.h manually and then tried to build JSC.

I had to fix the following opcodes, because GCC suggested parenthesis:
- OFFLINE_ASM_OPCODE_LABEL(op_pre_inc)
- OFFLINE_ASM_OPCODE_LABEL(op_pre_dec)
- OFFLINE_ASM_OPCODE_LABEL(op_post_inc)
- OFFLINE_ASM_OPCODE_LABEL(op_post_dec)
- OFFLINE_ASM_OPCODE_LABEL(op_add)
- OFFLINE_ASM_OPCODE_LABEL(op_sub)

SIGN_BIT32(b-a) --> SIGN_BIT32((b-a))
SIGN_BIT32(b+a) --> SIGN_BIT32((b+a))

I assume it can be fixed in the original code somewhere.

But the bigger problem is the following error messege:
/home/oszi/WebKit/Source/JavaScriptCore/llint/LLIntAssembly.h:125: error: ‘assert’ was not declared in this scope

Have you got any ide what can be the problem and how can we fix it?

[Mark Lam]

(In reply to comment #8)
> SIGN_BIT32(b-a) --> SIGN_BIT32((b-a))
> SIGN_BIT32(b+a) --> SIGN_BIT32((b+a))

> But the bigger problem is the following error messege:
> /home/oszi/WebKit/Source/JavaScriptCore/llint/LLIntAssembly.h:125: error: ‘assert’ was not declared in this scope

These are both bugs.  The fix is being submitted in:
https://bugs.webkit.org/show_bug.cgi?id=96127

[Mark Lam]

(In reply to comment #9)
> These are both bugs.  The fix is being submitted in:
> https://bugs.webkit.org/show_bug.cgi?id=96127

Fix landed at <http://trac.webkit.org/changeset/127938>.

[Csaba Osztrogonác]

Thanks for the quick fix, I'll try the LLInt C loop on monday again. ;-)

[Mark Lam]

r128015: <http://trac.webkit.org/changeset/128015>(In reply to comment #10)
> (In reply to comment #9)
> > These are both bugs.  The fix is being submitted in:
> > https://bugs.webkit.org/show_bug.cgi?id=96127
> 
> Fix landed at <http://trac.webkit.org/changeset/127938>.

r127938 was rolled out.  New fix landed in r128015: <http://trac.webkit.org/changeset/128015>.

[Csaba Osztrogonác]

Created attachment 163113 [details]
LLInt C loop

updated to top of trunk (automatical LLIntAssembly.h generating is still missing)

[Csaba Osztrogonác]

Created attachment 163114 [details]
LLInt C loop - JSC test results

With LLInt C loop 63 JSC tests fail (log attached) and ~1300 layout tests crash.

Is it "normal" now? Is there a bug report for it? 
Is there a build bot run "LLInt C loop" jsc and layout tests?

[Mark Lam]

(In reply to comment #14)
> Created an attachment (id=163114) [details]
> LLInt C loop - JSC test results
> 
> With LLInt C loop 63 JSC tests fail (log attached) and ~1300 layout tests crash.
> 
> Is it "normal" now? Is there a bug report for it? 
> Is there a build bot run "LLInt C loop" jsc and layout tests?

I only ran fast/js, fast/regex, ietestcenter/Javascript, and sputnik at the time.  There were only 3 failures: 1 GC time out (runs fine when run by itself), and 2 crashes in regex that were pre-existing bugs (https://bugs.webkit.org/show_bug.cgi?id=94191).  Can you try running those tests and see if you are getting the same result?

I will kick off a full layout test and see if there are issues due to the llint C loop.  If there are additional issues to this interpreter, I will fix it.  Thanks.

[Thiago Marcos P. Santos]

Comment on attachment 163113 [details]
LLInt C loop

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

> Source/WTF/wtf/Platform.h:922
> +    && (PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(GTK)) || PLATFORM(QT) \

Isn't the PLATFORM(QT) misplaced here outside the main ( )?

[Csaba Osztrogonác]

(In reply to comment #16)
> (From update of attachment 163113 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163113&action=review
> 
> > Source/WTF/wtf/Platform.h:922
> > +    && (PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(GTK)) || PLATFORM(QT) \
> 
> Isn't the PLATFORM(QT) misplaced here outside the main ( )?

Thanks, it is a typo.

[Csaba Osztrogonác]

Created attachment 163185 [details]
LLInt asm ver1

[Csaba Osztrogonác]

Created attachment 163186 [details]
LLInt asm ver1 - build log

[Csaba Osztrogonác]

Created attachment 163187 [details]
LLInt asm ver2

[Csaba Osztrogonác]

Created attachment 163188 [details]
LLInt asm ver2 - build log

[Csaba Osztrogonác]

I haven't managed to add the 3 step to Qt's build system. :-/

Tor Arne, could you check "LLInt asm ver1" and "LLInt asm ver2"
patches and their build logs? It seems we generating derived
sources always run before build anything else ... Did I something
wrong? I was lost in the qmake's magic world. :-/

[Mark Lam]

(In reply to comment #14)
> Created an attachment (id=163114) [details]
> LLInt C loop - JSC test results
> 
> With LLInt C loop 63 JSC tests fail (log attached) and ~1300 layout tests crash.
> 
> Is it "normal" now? Is there a bug report for it? 
> Is there a build bot run "LLInt C loop" jsc and layout tests?

(In reply to comment #15)
> I will kick off a full layout test and see if there are issues due to the llint C loop.

The full results of the layout test (on platform mac 64bit x86) based on  r128016 are recorded in comment <https://bugs.webkit.org/show_bug.cgi?id=91052#c33> and attachment <https://bug-91052-attachments.webkit.org/attachment.cgi?id=163231>.  Please use that as your reference.  A bug was filed to investigate the found failures <https://bugs.webkit.org/show_bug.cgi?id=96333>, but that is a lower priority right now because the failures don't appear to be regressions.

For comparison with your results, on the C++ llint, I only saw 11 failures w/ 2 crashes.  In short, I would not expect to see 63 JSC test failures, and certainly not 1300 layout test crashes.  Are you building for a 64bit x86 or some other target?

And no, there isn't currently any bot that runs the C++ llint.  That is unless one of the ports will adopt it as its main configuration going forward.

[Tor Arne Vestbø]

Comment on attachment 163185 [details]
LLInt asm ver1

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

> Source/JavaScriptCore/LLIntOffsetsExtractor.pro:2
> +# Project file for the jsc binary (interactive interpreter)

LLIntOffsetsExtractor

> Source/JavaScriptCore/LLIntOffsetsExtractor.pro:6
> + TEMPLATE = app

added space

> Source/JavaScriptCore/LLIntOffsetsExtractor.pro:9
> +DESTDIR = $${ROOT_BUILD_DIR}/bin

I think we can put this binary in $${ROOT_BUILD_DIR}/Source/JavaScriptCore/, or $$OUT_PWD, since it's not useful except for the build

> Source/JavaScriptCore/LLIntOffsetsExtractor.pro:11
> +QT -= gui

QT =

> Source/JavaScriptCore/LLIntOffsetsExtractor.pro:15
> +JAVASCRIPTCORE_GENERATED_SOURCES_DIR = $${ROOT_BUILD_DIR}/Source/JavaScriptCore/$${GENERATED_SOURCES_DESTDIR}

We don't need this, since the binary is built before the derived sources are generated

> Source/JavaScriptCore/LLIntOffsetsExtractor.pro:37
> +    $$SOURCE_DIR \
> +    $$SOURCE_DIR/.. \
> +    $$SOURCE_DIR/../WTF \
> +    $$SOURCE_DIR/assembler \
> +    $$SOURCE_DIR/bytecode \
> +    $$SOURCE_DIR/bytecompiler \
> +    $$SOURCE_DIR/heap \
> +    $$SOURCE_DIR/dfg \
> +    $$SOURCE_DIR/debugger \
> +    $$SOURCE_DIR/disassembler \
> +    $$SOURCE_DIR/interpreter \
> +    $$SOURCE_DIR/jit \
> +    $$SOURCE_DIR/llint \
> +    $$SOURCE_DIR/parser \
> +    $$SOURCE_DIR/profiler \
> +    $$SOURCE_DIR/runtime \
> +    $$SOURCE_DIR/tools \
> +    $$SOURCE_DIR/yarr \
> +    $$SOURCE_DIR/API \
> +    $$SOURCE_DIR/ForwardingHeaders \

Do we need all of these include paths?

> Source/JavaScriptCore/LLIntOffsetsExtractor.pro:42
> +win32-msvc*|win32-icc: INCLUDEPATH += $$ROOT_WEBKIT_DIR/Source/JavaScriptCore/os-win32

Do we need this?

> Source/JavaScriptCore/LLIntOffsetsExtractor.pro:56
> +mac {
> +    LIBS_PRIVATE += -framework AppKit
> +}
> +
> +win* {
> +    LIBS += -ladvapi32
> +}
> +
> +wince* {
> +    LIBS += mmtimer.lib
> +}

Do we need these?

> Source/JavaScriptCore/LLIntOffsetsExtractor.pro:72
> +#GENERATOR: LLIntOffsetsExtractor
> +llint.output = LLIntDesiredOffsets.h
> +llint.script = $$PWD/offlineasm/generate_offset_extractor.rb
> +llint.input = INPUT_FILES
> +llint.commands = ruby $$llint.script ${QMAKE_FILE_NAME} ${QMAKE_FILE_OUT}
> +llint.CONFIG += no_link
> +QMAKE_EXTRA_COMPILERS  += llint
> +#GENERATORS += llint

We might need an extra depends here, to ensure the compilation of LLIntOffsetsExtractor.cpp happens after the generator has run

> Tools/Scripts/build-jsc:66
> +    my @projects = ("WTF", "LLIntOffsetsExtractor", "JavaScriptCore");

No need for this

> WebKit.pro:50
> +LLIntOffsetsExtractor.file = Source/JavaScriptCore/LLIntOffsetsExtractor.pro
> +LLIntOffsetsExtractor.makefile = Makefile.LLIntOffsetsExtractor
> +SUBDIRS += LLIntOffsetsExtractor

This should be in JavaScriptCore.pro

[Tor Arne Vestbø]

Comment on attachment 163187 [details]
LLInt asm ver2

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

> Source/JavaScriptCore/JavaScriptCore.pro:16
> +SUBDIRS += LLIntOffsetsExtractor
>  derived_sources.file = DerivedSources.pri
>  target.file = Target.pri
>  
> -SUBDIRS += derived_sources target
> +SUBDIRS += LLIntOffsetsExtractor derived_sources target

Duplicate SUBDIRS += here

[Tor Arne Vestbø]

Comment on attachment 163187 [details]
LLInt asm ver2

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

> Source/WTF/wtf/Platform.h:33
> +#define ENABLE_JIT 0
> +#define ENABLE_LLINT 1
> +#define ENABLE_CLASSIC_INTERPRETER 0

I assume these are just for debugging purposes? The addition of || PLATFORM(QT) below, to enable LLINT, should be enough?

[Csaba Osztrogonác]

(In reply to comment #26)
> (From update of attachment 163187 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163187&action=review
> 
> > Source/WTF/wtf/Platform.h:33
> > +#define ENABLE_JIT 0
> > +#define ENABLE_LLINT 1
> > +#define ENABLE_CLASSIC_INTERPRETER 0
> 
> I assume these are just for debugging purposes? The addition of || PLATFORM(QT) below, to enable LLINT, should be enough?

It is for testing the LLInt C loop backend. (LLInt without classical JIT and classical interpreter) If we only add "PLATFORM(QT)", it enables LLINT, not
LLINT C loop.

[Csaba Osztrogonác]

Qt LLInt build will be fixed soon ... but it seems LLInt C loop build is 
broken now - https://bugs.webkit.org/show_bug.cgi?id=96509

Could you guys check it, please?

[Tor Arne Vestbø]

This patch should be ~ok build-wise 

https://gist.github.com/3706873

[Tor Arne Vestbø]

(In reply to comment #29)
> This patch should be ~ok build-wise 
> 
> https://gist.github.com/3706873

Though I see this build error:

https://gist.github.com/3707018

[Csaba Osztrogonác]

Created attachment 165611 [details]
patch with change log

We still need buildfix on Mac, and should check it on Windows.

[Csaba Osztrogonác]

Zoltán, could you try it on Windows, please?

[Csaba Osztrogonác]

Comment on attachment 165611 [details]
patch with change log

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

> Source/WTF/wtf/Platform.h:914
> +    && (PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(GTK) || PLATFORM(QT)) \

conservative solution: enable it for Linux and QNX only: PLATFORM(QT) && ( OS(LINUX) || OS(QNX) )
But the best solution would be if we can enable it for PLATFORM(QT)

[Csaba Osztrogonác]

OS(QNX) isn't needed, because JIT is disabled on it.

[Milian Wolff]

(In reply to comment #31)
> Created an attachment (id=165611) [details]
> =patch with change log
> 
> We still need buildfix on Mac, and should check it on Windows.

This patch is also required now to built qtwebkit for QNX, as there JIT is not enabled and thus LLInt is enabled. The build will otherwise fail at linking due to missing references to LLInt methods.

The patch above is good as-is, the changed hunk in Platform.h does not need to include QNX as LLint is automatically enabled when JIT is disabled.

[Simon Hausmann]

(In reply to comment #33)
> (From update of attachment 165611 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165611&action=review
> 
> > Source/WTF/wtf/Platform.h:914
> > +    && (PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(GTK) || PLATFORM(QT)) \
> 
> conservative solution: enable it for Linux and QNX only: PLATFORM(QT) && ( OS(LINUX) || OS(QNX) )
> But the best solution would be if we can enable it for PLATFORM(QT)

Sounds like a good start, along with bugs to enable it on Mac and Windows that should block the Qt 5 meta bug.

[Csaba Osztrogonác]

Mac build problem is tracked in https://bugs.webkit.org/show_bug.cgi?id=97587

[Csaba Osztrogonác]

Created attachment 165824 [details]
updated patch

Add linux-* guards not to break Mac and Windows builds

[Csaba Osztrogonác]

Comment on attachment 165824 [details]
updated patch

CQ- for now, because I'm not sure if we won't break the ARM build.

[Csaba Osztrogonác]

Created attachment 165825 [details]
updated patch

add OS(LINUX) guard to platform.h too

[Csaba Osztrogonác]

Landed in https://trac.webkit.org/changeset/129760 with guards for ARM.