Bug 143754

Summary: [W32] weakCompareAndSwap assembler code is not used when building with MinGW GCC
Product: WebKit Reporter: LRN <lrn1986>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: benjamin, cgarcia, cmarcelo, commit-queue, lrn1986, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 133028    
Attachments:
Description Flags
[GTK][W32] Allow MinGW-GCC to use GCC asm code for weakCompareAndSwap
ossy: review-
Allow MinGW-GCC to use GCC asm code for weakCompareAndSwap
none
Allow MinGW-GCC to use GCC asm code for weakCompareAndSwap none

Description LRN 2015-04-15 06:13:12 PDT
The ifdef says OS(WINDOWS), but means COMPILER(MSVC)
Comment 1 LRN 2015-04-15 08:28:39 PDT
Created attachment 250789 [details]
[GTK][W32] Allow MinGW-GCC to use GCC asm code for weakCompareAndSwap

OS(WINDOWS) != COMPILER(MSVC)
Comment 2 Csaba Osztrogonác 2015-04-15 09:14:47 PDT
Comment on attachment 250789 [details]
[GTK][W32] Allow MinGW-GCC to use GCC asm code for weakCompareAndSwap

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

> Source/JavaScriptCore/ChangeLog:8
> +        [W32] Fixup dummy HeapStatistics implementation
> +
> +        GCC warns that exitWithFailure is marked as noreturn, but it does return:
> +        ../webkitgtk-2.4.8/Source/JavaScriptCore/heap/HeapStatistics.cpp:135:1: warning: 'noreturn' function does return
> +
> +        Call exit(-1) to indicate that no, there is no returning from here.

It is unrelated to this patch.

> Source/JavaScriptCore/ChangeLog:16
> +        * heap/HeapStatistics.cpp:
> +        (JSC::HeapStatistics::exitWithFailure):

ditto

> Source/WTF/wtf/Atomics.h:263
> -#if !OS(WINDOWS) && (CPU(X86) || CPU(X86_64))
> +#if (!OS(WINDOWS) || COMPILER(GCC)) && (CPU(X86) || CPU(X86_64))

I think OS(WINDOWS) is very confusing and isn't good at all,
COMPILER(GCC) && (CPU(X86) || CPU(X86_64)) would be better.
Comment 3 LRN 2015-04-15 09:39:46 PDT
Created attachment 250797 [details]
Allow MinGW-GCC to use GCC asm code for weakCompareAndSwap

OS(WINDOWS) != COMPILER(MSVC)
Use COMPILER(GCC) as suggested.
Comment 4 Csaba Osztrogonác 2015-04-15 09:47:30 PDT
Comment on attachment 250797 [details]
Allow MinGW-GCC to use GCC asm code for weakCompareAndSwap

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

> Source/WTF/ChangeLog:12
> +        Allow MinGW-GCC to use GCC asm code for weakCompareAndSwap
> +
> +        OS(WINDOWS) != COMPILER(MSVC)
> +        Use COMPILER(GCC) as suggested.
> +
> +        [W32] weakCompareAndSwap assembler code is not used when building with MinGW GCC
> +        https://bugs.webkit.org/show_bug.cgi?id=143754
> +
> +        Reviewed by NOBODY (OOPS!).
> +

[W32] weakCompareAndSwap assembler code is not used when building with MinGW GCC
https://bugs.webkit.org/show_bug.cgi?id=143754

Reviewed by NOBODY (OOPS!).

... everyting else ...
Comment 5 LRN 2015-04-23 08:34:26 PDT
Created attachment 251436 [details]
Allow MinGW-GCC to use GCC asm code for weakCompareAndSwap

This version does so for *all* variants of this functions,
not just the one that needs it to compile.
Also uses saner conditions and explains the ones that are not as sane.

The code compiles and runs with this, but i can't be sure that i was able
to test all the cases where this function is used.
Comment 6 Carlos Garcia Campos 2015-05-18 23:51:10 PDT
Committed to 2.4 http://trac.webkit.org/changeset/184552

I've committed this unreviewed to 2.4 since it's needed to fix the win32 build, but for trunk someone should review this.
Comment 7 Csaba Osztrogonác 2015-09-24 06:52:27 PDT
After https://trac.webkit.org/changeset/190103 we don't need assembly CAS anymore.
Comment 8 Csaba Osztrogonác 2015-11-17 02:34:55 PST
Comment on attachment 251436 [details]
Allow MinGW-GCC to use GCC asm code for weakCompareAndSwap

Cleared review? from attachment 251436 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).