Bug 182610

Summary: [MIPS] JSC needs to be built with -latomic
Product: WebKit Reporter: Guillaume Emont <guijemont>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, commit-queue, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=182622
Attachments:
Description Flags
Patch
none
Patch none

Description Guillaume Emont 2018-02-08 11:48:55 PST
Since r228149, on MIPS we need to link with -latomic, since __atomic_fetch_add_8 is not available as a compiler intrinsic.
Comment 1 Guillaume Emont 2018-02-08 12:52:02 PST
Created attachment 333412 [details]
Patch

Patch fixing this.
Comment 2 Zan Dobersek 2018-02-08 12:56:49 PST
Comment on attachment 333412 [details]
Patch

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

> Source/JavaScriptCore/CMakeLists.txt:125
> +if (WTF_CPU_MIPS)
> +    list(APPEND JavaScriptCore_LIBRARIES
> +        -latomic
> +    )
> +endif ()

Similar comment to what's in the ChangeLog would be nice here as well.
Comment 3 Adrian Perez 2018-02-08 13:02:07 PST
Comment on attachment 333412 [details]
Patch

In Source/WebKit/CMakeLists.txt, after line 780, there's a check
that tries to compile a snippet which uses “std::atomic<int64_t>”
and determines whether using libatomic is needed or not. Instead
of unconditionally adding the library as your patch does, could
you please change the CMake code there so it adds it conditionally?

The resulting snippet patch should be something like:

        if (ATOMIC_INT64_REQUIRES_LIBATOMIC)
            list(APPEND WebKit_LIBRARIES atomic)
   +        list(APPEND JavaScriptCore_LIBRARIES atomic)
        endif ()

Thanks!
Comment 4 Adrian Perez 2018-02-08 13:04:45 PST
(In reply to Adrian Perez from comment #3)
> Comment on attachment 333412 [details]
> Patch
> 
> In Source/WebKit/CMakeLists.txt, after line 780, there's a check
> that tries to compile a snippet which uses “std::atomic<int64_t>”
> and determines whether using libatomic is needed or not. Instead
> of unconditionally adding the library as your patch does, could
> you please change the CMake code there so it adds it conditionally?
> 
> The resulting snippet patch should be something like:
> 
>         if (ATOMIC_INT64_REQUIRES_LIBATOMIC)
>             list(APPEND WebKit_LIBRARIES atomic)
>    +        list(APPEND JavaScriptCore_LIBRARIES atomic)
>         endif ()
> 
> Thanks!

Correction: This is in the CMakeLists.txt for *WebKit*, so it would
be better to move the check to one of the top-level CMakeLists.txt
and then in Source/JavaScriptCore/CMakeLists.txt append the library
conditionally to the list.
Comment 5 Guillaume Emont 2018-02-09 09:28:55 PST
Created attachment 333495 [details]
Patch

New patch addressing Zan's comments.
Comment 6 Guillaume Emont 2018-02-09 09:30:32 PST
Note that this is to fix the build quickly. A more correct solution will be developed in bug#182622.
Comment 7 WebKit Commit Bot 2018-02-11 23:32:20 PST
Comment on attachment 333495 [details]
Patch

Clearing flags on attachment: 333495

Committed r228370: <https://trac.webkit.org/changeset/228370>
Comment 8 WebKit Commit Bot 2018-02-11 23:32:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-02-11 23:33:28 PST
<rdar://problem/37451082>