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

[awarzynski]

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]

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]

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.

• add_flang_unittest: https://github.com/llvm/llvm-project/blob/main/flang/unittests/CMakeLists.txt#L4-L6
• add_unittestt: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/AddLLVM.cmake#L1437-L1478

[MehdiChinoune]

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.

• add_flang_unittest: https://github.com/llvm/llvm-project/blob/main/flang/unittests/CMakeLists.txt#L4-L6
• add_unittestt: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/AddLLVM.cmake#L1437-L1478

I don't see any explicit linking to Support.

[MehdiChinoune]

Update diff

[MehdiChinoune]

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]

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]

flang/unittests/RuntimeGTest/CMakeLists.txt

21 ↗

(On Diff #337328)

I have updated the diff.

[awarzynski]

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]

flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp

31

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

Reference please!

[tskeith]

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.

[ashermancinelli]

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]

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]

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.

[tskeith]

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.

[MehdiChinoune]

Could someone push this change p, please.

[ashermancinelli]

In D100442#2693549, @MehdiChinoune wrote:

Could someone push this change p, please.

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

[MehdiChinoune]

In D100442#2702575, @ashermancinelli wrote:

In D100442#2693549, @MehdiChinoune wrote:

Could someone push this change p, please.

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

[e-mail]
Thanks

[ashermancinelli]

Merged in commit 080d48f279e2.

flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp

llvm::errs()
       << (sourceFile ? sourceFile : "unknown source file") << '(' << sourceLine
       << "): " << buffer << '\n';
   std::exit(EXIT_FAILURE);
 }
 
 // Register the crash handler above when creating each unit test in this suite
 void CrashHandlerFixture::SetUp() {
   static bool isCrashHanlderRegistered{false};
-  if (not isCrashHanlderRegistered)
+  if (!isCrashHanlderRegistered)
     Fortran::runtime::Terminator::RegisterCrashHandler(CatchCrash);
   isCrashHanlderRegistered = true;
 }