Bug 40909 - stat produces incorrect result with w64-mingw under -O2
Summary: stat produces incorrect result with w64-mingw under -O2
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 4.5.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-30 07:21 UTC by Paul Pluzhnikov
Modified: 2009-07-31 18:16 UTC (History)
3 users (show)

See Also:
Host: x86_64-w64-mingw32
Target: x86_64-w64-mingw32
Build: i686-pc-cygwin
Known to work:
Known to fail:
Last reconfirmed:


Attachments
pre-processed t.c (9.94 KB, text/plain)
2009-07-30 14:03 UTC, Paul Pluzhnikov
Details
[ __attribute__((optimize("0"))) difference ] (553 bytes, text/plain)
2009-07-31 18:07 UTC, Ozkan Sezer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Pluzhnikov 2009-07-30 07:21:41 UTC
GDB compiled with
  x86_64-w64-mingw32-gcc (GCC) 4.5.0 20090726 (experimental)
doesn't work (refuses to load symbols for any executable).

This is happening because is_regular_file in gdb/source.c appears to be mis-optimized (disabling optimization for that one file produces a working GDB).

The source reads:
/* Return True if the file NAME exists and is a regular file */
static int
is_regular_file (const char *name)
{
  struct stat st;
  const int status = stat (name, &st);

  /* Stat should never fail except when the file does not exist.
     If stat fails, analyze the source of error and return True
     unless the file does not exist, to avoid returning false results
     on obscure systems where stat does not work as expected.
   */
  if (status != 0)
    return (errno != ENOENT);

  return S_ISREG (st.st_mode);
}

Oprimized code is:

00000000000005d0 <_is_regular_file>:
     5d0:       48 81 ec 98 00 00 00    sub    $0x98,%rsp
     5d7:       48 8d 54 24 20          lea    0x20(%rsp),%rdx
     5dc:       ff 15 00 00 00 00       callq  *0x0(%rip)        # 5e2 <_is_regular_file+0x12>
                        5de: R_X86_64_PC32      __imp___stat64
     5e2:       85 c0                   test   %eax,%eax
     5e4:       75 1d                   jne    603 <_is_regular_file+0x33>
     5e6:       0f b7 44 24 66          movzwl 0x66(%rsp),%eax
     5eb:       25 00 f0 00 00          and    $0xf000,%eax
     5f0:       3d 00 80 00 00          cmp    $0x8000,%eax
     5f5:       0f 94 c0                sete   %al
     5f8:       48 81 c4 98 00 00 00    add    $0x98,%rsp
     5ff:       0f b6 c0                movzbl %al,%eax
     602:       c3                      retq   
     603:       ff 15 00 00 00 00       callq  *0x0(%rip)        # 609 <_is_regular_file+0x39>
                        605: R_X86_64_PC32      __imp___errno
     609:       83 38 02                cmpl   $0x2,(%rax)
     60c:       0f 95 c0                setne  %al
     60f:       48 81 c4 98 00 00 00    add    $0x98,%rsp
     616:       0f b6 c0                movzbl %al,%eax
     619:       c3                      retq   


Without optimization:
0000000000000e89 <_is_regular_file>:
     e89:       55                      push   %rbp
     e8a:       48 89 e5                mov    %rsp,%rbp
     e8d:       48 83 ec 60             sub    $0x60,%rsp
     e91:       48 89 4d 10             mov    %rcx,0x10(%rbp)
     e95:       48 8d 45 c0             lea    -0x40(%rbp),%rax
     e99:       48 89 c2                mov    %rax,%rdx
     e9c:       48 8b 4d 10             mov    0x10(%rbp),%rcx
     ea0:       e8 00 00 00 00          callq  ea5 <_is_regular_file+0x1c>
                        ea1: R_X86_64_PC32      _stat
     ea5:       89 45 fc                mov    %eax,-0x4(%rbp)
     ea8:       83 7d fc 00             cmpl   $0x0,-0x4(%rbp)
     eac:       74 16                   je     ec4 <_is_regular_file+0x3b>
     eae:       48 8b 05 00 00 00 00    mov    0x0(%rip),%rax        # eb5 <_is_regular_file+0x2c>
                        eb1: R_X86_64_PC32      __imp___errno
     eb5:       ff d0                   callq  *%rax
     eb7:       8b 00                   mov    (%rax),%eax
     eb9:       83 f8 02                cmp    $0x2,%eax
     ebc:       0f 95 c0                setne  %al
     ebf:       0f b6 c0                movzbl %al,%eax
     ec2:       eb 17                   jmp    edb <_is_regular_file+0x52>
     ec4:       0f b7 45 c6             movzwl -0x3a(%rbp),%eax
     ec8:       0f b7 c0                movzwl %ax,%eax
     ecb:       25 00 f0 00 00          and    $0xf000,%eax
     ed0:       3d 00 80 00 00          cmp    $0x8000,%eax
     ed5:       0f 94 c0                sete   %al
     ed8:       0f b6 c0                movzbl %al,%eax
     edb:       c9                      leaveq 
     edc:       c3                      retq   

It appears that unoptimized code calls _stat, which jumps to _stat64i32, which has this:

  extern __inline__ int __attribute__((__cdecl__)) _stat64i32(const char *_Name,struct _stat64i32 *_Stat)
  {
    struct _stat64 st;
    int ret=_stat64(_Name,&st);  // calls _imp___stat64
    _Stat->st_dev=st.st_dev;
    _Stat->st_ino=st.st_ino;
    _Stat->st_mode=st.st_mode;
    _Stat->st_nlink=st.st_nlink;
    _Stat->st_uid=st.st_uid;
    _Stat->st_gid=st.st_gid;
    _Stat->st_rdev=st.st_rdev;
    _Stat->st_size=(_off_t) st.st_size;
    _Stat->st_atime=st.st_atime;
    _Stat->st_mtime=st.st_mtime;
    _Stat->st_ctime=st.st_ctime;
    return ret;
  }

whereas the optimized code calls into _imp__stat64 directly and doesn't perform the _stat64 -> _stat64i32 transformation.

In the optimized case, immediately after _imp___stat64 returns:

(gdb) p/x st
$1 = {st_dev = 0x22a520, st_ino = 0x0, st_mode = 0x0, st_nlink = 0x4, 
  st_uid = 0x0, st_gid = 0x0, st_rdev = 0x68a4e5, st_size = 0x0, 
  st_atime = 0x7ff7fc35af9, st_mtime = 0x0, st_ctime = 0x1}

In the non-optimized case, immediately after _stat returns:

(gdb) p/x st
$1 = {st_dev = 0x2, st_ino = 0x0, st_mode = 0x81ff, st_nlink = 0x1, 
  st_uid = 0x0, st_gid = 0x0, st_rdev = 0x2, st_size = 0x12c9aa9, 
  st_atime = 0x4a713f85, st_mtime = 0x4a713f85, st_ctime = 0x4a713f83}

--- reproduces with t.c ---
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>

int main(int argc, char *argv[])
{
    struct stat st;
    if (0 == stat(argv[0], &st))
        printf("mode = %x\n", st.st_mode);
    return 0;
}
--- t.c ---

/usr/local/mingw-w64/bin/x86_64-w64-mingw32-gcc -g t.c -O2 && ./a.exe
mode = 0


/usr/local/mingw-w64/bin/x86_64-w64-mingw32-gcc -g t.c -O0 && ./a.exe
mode = 81ff
Comment 1 Kai Tietz 2009-07-30 08:38:55 UTC
Hmm, possibly this is a bug in our inline functions of mingw-w64. Does the switch -fno-strict-aliasing solves this issue, too?
We have here struct casts, which maybe are reasoning here strict aliasing issues.

Kai
Comment 2 Ozkan Sezer 2009-07-30 09:58:38 UTC
Hmm, with gcc-4.4.2 (branch rev. 150249), I always get "mode = 81ff" reported on the console with both -O0 and -O2 compiled exes from t.c test.  This is with mingw-w64 headers and crt revision 1101, the exes cross-compiled from an i686-linux host.  It ~may~ be a gcc-4.5 thing, but we don't know which revision of mingw-w64-crt and mingw-w64-headers the reporter used, either.
Comment 3 Richard Biener 2009-07-30 10:24:23 UTC
It indeed smells like a alias violation.  Preprocessed source would help here.
Comment 4 Paul Pluzhnikov 2009-07-30 14:03:07 UTC
Created attachment 18272 [details]
pre-processed t.c

Some answers:

The -fno-strict-aliasing does help:

/usr/local/mingw-w64/bin/x86_64-w64-mingw32-gcc -g t.c -O2  -fno-strict-aliasing && ./a.exe
mode = 81ff

> which revision of mingw-w64-crt and mingw-w64-headers the reporter used

For the record, I am just following up on GDB bug report from here:
http://sourceware.org/ml/gdb-patches/2009-07/msg00634.html

I did not build mingw-w64 myself, but used a snapshot build from here:
http://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win64/Automated%20Builds/mingw-w64-bin_i686-cygwin-1.5.25-15_20090726.tar.bz2/download
Comment 5 Richard Biener 2009-07-30 14:34:49 UTC
Yep:

extern __inline__ int __attribute__((__cdecl__)) stat(const char *_Filename,struct stat *_Stat) {
  return _stat64i32(_Filename,(struct _stat64i32 *)_Stat);
}

that isn't going to fly.

  struct stat {
    _dev_t st_dev;
    _ino_t st_ino;
    unsigned short st_mode;
    short st_nlink;
    short st_uid;
    short st_gid;
    _dev_t st_rdev;
    _off_t st_size;
    time_t st_atime;
    time_t st_mtime;
    time_t st_ctime;
  };

  struct _stat64i32 {
    _dev_t st_dev;
    _ino_t st_ino;
    unsigned short st_mode;
    short st_nlink;
    short st_uid;
    short st_gid;
    _dev_t st_rdev;
    _off_t st_size;
    __time64_t st_atime;
    __time64_t st_mtime;
    __time64_t st_ctime;
  };

I suggest you mark _stat32i64 with attribute((may_alias)).
Comment 6 Ozkan Sezer 2009-07-30 19:30:29 UTC
(In reply to comment #5)
> Yep:
> 
> extern __inline__ int __attribute__((__cdecl__)) stat(const char
> *_Filename,struct stat *_Stat) {
>   return _stat64i32(_Filename,(struct _stat64i32 *)_Stat);
> }
> 
> that isn't going to fly.
[...]
> I suggest you mark _stat32i64 with attribute((may_alias)).

Why does 4.4 not have any problem with this?
Comment 7 Ozkan Sezer 2009-07-31 06:30:38 UTC
Besides my question in comment #6, I wonder why is this actually considered an aliasing violation?  The only difference between struct stat and struct _stat64i32 is the time fields: _stat64i32 has __time64_t and stat has time_t which, in this particular case, is typedef'ed as __time64_t already..
Comment 8 Richard Biener 2009-07-31 09:48:56 UTC
With 4.4 you are probably simply lucky ;)  This is an aliasing violation because
C even considers

struct A { int i; };
struct B { int i; };

to be different.  If the indirection isn't needed why provide two structs
at all?
Comment 9 Ozkan Sezer 2009-07-31 09:59:15 UTC
Interesting that the problem occurs only with the inlined version: if the test object is linked with a non-inline version in a separate *.a file, the test behaves correctly..  BTW, neither 4.4 nor 4.5 complains even with -Wstrict-aliasing=2.
Comment 10 Paul Pluzhnikov 2009-07-31 15:56:55 UTC
Filed MingW bug here:
https://sourceforge.net/tracker/?func=detail&aid=2830386&group_id=2435&atid=102435
Comment 11 Ozkan Sezer 2009-07-31 16:02:44 UTC
(In reply to comment #10)
> Filed MingW bug here:
> https://sourceforge.net/tracker/?func=detail&aid=2830386&group_id=2435&atid=102435
> 

Wrong project tracker. Please go to https://sourceforge.net/tracker/?group_id=202880 for the mingw-w64 tracker.  BTW, I worked around the issue in our svn by adding optimize("0") attributes to those one-line inlines for now.  Use revision 1108 and it'll be fine.
Comment 12 Richard Biener 2009-07-31 16:42:23 UTC
noinline attributes would be better I think.
Comment 13 Ozkan Sezer 2009-07-31 16:46:22 UTC
(In reply to comment #12)
> noinline attributes would be better I think.
> 

noinline for the inline functions??
Comment 14 Richard Biener 2009-07-31 17:44:12 UTC
Yes, and of course remove the inline qualifier ;)

I have no idea what optimize(0) will do and why it should affect the bug
you are seeing (I guess it disallows inlining, which is why I think noinline
is a better choice here).
Comment 15 Ozkan Sezer 2009-07-31 18:07:58 UTC
Created attachment 18279 [details]
[ __attribute__((optimize("0"))) difference ]

Hmm, __attribute__((optimize("0"))) does seem to disable inlining and the code is linked to libmingwex.a version of non-inlined stat().  [Attached the diff of *.s files generated without and with __attribute__((optimize("0"))) added at -O3. ]  The thing is, the non-inlined code is the same and why does it not have the issue...
Comment 16 Ozkan Sezer 2009-07-31 18:16:51 UTC
(In reply to comment #15)
> Created an attachment (id=18279) [edit]
> [ __attribute__((optimize("0"))) difference ]
> 
> Hmm, __attribute__((optimize("0"))) does seem to disable inlining and the code
> is linked to libmingwex.a version of non-inlined stat().  [Attached the diff of
> *.s files generated without and with __attribute__((optimize("0"))) added at
> -O3. ]  The thing is, the non-inlined code is the same and why does it not have
> the issue...
> 

Nah.. the file is from using 4.4, but the disabling of inline shouldn't change from 4.4 to 4.5, I guess..