Bug 90098 – [GTK] LLint build fails with -g -02

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alban Browaeys]

Created attachment 149791 [details]
fix llint build wwith -02 -g 

/usr/bin/ruby ./Source/JavaScriptCore/offlineasm/asm.rb ./Source/JavaScriptCore/llint/LowLevelInterpreter.asm Programs/LLIntOffsetsExtractor DerivedSources/JavaScriptCore/LLIntAssembly.h
 offsetsAndConfigurationIndex in Source/JavaScriptCore/offlineasm/offsets.rb raise an unhandled exception during build if -02 -g flags are used (ie set CFLAGS to "-O0 -g" and not set webkit debug configure switch which ends up with "-00 -g - 02" => "-g -O2").
the exception is raised at:
raise if result.map{|v| v[1]}.uniq.size < result.map{|v| v[1]}.size

It turns out that the way extractorTable is defined inside the LLIntOffsetsExtractor::dummy which itself is defined as a class member inside the class declaration leads to it behing defined in the assembly twice. That is the class member with -O2 is inlined (which is common for class members at least with g++). The static local variable extractorTable is thus duplicated for each translation unit. And ends up in the usual symbols  and the debug_info symbols. Ie twice. Then offsetsAndConfigurationIndex in Source/JavaScriptCore/offlineasm/offsets.rb  finds extractorTable magic numbers twice in the binary object and the result.map{|v| v[1]}.uniq.size < result.map{|v| v[1]}.size becomes 1 < 2 and the exception is raised.

Even if done by mistake (I intended to build -O0 -g) -g -02 is quite common for distribution that provide -dbg packages. So this issue might be major instead of normal.

Running:
$ /usr/bin/ruby ./Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb ./Source/JavaScriptCore/llint/LowLevelInterpreter.asm DerivedSources/JavaScriptCore/LLIntDesiredOffsets.h
$ g++ -DHAVE_CONFIG_H -I.  -Wall -W -Wcast-align -Wchar-subscripts -Wreturn-type -Wformat -Wformat-security -Wno-format-y2k -Wundef -Wmissing-format-attribute -Wpointer-arith -Wwrite-strings -Wno-unused-parameter -Wno-parentheses -fno-exceptions -DENABLE_GLIB_SUPPORT=1 -DBUILDING_CAIRO__=1 -DBUILDING_GTK__=1 -DBUILDING_SOUP__=1 -DWTF_CHANGES -DBUILDING_WEBKIT2__=1 -DXP_UNIX -DMOZ_X11  -DWTF_USE_ICU_UNICODE=1   -DWTF_USE_GSTREAMER=1 -DWTF_USE_ACCELERATED_COMPOSITING=1 -DENABLE_3D_RENDERING=1  -DWTF_USE_TEXTURE_MAPPER=1 -DWTF_USE_TEXTURE_MAPPER_GL=1  -DWTF_USE_GLX=1 -DWTF_USE_OPENGL=1  -DNDEBUG    -I./Source -I./Source/JavaScriptCore -I./Source/JavaScriptCore/API -I./Source/JavaScriptCore/assembler -I./Source/JavaScriptCore/bytecode -I./Source/JavaScriptCore/bytecompiler -I./Source/JavaScriptCore/dfg -I./Source/JavaScriptCore/heap -I./Source/JavaScriptCore/debugger -I./Source/JavaScriptCore/ForwardingHeaders -I./Source/JavaScriptCore/interpreter -I./Source/JavaScriptCore/jit -I./Source/JavaScriptCore/jit -I./Source/JavaScriptCore/llint -I./Source/JavaScriptCore/parser -I./Source/JavaScriptCore/profiler -I./Source/JavaScriptCore/runtime -I./Source/JavaScriptCore/tools -I./Source/JavaScriptCore/yarr -I./DerivedSources/JavaScriptCore -I./Source/WTF   -O0 -g -Wno-c++0x-compat -O2 -MT Source/JavaScriptCore/llint/Programs_LLIntOffsetsExtractor-LLIntOffsetsExtractor.o -MD -MP -MF Source/JavaScriptCore/llint/.deps/Programs_LLIntOffsetsExtractor-LLIntOffsetsExtractor.Tpo -c -o Source/JavaScriptCore/llint/Programs_LLIntOffsetsExtractor-LLIntOffsetsExtractor.o `test -f 'Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp' || echo './'`Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp
$ /usr/bin/ruby ./Source/JavaScriptCore/offlineasm/asm.rb ./Source/JavaScriptCore/llint/LowLevelInterpreter.asm Programs/LLIntOffsetsExtractor DerivedSources/JavaScriptCore/LLIntAssembly.h
from an existing build tree of webkit should give the above error.


The attached patch fixes this.


This is tested against git mirror master 4c742312f5b69a166256a28390aedc4c0bd45ac1, ie :

commit 4c742312f5b69a166256a28390aedc4c0bd45ac1
Author: [e-mail] <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Thu Jun 21 02:03:44 2012 +0000

    Unreviewed, rolling out r120889.
    http://trac.webkit.org/changeset/120889
    https://bugs.webkit.org/show_bug.cgi?id=89630
    
    [Chromium] webkit_unit_tests didDrawNotCalledOnHiddenLayer
    start failing (Requested by ukai on #webkit).
    
    Patch by Sheriff Bot <[e-mail]> on 2012-06-20
    
    Source/WebCore:
    
    * platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:
    (WebCore::CCLayerTreeHostImpl::calculateRenderSurfaceLayerList):
    
    Source/WebKit/chromium:
    
    * tests/CCLayerTreeHostImplTest.cpp:
    
    git-svn-id: http://svn.webkit.org/repository/webkit/trunk@120899 268f45cc-cd09-0410-ab3c-d5

[Filip Pizlo]

How does your patch handle fat binaries?

[Filip Pizlo]

(In reply to comment #0)
> Created an attachment (id=149791) [details]
> fix llint build wwith -02 -g 
> 
> /usr/bin/ruby ./Source/JavaScriptCore/offlineasm/asm.rb ./Source/JavaScriptCore/llint/LowLevelInterpreter.asm Programs/LLIntOffsetsExtractor DerivedSources/JavaScriptCore/LLIntAssembly.h
>  offsetsAndConfigurationIndex in Source/JavaScriptCore/offlineasm/offsets.rb raise an unhandled exception during build if -02 -g flags are used (ie set CFLAGS to "-O0 -g" and not set webkit debug configure switch which ends up with "-00 -g - 02" => "-g -O2").
> the exception is raised at:
> raise if result.map{|v| v[1]}.uniq.size < result.map{|v| v[1]}.size
> 
> It turns out that the way extractorTable is defined inside the LLIntOffsetsExtractor::dummy which itself is defined as a class member inside the class declaration leads to it behing defined in the assembly twice.

In that case, the correct solution is to simply remove the assertion and harden the code against duplicates (i.e. if it finds a duplicate then return the first one, or the last one, or the middle one, or whichever one you like).

On the other hand, your fix will break fat binary builds, which is a show-stopper for us.  A fat binary build produces a binary with two or more different builds in it; in that case the offsets extractor needs to find all of the extractor tables.  Your patch prevents finding all extractor tables because all but the first will have a corrupted magic number.

> That is the class member with -O2 is inlined (which is common for class members at least with g++). The static local variable extractorTable is thus duplicated for each translation unit. And ends up in the usual symbols  and the debug_info symbols. Ie twice. Then offsetsAndConfigurationIndex in Source/JavaScriptCore/offlineasm/offsets.rb  finds extractorTable magic numbers twice in the binary object and the result.map{|v| v[1]}.uniq.size < result.map{|v| v[1]}.size becomes 1 < 2 and the exception is raised.
> 
> Even if done by mistake (I intended to build -O0 -g) -g -02 is quite common for distribution that provide -dbg packages. So this issue might be major instead of normal.
> 
> Running:
> $ /usr/bin/ruby ./Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb ./Source/JavaScriptCore/llint/LowLevelInterpreter.asm DerivedSources/JavaScriptCore/LLIntDesiredOffsets.h
> $ g++ -DHAVE_CONFIG_H -I.  -Wall -W -Wcast-align -Wchar-subscripts -Wreturn-type -Wformat -Wformat-security -Wno-format-y2k -Wundef -Wmissing-format-attribute -Wpointer-arith -Wwrite-strings -Wno-unused-parameter -Wno-parentheses -fno-exceptions -DENABLE_GLIB_SUPPORT=1 -DBUILDING_CAIRO__=1 -DBUILDING_GTK__=1 -DBUILDING_SOUP__=1 -DWTF_CHANGES -DBUILDING_WEBKIT2__=1 -DXP_UNIX -DMOZ_X11  -DWTF_USE_ICU_UNICODE=1   -DWTF_USE_GSTREAMER=1 -DWTF_USE_ACCELERATED_COMPOSITING=1 -DENABLE_3D_RENDERING=1  -DWTF_USE_TEXTURE_MAPPER=1 -DWTF_USE_TEXTURE_MAPPER_GL=1  -DWTF_USE_GLX=1 -DWTF_USE_OPENGL=1  -DNDEBUG    -I./Source -I./Source/JavaScriptCore -I./Source/JavaScriptCore/API -I./Source/JavaScriptCore/assembler -I./Source/JavaScriptCore/bytecode -I./Source/JavaScriptCore/bytecompiler -I./Source/JavaScriptCore/dfg -I./Source/JavaScriptCore/heap -I./Source/JavaScriptCore/debugger -I./Source/JavaScriptCore/ForwardingHeaders -I./Source/JavaScriptCore/interpreter -I./Source/JavaScriptCore/jit -I./Source/JavaScriptCore/jit -I./Source/JavaScriptCore/llint -I./Source/JavaScriptCore/parser -I./Source/JavaScriptCore/profiler -I./Source/JavaScriptCore/runtime -I./Source/JavaScriptCore/tools -I./Source/JavaScriptCore/yarr -I./DerivedSources/JavaScriptCore -I./Source/WTF   -O0 -g -Wno-c++0x-compat -O2 -MT Source/JavaScriptCore/llint/Programs_LLIntOffsetsExtractor-LLIntOffsetsExtractor.o -MD -MP -MF Source/JavaScriptCore/llint/.deps/Programs_LLIntOffsetsExtractor-LLIntOffsetsExtractor.Tpo -c -o Source/JavaScriptCore/llint/Programs_LLIntOffsetsExtractor-LLIntOffsetsExtractor.o `test -f 'Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp' || echo './'`Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp
> $ /usr/bin/ruby ./Source/JavaScriptCore/offlineasm/asm.rb ./Source/JavaScriptCore/llint/LowLevelInterpreter.asm Programs/LLIntOffsetsExtractor DerivedSources/JavaScriptCore/LLIntAssembly.h
> from an existing build tree of webkit should give the above error.
> 
> 
> The attached patch fixes this.
> 
> 
> This is tested against git mirror master 4c742312f5b69a166256a28390aedc4c0bd45ac1, ie :
> 
> commit 4c742312f5b69a166256a28390aedc4c0bd45ac1
> Author: [e-mail] <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
> Date:   Thu Jun 21 02:03:44 2012 +0000
> 
>     Unreviewed, rolling out r120889.
>     http://trac.webkit.org/changeset/120889
>     https://bugs.webkit.org/show_bug.cgi?id=89630
> 
>     [Chromium] webkit_unit_tests didDrawNotCalledOnHiddenLayer
>     start failing (Requested by ukai on #webkit).
> 
>     Patch by Sheriff Bot <[e-mail]> on 2012-06-20
> 
>     Source/WebCore:
> 
>     * platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:
>     (WebCore::CCLayerTreeHostImpl::calculateRenderSurfaceLayerList):
> 
>     Source/WebKit/chromium:
> 
>     * tests/CCLayerTreeHostImplTest.cpp:
> 
>     git-svn-id: http://svn.webkit.org/repository/webkit/trunk@120899 268f45cc-cd09-0410-ab3c-d5

[Priit Laes (IRC: plaes)]

Can't we add some extra checks, to see what target (regular elf vs fat binary) we are trying to build?

Also, some distros, like Gentoo allow splitting off the debugging info into separate files:
/usr/lib64/libwebkitgtk-3.0.so.0.13.2
/usr/lib64/debug/usr/lib64/libwebkitgtk-3.0.so.0.13.2.debug

[snip]
With splitdebug enabled, Portage will still strip the binaries installed in the system. But before doing that, all the useful debug information is copied to a ".debug" file, which is then installed inside /usr/lib/debug (the complete name of the file would be given by appending to that the path where the file is actually installed). The path to that file is then saved in the original file inside an ELF section called ".gnu_debuglink", so that gdb knows which file to load the symbols from.
[/snip]

[Filip Pizlo]

(In reply to comment #3)
> Can't we add some extra checks, to see what target (regular elf vs fat binary) we are trying to build?

Why?  Can't you just remove the assertion and be done with it?

> 
> Also, some distros, like Gentoo allow splitting off the debugging info into separate files:
> /usr/lib64/libwebkitgtk-3.0.so.0.13.2
> /usr/lib64/debug/usr/lib64/libwebkitgtk-3.0.so.0.13.2.debug

Why does this matter?  The offsets extractor doesn't care about debug data.

> 
> [snip]
> With splitdebug enabled, Portage will still strip the binaries installed in the system. But before doing that, all the useful debug information is copied to a ".debug" file, which is then installed inside /usr/lib/debug (the complete name of the file would be given by appending to that the path where the file is actually installed). The path to that file is then saved in the original file inside an ELF section called ".gnu_debuglink", so that gdb knows which file to load the symbols from.
> [/snip]

[Priit Laes (IRC: plaes)]

Created attachment 151491 [details]
webkit-bug-90098-gtk-llint-build-failure.patch

[Priit Laes (IRC: plaes)]

Comment on attachment 151491 [details]
webkit-bug-90098-gtk-llint-build-failure.patch

That approach fails with errors about duplicate symbols (unfortunately missed the CFLAGS on first try):
[snip]
{standard input}: Assembler messages:
{standard input}:4432: Error: symbol `llint_begin' is already defined
{standard input}:4438: Error: symbol `llint_program_prologue' is already defined
{standard input}:4460: Error: symbol `llint_eval_prologue' is already defined
...
[/snip]

[Alban Browaeys]

(In reply to comment #4)
> (In reply to comment #3)
> > Can't we add some extra checks, to see what target (regular elf vs fat binary) we are trying to build?
> 
> Why?  Can't you just remove the assertion and be done with it?
> 
> > 
> > Also, some distros, like Gentoo allow splitting off the debugging info into separate files:
> > /usr/lib64/libwebkitgtk-3.0.so.0.13.2
> > /usr/lib64/debug/usr/lib64/libwebkitgtk-3.0.so.0.13.2.debug
> 
> Why does this matter?  The offsets extractor doesn't care about debug data.

> > [snip]
> > With splitdebug enabled, Portage will still strip the binaries installed in the system. But before doing that, all the useful debug information is copied to a ".debug" file, which is then installed inside /usr/lib/debug (the complete name of the file would be given by appending to that the path where the file is actually installed). The path to that file is then saved in the original file inside an ELF section called ".gnu_debuglink", so that gdb knows which file to load the symbols from.
> > [/snip]

it cares about the debug_data because the debug_info section contains a duplicate of the array including the magic numbers . This is the issue from the start. duplicate which leads to the exception being raised comes from the debug info section. 
In fact this issue is critical to distributions as they all build with -g -02 "then" strip the symbols into another file. Thus build will always fails except for devs.

The issue is that the offsets are the same (thus duplicates) in the debug section than in the non debug section.

[Alban Browaeys]

Created attachment 151701 [details]
Avoid duplicate offsets for llint, discarding them.

This might work (using only the first of the duplicates). I currently fails to build Programs/WebKitPluginProcess though the issue seems unrelated. So patch no even tested .

[Priit Laes (IRC: plaes)]

(In reply to comment #8)
> Created an attachment (id=151701) [details]
> Avoid duplicate offsets for llint, discarding them.
> 
> This might work (using only the first of the duplicates). I currently fails to build Programs/WebKitPluginProcess though the issue seems unrelated. So patch no even tested .

Works for me!

[Alban Browaeys]

> > This might work (using only the first of the duplicates). I currently fails to build Programs/WebKitPluginProcess though the issue seems unrelated. So patch no even tested .
> 
> Works for me!

Me too  !
in the end (the issue about malformed archive was bug 91154 which I just reported and came from the size libWebCore.a exceed the ar format limit when built with -g -02 , that is 4.4G)

[Priit Laes (IRC: plaes)]

Could you please create a proper patch (with ChangeLog and stuff, you can use Tools/Scripts/prepare-changelog script for that) and mark it up for review/commit queue?

[Priit Laes (IRC: plaes)]

Created attachment 160565 [details]
webkit-bug-90098-gtk-llint-build-failure.patch

I took the patch, added ChangeLog with original author name, so there it goes...

This build issue has been popping up mostly when building with '-g -O2' on various Linux distros.

[Filip Pizlo]

Comment on attachment 160565 [details]
webkit-bug-90098-gtk-llint-build-failure.patch

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

> Source/JavaScriptCore/offlineasm/offsets.rb:158
> +                #result << [offsets, index]

You should just remove this line.

[Priit Laes (IRC: plaes)]

Created attachment 160922 [details]
webkit-bug-90098-gtk-llint-build-failure-v2.patch

[WebKit Review Bot]

Comment on attachment 160922 [details]
webkit-bug-90098-gtk-llint-build-failure-v2.patch

Clearing flags on attachment: 160922

Committed r126886: <http://trac.webkit.org/changeset/126886>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.