Page MenuHomePhabricator

[flang][Driver] Update link job on windows
ClosedPublic

Authored by rovka on May 24 2022, 4:38 AM.

Details

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 <markus.muetzel@gmx.de>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.

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?= <markus.muetzel@gmx.de>
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);.

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?= <markus.muetzel@gmx.de>
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).

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'}

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.

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.

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.

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 :(

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).

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 updated this revision to Diff 435150.Wed, Jun 8, 7:26 AM
rovka edited the summary of this revision. (Show Details)
  • 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 added a comment.EditedWed, Jun 8, 8:45 AM

@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

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 added inline comments.Fri, Jun 10, 12:54 AM
clang/lib/Driver/ToolChains/MSVC.cpp
133

Right, that would be nice :D

rovka updated this revision to Diff 435842.Fri, Jun 10, 2:11 AM

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

mmuetzel added a comment.EditedFri, Jun 10, 2:33 AM

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

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
6 ↗(On Diff #435842)

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
9 ↗(On Diff #435842)

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 added inline comments.Fri, Jun 10, 2:42 AM
clang/lib/Driver/ToolChains/MinGW.cpp
224

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 added inline comments.Fri, Jun 10, 2:54 AM
flang/test/Driver/linker-flags-windows.f90
7 ↗(On Diff #435842)

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?

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! 👍🏻

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):`

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

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?

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.

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?

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 added inline comments.Fri, Jun 10, 10:56 AM
clang/lib/Driver/ToolChains/Darwin.cpp
640

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

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 added inline comments.Mon, Jun 13, 2:03 AM
flang/test/Driver/linker-flags-windows.f90
6 ↗(On Diff #435842)

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 added a comment.Mon, Jun 13, 2:07 AM

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 updated this revision to Diff 436300.Mon, Jun 13, 2:09 AM
  • 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 updated this revision to Diff 436301.Mon, Jun 13, 2:12 AM

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

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 added a comment.Mon, Jun 13, 2:18 AM

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 added a comment.Mon, Jun 13, 3:08 AM

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.

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 -### :)

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!

🤔 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
32

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?

37–38

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

rovka added a comment.Mon, Jun 13, 4:15 AM

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
32

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.

37–38

I guess it won't hurt

rovka updated this revision to Diff 436340.Mon, Jun 13, 4:26 AM
  • 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 added inline comments.Mon, Jun 13, 8:00 AM
flang/test/Driver/linker-flags.f90
34

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 updated this revision to Diff 436687.Tue, Jun 14, 1:01 AM

Clarified test checks.

mmuetzel added inline comments.Tue, Jun 14, 1:11 AM
flang/test/Driver/linker-flags.f90
56

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

rovka added inline comments.Tue, Jun 14, 1:21 AM
flang/test/Driver/linker-flags.f90
56

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 added inline comments.Tue, Jun 14, 2:29 AM
flang/test/Driver/linker-flags.f90
56

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 added inline comments.Tue, Jun 14, 3:05 AM
flang/test/Driver/linker-flags.f90
56

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!

kiranchandramohan resigned from this revision.Tue, Jun 14, 3:13 AM
rovka updated this revision to Diff 436722.EditedTue, Jun 14, 3:16 AM
  • Use --implicit-check-not <3
awarzynski added inline comments.Tue, Jun 14, 5:33 AM
flang/test/Driver/linker-flags.f90
56

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 added inline comments.Wed, Jun 15, 12:28 AM
flang/test/Driver/linker-flags.f90
56

Yep, that's what I was saying :)

awarzynski accepted this revision.Thu, Jun 16, 2:29 AM

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
12–16

This is a nit.

This revision is now accepted and ready to land.Thu, Jun 16, 2:29 AM

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

clang/lib/Driver/ToolChains/CommonArgs.cpp
761

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 added a comment.Fri, Jun 17, 1:05 AM

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
761

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 added inline comments.Fri, Jun 17, 1:15 AM
clang/lib/Driver/ToolChains/CommonArgs.cpp
761

Ok, fair enough then!

This revision was landed with ongoing or failed builds.Mon, Jun 20, 12:33 AM
This revision was automatically updated to reflect the committed changes.