Bug 180637 – undefined reference to 'JSC::B3::BasicBlock::fallThrough() const

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

[Michael Catanzaro]

This build failure occurs in debug builds with the following compiler flags set:

os.environ['CFLAGS'] = '-m64 -mtune=generic -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1'
os.environ['CXXFLAGS'] = '-m64 -mtune=generic -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1'
os.environ['LDFLAGS'] = '-Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld'

$ cat /usr/lib/rpm/redhat/redhat-hardened-cc1
*cc1_options:
+ %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}

$ cat /usr/lib/rpm/redhat/redhat-hardened-ld
*self_spec:
+ %{!static:%{!shared:%{!r:-pie}}}

These compiler flags are commonly-used in Linux distros. However, I'm not sure which particular flag is to blame for the failure. Our test bots do not set these flags and are all happy.

The failure:

[609/2808] Linking CXX executable bin/testair
FAILED: bin/testair 
: && /usr/lib64/ccache/c++  -fdiagnostics-color=always -Wno-expansion-to-defined -Wno-noexcept-type -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wextra -Wall -m64 -mtune=generic -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fno-strict-aliasing -fno-exceptions -std=c++14 -fno-rtti -gsplit-dwarf -g  -L/home/mcatanzaro/Projects/GNOME/install/lib -Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -fuse-ld=gold -Wl,--disable-new-dtags -Wl,--gdb-index   -rdynamic Source/JavaScriptCore/shell/CMakeFiles/testair.dir/__/b3/air/testair.cpp.o  -o bin/testair  -Wl,-rpath,/home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/lib -ldl lib/libjavascriptcoregtk-4.0.so.18.7.1 lib/libWTFGTK.a -lglib-2.0 lib/libbmalloc.a -ldl -licudata -licuuc -lpthread -lgio-2.0 -lgobject-2.0 -lz -licui18n -lglib-2.0 && :
lib/libjavascriptcoregtk-4.0.so.18.7.1: error: undefined reference to 'JSC::B3::BasicBlock::fallThrough() const'
collect2: error: ld returned 1 exit status

Removing the inline keyword and moving the functions to B3BasicBlock.cpp solves the problem. Of course, symbols required from outside JSC cannot be marked inline. But it's not clear to me why the fallThrough symbols are being required for testair, and why that is only the case with the above compiler flags.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> This build failure occurs in debug builds with the following compiler flags
> set:

It's probably not related to compiler flags at all, but to DEVELOPER_MODE. Likely the same as bug #179914.

[Michael Catanzaro]

Probably fixed by r226971.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> This build failure occurs in debug builds with the following compiler flags
> set

Also: I think this occurred only in debug builds without DEVELOPER_MODE set. Those probably can't be expected to work, anyway.

[Alejandro G. Castro]

I can reproduce it, the problem seems to be B3SwitchValue.cpp:

...
BasicBlock* SwitchValue::fallThrough(const BasicBlock* owner)
{
    ASSERT(hasFallThrough());
    BasicBlock* fallThrough = owner->successor(owner->numSuccessors() - 1).block();
    ASSERT(fallThrough == owner->fallThrough().block());
    return fallThrough;
}
...

The ASSERT is just compiled for Debug, that is why it just happens in Debug, if we use RELEASE_ASSERT it also happens on Release. In the ASSERT we use the fallThrough function that is just declared in the B3BasicBlock.h header without the inline, so the compiler seems to leave the symbol undefined when compiling the UnifiedSource and be happy about it. But when merging the Unified object bundles it does not add the inline definition found later, and when using the library to link the binaries the symbol is still undefined because de implementation was inline.

Adding the include to that file solves the problem, or adding the include to that unified bundle manually.

I'm adding Keith to check the proper solution to this in case it is a known limitation of the unified compilation, it seems a problem when declaring a function not inline and implementing it later inline?

[Alejandro G. Castro]

Created attachment 332130 [details]
Patch

[Alejandro G. Castro]

Created attachment 332135 [details]
Patch

[Alejandro G. Castro]

It is the -O2, you can see Michael compilation in the first comment, that makes the inlines real inlines, if you compile Debug with O0 it should not happen because the compiler does not inline the functions. And in release it does not happen because it is an ASSERT, if you use RELEASE_ASSERT everyone should be able to reproduce it becuase Release does use -O2 commonly. In my case and Michaels the compiler package decides to use O2 also in Debug, JFTR it is Fedora in my case.

[Michael Catanzaro]

Comment on attachment 332135 [details]
Patch

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

> Source/JavaScriptCore/b3/B3SwitchValue.cpp:32
> +#include "B3BasicBlockInlines.h"

I agree that this patch is right.

I guess the theory behind splitting *Inlines.h into separate files is that .cpp files that don't use the inline functions don't need to #include them? That is, it's just to speed up the build a little bit? This seems like a bad idea....

[Mark Lam]

Comment on attachment 332135 [details]
Patch

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

>> Source/JavaScriptCore/b3/B3SwitchValue.cpp:32
>> +#include "B3BasicBlockInlines.h"
> 
> I agree that this patch is right.
> 
> I guess the theory behind splitting *Inlines.h into separate files is that .cpp files that don't use the inline functions don't need to #include them? That is, it's just to speed up the build a little bit? This seems like a bad idea....

This fix is incorrect.  The proper fix is to replace the #include of B3BasicBlock.h with B3BasicBlockInlines.h.

The reason for putting inline functions in an Inlines.h file is not primarily for speeding up compilation, but rather to workaround circular dependencies.  As a general rule, header files should never #include Inlines.h files.  Only .cpp files should #include them.  There may be 1 or 2 exception to this (eg in JSCInlines.h), but those are special cases for specific reasons.

[Alejandro G. Castro]

(In reply to Mark Lam from comment #9) 
> This fix is incorrect.  The proper fix is to replace the #include of
> B3BasicBlock.h with B3BasicBlockInlines.h.
> 
> The reason for putting inline functions in an Inlines.h file is not
> primarily for speeding up compilation, but rather to workaround circular
> dependencies.  As a general rule, header files should never #include
> Inlines.h files.  Only .cpp files should #include them.  There may be 1 or 2
> exception to this (eg in JSCInlines.h), but those are special cases for
> specific reasons.

Makes sense, thanks for the comment Mark, I'll rework the patch.

[Alejandro G. Castro]

Created attachment 332166 [details]
Patch

[Michael Catanzaro]

Comment on attachment 332166 [details]
Patch

OK, this is what Mark wanted.

[WebKit Commit Bot]

Comment on attachment 332166 [details]
Patch

Clearing flags on attachment: 332166

Committed r227597: <https://trac.webkit.org/changeset/227597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36858899>