[flang-commits] [PATCH] D126291: [flang][Driver] Update link job on windows

[Markus Mützel via Phabricator via flang-commits]

mmuetzel added a comment.

In D126291#3572777 <https://reviews.llvm.org/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?



================
Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:134-135
+  if (C.getDriver().IsFlangMode()) {
+    tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
+    tools::addFortranRuntimeLibs(TC, CmdArgs);
+
----------------
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.


================
Comment at: clang/lib/Driver/ToolChains/MinGW.cpp:224
+    tools::addFortranRuntimeLibs(TC, CmdArgs);
+  }
+
----------------
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?


================
Comment at: flang/test/Driver/linker-flags-windows.f90:6
+! NOTE: The additional linker flags tested here are currently specified in
+! clang/lib/Driver/Toolchains/MSVC.cpp.
+! REQUIRES: windows-msvc
----------------
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
```



================
Comment at: flang/test/Driver/linker-flags-windows.f90:9
+
+! RUN: %flang -### %S/Inputs/hello.f90 2>&1 | FileCheck %s
+
----------------
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
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126291/new/

https://reviews.llvm.org/D126291