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

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

mmuetzel added a comment.

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`?


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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?


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added inline comments.



Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:645
   if (getToolChain().getDriver().IsFlangMode() &&
   Args.hasArg(options::OPT_flang_experimental_exec)) {
 addFortranRuntimeLibraryPath(getToolChain(), Args, CmdArgs);

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


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added inline comments.



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

mmuetzel wrote:
> 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
> ```
> 
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.


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added inline comments.



Comment at: flang/test/Driver/linker-flags.f90:51
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: "[[object_file]]"

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


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

Just wondering if those changes are correct for MinGW.




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:753
+  llvm::opt::ArgStringList &CmdArgs) {
+  if (TC.getTriple().isOSWindows()) {
+CmdArgs.push_back("Fortran_main.lib");

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



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:774
   llvm::sys::path::append(DefaultLibPath, "lib");
-  CmdArgs.push_back(Args.MakeArgString("-L" + DefaultLibPath));
+  if (TC.getTriple().isOSWindows())
+CmdArgs.push_back(Args.MakeArgString("-libpath:" + DefaultLibPath));

same


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

In D126291#356 , @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.


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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`?


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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?


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

I made this additional change:

  From 26cee469225e80ac9bae22ebb6e60d47373fc19d Mon Sep 17 00:00:00 2001
  From: =?UTF-8?q?Markus=20M=C3=BCtzel?= 
  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,13 @@ 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);
  +  }
  +
  +  // Add the compiler-rt library directories to libpath if they exist to help
  +
 Arg *SubsysArg =
 Args.getLastArg(options::OPT_mwindows, options::OPT_mconsole);
 if (SubsysArg && SubsysArg->getOption().matches(options::OPT_mwindows)) {
  -- 
  2.35.3.windows.1

No 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. 🤷‍♂️


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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

  From d74a276679778b943b1e2e50f5dd45f65c530252 Mon Sep 17 00:00:00 2001
  From: =?UTF-8?q?Markus=20M=C3=BCtzel?= 
  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!


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added inline comments.



Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:140
+// defined in flang's runtime libraries.
+if (TC.getTriple().isKnownWindowsMSVCEnvironment())
+  CmdArgs.push_back("/subsystem:console");

Is the MSVC toolchain used anywhere else but on Windows?


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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)




Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:140
+// defined in flang's runtime libraries.
+if (TC.getTriple().isKnownWindowsMSVCEnvironment())
+  CmdArgs.push_back("/subsystem:console");

awarzynski wrote:
> mmuetzel wrote:
> > Is the MSVC toolchain used anywhere else but on Windows?
> No: 
> https://github.com/llvm/llvm-project/blob/5fee1799f4d8da59c251e2d04172fc2f387cbe54/llvm/include/llvm/ADT/Triple.h#L576-L578
>  ;-) 
In that case, this argument could probably be added unconditionally.


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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

  From 62920169c43de27569ecae9ba0ec0fe4ec01633a Mon Sep 17 00:00:00 2001
  From: =?UTF-8?q?Markus=20M=C3=BCtzel?= 
  Date: Fri, 3 Jun 2022 19:17:34 +0200
  Subject: [PATCH] lock without  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..bd8c1ca0e747 100644
  --- a/flang/runtime/lock.h
  +++ b/flang/runtime/lock.h
  @@ -15,23 +15,17 @@
   
   // Avoid  if possible to avoid introduction of C++ runtime
   // library dependence.
  -#ifndef _WIN32
  -#define USE_PTHREADS 1
  +#ifdef _WIN32
  +#include 
   #else
  -#undef USE_PTHREADS
  -#endif
  -
  -#if USE_PTHREADS
   #include 
  -#else
  -#include 
   #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); }
  +  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  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).


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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?= 
  Date: Fri, 3 Jun 2022 21:59:02 +0200
  Subject: [PATCH] lock without  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  if possible to avoid introduction of C++ runtime
   // library dependence.
  -#ifndef _WIN32
  -#define USE_PTHREADS 1
  +#ifdef _WIN32
  +#include 
   #else
  -#undef USE_PTHREADS
  -#endif
  -
  -#if USE_PTHREADS
   #include 
  -#else
  -#include 
   #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).


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

In D126291#3561854 , @Meinersbur 
wrote:

> In D126291#3555639 , @awarzynski 
> wrote:
>
>> In D126291#391 , @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"...




Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:140
+// defined in flang's runtime libraries.
+if (TC.getTriple().isKnownWindowsMSVCEnvironment())
+  CmdArgs.push_back("/subsystem:console");

Meinersbur wrote:
> mmuetzel wrote:
> > awarzynski wrote:
> > > mmuetzel wrote:
> > > > Is the MSVC toolchain used anywhere else but on Windows?
> > > No: 
> > > https://github.com/llvm/llvm-project/blob/5fee1799f4d8da59c251e2d04172fc2f387cbe54/llvm/include/llvm/ADT/Triple.h#L576-L578
> > >  ;-) 
> > In that case, this argument could probably be added unconditionally.
> 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.
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.


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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




Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:133
 
+  if (C.getDriver().IsFlangMode()) {
+tools::addFortranRuntimeLibraryPath(TC, Args, CmdArgs);

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.


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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?




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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added inline comments.



Comment at: flang/test/Driver/linker-flags-windows.f90:7
+! clang/lib/Driver/Toolchains/MSVC.cpp.
+! REQUIRES: windows-msvc
+

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?


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

mmuetzel added a comment.

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


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

https://reviews.llvm.org/D126291

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits