Bug 110236 – [CMake] Undefined references should be detected at build time

• Component: Platform
• Status: RESOLVED FIXED

[Balazs Kelemen]

Currently the build can survive undefined references, this is not convenient.

[Balazs Kelemen]

Created attachment 189110 [details]
Patch

[EFL EWS Bot]

Comment on attachment 189110 [details]
Patch

Attachment 189110 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16613839

[Balazs Kelemen]

Created attachment 190258 [details]
Patch

[Balazs Kelemen]

Could you reevaluate this thing? According to my memory Laszlo and others were ok with the change, but Raphael mentioned we should not force such build options out-of-box because packagers should be free to decide such things. Personally I don't agree with that, because first it does not cause any harm for packagers since it's just a sanity check, second not having the build system find out link issues automatically is harmful for productivity. Anyway, I'm ok with either decision, just let's put and end on the question.

[Chris Dumez]

I also support this change. This makes debugging "undefined symbols" errors a lot easier. I also don't think it is normal if our library has missing symbols and such thing should be caught as early as possible.

[Laszlo Gombos]

Maybe we should break this patch into 2 parts - the somewhat controversial linker flag and the rest of the changes that should be good to land..

[Balazs Kelemen]

(In reply to comment #6)
> Maybe we should break this patch into 2 parts - the somewhat controversial linker flag and the rest of the changes that should be good to land..

Actually I'm not sure there is too much value of those changes without the flag. These dependencies are actually satisfied at runtime or some implicit or transitive way. I just had to add them because otherwise linking fails with --no-undefined. Do you think it's really useful to harden the linking by explicitly define these dependencies?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 190258 [details]
Patch

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

As we've just discussed on IRC, I don't really have much against passing --no-undefined to the linker these days. I've added some items that should be addressed before landing.

Additionally, I agree with Laszlo that it makes sense to land this in two parts: the part which adds dependencies can be landed first since it doesn't depend on --no-undefined, and the rest can be added later.

> Source/WebKit2/CMakeLists.txt:586
> +    rt

-lrt is Linux-specific. Instead, I suggest looking for it via FIND_LIBRARY(). It could even be done in the top-level CMakeLists.txt.

> Source/cmake/OptionsCommon.cmake:36
> +if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")

Can't we use if (UNIX) or if (UNIX AND NOT APPLE) if it that really breaks OS X (gcc -Wl,--foobarbla worked here, even if ld --foobarbla doesn't)?

> Tools/DumpRenderTree/TestNetscapePlugIn/CMakeLists.txt:1
> +find_package(X11 REQUIRED)

We already call this in OptionsEFL.cmake; if it's needed by other ports, it's better to just move the call to OptionsCommon.cmake.

[Laszlo Gombos]

Comment on attachment 190258 [details]
Patch

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

Overall this patch needs another iteration to address Raphael's and my comments.

> Source/WebCore/ChangeLog:8
> +        Push --no-undefined to the linker if building on Linux.

Pass the --no-undefined argument to the linker when building on Linux.

> Source/WebKit2/ChangeLog:8
> +        Push --no-undefined to the linker if building on Linux.

Ditto.

[Balazs Kelemen]

(In reply to comment #8)
> (From update of attachment 190258 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190258&action=review
> 
> As we've just discussed on IRC, I don't really have much against passing --no-undefined to the linker these days. I've added some items that should be addressed before landing.
> 
> Additionally, I agree with Laszlo that it makes sense to land this in two parts: the part which adds dependencies can be landed first since it doesn't depend on --no-undefined, and the rest can be added later.

I don't think this would buy us anything. The only reason to fix the dependencies is to be able to use --no-undefined and the only thing that can prove the correctness of those changes is to add the flag.

> 
> > Source/WebKit2/CMakeLists.txt:586
> > +    rt
> 
> -lrt is Linux-specific. Instead, I suggest looking for it via FIND_LIBRARY(). It could even be done in the top-level CMakeLists.txt.

find_library is a good idea. This is only a dependency of WebKit2 so I would keep it there (i.e. ports that don't support WebKit2 do not need this).

> 
> > Source/cmake/OptionsCommon.cmake:36
> > +if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")
> 
> Can't we use if (UNIX) or if (UNIX AND NOT APPLE) if it that really breaks OS X (gcc -Wl,--foobarbla worked here, even if ld --foobarbla doesn't)?

Good idea.

> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/CMakeLists.txt:1
> > +find_package(X11 REQUIRED)
> 
> We already call this in OptionsEFL.cmake; if it's needed by other ports, it's better to just move the call to OptionsCommon.cmake.

I think OptionsCommon.cmake should not contain any such platform specific bits. I will remove it and and we can see whether all ports that build this are fine with it.

[Balazs Kelemen]

Created attachment 205410 [details]
Patch

[EFL EWS Bot]

Comment on attachment 205410 [details]
Patch

Attachment 205410 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/926633

[Balazs Kelemen]

Created attachment 205417 [details]
Patch

[Build Bot]

Comment on attachment 205417 [details]
Patch

Attachment 205417 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/913386

New failing tests:
fast/workers/worker-close-more.html

[Build Bot]

Created attachment 205436 [details]
Archive of layout-test-results from APPLE-EWS-5 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-5  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Balazs Kelemen]

Everything is green. Could you review the new patch?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 205417 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:2953
> +set(WebCoreTestSupport_LIBRARIES
> +    JavaScriptCore
> +    WTF
> +    WebCore
> +)

Did you get any crashes without this? It worked fine for me without defining any libraries (and SHARED_CORE=ON).

> Source/WebKit2/CMakeLists.txt:611
> +if (WTF_OS_LINUX)

I thought we were moving away from defining WTF variables in CMake? The following looks better to me:
  # librt is needed for shm_open in libc.
  find_library(LIBRT_LIBRARIES NAMES rt)
  if (LIBRT_LIBRARIES)
      list(APPEND WebKit2_LIBRARIES ${LIBRT_LIBRARIES})
  endif ()

> Tools/DumpRenderTree/TestNetscapePlugIn/CMakeLists.txt:1
> +find_package(X11 REQUIRED)

Weren't you going to experiment with not calling this one?

[Balazs Kelemen]

Created attachment 206150 [details]
Patch

[Balazs Kelemen]

(In reply to comment #17)
> (From update of attachment 205417 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205417&action=review
> 
> > Source/WebCore/CMakeLists.txt:2953
> > +set(WebCoreTestSupport_LIBRARIES
> > +    JavaScriptCore
> > +    WTF
> > +    WebCore
> > +)
> 
> Did you get any crashes without this? It worked fine for me without defining any libraries (and SHARED_CORE=ON).

No crash but undefined references makes the build fail. The linker don't know that at runtime those libraries will be loaded before this one.

> 
> > Source/WebKit2/CMakeLists.txt:611
> > +if (WTF_OS_LINUX)
> 
> I thought we were moving away from defining WTF variables in CMake? The following looks better to me:
>   # librt is needed for shm_open in libc.
>   find_library(LIBRT_LIBRARIES NAMES rt)
>   if (LIBRT_LIBRARIES)
>       list(APPEND WebKit2_LIBRARIES ${LIBRT_LIBRARIES})
>   endif ()

Fine to me, fixed.

> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/CMakeLists.txt:1
> > +find_package(X11 REQUIRED)
> 
> Weren't you going to experiment with not calling this one?

Right, I was forgetting this. Fixed.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #19)
> (In reply to comment #17)
> > (From update of attachment 205417 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=205417&action=review
> > 
> > > Source/WebCore/CMakeLists.txt:2953
> > > +set(WebCoreTestSupport_LIBRARIES
> > > +    JavaScriptCore
> > > +    WTF
> > > +    WebCore
> > > +)
> > 
> > Did you get any crashes without this? It worked fine for me without defining any libraries (and SHARED_CORE=ON).
> 
> No crash but undefined references makes the build fail. The linker don't know that at runtime those libraries will be loaded before this one.

Sorry, I meant I tried your patch and removed those lines, and everything still worked and linked fine.

[Balazs Kelemen]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > (From update of attachment 205417 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=205417&action=review
> > > 
> > > > Source/WebCore/CMakeLists.txt:2953
> > > > +set(WebCoreTestSupport_LIBRARIES
> > > > +    JavaScriptCore
> > > > +    WTF
> > > > +    WebCore
> > > > +)
> > > 
> > > Did you get any crashes without this? It worked fine for me without defining any libraries (and SHARED_CORE=ON).
> > 
> > No crash but undefined references makes the build fail. The linker don't know that at runtime those libraries will be loaded before this one.
> 
> Sorry, I meant I tried your patch and removed those lines, and everything still worked and linked fine.

I see. Thanks for trying. I think I added these because first time the build was failing on the ews. I guess it fails without SHARED_CORE=ON. I am going to try that locally.

[Balazs Kelemen]

> > Sorry, I meant I tried your patch and removed those lines, and everything still worked and linked fine.
> 
> I see. Thanks for trying. I think I added these because first time the build was failing on the ews. I guess it fails without SHARED_CORE=ON. I am going to try that locally.

Yes, if I not make libTestWebKitSupport explicitly link against the other libraries the build fails _without_ SHARED_CORE=ON.

[Raphael Kubo da Costa (:rakuco)]

Right, LGTM then. Thank you :-)

[Chris Dumez]

Comment on attachment 206150 [details]
Patch

Ok, r=me. Thanks for the informal review Rakuco.

[Csaba Osztrogonác]

already landed in http://trac.webkit.org/changeset/152774