Page MenuHomePhabricator

[flang][msvc] Fix compilation of RuntimeGtest tests.
ClosedPublic

Authored by MehdiChinoune on Apr 13 2021, 9:40 PM.

Diff Detail

Event Timeline

MehdiChinoune created this revision.Apr 13 2021, 9:40 PM
MehdiChinoune requested review of this revision.Apr 13 2021, 9:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2021, 9:40 PM

Thank you for submitting this @MehdiChinoune ! I'm a bit confused - what's specific about MSVC here and in the failure that you are seeing?

flang/unittests/RuntimeGTest/CMakeLists.txt
21 ↗(On Diff #337328)

This will basically add a new dependency for all configurations - how come this didn't fail before on other platforms? Also, there's a Flang buildbot worker that sets LLVM_LINK_LLVM_DYLIB to On: https://lab.llvm.org/buildbot/#/builders/135 and it's currently :green:.

Separately, Support is added as a dependency for unit tests here: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/AddLLVM.cmake#L1452.

So, is this change really needed?

flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31

Is this change required to fix the build?

MehdiChinoune added inline comments.Apr 14 2021, 2:03 AM
flang/unittests/RuntimeGTest/CMakeLists.txt
21 ↗(On Diff #337328)

I don't know but the compiler doesn't recognize llvm::errs() CrashHandlerFixture.cpp:19 Unless explicitly linked with Support component, I saw the same here https://github.com/llvm/llvm-project/blob/main/flang/unittests/Evaluate/CMakeLists.txt#L6

awarzynski added inline comments.Apr 14 2021, 2:33 AM
flang/unittests/RuntimeGTest/CMakeLists.txt
21 ↗(On Diff #337328)

The tests that you are referring to are not using GTest and use different CMake macros. The tests defined here use the add_flang_unittest macro, which calls LLVM's add_unittest. That should be adding Support as a dependency.

MehdiChinoune added inline comments.Apr 14 2021, 2:53 AM
flang/unittests/RuntimeGTest/CMakeLists.txt
21 ↗(On Diff #337328)

The tests that you are referring to are not using GTest and use different CMake macros. The tests defined here use the add_flang_unittest macro, which calls LLVM's add_unittest. That should be adding Support as a dependency.

I don't see any explicit linking to Support.

MehdiChinoune marked 2 inline comments as not done.

Update diff

MehdiChinoune added inline comments.Apr 14 2021, 3:24 AM
flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31

Is this change required to fix the build?

Yes

[2/34] Building CXX object tools\flang\unittests\RuntimeGTest\CMakeFiles\FlangRuntimeTests.dir\CrashHandlerFixture.cpp.obj
FAILED: tools/flang/unittests/RuntimeGTest/CMakeFiles/FlangRuntimeTests.dir/CrashHandlerFixture.cpp.obj
C:\PROGRA~2\MICROS~2\2019\COMMUN~1\VC\Tools\MSVC\1428~1.299\bin\Hostx64\x64\cl.exe  /nologo /TP -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_SILENCE_ALL_CXX17_DEPRECATION_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\flang\unittests\RuntimeGTest -ID:\dev\llvm-project\flang\unittests\RuntimeGTest -ID:\dev\llvm-project\flang\include -Itools\flang\include -Iinclude -ID:\dev\llvm-project\llvm\include -ID:\dev\llvm-project\llvm\utils\unittest\googletest\include -ID:\dev\llvm-project\llvm\utils\unittest\googlemock\include -ID:\dev\llvm-project\llvm\..\mlir\include -Itools\mlir\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2  /EHs-c- /GR- -UNDEBUG -std:c++17 /showIncludes /Fotools\flang\unittests\RuntimeGTest\CMakeFiles\FlangRuntimeTests.dir\CrashHandlerFixture.cpp.obj /Fdtools\flang\unittests\RuntimeGTest\CMakeFiles\FlangRuntimeTests.dir\ /FS -c D:\dev\llvm-project\flang\unittests\RuntimeGTest\CrashHandlerFixture.cpp
D:\dev\llvm-project\flang\unittests\RuntimeGTest\CrashHandlerFixture.cpp(31): error C2065: 'not': undeclared identifier
D:\dev\llvm-project\flang\unittests\RuntimeGTest\CrashHandlerFixture.cpp(31): error C2146: syntax error: missing ')' before identifier 'isCrashHanlderRegistered'
D:\dev\llvm-project\flang\unittests\RuntimeGTest\CrashHandlerFixture.cpp(31): error C2059: syntax error: ')'
D:\dev\llvm-project\flang\unittests\RuntimeGTest\CrashHandlerFixture.cpp(32): error C2143: syntax error: missing ';' before 'Fortran::runtime::Terminator::RegisterCrashHandler'
awarzynski added inline comments.Apr 14 2021, 4:29 AM
flang/unittests/RuntimeGTest/CMakeLists.txt
21 ↗(On Diff #337328)

I've already linked this above:
https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/AddLLVM.cmake#L1452

list(APPEND LLVM_LINK_COMPONENTS Support) # gtest needs it for raw_ostream

Or am I misreading this?

MehdiChinoune added inline comments.Apr 14 2021, 4:39 AM
flang/unittests/RuntimeGTest/CMakeLists.txt
21 ↗(On Diff #337328)

I have updated the diff.

awarzynski added inline comments.Apr 14 2021, 6:58 AM
flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31

I'm not familiar with MSVC, but AFAIK, not is a fairly standard C++ keyword. It's not clear to my *why* this change is needed. I appreciate that it fixes things for you, but it feels a bit random.

Just to clarify, I have no objections to this change, but I would appreciate a bit more justification, ideally with a reference. Something that would clarify *why*. This could be a very short explanation in the commit message.

MehdiChinoune added inline comments.Apr 14 2021, 7:18 AM
flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31

but AFAIK, not is a fairly standard C++ keyword.

Reference please!

tskeith added inline comments.
flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31

MSVC requires /permissive- or /Za to get alternative operators, according to this: https://docs.microsoft.com/en-us/cpp/cpp/cpp-built-in-operators-precedence-and-associativity

That seems strange as they have been part of C++ for a long time, but I don't think we have any reason to use them anyway.

MehdiChinoune marked an inline comment as not done.Apr 14 2021, 7:49 AM
flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31

Here's a link on cppref, without any warning of not being nonstandard. I support the change especially if it fixes builds with MSVC, but it does seem strange that MSVC would not enable this by default.

Meinersbur added inline comments.Apr 15 2021, 12:57 PM
flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31

MSVC supports them when adding

#include <iso646.h>

https://www.cplusplus.com/reference/ciso646/

or when compiling with /Za (not recommended).

They part of C++ even without this header since day one, ie. MSVC is not standards compliant here.

However, in the interest of a common code style, we should prefer ! over not even if compatibility was not an issue.

ashermancinelli accepted this revision.Apr 15 2021, 1:33 PM
ashermancinelli added inline comments.
flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31

It seems like we can agree on not -> !. I wouldn't mind adding a line in flang/docs/C++style.md about alternative operator spellings.

This revision is now accepted and ready to land.Apr 15 2021, 1:33 PM
tskeith added inline comments.Apr 15 2021, 1:34 PM
flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31

Has anyone tried building with /permissive-? Not because we want to use alternative operators but just for better standards compliance.

Could someone push this change p, please.

Could someone push this change p, please.

Do you have an email I can use for the commit author?

Could someone push this change p, please.

Do you have an email I can use for the commit author?

mehdi.chinoune@hotmail.com
Thanks

This revision was automatically updated to reflect the committed changes.