Bug 16406 – [Gtk] JavaScriptCore needs -lpthread

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Seo Sanghyeon]

JavaScriptCore build is missing -lpthread. This results in a build failure like:

tmp/collector.o: In function `KJS::currentThreadStackBase()':
collector.cpp:(.text+0x203): undefined reference to `pthread_getattr_np'
collector.cpp:(.text+0x21b): undefined reference to `pthread_attr_getstack'
tmp/collector.o: In function `KJS::Collector::markCurrentThreadConservatively()':
collector.cpp:(.text+0x825): undefined reference to `pthread_getattr_np'
collector.cpp:(.text+0x83d): undefined reference to `pthread_attr_getstack'
collect2: ld returned 1 exit status

In Gtk port, JavaScriptCore is built using compiler flags from icu-config. Normally, icu-config output includes -lpthread, like:

-lpthread -lm -L/usr/lib -licui18n -licuuc -licudata -lpthread -lm

But one doesn't need to link against libpthread when one is using only ICU functions. So as a part of general campaign to reduce spurious library dependency, Debian's icu-config doesn't output -lpthread starting from 3.8-5. Then the output looks like:

-lm -L/usr/lib -licui18n -licuuc -licudata -lm

[Seo Sanghyeon]

Created attachment 17858 [details]
Include -lpthread

[Mark Rowe (bdash)]

Comment on attachment 17858 [details]
Include -lpthread

You should set the review flag to ? if you'd like your patch looked at for review.  You should also include a description of your change in the ChangeLog at JavaScriptCore/ChangeLog.  See http://webkit.org/coding/contributing.html for more info.

The change looks fine to me.  It's not great to always add -lpthread as some platforms may not use pthreads, but as JavaScriptCore makes the assumption that they do when building multi-threaded I think it's fine for the meantime.

Marking as r- for now pending a ChangeLog entry.  Thanks for the patch!

[Alp Toker]

Created attachment 17859 [details]
Remove kjs pthread dependency

Proposed fix (needs review from someone who knows the collector and what correct behaviour is when threading is disabled)

[Mark Rowe (bdash)]

Comment on attachment 17859 [details]
Remove kjs pthread dependency

That will prevent the stack from being scanned at all, which is really bad.

[Mike Hommey]

Check out bug #15118, which is exactly the same issue, except that in the end, I couldn't reproduce with the icu not having -lpthread in its -config script.

[Mike Hommey]

(In reply to comment #5)
> Check out bug #15118, which is exactly the same issue, except that in the end,
> I couldn't reproduce with the icu not having -lpthread in its -config script.

It could actually be reproduced by debian buildds...

http://buildd.debian.org/fetch.cgi?pkg=webkit;ver=0%7Esvn27674-1;arch=sparc;stamp=1197008842

[Alp Toker]

Created attachment 17864 [details]
Prospective build fix

This patch links to pthread only when not building on Windows, and does it in JavaScriptCore.pri rather than testkjs.pro as proposed previously.

Untested since I can't reproduce the build breakage. Can someone try this?

[Alp Toker]

Comment on attachment 17864 [details]
Prospective build fix

Build fix; tested and works.

[Alp Toker]

Landed in r28692.