672035 - Use GCC atomic builtins when building with MinGW with -march=i486 or higher

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect)

Description

[nospam.kotarou.dono]

Attachment: For MinGW

This patch makes NSPR use GCC atomic builtins when building with MinGW with -march=i486 or higher. It also modifies the formatting of the block of define tests so it's more readable

Comment 1

[nospam.kotarou.dono]

Attachment: Fallback on Win32 API

Made it fallback on the win32 api instead of the manual version when __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 on MinGW

Comment 2

[Jacek Caban]

Why do you want that change? Interlocked* APIs should work equally well and I think it's better to keep mingw setting as near to MSC as possible.

Comment 3

[nospam.kotarou.dono]

The Interlocked* API has the extra overhead pushing the arguments on to the stack and a call, while the __sync_* functions translate directly into the correct instructions. It's basically making the Interlocked* functions an intrinsic in MSVC terms

Comment 4

[Jacek Caban]

Interlocked* functions are defined as inline functions in headers, containing nothing but inline assembly, so I'd expect them to be inlined and thus behave like intrinsic as well.

Comment 5

[nospam.kotarou.dono]

A brief test says otherwise (I've removed the irrelevant startup code)

% echo "#include <windows.h>
int main() { int a = 0; InterlockedIncrement(&a); return 0; }
" | gcc -O3 -fomit-frame-pointer -xc - -S -o out.S
<stdin>: In function 'main':
<stdin>:2:1: warning: passing argument 1 of 'InterlockedIncrement' from incompatible pointer type
c:\mingw\bin\../lib/gcc/mingw32/4.5.2/../../../../include/winbase.h:1857:13: note: expected 'volatile LONG *' but argument is of type 'int *'

% cat out.S
        ; Snipped
        movl    $0, -12(%ebp)
        leal    -12(%ebp), %eax
        movl    %eax, (%esp)
        call    _InterlockedIncrement@4
        pushl   %eax
        xorl    %eax, %eax
        movl    -4(%ebp), %ecx
        leave
        leal    -4(%ecx), %esp
        ret
        .def    _InterlockedIncrement@4;        .scl    2;      .type   32;     .endef

% echo "#include <windows.h>
int main() { int a = 0; __sync_fetch_and_add(&a, 1); return 0; }
" | gcc -O3 -fomit-frame-pointer -xc - -S -o out.S

% cat out.S
        ; Snipped
        movl    $0, 12(%esp)
        leal    12(%esp), %eax
        lock incl       (%eax)
        xorl    %eax, %eax
        leave
        ret

Comment 6

[Jacek Caban]

That's a matter of fixing mingw. With a quick hack to (mingw-w64) winbase.h, I could get:

        movl    $1, %eax
        movl    $0, 12(%esp)
/APP
 # 1098 "/usr/local/lib/gcc/i686-w64-mingw32/4.6.1/../../../../i686-w64-mingw32/include/winbase.h" 1
        lock
        xaddl %eax,12(%esp)
 # 0 "" 2
/NO_APP
        xorl    %eax, %eax
        leave
        ret


with your test.

Comment 7

[nospam.kotarou.dono]

But I thought you said that hacks are bad? And IIRC, the people over at mingw stated that they were trying to keep as close to the official win32 api as possible

Comment 8

[Jacek Caban]

(In reply to comment #7)
> But I thought you said that hacks are bad?

They are, that's why I didn't send it to upstream then. By hack I meant a quick change showing the possibilities that may be turned into a proper patch. My patch is on its way to mingw-w64, BTW.

> And IIRC, the people over at mingw stated that they were trying to keep as close to the official win32
> api as possible

Sure, and it means that you shouldn't need to change the code for mingw, like you do, to get expected behavior (unless it's really impossible to do the other way).

Comment 9

[Jacek Caban]

The patch for mingw-w64 is in the tree (it changes only a few Interlocked* functions, more should be fixed the same way):

http://repo.or.cz/w/mingw-w64/jacek.git/commitdiff/5a22dc1567c80b12b46f72e14637acc195165f8e

There is nothing to fix on Mozilla now, IMO.

Comment 10

[Wan-Teh Chang]

Thank you for the patch and investigation.  I marked the bug WONTFIX.

Comment 11

[nospam.kotarou.dono]

Small problem though - Mozilla declares it's own _Interlocked* and then applies an intrinsic pragma to them. Does that patch still cover that?

Comment 12

[Jacek Caban]

In practice, almost all Windows code includes base headers like winnt.h. Even if some single files like that exist, the cost of function call is quite low price.