[flang][Driver] Update link job on windows

[rovka]

Summary

When linking a Fortran program, we need to add the runtime libraries to
the command line. This is exactly what we do for Linux/Darwin, but the
interface is slightly different (e.g. -libpath instead of -L).

We also remove oldnames and libcmt, since they're not
needed at the moment and they bring in more dependencies.

We also pass /subsystem:console to the linker so it can figure out the
right entry point. This is only needed for MSVC's link.exe. For LLD it
is redundant but doesn't hurt.

Co-authored-by: Markus Mützel <[e-mail]>

[kiranchandramohan]

@rovka in case you missed this, the test is failing in windows.

[Meinersbur]

flang/test/Driver/flang-linker-flags-windows.f90

16 ↗

(On Diff #431623)

This is failing for me. Instead of lld-link.exe, it is using link.exe (the Microsoft linker).

Without the -### flag, the link command is failing:

flang-new version 15.0.0 (C:/Users/meinersbur/src/llvm-project/clang 09fdf5f6f5e70ecb7d95cdbb98442c998a55ce23)
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: c:\users\meinersbur\build\llvm-project\release\bin
 "c:\\users\\meinersbur\\build\\llvm-project\\release\\bin\\flang-new" -fc1 -triple x86_64-pc-windows-msvc19.32.31329 -emit-obj -o "C:\\Users\\MEINER~1\\AppData\\Local\\Temp\\hello-ae4145.o" "C:\\Users\\meinersbur\\src\\llvm-project\\flang\\test\\Driver/Inputs/hello.f90"
 "C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.32.31326\\bin\\Hostx64\\x64\\link.exe" -out:a.exe "-libpath:c:\\users\\meinersbur\\build\\llvm-project\\release\\lib" Fortran_main.lib FortranRuntime.lib FortranDecimal.lib -nologo "C:\\Users\\MEINER~1\\AppData\\Local\\Temp\\hello-ae4145.o"
LINK : fatal error LNK1561: entry point must be defined
flang-new: error: linker command failed with exit code 1561 (use -v to see invocation)

[rovka]

In D126291#3536718, @kiranchandramohan wrote:

@rovka in case you missed this, the test is failing in windows.

Yeah, thanks, I was out of office a bit so I couldn't get to it sooner. I'll give it a try without lld.

[rovka]

Updated test to check for link.exe instead of lld-link.exe. This doesn't look great but it works for both lld-link.exe and the default link.exe. I tried to use --ld-path=various/paths instead, but it seems to be ignored on Windows (I even get a warning about "argument unused during compilation"). If anyone has any ideas about how to make the test more robust, please let me know.

[rovka]

flang/test/Driver/flang-linker-flags-windows.f90

16 ↗

(On Diff #431623)

This might be because you're not passing any subsystem. Can you try with -Xlinker -subsystem:console?

[awarzynski]

Thanks for adding this, Diana! :)

Overall, makes sense to me. I have a couple of questions/points.

In D126291#3547023, @rovka wrote:

With this patch in place, we still need to add `-Xlinker -subsystem:console' in order to compile a simple 'Hello World' to run from the console.

Is this behavior consistent with "clang.exe"? Also, any clue why is this needed? Some documentation here. I've scanned it and am no wiser :/

I tried to use --ld-path=various/paths instead, but it seems to be ignored on Windows (I even get a warning about "argument unused during compilation"). If anyone has any ideas about how to make the test more robust, please let me know.

From what I can see, --ld-path is parsed inside ToolChain::GetLinkerPath. This method is used in Gnu.cpp, but not in MSVC.cpp. This is consistent with what you've observed, i.e. that --ld-path is ignored on Windows. I'm not aware of an alternative.

flang/test/Driver/linker-flags-windows.f90

6–7

[nit] Would you mind updating linker-flags.f90 so that the comments in both files are more consistent? In particular, this patch makes the following comment redundant:

If you are running this test on a yet another platform and it is failing for you, please either update the following or (preferably) update the linker invocation accordingly.

I added it there as a hint for people adding support for platforms inconsistent with Unix. Having a dedicated Unix and Windows tests is better :)

[rovka]

In D126291#3547142, @awarzynski wrote:

In D126291#3547023, @rovka wrote:

With this patch in place, we still need to add `-Xlinker -subsystem:console' in order to compile a simple 'Hello World' to run from the console.

Is this behavior consistent with "clang.exe"? Also, any clue why is this needed? Some documentation here. I've scanned it and am no wiser :/

I need to look into this some more. With clang it seems to detect that we're linking a main in and chooses the right susbsystem.

[rovka]

Updated comment in linker-flags.f90 test

[Meinersbur]

While the test passes now, I still get LINK : fatal error LNK1561: entry point must be defined when trying to actually link. Isn't it expected to not work yet?

Linking with lld-link.exe does work. objdump says there is a _QQmain symbol, why does lld consider this to be the entry point?

[mmuetzel]

Just wondering if those changes are correct for MinGW.

clang/lib/Driver/ToolChains/CommonArgs.cpp

753

Are those correct for MinGW? Should this be checking for TC.getTriple().isKnownWindowsMSVCEnvironment() instead?

774–777

same

[rovka]

In D126291#3550563, @Meinersbur wrote:

While the test passes now, I still get LINK : fatal error LNK1561: entry point must be defined when trying to actually link. Isn't it expected to not work yet?

Linking with lld-link.exe does work. objdump says there is a _QQmain symbol, why does lld consider this to be the entry point?

It doesn't - we're linking in a main as part of the Fortran_main.lib, which then calls _QQmain.

I don't really know why link.exe doesn't work. According to the docs, it should find main and choose the subsystem on its own. The only hunch I have is that maybe it doesn't work because main is in a library as opposed to one of the object files, but I'm still going through the docs trying to figure this out. I don't have a lot of experience with link.exe so I might be missing something obvious.

[rovka]

clang/lib/Driver/ToolChains/CommonArgs.cpp

753

Right, I hadn't considered MinGW. Thanks for pointing it out, I'll update the patch.

[Meinersbur]

In D126291#3552573, @rovka wrote://italic text//

I don't really know why link.exe doesn't work. According to the docs, it should find main and choose the subsystem on its own. The only hunch I have is that maybe it doesn't work because main is in a library as opposed to one of the object files, but I'm still going through the docs trying to figure this out. I don't have a lot of experience with link.exe so I might be missing something obvious.

It does work when passing /SUBSYSTEM:CONSOLE explicitly. Indeed maybe the autoselect only looks into symbols found in object file. Maybe pass /SUBSYSTEM:CONSOLE as well to the linker command? llvm-link should accept it as well. AFAIU there is no other subsystem Flang supports anyway. Even if, it requires changes to Fortran_main.lib.

[rovka]

• Update for MinGW.
• Add /subsystem:console to help link.exe understand what's going on.

Thanks for all the comments. I don't have a MinGW environment readily available for testing. @mmuetzel Could you test this? Alternatively, do you know whether we have any buildbots that would test this?

[mmuetzel]

In D126291#3555556, @rovka wrote:

• Update for MinGW.
• Add /subsystem:console to help link.exe understand what's going on.

Thanks for all the comments. I don't have a MinGW environment readily available for testing. @mmuetzel Could you test this? Alternatively, do you know whether we have any buildbots that would test this?

Sorry. I don't know how to compile flang in MinGW. (ISTR, I somewhere read that Windows isn't a supported host currently. Is this no longer the case?)
I also know nothing about the available buildbots.

If you could give some pointers to available instructions, I could potentially try to set up an MSYS2 environment for this.

[awarzynski]

In D126291#3555591, @mmuetzel wrote:

ISTR, I somewhere read that Windows isn't a supported host currently. Is this no longer the case?)

Windows is supported: https://lab.llvm.org/buildbot/#/builders/172 :)

If you could give some pointers to available instructions, I could potentially try to set up an MSYS2 environment for this.

The only thing that's different to other sub-projects is the value of LLVM_ENABLE_PROJECTS, i.e. use -DLLVM_ENABLE_PROJECTS=clang;mlir;flang;llvm when running CMake. Keep in mind that Flang is slow to build and consumes lots of memory, sadly.

If we can't test on MinGW, then we could also just add a note in the docs.

[mmuetzel]

I checked out the LLVM repository from https://github.com/llvm/llvm-project.git and applied your patch with patch -Np0 -i D126291.433988.patch.

After some failing attempts, I finally found a configuration for which building succeeded. I struggled with duplicate symbols and two many symbols when linking some libraries.
I ended up using these switches:

cmake \
  -Sllvm \
  -Bbuild \
  -GNinja \
  -DCMAKE_INSTALL_PREFIX=pkg \
  -DCMAKE_C_COMPILER=clang \
  -DCMAKE_CXX_COMPILER=clang++ \
  -DCMAKE_C_COMPILER_LAUNCHER=ccache \
  -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
  -DLLVM_ENABLE_PROJECTS="clang;mlir;flang;llvm" \
  -DLLVM_TARGETS_TO_BUILD="X86" \
  -DCMAKE_BUILD_TYPE=Release \
  -DLLVM_ENABLE_LIBCXX=ON \
  -DCLANG_DEFAULT_RTLIB=compiler-rt \
  -DCLANG_DEFAULT_UNWINDLIB=libunwind \
  -DCLANG_DEFAULT_LINKER=lld \
  -DLLVM_BUILD_LLVM_DYLIB=OFF \
  -DLLVM_BUILD_STATIC=OFF \
  -DLLVM_ENABLE_ASSERTIONS=OFF \
  -DLLVM_ENABLE_FFI=ON \
  -DLLVM_ENABLE_THREADS=ON \
  -DLLVM_INCLUDE_EXAMPLES=OFF \
  -DLLVM_INSTALL_UTILS=ON

I hope those make sense.

With those, flang-new was built in a CLANG64 build environment of MSYS2.
After installation, I got:

$ PATH=./pkg/bin:$PATH flang-new --version
flang-new version 15.0.0
Target: x86_64-w64-windows-gnu
Thread model: posix
InstalledDir: D:/llvm-project/pkg/bin

However, when trying to build a simple "Hello World" program with it, I got:

$ PATH=./pkg/bin:$PATH flang-new hello.f90 -L/clang64/lib
ld.lld: error: could not open 'D:/llvm-project/pkg/lib/clang/15.0.0/lib/windows/libclang_rt.builtins-x86_64.a': No such file or directory
ld.lld: error: could not open 'D:/llvm-project/pkg/lib/clang/15.0.0/lib/windows/libclang_rt.builtins-x86_64.a': No such file or directory
flang-new: error: linker command failed with exit code 1 (use -v to see invocation)

Those files don't exist indeed.
Is that expected? Did I do something wrong? Incorrect configuration? Do I also need to enable compiler-rt?

[rovka]

@mmuetzel Thanks for looking into this! I think since you're passing -DCLANG_DEFAULT_RTLIB=compiler-rt, you might indeed need to build compiler-rt (or at least the builtins part of it).

FYI, I'll be out of office until Wednesday, but I'll definitely check all the comments here when I get back (and also try to fix the CI failures). Thanks again for all the help.

[awarzynski]

Thanks for checking @mmuetzel !

In D126291#3555780, @mmuetzel wrote:

I ended up using these switches:

cmake \
  -Sllvm \
  -Bbuild \
  -GNinja \
  -DCMAKE_INSTALL_PREFIX=pkg \
  -DCMAKE_C_COMPILER=clang \
  -DCMAKE_CXX_COMPILER=clang++ \
  -DCMAKE_C_COMPILER_LAUNCHER=ccache \
  -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
  -DLLVM_ENABLE_PROJECTS="clang;mlir;flang;llvm" \
  -DLLVM_TARGETS_TO_BUILD="X86" \
  -DCMAKE_BUILD_TYPE=Release \
  -DLLVM_ENABLE_LIBCXX=ON \
  -DCLANG_DEFAULT_RTLIB=compiler-rt \
  -DCLANG_DEFAULT_UNWINDLIB=libunwind \
  -DCLANG_DEFAULT_LINKER=lld \
  -DLLVM_BUILD_LLVM_DYLIB=OFF \
  -DLLVM_BUILD_STATIC=OFF \
  -DLLVM_ENABLE_ASSERTIONS=OFF \
  -DLLVM_ENABLE_FFI=ON \
  -DLLVM_ENABLE_THREADS=ON \
  -DLLVM_INCLUDE_EXAMPLES=OFF \
  -DLLVM_INSTALL_UTILS=ON

That's actually quite complex. How about this (that's what I'd normally use):

cmake \
  -Sllvm \
  -Bbuild \
  -GNinja \
  -DLLVM_ENABLE_PROJECTS="clang;mlir;flang;llvm" \
  -DLLVM_TARGETS_TO_BUILD="X86" \
  -DCMAKE_BUILD_TYPE=Release \

Like @rovka pointed out, skipping CLANG_DEFAULT_RTLIB should solve your issue with missing libs, In general, most of the CMake options have "sane" defaults.

[mstorsjo]

In D126291#3555805, @awarzynski wrote:

Like @rovka pointed out, skipping CLANG_DEFAULT_RTLIB should solve your issue with missing libs

Skipping that woulnd't help/work in the clang64 environment in msys2; there's no libgcc there but it's expected to use compiler-rt in general.

The preexisting systemwide corresponding files do exist in /clang64/lib/clang/14.0.0, but your toolchain would want similar ones in <yourtoolchain>/lib/clang/15.0.0. For testing, just copy over the clang/<version>/lib directory to your testroot.

Adding compiler-rt to LLVM_ENABLE_RUNTIMES might fix it properly, but at least personally, I build compiler-rt in a separate step afterwards (and I think msys2 builds it in that way too).

[mmuetzel]

Thank you for the suggestions. In the meantime, I added -DLLVM_ENABLE_PROJECTS="clang;compiler-rt;mlir;flang;llvm" -DCOMPILER_RT_USE_BUILTINS_LIBRARY=ON to the compiler flags which seems to have advanced a bit further.
Now, I get the following when trying to compile a simple "Hello World":

$ PATH=./pkg/bin:$PATH flang-new hello.f90 -L/clang64/lib -v
flang-new version 15.0.0
Target: x86_64-w64-windows-gnu
Thread model: posix
InstalledDir: D:/llvm-project/pkg/bin
 "D:/llvm-project/pkg/bin/flang-new" -fc1 -triple x86_64-w64-windows-gnu -emit-obj -o C:/msys64/tmp/hello-076f00.o hello.f90
 "C:/msys64/clang64/bin/ld.lld.exe" -m i386pep -Bdynamic -o a.exe crt2.o crtbegin.o -LC:/msys64/clang64/lib -LD:/llvm-project/pkg/x86_64-w64-mingw32/lib -LD:/llvm-project/pkg/lib -LD:/llvm-project/pkg/x86_64-w64-mingw32/sys-root/mingw/lib -LD:/llvm-project/pkg/lib/clang/15.0.0/lib/windows C:/msys64/tmp/hello-076f00.o -lmingw32 D:/llvm-project/pkg/lib/clang/15.0.0/lib/windows/libclang_rt.builtins-x86_64.a -lunwind -lmoldname -lmingwex -lmsvcrt -ladvapi32 -lshell32 -luser32 -lkernel32 -lmingw32 D:/llvm-project/pkg/lib/clang/15.0.0/lib/windows/libclang_rt.builtins-x86_64.a -lunwind -lmoldname -lmingwex -lmsvcrt -lkernel32 crtend.o
ld.lld: error: undefined symbol: _FortranAioBeginExternalListOutput
>>> referenced by D:/llvm-project/./hello.f90:2
>>>               C:/msys64/tmp/hello-6d9e30.o:(_QQmain)

ld.lld: error: undefined symbol: _FortranAioOutputAscii
>>> referenced by D:/llvm-project/./hello.f90:2
>>>               C:/msys64/tmp/hello-6d9e30.o:(_QQmain)

ld.lld: error: undefined symbol: _FortranAioEndIoStatement
>>> referenced by D:/llvm-project/./hello.f90:2
>>>               C:/msys64/tmp/hello-6d9e30.o:(_QQmain)

ld.lld: error: undefined symbol: WinMain
>>> referenced by C:/M/mingw-w64-crt-git/src/mingw-w64/mingw-w64-crt/crt/crt0_c.c:18
>>>               libmingw32.a(lib64_libmingw32_a-crt0_c.o):(main)
flang-new: error: linker command failed with exit code 1 (use -v to see invocation)

Is this still a configuration error?

[awarzynski]

Nice!

In D126291#3555835, @mmuetzel wrote:

Is this still a configuration error?

No :) Clearly the following if block from tools::addFortranRuntimeLibs is not entered:

if (TC.getTriple().isKnownWindowsMSVCEnvironment()) {
    CmdArgs.push_back("Fortran_main.lib");
    CmdArgs.push_back("FortranRuntime.lib");
    CmdArgs.push_back("FortranDecimal.lib");
  }

The missing symbols reported by the linker are from thes Fortran runtime libraries listed above. These libs should be included in the linker invocation generated by flang-new. I'm assuming that this makes sense - given the definition of isKnownWindowsMSVCEnvironment. If you use isOSWindows instead then it should work, right?

[mmuetzel]

In D126291#3555848, @awarzynski wrote:

Nice!

In D126291#3555835, @mmuetzel wrote:

Is this still a configuration error?

No :) Clearly the following if block from tools::addFortranRuntimeLibs is not entered:

if (TC.getTriple().isKnownWindowsMSVCEnvironment()) {
    CmdArgs.push_back("Fortran_main.lib");
    CmdArgs.push_back("FortranRuntime.lib");
    CmdArgs.push_back("FortranDecimal.lib");
  }

The missing symbols reported by the linker are from thes Fortran runtime libraries listed above. These libs should be included in the linker invocation generated by flang-new. I'm assuming that this makes sense - given the definition of isKnownWindowsMSVCEnvironment. If you use isOSWindows instead then it should work, right?

These libraries are named like this in MinGW:

$ ls ./pkg/lib/*Fortran*
./pkg/lib/libFortran_main.a   ./pkg/lib/libFortranDecimal.a   ./pkg/lib/libFortranLower.a   ./pkg/lib/libFortranRuntime.a
./pkg/lib/libFortranCommon.a  ./pkg/lib/libFortranEvaluate.a  ./pkg/lib/libFortranParser.a  ./pkg/lib/libFortranSemantics.a

[mmuetzel]

I made this additional change:

From 26cee469225e80ac9bae22ebb6e60d47373fc19d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=BCtzel?= <[e-mail]>
Date: Fri, 3 Jun 2022 16:23:47 +0200
Subject: [PATCH] MinGW

---
 clang/lib/Driver/ToolChains/MinGW.cpp | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp b/clang/lib/Driver/ToolChains/MinGW.cpp
index ceeaa79bc202..eba8e9db82ea 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -138,6 +138,11 @@ void tools::MinGW::Linker::ConstructJob(Compilation &C, const JobAction &JA,
     llvm_unreachable("Unsupported target architecture.");
   }
 
+  if (C.getDriver().IsFlangMode()) {
+    tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+    tools::addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   Arg *SubsysArg =
       Args.getLastArg(options::OPT_mwindows, options::OPT_mconsole);
   if (SubsysArg && SubsysArg->getOption().matches(options::OPT_mwindows)) {
-- 
2.35.3.windows.1

Now the error message changed to the following:

$ PATH=./pkg/bin:$PATH flang-new hello.f90 -L/clang64/lib -v
flang-new version 15.0.0
Target: x86_64-w64-windows-gnu
Thread model: posix
InstalledDir: D:/llvm-project/pkg/bin
 "D:/llvm-project/pkg/bin/flang-new" -fc1 -triple x86_64-w64-windows-gnu -emit-obj -o C:/msys64/tmp/hello-f38fa1.o hello.f90
 "C:/msys64/clang64/bin/ld.lld.exe" -m i386pep -LD:/llvm-project/pkg/lib -lFortran_main -lFortranRuntime -lFortranDecimal -Bdynamic -o a.exe crt2.o crtbegin.o -LC:/msys64/clang64/lib -LD:/llvm-project/pkg/x86_64-w64-mingw32/lib -LD:/llvm-project/pkg/lib -LD:/llvm-project/pkg/x86_64-w64-mingw32/sys-root/mingw/lib -LD:/llvm-project/pkg/lib/clang/15.0.0/lib/windows C:/msys64/tmp/hello-f38fa1.o -lmingw32 D:/llvm-project/pkg/lib/clang/15.0.0/lib/windows/libclang_rt.builtins-x86_64.a -lunwind -lmoldname -lmingwex -lmsvcrt -ladvapi32 -lshell32 -luser32 -lkernel32 -lmingw32 D:/llvm-project/pkg/lib/clang/15.0.0/lib/windows/libclang_rt.builtins-x86_64.a -lunwind -lmoldname -lmingwex -lmsvcrt -lkernel32 crtend.o
ld.lld: error: undefined symbol: std::__1::mutex::lock()
>>> referenced by libFortranRuntime.a(io-api.cpp.obj):(Fortran::runtime::io::IoStatementState* Fortran::runtime::io::BeginExternalListIO<(Fortran::runtime::io::Direction)0, Fortran::runtime::io::ExternalListIoStatementState>(int, char const*, int))
>>> referenced by libFortranRuntime.a(io-api.cpp.obj):(Fortran::runtime::io::IoStatementState* Fortran::runtime::io::BeginExternalListIO<(Fortran::runtime::io::Direction)0, Fortran::runtime::io::ExternalListIoStatementState>(int, char const*, int))
>>> referenced by libFortranRuntime.a(io-api.cpp.obj):(Fortran::runtime::io::IoStatementState* Fortran::runtime::io::BeginExternalListIO<(Fortran::runtime::io::Direction)1, Fortran::runtime::io::ExternalListIoStatementState>(int, char const*, int))
>>> referenced 36 more times

ld.lld: error: undefined symbol: std::__1::mutex::unlock()
>>> referenced by libFortranRuntime.a(unit.cpp.obj):(Fortran::runtime::io::FlushOutputOnCrash(Fortran::runtime::Terminator const&))
>>> referenced by libFortranRuntime.a(unit.cpp.obj):(Fortran::runtime::io::ExternalFileUnit::LookUp(int))
>>> referenced by libFortranRuntime.a(unit.cpp.obj):(Fortran::runtime::io::ExternalFileUnit::GetUnitMap())
>>> referenced 23 more times
flang-new: error: linker command failed with exit code 1 (use -v to see invocation)

Not sure what next. 🤷‍♂️

[mmuetzel]

In case you don't receive notifications on edits. (Please forgive the noise if you do.)
After applying this additional patch (not the one in the previous comment):

From d74a276679778b943b1e2e50f5dd45f65c530252 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=BCtzel?= <[e-mail]>
Date: Fri, 3 Jun 2022 16:36:19 +0200
Subject: [PATCH] MinGW

---
 clang/lib/Driver/ToolChains/MinGW.cpp | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp b/clang/lib/Driver/ToolChains/MinGW.cpp
index ceeaa79bc202..9e2200467548 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -218,6 +218,11 @@ void tools::MinGW::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
+  if (C.getDriver().IsFlangMode()) {
+    tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+    tools::addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // TODO: Add profile stuff here
 
   if (TC.ShouldLinkCXXStdlib(Args)) {
-- 
2.35.3.windows.1

The Hello World program compiled and linked successfully with an additional -lc++ switch:

$ PATH=./pkg/bin:$PATH flang-new hello.f90 -L/clang64/lib -lc++
$ ./a.exe
 Hello, World!

[mmuetzel]

clang/lib/Driver/ToolChains/MSVC.cpp

140

Is the MSVC toolchain used anywhere else but on Windows?

[awarzynski]

In D126291#3555966, @mmuetzel wrote:

In case you don't receive notifications on edits. (Please forgive the noise if you do.)

That's not a problem at all!

After applying this additional patch (not the one in the previous comment):

Ah, so we were looking in the wrong toolchain file to begin with - good catch!

The Hello World program compiled and linked successfully

🎉

with an additional -lc++ switch

What error do you get if you skip -lc++? We need to make sure that Flang does no depend on the C++ runtime (it's one of the design requirements). I suspect that there's other C library that's required and that's pulled together with libc++.

Again - many thanks for checking all this!

clang/lib/Driver/ToolChains/MSVC.cpp

140

No: https://github.com/llvm/llvm-project/blob/5fee1799f4d8da59c251e2d04172fc2f387cbe54/llvm/include/llvm/ADT/Triple.h#L576-L578 ;-)

[mmuetzel]

The error message I get without -lc++:

ld.lld: error: undefined symbol: std::__1::mutex::lock()
>>> referenced by libFortranRuntime.a(io-api.cpp.obj):(Fortran::runtime::io::IoStatementState* Fortran::runtime::io::BeginExternalListIO<(Fortran::runtime::io::Direction)0, Fortran::runtime::io::ExternalListIoStatementState>(int, char const*, int))
>>> referenced by libFortranRuntime.a(io-api.cpp.obj):(Fortran::runtime::io::IoStatementState* Fortran::runtime::io::BeginExternalListIO<(Fortran::runtime::io::Direction)0, Fortran::runtime::io::ExternalListIoStatementState>(int, char const*, int))
>>> referenced by libFortranRuntime.a(io-api.cpp.obj):(Fortran::runtime::io::IoStatementState* Fortran::runtime::io::BeginExternalListIO<(Fortran::runtime::io::Direction)1, Fortran::runtime::io::ExternalListIoStatementState>(int, char const*, int))
>>> referenced 36 more times

ld.lld: error: undefined symbol: std::__1::mutex::unlock()
>>> referenced by libFortranRuntime.a(unit.cpp.obj):(Fortran::runtime::io::FlushOutputOnCrash(Fortran::runtime::Terminator const&))
>>> referenced by libFortranRuntime.a(unit.cpp.obj):(Fortran::runtime::io::ExternalFileUnit::LookUp(int))
>>> referenced by libFortranRuntime.a(unit.cpp.obj):(Fortran::runtime::io::ExternalFileUnit::GetUnitMap())
>>> referenced 23 more times
flang-new: error: linker command failed with exit code 1 (use -v to see invocation)

Edit: That's probably because of this:
https://github.com/llvm/llvm-project/blob/main/flang/runtime/lock.h

// Avoid <mutex> if possible to avoid introduction of C++ runtime
// library dependence.
#ifndef _WIN32
#define USE_PTHREADS 1
#else
#undef USE_PTHREADS
#endif

#if USE_PTHREADS
#include <pthread.h>
#else
#include <mutex>
#endif

So, there will be a C++ runtime library dependency on Windows.

clang/lib/Driver/ToolChains/MSVC.cpp

140

In that case, this argument could probably be added unconditionally.

[mstorsjo]

In D126291#3556096, @mmuetzel wrote:

The error message I get without -lc++:

ld.lld: error: undefined symbol: std::__1::mutex::lock()
>>> referenced by libFortranRuntime.a(io-api.cpp.obj):(Fortran::runtime::io::IoStatementState* Fortran::runtime::io::BeginExternalListIO<(Fortran::runtime::io::Direction)0, Fortran::runtime::io::ExternalListIoStatementState>(int, char const*, int))
>>> referenced by libFortranRuntime.a(io-api.cpp.obj):(Fortran::runtime::io::IoStatementState* Fortran::runtime::io::BeginExternalListIO<(Fortran::runtime::io::Direction)0, Fortran::runtime::io::ExternalListIoStatementState>(int, char const*, int))
>>> referenced by libFortranRuntime.a(io-api.cpp.obj):(Fortran::runtime::io::IoStatementState* Fortran::runtime::io::BeginExternalListIO<(Fortran::runtime::io::Direction)1, Fortran::runtime::io::ExternalListIoStatementState>(int, char const*, int))
>>> referenced 36 more times

ld.lld: error: undefined symbol: std::__1::mutex::unlock()
>>> referenced by libFortranRuntime.a(unit.cpp.obj):(Fortran::runtime::io::FlushOutputOnCrash(Fortran::runtime::Terminator const&))
>>> referenced by libFortranRuntime.a(unit.cpp.obj):(Fortran::runtime::io::ExternalFileUnit::LookUp(int))
>>> referenced by libFortranRuntime.a(unit.cpp.obj):(Fortran::runtime::io::ExternalFileUnit::GetUnitMap())
>>> referenced 23 more times
flang-new: error: linker command failed with exit code 1 (use -v to see invocation)

Edit: That's probably because of this:
https://github.com/llvm/llvm-project/blob/main/flang/runtime/lock.h

// Avoid <mutex> if possible to avoid introduction of C++ runtime
// library dependence.
#ifndef _WIN32
#define USE_PTHREADS 1
#else
#undef USE_PTHREADS
#endif

#if USE_PTHREADS
#include <pthread.h>
#else
#include <mutex>
#endif

So, there will be a C++ runtime library dependency on Windows.

If the only need is for mutexes, using a corresponding native windows api is quite straightforward. (If other synchronization primitives are needed, it may require a bit more effort.)

[mmuetzel]

With this additional change, I no longer need the -lc++ flag:

From 965343d8b05bf3cf7a9a3873ea4d2ddcc00a3703 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=BCtzel?= <[e-mail]>
Date: Fri, 3 Jun 2022 19:27:27 +0200
Subject: [PATCH] lock without <mutex> on Windows

---
 flang/runtime/lock.h | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/flang/runtime/lock.h b/flang/runtime/lock.h
index 0abc1158c2c1..b088bd6bb8e1 100644
--- a/flang/runtime/lock.h
+++ b/flang/runtime/lock.h
@@ -15,23 +15,17 @@
 
 // Avoid <mutex> if possible to avoid introduction of C++ runtime
 // library dependence.
-#ifndef _WIN32
-#define USE_PTHREADS 1
+#ifdef _WIN32
+#include <windows.h>
 #else
-#undef USE_PTHREADS
-#endif
-
-#if USE_PTHREADS
 #include <pthread.h>
-#else
-#include <mutex>
 #endif
 
 namespace Fortran::runtime {
 
 class Lock {
 public:
-#if USE_PTHREADS
+#ifndef _WIN32
   Lock() { pthread_mutex_init(&mutex_, nullptr); }
   ~Lock() { pthread_mutex_destroy(&mutex_); }
   void Take() {
@@ -41,9 +35,11 @@ public:
   bool Try() { return pthread_mutex_trylock(&mutex_) == 0; }
   void Drop() { pthread_mutex_unlock(&mutex_); }
 #else
-  void Take() { mutex_.lock(); }
-  bool Try() { return mutex_.try_lock(); }
-  void Drop() { mutex_.unlock(); }
+  Lock() { mutex_=CreateMutex(nullptr, FALSE, nullptr); }
+  ~Lock() { CloseHandle(mutex_); }
+  void Take() { WaitForSingleObject(mutex_, INFINITE); }
+  bool Try() { return WaitForSingleObject(mutex_, 0) == 0; }
+  void Drop() { ReleaseMutex(mutex_); }
 #endif
 
   void CheckLocked(const Terminator &terminator) {
@@ -54,10 +50,10 @@ public:
   }
 
 private:
-#if USE_PTHREADS
+#ifndef _WIN32
   pthread_mutex_t mutex_{};
 #else
-  std::mutex mutex_;
+  HANDLE mutex_;
 #endif
 };
 
-- 
2.35.3.windows.1

The previous logic was: Use <mutex> on Windows and pthread everywhere else. The new logic is: Use the WinAPI (CreateMutex and Co) on Windows and pthread everywhere else.
Someone should check if that change is sane. But it seems to work for me (with the "Hello World" example).

[awarzynski]

In D126291#3556324, @mmuetzel wrote:

With this additional change, I no longer need the -lc++ flag:

Great find, thanks for digging!

Someone should check if that change is sane. But it seems to work for me (with the "Hello World" example).

@rovka Does this still work for you? Also, we should probably split this into two changes.

[mstorsjo]

In D126291#3556324, @mmuetzel wrote:

With this additional change, I no longer need the -lc++ flag:

From 965343d8b05bf3cf7a9a3873ea4d2ddcc00a3703 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=BCtzel?= <[e-mail]>
Date: Fri, 3 Jun 2022 19:27:27 +0200
Subject: [PATCH] lock without <mutex> on Windows

---
 flang/runtime/lock.h | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/flang/runtime/lock.h b/flang/runtime/lock.h
index 0abc1158c2c1..b088bd6bb8e1 100644
--- a/flang/runtime/lock.h
+++ b/flang/runtime/lock.h
@@ -15,23 +15,17 @@
 
 // Avoid <mutex> if possible to avoid introduction of C++ runtime
 // library dependence.
-#ifndef _WIN32
-#define USE_PTHREADS 1
+#ifdef _WIN32
+#include <windows.h>
 #else
-#undef USE_PTHREADS
-#endif
-
-#if USE_PTHREADS
 #include <pthread.h>
-#else
-#include <mutex>
 #endif
 
 namespace Fortran::runtime {
 
 class Lock {
 public:
-#if USE_PTHREADS
+#ifndef _WIN32
   Lock() { pthread_mutex_init(&mutex_, nullptr); }
   ~Lock() { pthread_mutex_destroy(&mutex_); }
   void Take() {
@@ -41,9 +35,11 @@ public:
   bool Try() { return pthread_mutex_trylock(&mutex_) == 0; }
   void Drop() { pthread_mutex_unlock(&mutex_); }
 #else
-  void Take() { mutex_.lock(); }
-  bool Try() { return mutex_.try_lock(); }
-  void Drop() { mutex_.unlock(); }
+  Lock() { mutex_=CreateMutex(nullptr, FALSE, nullptr); }
+  ~Lock() { CloseHandle(mutex_); }
+  void Take() { WaitForSingleObject(mutex_, INFINITE); }
+  bool Try() { return WaitForSingleObject(mutex_, 0) == 0; }
+  void Drop() { ReleaseMutex(mutex_); }
 #endif
 
   void CheckLocked(const Terminator &terminator) {
@@ -54,10 +50,10 @@ public:
   }
 
 private:
-#if USE_PTHREADS
+#ifndef _WIN32
   pthread_mutex_t mutex_{};
 #else
-  std::mutex mutex_;
+  HANDLE mutex_;
 #endif
 };
 
-- 
2.35.3.windows.1

The previous logic was: Use <mutex> on Windows and pthread everywhere else. The new logic is: Use the WinAPI (CreateMutex and Co) on Windows and pthread everywhere else.
Someone should check if that change is sane. But it seems to work for me (with the "Hello World" example).

This looks mostly reasonable, but I'd recommend using a windows critical section instead of a mutex - a critical section doesn't invoke the kernel when there's no contention of the lock. For that, you'd use CRITICAL_SECTION, InitializeCriticalSection(&cs);, EnterCriticalSection(&cs);, LeaveCriticalSection(&cs);, DeleteCriticalSection(&cs);.

[mmuetzel]

In D126291#3556725, @mstorsjo wrote:

This looks mostly reasonable, but I'd recommend using a windows critical section instead of a mutex - a critical section doesn't invoke the kernel when there's no contention of the lock. For that, you'd use CRITICAL_SECTION, InitializeCriticalSection(&cs);, EnterCriticalSection(&cs);, LeaveCriticalSection(&cs);, DeleteCriticalSection(&cs);.

Thanks for the feedback.
Updated patch:

From 511c2f7695fe667486b5c07fb024f4badec6b23a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Markus=20M=C3=BCtzel?= <[e-mail]>
Date: Fri, 3 Jun 2022 21:59:02 +0200
Subject: [PATCH] lock without <mutex> on Windows

---
 flang/runtime/lock.h | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/flang/runtime/lock.h b/flang/runtime/lock.h
index 0abc1158c2c1..c3572c72d17d 100644
--- a/flang/runtime/lock.h
+++ b/flang/runtime/lock.h
@@ -15,23 +15,17 @@
 
 // Avoid <mutex> if possible to avoid introduction of C++ runtime
 // library dependence.
-#ifndef _WIN32
-#define USE_PTHREADS 1
+#ifdef _WIN32
+#include <windows.h>
 #else
-#undef USE_PTHREADS
-#endif
-
-#if USE_PTHREADS
 #include <pthread.h>
-#else
-#include <mutex>
 #endif
 
 namespace Fortran::runtime {
 
 class Lock {
 public:
-#if USE_PTHREADS
+#ifndef _WIN32
   Lock() { pthread_mutex_init(&mutex_, nullptr); }
   ~Lock() { pthread_mutex_destroy(&mutex_); }
   void Take() {
@@ -41,9 +35,11 @@ public:
   bool Try() { return pthread_mutex_trylock(&mutex_) == 0; }
   void Drop() { pthread_mutex_unlock(&mutex_); }
 #else
-  void Take() { mutex_.lock(); }
-  bool Try() { return mutex_.try_lock(); }
-  void Drop() { mutex_.unlock(); }
+  Lock() { InitializeCriticalSection(&cs_); }
+  ~Lock() { DeleteCriticalSection(&cs_); }
+  void Take() { EnterCriticalSection(&cs_); }
+  bool Try() { return TryEnterCriticalSection(&cs_); }
+  void Drop() { LeaveCriticalSection(&cs_); }
 #endif
 
   void CheckLocked(const Terminator &terminator) {
@@ -54,10 +50,10 @@ public:
   }
 
 private:
-#if USE_PTHREADS
+#ifndef _WIN32
   pthread_mutex_t mutex_{};
 #else
-  std::mutex mutex_;
+  CRITICAL_SECTION cs_;
 #endif
 };
 
-- 
2.35.3.windows.1

Also works for me with the simple Hello World on MinGW (MSYS2 CLANG64).

[Meinersbur]

I looked into Google Benchmark because it also has its main in a .lib library and how it handles it. Turns out it is using cmake which by default always adds /subsystem:console unless setting
WIN32_EXECUTABLE=ON.

I tested the updated patch again it is failing:

$ ":" "RUN: at line 16"
$ "c:\users\meinersbur\build\llvm-project\release\bin\flang-new.exe" "-###" "-flang-experimental-exec" "--ld-path=/usr/bin/ld" "C:\Users\meinersbur\src\llvm-project\flang\test\Driver/Inputs/hello.f90"
$ "c:\users\meinersbur\build\llvm-project\release\bin\filecheck.exe" "C:\Users\meinersbur\src\llvm-project\flang\test\Driver\linker-flags.f90"
# command stderr:
C:\Users\meinersbur\src\llvm-project\flang\test\Driver\linker-flags.f90:26:16: error: CHECK-LABEL: expected string not found in input
! CHECK-LABEL: "/usr/bin/ld"
               ^
<stdin>:7:136: note: scanning from here
 "c:\\users\\meinersbur\\build\\llvm-project\\release\\bin\\flang-new" "-fc1" "-triple" "x86_64-pc-windows-msvc19.32.31329" "-emit-obj" "-o" "C:\\Users\\MEINER~1\\AppData\\Local\\Temp\\lit-tmp-0rwi220l\\hello-bbdcc9.o" "C:\\Users\\meinersbur\\src\\llvm-project\\flang\\test\\Driver/Inputs/hello.f90"

Presumably the feature 'windows-msvc' is not registered. print(config.available_features) results in:

{'asserts', 'x86-registered-target', 'nvptx-registered-target', 'system-windows'}

In D126291#3555639, @awarzynski wrote:

In D126291#3555591, @mmuetzel wrote:

ISTR, I somewhere read that Windows isn't a supported host currently. Is this no longer the case?)

Windows is supported: https://lab.llvm.org/buildbot/#/builders/172 :)

The bot just compiles flang and runs the unit-test, none of which actually try to compile, link & run a Fortan program. Until recently the flang driver would not compile itself, but delegate it to gfortran anyway.

clang/lib/Driver/ToolChains/MSVC.cpp

140

How do you mean this? /subsystem:console is a link.exe flag, also supported by lld-link.exe sind it tries to be an msvc-compatible wrapper. It is not a valid flag for eg GNU ld even in msys.

[mmuetzel]

In D126291#3561854, @Meinersbur wrote:

In D126291#3555639, @awarzynski wrote:

In D126291#3555591, @mmuetzel wrote:

ISTR, I somewhere read that Windows isn't a supported host currently. Is this no longer the case?)

Windows is supported: https://lab.llvm.org/buildbot/#/builders/172 :)

The bot just compiles flang and runs the unit-test, none of which actually try to compile, link & run a Fortan program. Until recently the flang driver would not compile itself, but delegate it to gfortran anyway.

Found why I thought building on Windows wasn't supported:
https://github.com/llvm/llvm-project/blob/main/flang/README.md?plain=1#L153

The code does not compile with Windows and a compiler that does not have support for C++17.

But re-reading this again, I might have mis-read the "and" for an "or"...

clang/lib/Driver/ToolChains/MSVC.cpp

140

Sorry my questions weren't very clear. What I meant to ask:
The name of this file is ToolChains/MSVC.cpp. I was wondering whether that means that these settings are only relevant when using a MSVC (compatible) toolchain. If that is the case, checking for isKnownWindowsMSVCEnvironment() here might be redundant. At least, I can't find that being checked anywhere else in this file.

[mstorsjo]

In D126291#3562471, @mmuetzel wrote:

Found why I thought building on Windows wasn't supported:
https://github.com/llvm/llvm-project/blob/main/flang/README.md?plain=1#L153

The code does not compile with Windows and a compiler that does not have support for C++17.

But re-reading this again, I might have mis-read the "and" for an "or"...

Don't read too much into it - it used to be the case that flang wasn't tested/supported at all on Windows, but at this point I think this comment just is outdated/leftovr.

clang/lib/Driver/ToolChains/MSVC.cpp

140

Yes, you can assume isKnownWindowsMSVCEnvironment() within ToolChains/MSVC.cpp, so link.exe style linker options like /subsystem:console can be added unconditionally.

[awarzynski]

In D126291#3562542, @mstorsjo wrote:

In D126291#3562471, @mmuetzel wrote:

Found why I thought building on Windows wasn't supported:
https://github.com/llvm/llvm-project/blob/main/flang/README.md?plain=1#L153

The code does not compile with Windows and a compiler that does not have support for C++17.

But re-reading this again, I might have mis-read the "and" for an "or"...

Don't read too much into it - it used to be the case that flang wasn't tested/supported at all on Windows, but at this point I think this comment just is outdated/leftovr.

That's correct - that note is out-of-date and I have just deleted it. There are also old release notes that are also incorrect :(

In D126291#3561854, @Meinersbur wrote:

The bot just compiles flang and runs the unit-test, none of which actually try to compile, link & run a Fortan program.

My point was that the level of support on Windows is in general on a par with other platforms. There are small differences (there always will be), but patches like this one aim to fix that. In particular, the level of testing coverage is very similar.

Until recently the flang driver would not compile itself, but delegate it to gfortran anyway.

The "delegation" is done by the flang-to-external-fc bash wrapper script for the driver rather than the driver itself (until recently, this script used to be called flang).

In D126291#3561854, @Meinersbur wrote:

I tested the updated patch again it is failing:

The test that is failing for you is not intended for Windows. @rovka , I think that that you meant ! UNSUPPORTED: system-windows rather than ! UNSUPPORTED: windows-msvc.

[rovka]

• Fixed test
• Unconditionally added the subsystem arg
• Incorporated the MinGW toolchain changes (Thanks again @mmuetzel, I'm adding you as co-author)

@awarzynski I agree that the other patch (replacing <mutex>) should be committed separately. @mmuetzel are you going to upload it to Phab? I gave it a try on my machine and I can't compile the runtime with it, but I'll try to fix it on my end and we can continue the discussion in a new revision.

[mmuetzel]

In D126291#3566692, @rovka wrote:

@mmuetzel are you going to upload it to Phab? I gave it a try on my machine and I can't compile the runtime with it, but I'll try to fix it on my end and we can continue the discussion in a new revision.

I can try. But I never worked with Phabricator before...

Edit: See D127316

[mmuetzel]

Should this be conditional on the command line flag -flang-experimental-exec for the time being (like for the GNU toolchain)?
See D122008

clang/lib/Driver/ToolChains/MSVC.cpp

133

The GNU toolchain has this conditional on Args.hasArg(options::OPT_flang_experimental_exec).
Should this require that command line flag on MSVC, too?

Same for the MinGW toolchain file.

[rovka]

clang/lib/Driver/ToolChains/MSVC.cpp

133

Right, that would be nice :D

[rovka]

Moved the check for -flang-experimental-exec into addFortranRuntimeLibraryPath, so it affects all the toolchains. @awarzynski does this look like a good idea?

[mmuetzel]

In D126291#3572777, @rovka wrote:

Moved the check for -flang-experimental-exec into addFortranRuntimeLibraryPath, so it affects all the toolchains. @awarzynski does this look like a good idea?

If moving that check to inside that function is ok, should the same check be added to addFortranRuntimeLibs, too?
Edit: And also retain that condition for any flags that are added in the respective part of the toolchain files that don't use any of these two functions?

clang/lib/Driver/ToolChains/MSVC.cpp

134–135

Nit: The other toolchain files (Gnu and Darwin) don't explicitly specify the tools namespace for these functions.
Is there a preferred coding style when it comes to this?

Same for the MinGW toolchain file.

clang/lib/Driver/ToolChains/MinGW.cpp

224 ↗

(On Diff #435842)

MinGW behaves similarly to GNU in many respects. The GNU toolchain file adds CmdArgs.push_back("-lm"); here as well.
I didn't need that for the simple "Hello World" program. But that didn't invoke any math functions...
Do we need to add that flag here, too?

flang/test/Driver/linker-flags-windows.f90

7

Since this test only applies to MSVC toolchains (not to MinGW), should the file be renamed to, e.g., linker-flags-msvc.f90?

Should there be a similar test for MinGW? Which REQUIRES would that need?

Could maybe look like this (pending the correct REQUIRES line):

! Verify that the Fortran runtime libraries are present in the linker
! invocation. These libraries are added on top of other standard runtime
! libraries that the Clang driver will include.

! NOTE: The additional linker flags tested here are currently specified in
! clang/lib/Driver/Toolchains/MinGW.cpp.
! REQUIRES: windows-mingw

!------------
! RUN COMMAND
!------------
! RUN: %flang -### -flang-experimental-exec %S/Inputs/hello.f90 2>&1 | FileCheck %s

!----------------
! EXPECTED OUTPUT
!----------------
! Compiler invocation to generate the object file
! CHECK-LABEL: {{.*}} "-emit-obj"
! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90

! Linker invocation to generate the executable
! CHECK-SAME: "[[object_file]]"
! CHECK-SAME: -lFortran_main
! CHECK-SAME: -lFortranRuntime
! CHECK-SAME: -lFortranDecimal
! CHECK-SAME: -lm

10

This will probably need to be changed like this

-! RUN: %flang -### %S/Inputs/hello.f90 2>&1 | FileCheck %s
+! RUN: %flang -### -flang-experimental-exec %S/Inputs/hello.f90 2>&1 | FileCheck %s

[mstorsjo]

clang/lib/Driver/ToolChains/MinGW.cpp

224 ↗

(On Diff #435842)

MinGW doesn't need a separate -lm. (A libm.a is provided for compatibility if the option is specified, but it's just a dummy empty library.)

[mmuetzel]

flang/test/Driver/linker-flags-windows.f90

8

It looks like this test is skipped on windows (MSVC):
https://buildkite.com/llvm-project/premerge-checks/builds/97054#01814ce3-d707-422b-b4da-245e9c9e01c3/6-14462

Is the REQUIRES correct?

[awarzynski]

In D126291#3572777, @rovka wrote:

Moved the check for -flang-experimental-exec into addFortranRuntimeLibraryPath, so it affects all the toolchains. @awarzynski does this look like a good idea?

OK with me! 👍🏻

[mmuetzel]

Not sure if this is the right place to add this. But maybe something like this could be used to add 'windows-msvc' and 'windows-gnu' features that could be used to run tests conditionally on Windows with MSVC or MinGW toolchains:

diff --git a/llvm/utils/lit/lit/llvm/config.py b/llvm/utils/lit/lit/llvm/config.py
index b65316128146..8b911997a876 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -134,6 +134,10 @@ class LLVMConfig(object):
                 features.add('target-aarch64')
             elif re.match(r'^arm.*', target_triple):
                 features.add('target-arm')
+            if re.match(r'.*-windows-msvc', target_triple):
+                features.add('windows-msvc')
+            elif re.match(r'.*-windows-gnu', target_triple):
+                features.add('windows-gnu')
 
         use_gmalloc = lit_config.params.get('use_gmalloc', None)
         if lit.util.pythonize_bool(use_gmalloc):`

[mstorsjo]

In D126291#3573137, @mmuetzel wrote:

Not sure if this is the right place to add this. But maybe something like this could be used to add 'windows-msvc' and 'windows-gnu' features that could be used to run tests conditionally on Windows with MSVC or MinGW toolchains:

diff --git a/llvm/utils/lit/lit/llvm/config.py b/llvm/utils/lit/lit/llvm/config.py
index b65316128146..8b911997a876 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -134,6 +134,10 @@ class LLVMConfig(object):
                 features.add('target-aarch64')
             elif re.match(r'^arm.*', target_triple):
                 features.add('target-arm')
+            if re.match(r'.*-windows-msvc', target_triple):
+                features.add('windows-msvc')
+            elif re.match(r'.*-windows-gnu', target_triple):
+                features.add('windows-gnu')
 
         use_gmalloc = lit_config.params.get('use_gmalloc', None)
         if lit.util.pythonize_bool(use_gmalloc):`

FWIW, something very similar was added just a couple days ago in an lldb specific lit.cfg.py: https://reviews.llvm.org/D127048#change-KJ7QgKPHtN1S

[mmuetzel]

In D126291#3573607, @mstorsjo wrote:

FWIW, something very similar was added just a couple days ago in an lldb specific lit.cfg.py: https://reviews.llvm.org/D127048#change-KJ7QgKPHtN1S

Thank you for pointing this out.
Those feature definitions for the lldb subproject probably stem from around here:
https://github.com/llvm/llvm-project/blob/main/lldb/test/Shell/lit.cfg.py#L65

Instead of every subproject adding their own conditions for similar features, would it make sense to move these definitions to somewhere where all subprojects could use those same lit features? Would that be in /llvm/utils/lit/lit/llvm/config.py?

[mstorsjo]

In D126291#3573742, @mmuetzel wrote:

In D126291#3573607, @mstorsjo wrote:

FWIW, something very similar was added just a couple days ago in an lldb specific lit.cfg.py: https://reviews.llvm.org/D127048#change-KJ7QgKPHtN1S

Thank you for pointing this out.
Those feature definitions for the lldb subproject probably stem from around here:
https://github.com/llvm/llvm-project/blob/main/lldb/test/Shell/lit.cfg.py#L65

Instead of every subproject adding their own conditions for similar features, would it make sense to move these definitions to somewhere where all subprojects could use those same lit features? Would that be in /llvm/utils/lit/lit/llvm/config.py?

Maybe, but keep in mind that those kinds of tests should be the exception, not the rule.

As clang (and flang too, I would presume) generally can be cross compiling, one shouldn't imply that you need to be running on windows, to be able to test how the clang driver behaves when targeting windows. You can test that on any system, by passing -target x86_64-windows-msvc to the e.g. clang command in a lit test, so you can get test coverage for that aspect of the functionality regardless of what system you're running. Most tests in clang/test/Driver work that way.

[mmuetzel]

In D126291#3573751, @mstorsjo wrote:

Maybe, but keep in mind that those kinds of tests should be the exception, not the rule.

As clang (and flang too, I would presume) generally can be cross compiling, one shouldn't imply that you need to be running on windows, to be able to test how the clang driver behaves when targeting windows. You can test that on any system, by passing -target x86_64-windows-msvc to the e.g. clang command in a lit test, so you can get test coverage for that aspect of the functionality regardless of what system you're running. Most tests in clang/test/Driver work that way.

Ah ok. So instead of having ! REQUIRES: lines, those tests should add -target x86_64-windows-msvc to the flang invocation?
On platforms that didn't build the necessary libraries (e.g., a GNU system not explicitly configured to build a cross-compiler for that target), linking will likely fail. I don't know what FileCheck is doing. Can it cope with that?

[mstorsjo]

In D126291#3573937, @mmuetzel wrote:

Ah ok. So instead of having ! REQUIRES: lines, those tests should add -target x86_64-windows-msvc to the flang invocation?

Exactly

On platforms that didn't build the necessary libraries (e.g., a GNU system not explicitly configured to build a cross-compiler for that target), linking will likely fail. I don't know what FileCheck is doing. Can it cope with that?

These tests don't try to run the full link command - the -### parameter means that it only prints what subcommands it would have executed. So on any system, you can run the codepath in clang that just prints that it should execute link ... Fortran_main.lib, even if Fortran_main.lib doesn't actually exist. This is the basis of how all of LLVM's shell tests work - you don't need to be able to execute the full compiler toolchain, but you can verify that each individual step does what it's supposed to do.

[mmuetzel]

clang/lib/Driver/ToolChains/Darwin.cpp

645

If this condition is removed from the GNU, MSVC, and MinGW toolchain files, it should probably also be removed from here.

[mmuetzel]

Possible alternative version of linker-flags-windows.f90 that also tests MinGW and takes the feedback by @mstorsjo into account:

! Verify that the Fortran runtime libraries are present in the linker
! invocation. These libraries are added on top of other standard runtime
! libraries that the Clang driver will include.

! -----------
! target MSVC
! -----------

! NOTE: The additional linker flags tested here are currently specified in
! clang/lib/Driver/Toolchains/MSVC.cpp.

!------------
! RUN COMMAND
!------------
! RUN: %flang -### -target x86_64-windows-msvc -flang-experimental-exec %S/Inputs/hello.f90 2>&1 | FileCheck %s

!----------------
! EXPECTED OUTPUT
!----------------
! Compiler invocation to generate the object file
! CHECK-LABEL: {{.*}} "-emit-obj"
! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90

! Linker invocation to generate the executable
! NOTE: This check should also match if the default linker is lld-link.exe
! CHECK-LABEL: link.exe
! CHECK-NOT: libcmt
! CHECK-NOT: oldnames
! CHECK-SAME: Fortran_main.lib
! CHECK-SAME: FortranRuntime.lib
! CHECK-SAME: FortranDecimal.lib
! CHECK-SAME: /subsystem:console
! CHECK-SAME: "[[object_file]]"

! ------------
! target MinGW
! ------------

! NOTE: The additional linker flags tested here are currently specified in
! clang/lib/Driver/Toolchains/MinGW.cpp.

!------------
! RUN COMMAND
!------------
! RUN: %flang -### -target x86_64-windows-gnu -flang-experimental-exec %S/Inputs/hello.f90 2>&1 | FileCheck %s

!----------------
! EXPECTED OUTPUT
!----------------
! Compiler invocation to generate the object file
! CHECK-LABEL: {{.*}} "-emit-obj"
! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90

! Linker invocation to generate the executable
! CHECK-SAME: "[[object_file]]"
! CHECK-SAME: -lFortran_main
! CHECK-SAME: -lFortranRuntime
! CHECK-SAME: -lFortranDecimal

[mmuetzel]

flang/test/Driver/linker-flags-windows.f90

7

I don't know how to edit or retract inline comments.
My previous comments to this file are no longer valid should we follow @mstorsjo's suggestion on running these tests on all platforms.

[rovka]

I had the same idea about switching the tests to using target triples instead of having separate files for it, but initially I had some issues getting that to work properly. When specifying a triple, we need to provide an architecture. Leaving the triple as unkown-linux-gnu or just linux-gnu gives us an error along the lines of flang-new: error: unknown target triple 'unknown-unknown-linux-gnu', please use -triple or -arch. OTOH, hardcoding an architecture like x86 or aarch64 fails if we're not building that specific backend. We could do that and make the test REQUIRE the architecture that we're hardcoding, but this isn't really an architecture-specific test. So what I've finally done instead is to check for flang supported architectures and add a lit substitution for the first one that we find (be it aarch64, powerpc or x86) and use that in the test. We'll still get an error if someone tries to build the test without enabling any of these targets, but I think that's a good thing, since then people can decide either to add their architecture to lit or just, you know, not build flang on platforms where it isn't supported :) Patch incoming.

[rovka]

• Switch to the new style of testing (using triples rather than separate files)
• Fix oversight in Darwin toolchain file

I hope I've addressed all the comments and nothing slipped past during rebase. Thanks again for all the contributions :)

[rovka]

Missed a spot (removing the namespace in MinGW.cpp)

[mstorsjo]

In D126291#3577133, @rovka wrote:

I had the same idea about switching the tests to using target triples instead of having separate files for it, but initially I had some issues getting that to work properly. When specifying a triple, we need to provide an architecture. Leaving the triple as unkown-linux-gnu or just linux-gnu gives us an error along the lines of flang-new: error: unknown target triple 'unknown-unknown-linux-gnu', please use -triple or -arch. OTOH, hardcoding an architecture like x86 or aarch64 fails if we're not building that specific backend.

If you actually execute code generation, then yes, it fails if that specific arch isn't enabled. But for general compiler driver level tests, which just print out the command the would have executed (when running with -###), it should work without the actual code generation target being available. This is at least how it's done for clang's corresponding tests, most files in clang/test/Driver have hardcoded arch triples, without any REQUIRES lines or other exclusions.

[rovka]

In D126291#3572796, @mmuetzel wrote:

In D126291#3572777, @rovka wrote:

Moved the check for -flang-experimental-exec into addFortranRuntimeLibraryPath, so it affects all the toolchains. @awarzynski does this look like a good idea?

If moving that check to inside that function is ok, should the same check be added to addFortranRuntimeLibs, too?
Edit: And also retain that condition for any flags that are added in the respective part of the toolchain files that don't use any of these two functions?

I don't think we need to add it to the other function since we'll get an error anyway if the linker can't find the libraries. In any case, the right way to handle this is probably to error out in the driver before trying to compose a link job when -flang-experimental-exec is not specified, rather than rely on a specific linker error. But that should probably be a different patch. @awarzynski wdyt?

[rovka]

In D126291#3577159, @mstorsjo wrote:

In D126291#3577133, @rovka wrote:

I had the same idea about switching the tests to using target triples instead of having separate files for it, but initially I had some issues getting that to work properly. When specifying a triple, we need to provide an architecture. Leaving the triple as unkown-linux-gnu or just linux-gnu gives us an error along the lines of flang-new: error: unknown target triple 'unknown-unknown-linux-gnu', please use -triple or -arch. OTOH, hardcoding an architecture like x86 or aarch64 fails if we're not building that specific backend.

If you actually execute code generation, then yes, it fails if that specific arch isn't enabled. But for general compiler driver level tests, which just print out the command the would have executed (when running with -###), it should work without the actual code generation target being available. This is at least how it's done for clang's corresponding tests, most files in clang/test/Driver have hardcoded arch triples, without any REQUIRES lines or other exclusions.

Oops, you're right, I was testing with a wrong x86 triple >.< I'll simplify the patch then.

[awarzynski]

In D126291#3577133, @rovka wrote:

I had the same idea about switching the tests to using target triples instead of having separate files for it, but initially I had some issues getting that to work properly.

Could you give it another go? I agree with @mstorsjo that ... it should just work when using -### :)

In D126291#3577160, @rovka wrote:

In D126291#3572796, @mmuetzel wrote:

In D126291#3572777, @rovka wrote:

Moved the check for -flang-experimental-exec into addFortranRuntimeLibraryPath, so it affects all the toolchains. @awarzynski does this look like a good idea?

If moving that check to inside that function is ok, should the same check be added to addFortranRuntimeLibs, too?
Edit: And also retain that condition for any flags that are added in the respective part of the toolchain files that don't use any of these two functions?

I don't think we need to add it to the other function since we'll get an error anyway if the linker can't find the libraries. In any case, the right way to handle this is probably to error out in the driver before trying to compose a link job when -flang-experimental-exec is not specified, rather than rely on a specific linker error. But that should probably be a different patch. @awarzynski wdyt?

IMHO, addFortranRuntimeLibraryPath should be adding Fortran runtime library path unconditionally (as the name suggests). Similarly, addFortranRuntimeLibs should be adding Fortran library libs unconditionally. But unfortunately, we need to deal with -flang-experimental-exec (I really wish we didn't). What you are suggesting here makes a lot of sense to me, but it is also worth keeping in mind that this flag is here only temporarily ;-) So, I'd mostly focus on making sure that removing it is easy (and that everything else is rock solid). We do need Args.hasArg(options::OPT_flang_experimental_exec) there somewhere - I'll be happy with whatever you decide!

[awarzynski]

🤔 linker-flags.f90 is failing on Windows in the pre-commit CI. Not sure why - seems fine on Debian.

flang/test/Driver/linker-flags.f90

37

Does -SAME makese sense here? As in, this makes sense to me:

! GNU: -lFortranDecimal
! GNU-SAME: -lm

and this:

! WITHLM: -lFortranDecimal
! WITHLM-SAME: -lm

but for ! WITHLM-SAME: -lm there's no previous match, is there?

42–43

Is it worth adding a comment to explain why to single these out?

[rovka]

I'm guessing the Windows precommit is failing because --ld-path is ignored on Windows, even if we use a Linux target? I have a fix that works on my Windows machine, coming right up.

flang/test/Driver/linker-flags.f90

37

It doesn't say that the previous match has to be with the same prefix, and it seems to work :)
But since it looks funny to at least one person, I'll update it to use the same prefix.

42–43

I guess it won't hurt

[rovka]

• Stop using --ld-path, since it seems to be ignored on Windows. Instead, just match any path that ends in ld (should work with ld, lld, gold). This isn't very robust, but I don't have any better ideas (other than actually support --ld-path everywhere, not sure how much work that would be).

[awarzynski]

flang/test/Driver/linker-flags.f90

39

I think that GNU in this case might be a bit misleading. These linker invocations are defined in almost completely independent toolchains: MachOTool, gnutools.

I't be inclined to try this instead:

! RUN: %flang -### -flang-experimental-exec -target ppc64le-linux-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,GNU
! RUN: %flang -### -flang-experimental-exec -target aarch64-apple-darwin %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,DARWIN
! RUN: %flang -### -flang-experimental-exec -target x86_64-windows-gnu %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MINGW
! RUN: %flang -### -flang-experimental-exec -target aarch64-windows-msvc %S/Inputs/hello.f90 2>&1 | FileCheck %s --check-prefixes=CHECK,MSVC

This will lead to more duplication, but would clarify things a bit.

[rovka]

Clarified test checks.

[mmuetzel]

flang/test/Driver/linker-flags.f90

61

Lines 50-51 seem to be duplicates of lines 44-45. Is this intentional?

[rovka]

flang/test/Driver/linker-flags.f90

61

Yes, I don't want those to appear either before or after the Fortran libs. I guess if we wanted to be pedantic we'd also check that they don't appear after the object_file, or between the libs and the subsystem, but that seems a bit much.

[awarzynski]

flang/test/Driver/linker-flags.f90

61

Based on the docs, I'd say that this would be the idiomatic way to do this:

! MSVC-LABEL:
! MSVC-NOT: 
! MSVC-SAME:

IIUC, the following would only be needed if there's a potential for libcmt or oldnames to appear on a separate line:

```lang=bash
! MSVC-LABEL:
! MSVC-NOT: 
! MSVC-SAME:
! MSVC-NOT:

But this wouldn't happen, right? (there's going to be only one linker invocation). Also, you could just use --implicit-check-not :)

[rovka]

flang/test/Driver/linker-flags.f90

61

Based on the docs, I'd say that this would be the idiomatic way to do this:

! MSVC-LABEL:
! MSVC-NOT: 
! MSVC-SAME:

Based on the same docs, I would say it shouldn't be enough to mention it just once. But that's just what I expect, the docs are completely unhelpful about the actual behaviour here. For instance, I would expect to be able to write

! MSVC-NOT: should-only-come-after-X
! MSVC-SAME: X

If the MSVC-NOT applies to the whole line, then lines with 'X should-come-after-X' get rejected, but imo they should be accepted (I'll admit I didn't actually verify this, and anyway the implicit-check-not makes the whole discussion moot).

IIUC, the following would only be needed if there's a potential for libcmt or oldnames to appear on a separate line:

```lang=bash
! MSVC-LABEL:
! MSVC-NOT: 
! MSVC-SAME:
! MSVC-NOT:

I agree that if there's no MSVC-SAME after the last MSVC-NOT, then the MSVC-NOT would apply to the following line.

But this wouldn't happen, right? (there's going to be only one linker invocation). Also, you could just use --implicit-check-not :)

Wooow 😍 I didn't know about that one, I'll definitely update the test to use it, thanks!

[rovka]

• Use --implicit-check-not <3

[awarzynski]

flang/test/Driver/linker-flags.f90

61

Based on the same docs, I would say it shouldn't be enough to mention it just once.

I'm re-rereading the docs and agree. Sounds like --implicit-check-not is the best option.

For instance, I would expect to be able to write

! MSVC-NOT: should-only-come-after-X
! MSVC-SAME: X

If the MSVC-NOT applies to the whole line, then lines with 'X should-come-after-X' get rejected

I think that it should and is accepted. Example below:

file.f90

! RUN: cat %S/test.f90 | FileCheck %s

! CHECK-LABEL: my-label
! CHECK-NOT: should-only-come-after-X
! CHECK-SAME: X
! CHECK-SAME: should-only-come-after-X

test.f90

my-label X should-only-come-after-X

I copied the above into the Driver test dir and run like this:

$ bin/llvm-lit -va ../../flang/test/Driver/file.f90
-- Testing: 1 tests, 1 workers --
PASS: Flang :: Driver/file.f90 (1 of 1)

Testing Time: 0.03s
  Passed: 1

Does this agree with your experiments?

#filecheck-is-confusing :)

[rovka]

flang/test/Driver/linker-flags.f90

61

Yep, that's what I was saying :)

[awarzynski]

LGTM, many thanks for this non trivial effort! :) I've left a few nits, feel free to ignore! @mstorsjo , are you also OK with this change?

[nit] "This is exactly what we do for Linux/Darwin, but the interface is slightly different (e.g. -libpath instead of -L)." -> Perhaps clarify what "interface" you have in mind (e.g. "Windows interface").

flang/test/Driver/linker-flags.f90

16

This is a nit.

[mstorsjo]

Looks good to me in general, one general question though.

clang/lib/Driver/ToolChains/CommonArgs.cpp

770

Don't you need to have the same check in addFortranRuntimeLibs above too? As both of these functions are called without checking the condition in the caller.

[rovka]

I'm going to go ahead and commit this on Monday if nobody raises any objections until then. Thanks again to everyone for all the help & feedback!

clang/lib/Driver/ToolChains/CommonArgs.cpp

770

I'd rather not, since that would require adding the ArgList as an input to addFortranRuntimeLibs. I don't think it's worth changing the interface just for the sake of something temporary. The whole point of checking the flag inside addFortranRuntimeLibraryPath is to make it really easy to remove the check later, without having to update all the callsites.

See also the previous discussion (I don't know how to get a link to a comment in Phab, so I just linked some text inside it).

[mstorsjo]

clang/lib/Driver/ToolChains/CommonArgs.cpp

770

Ok, fair enough then!

[amyk]

Hi,
I have come across a failure involving flang/test/Driver/linker-flags.f90 that occurs when the default linker is lld. I have opened an external issue for it here: https://github.com/llvm/llvm-project/issues/56955

clang/lib/Driver/ToolChains/CommonArgs.h

void addOpenMPRuntimeLibraryPath(const ToolChain &TC,
                                  llvm::opt::ArgStringList &CmdArgs);
 /// Returns true, if an OpenMP runtime has been added.
 bool addOpenMPRuntime(llvm::opt::ArgStringList &CmdArgs, const ToolChain &TC,
                       const llvm::opt::ArgList &Args,
                       bool ForceStaticHostRuntime = false,
                       bool IsOffloadingHost = false, bool GompNeedsRT = false);
 
 /// Adds Fortran runtime libraries to \p CmdArgs.
-void addFortranRuntimeLibs(llvm::opt::ArgStringList &CmdArgs);
+void addFortranRuntimeLibs(const ToolChain &TC,
+                           llvm::opt::ArgStringList &CmdArgs);
 
 /// Adds the path for the Fortran runtime libraries to \p CmdArgs.
 void addFortranRuntimeLibraryPath(const ToolChain &TC,
                                   const llvm::opt::ArgList &Args,
                                   llvm::opt::ArgStringList &CmdArgs);
 
 void addHIPRuntimeLibArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
                           llvm::opt::ArgStringList &CmdArgs);

clang/lib/Driver/ToolChains/CommonArgs.cpp

bool tools::addOpenMPRuntime(ArgStringList &CmdArgs, const ToolChain &TC,
 
   if (RTKind == Driver::OMPRT_OMP)
     addOpenMPRuntimeSpecificRPath(TC, Args, CmdArgs);
   addOpenMPRuntimeLibraryPath(TC, Args, CmdArgs);
 
   return true;
 }
 
-void tools::addFortranRuntimeLibs(llvm::opt::ArgStringList &CmdArgs) {
+void tools::addFortranRuntimeLibs(const ToolChain &TC,
+                                  llvm::opt::ArgStringList &CmdArgs) {
+  if (TC.getTriple().isOSWindows()) {
+    CmdArgs.push_back("Fortran_main.lib");
+    CmdArgs.push_back("FortranRuntime.lib");
+    CmdArgs.push_back("FortranDecimal.lib");
+  } else {
     CmdArgs.push_back("-lFortran_main");
     CmdArgs.push_back("-lFortranRuntime");
     CmdArgs.push_back("-lFortranDecimal");
   }
+}
 
 void tools::addFortranRuntimeLibraryPath(const ToolChain &TC,
                                          const llvm::opt::ArgList &Args,
                                          ArgStringList &CmdArgs) {
   // Default to the <driver-path>/../lib directory. This works fine on the
   // platforms that we have tested so far. We will probably have to re-fine
   // this in the future. In particular, on some platforms, we may need to use
   // lib64 instead of lib.
   SmallString<256> DefaultLibPath =
       llvm::sys::path::parent_path(TC.getDriver().Dir);
   llvm::sys::path::append(DefaultLibPath, "lib");
+  if (TC.getTriple().isOSWindows())
+    CmdArgs.push_back(Args.MakeArgString("-libpath:" + DefaultLibPath));
+  else
     CmdArgs.push_back(Args.MakeArgString("-L" + DefaultLibPath));
 }
 
 static void addSanitizerRuntime(const ToolChain &TC, const ArgList &Args,
                                 ArgStringList &CmdArgs, StringRef Sanitizer,
                                 bool IsShared, bool IsWhole) {
   // Wrap any static runtimes that must be forced into executable in
   // whole-archive.
   if (IsWhole) CmdArgs.push_back("--whole-archive");

clang/lib/Driver/ToolChains/Darwin.cpp

void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   // Additional linker set-up and flags for Fortran. This is required in order
   // to generate executables.
   //
   // NOTE: Generating executables by Flang is considered an "experimental"
   // feature and hence this is guarded with a command line option.
   // TODO: Make this work unconditionally once Flang is mature enough.
   if (getToolChain().getDriver().IsFlangMode() &&
       Args.hasArg(options::OPT_flang_experimental_exec)) {
     addFortranRuntimeLibraryPath(getToolChain(), Args, CmdArgs);
-    addFortranRuntimeLibs(CmdArgs);
+    addFortranRuntimeLibs(getToolChain(), CmdArgs);
   }
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs))
     addOpenMPRuntime(CmdArgs, getToolChain(), Args);
 
   if (isObjCRuntimeLinked(Args) &&
       !Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
     // We use arclite library for both ARC and subscripting support.

clang/lib/Driver/ToolChains/Gnu.cpp

void tools::gnutools::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   // these dependencies need to be listed before the C runtime below (i.e.
   // AddRuntTimeLibs).
   //
   // NOTE: Generating executables by Flang is considered an "experimental"
   // feature and hence this is guarded with a command line option.
   // TODO: Make this work unconditionally once Flang is mature enough.
   if (D.IsFlangMode() && Args.hasArg(options::OPT_flang_experimental_exec)) {
     addFortranRuntimeLibraryPath(ToolChain, Args, CmdArgs);
-    addFortranRuntimeLibs(CmdArgs);
+    addFortranRuntimeLibs(ToolChain, CmdArgs);
     CmdArgs.push_back("-lm");
   }
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_r)) {
     if (!Args.hasArg(options::OPT_nodefaultlibs)) {
       if (IsStatic || IsStaticPIE)
         CmdArgs.push_back("--start-group");
 

clang/lib/Driver/ToolChains/MSVC.cpp

void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   auto &TC = static_cast<const toolchains::MSVCToolChain &>(getToolChain());
 
   assert((Output.isFilename() || Output.isNothing()) && "invalid output");
   if (Output.isFilename())
     CmdArgs.push_back(
         Args.MakeArgString(std::string("-out:") + Output.getFilename()));
 
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles) &&
-      !C.getDriver().IsCLMode()) {
+      !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) {
     CmdArgs.push_back("-defaultlib:libcmt");
     CmdArgs.push_back("-defaultlib:oldnames");
   }
 
   // If the VC environment hasn't been configured (perhaps because the user
   // did not run vcvarsall), try to build a consistent link environment.  If
   // the environment variable is set however, assume the user knows what
   // they're doing. If the user passes /vctoolsdir or /winsdkdir, trust that
if (TC.useUniversalCRT()) {
             Args.MakeArgString(Twine("-libpath:") + UniversalCRTLibPath));
     }
     std::string WindowsSdkLibPath;
     if (TC.getWindowsSDKLibraryPath(Args, WindowsSdkLibPath))
       CmdArgs.push_back(
           Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (C.getDriver().IsFlangMode()) {
+    tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+    tools::addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
   // Add the compiler-rt library directories to libpath if they exist to help
   // the linker find the various sanitizer, builtin, and profiling runtimes.
   for (const auto &LibPath : TC.getLibraryPaths()) {
     if (TC.getVFS().exists(LibPath))
       CmdArgs.push_back(Args.MakeArgString("-libpath:" + LibPath));
   }
   auto CRTPath = TC.getCompilerRTPath();
   if (TC.getVFS().exists(CRTPath))
     CmdArgs.push_back(Args.MakeArgString("-libpath:" + CRTPath));
 
   if (!C.getDriver().IsCLMode() && Args.hasArg(options::OPT_L))

flang/test/Driver/linker-flags-windows.f90

+! Verify that the Fortran runtime libraries are present in the linker
+! invocation. These libraries are added on top of other standard runtime
+! libraries that the Clang driver will include.
+
+! NOTE: The additional linker flags tested here are currently specified in
+! clang/lib/Driver/Toolchains/MSVC.cpp.
+! REQUIRES: system-windows
+
+! RUN: %flang -### %S/Inputs/hello.f90 2>&1 | FileCheck %s
+
+! Compiler invocation to generate the object file
+! CHECK-LABEL: {{.*}} "-emit-obj"
+! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90
+
+! Linker invocation to generate the executable
+! NOTE: This check should also match if the default linker is lld-link.exe
+! CHECK-LABEL: link.exe
+! CHECK-NOT: libcmt
+! CHECK-NOT: oldnames
+! CHECK-SAME: Fortran_main.lib
+! CHECK-SAME: FortranRuntime.lib
+! CHECK-SAME: FortranDecimal.lib
+! CHECK-SAME: "[[object_file]]"
+

flang/test/Driver/linker-flags.f90

 ! Verify that the Fortran runtime libraries are present in the linker
 ! invocation. These libraries are added on top of other standard runtime
 ! libraries that the Clang driver will include.
 
 ! NOTE: The additional linker flags tested here are currently only specified for
 ! GNU and Darwin. The following line will make sure that this test is skipped on
 ! Windows. If you are running this test on a yet another platform and it is
-! failing for you, please either update the following or (preferably) update the
-! linker invocation accordingly.
+! failing for you, please either update the following or (preferably)
+! create a similar test for your platform.
 ! UNSUPPORTED: system-windows
 
 !------------
 ! RUN COMMAND
 !------------
 ! Use `--ld-path` so that the linker location (used in the LABEL below) is deterministic.
 ! RUN: %flang -### -flang-experimental-exec --ld-path=/usr/bin/ld %S/Inputs/hello.f90 2>&1 | FileCheck %s
 
 !----------------
 ! EXPECTED OUTPUT
 !----------------
 ! Compiler invocation to generate the object file
 ! CHECK-LABEL: {{.*}} "-emit-obj"
 ! CHECK-SAME:  "-o" "[[object_file:.*]]" {{.*}}Inputs/hello.f90
 
 ! Linker invocation to generate the executable
 ! CHECK-LABEL:  "/usr/bin/ld"
 ! CHECK-SAME: "[[object_file]]"
 ! CHECK-SAME: -lFortran_main
 ! CHECK-SAME: -lFortranRuntime
 ! CHECK-SAME: -lFortranDecimal
 ! CHECK-SAME: -lm