Page MenuHomePhabricator

[Flang] Fix compilation on MinGW-w64
ClosedPublic

Authored by ChinouneMehdi on Jan 14 2021, 12:10 PM.

Details

Diff Detail

Event Timeline

ChinouneMehdi created this revision.Jan 14 2021, 12:10 PM
ChinouneMehdi requested review of this revision.Jan 14 2021, 12:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2021, 12:10 PM

What part of the code in the ifdefs is msvc-specific? On a first glance, it looks like regular win32 code that should work fine on mingw. What's the error that it tries to fix?

ChinouneMehdi added a comment.EditedJan 15 2021, 3:04 AM

What part of the code in the ifdefs is msvc-specific? On a first glance, it looks like regular win32 code that should work fine on mingw. What's the error that it tries to fix?

D:/dev/llvm-project/flang/runtime/file.cpp:48:57: error: use of undeclared identifier '_S_IREAD'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};
                                                        ^
D:/dev/llvm-project/flang/runtime/file.cpp:48:68: error: use of undeclared identifier '_S_IWRITE'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};
                                                                   ^
D:/dev/llvm-project/flang/runtime/file.cpp:404:9: warning: 'F_OK' macro redefined [-Wmacro-redefined]
#define F_OK 00
        ^
D:\Programs\msys64\mingw64\x86_64-w64-mingw32\include\io.h:182:9: note: previous definition is here
#define F_OK    0       /* Check for file existence */
        ^
D:/dev/llvm-project/flang/runtime/file.cpp:405:9: warning: 'W_OK' macro redefined [-Wmacro-redefined]
#define W_OK 02
        ^
D:\Programs\msys64\mingw64\x86_64-w64-mingw32\include\io.h:184:9: note: previous definition is here
#define W_OK    2       /* Check for write permission */
        ^
D:/dev/llvm-project/flang/runtime/file.cpp:406:9: warning: 'R_OK' macro redefined [-Wmacro-redefined]
#define R_OK 04
        ^
D:\Programs\msys64\mingw64\x86_64-w64-mingw32\include\io.h:185:9: note: previous definition is here
#define R_OK    4       /* Check for read permission */
        ^
4 warnings and 2 errors generated.

What part of the code in the ifdefs is msvc-specific? On a first glance, it looks like regular win32 code that should work fine on mingw. What's the error that it tries to fix?

D:/dev/llvm-project/flang/runtime/file.cpp:48:57: error: use of undeclared identifier '_S_IREAD'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};
                                                        ^
D:/dev/llvm-project/flang/runtime/file.cpp:48:68: error: use of undeclared identifier '_S_IWRITE'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};

The problem here is just that the right header for the _S_IREAD and _S_IWRITE constants haven't been included. Include sys/stat.h (which exists in both MSVC and mingw environments), i.e. move that include out of the ifdef/else, and this should work for mingw too.

What part of the code in the ifdefs is msvc-specific? On a first glance, it looks like regular win32 code that should work fine on mingw. What's the error that it tries to fix?

D:/dev/llvm-project/flang/runtime/file.cpp:48:57: error: use of undeclared identifier '_S_IREAD'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};
                                                        ^
D:/dev/llvm-project/flang/runtime/file.cpp:48:68: error: use of undeclared identifier '_S_IWRITE'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};

The problem here is just that the right header for the _S_IREAD and _S_IWRITE constants haven't been included. Include sys/stat.h (which exists in both MSVC and mingw environments), i.e. move that include out of the ifdef/else, and this should work for mingw too.

This is the easiest fix I could find.
You are free to make modifications on my patch.

What part of the code in the ifdefs is msvc-specific? On a first glance, it looks like regular win32 code that should work fine on mingw. What's the error that it tries to fix?

D:/dev/llvm-project/flang/runtime/file.cpp:48:57: error: use of undeclared identifier '_S_IREAD'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};
                                                        ^
D:/dev/llvm-project/flang/runtime/file.cpp:48:68: error: use of undeclared identifier '_S_IWRITE'
  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};

The problem here is just that the right header for the _S_IREAD and _S_IWRITE constants haven't been included. Include sys/stat.h (which exists in both MSVC and mingw environments), i.e. move that include out of the ifdef/else, and this should work for mingw too.

This is the easiest fix I could find.
You are free to make modifications on my patch.

I'd prefer if you update your patch in this fashion. I'm not a flang reviewer/approver, so someone else needs to ack it in the end anyway.

ChinouneMehdi added a reviewer: Restricted Project.Jan 20 2021, 12:10 PM
ChinouneMehdi added a subscriber: flang-commits.
DavidTruby resigned from this revision.Jan 20 2021, 12:27 PM

Ping

Before pinging, please update the patch with the change I suggested; don't change the ifdefs, but just move sys/stat.h out of the ifdef, above the #ifdef _WIN32.

LGTM, although I think maybe some the flang team should comment as well. (The change for isCrash wasn't there in the previous iteration of the patch, but that bit looks correct as well, and affects any windows compilation, not only mingw.)

Thank you for working on this @ChinouneMehdi !

The bit in flang/tools/flang-driver/driver.cpp is an obvious typo - thank you for fixing that! The other change (sys/stat.h) makes sense to me, but I haven't worked on Flang's runtime and I don't use Windows. I know that @Meinersbur worked on Windows support.

This revision is now accepted and ready to land.Feb 17 2021, 8:45 AM

Do you need again help to commit?

Do you need again help to commit?

Yes
Thanks.

This revision was landed with ongoing or failed builds.Feb 17 2021, 7:54 PM
This revision was automatically updated to reflect the committed changes.