Bug 182610 - [MIPS] JSC needs to be built with -latomic
Summary: [MIPS] JSC needs to be built with -latomic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-08 11:48 PST by Guillaume Emont
Modified: 2018-02-11 23:33 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.41 KB, patch)
2018-02-08 12:52 PST, Guillaume Emont
no flags Details | Formatted Diff | Diff
Patch (1.65 KB, patch)
2018-02-09 09:28 PST, Guillaume Emont
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>