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

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski added a comment.

Thanks for adding this, Diana! :)

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

In D126291#3547023 <https://reviews.llvm.org/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 <https://docs.microsoft.com/en-us/cpp/build/reference/subsystem-specify-subsystem?view=msvc-170>. 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 <https://github.com/llvm/llvm-project/blob/e601b2a1542710789395ab1121b1ccc7076e39d1/clang/lib/Driver/ToolChain.cpp#L597>. This method **is used** in  Gnu.cpp <https://github.com/llvm/llvm-project/blob/e601b2a1542710789395ab1121b1ccc7076e39d1/clang/lib/Driver/ToolChains/Gnu.cpp#L690>, 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.



================
Comment at: flang/test/Driver/linker-flags-windows.f90:5-6
+
+! NOTE: The additional linker flags tested here are currently specified in
+! clang/lib/Driver/Toolchains/MSVC.cpp.
+! REQUIRES: system-windows
----------------
[nit] Would you mind updating [[ https://github.com/llvm/llvm-project/blob/e601b2a1542710789395ab1121b1ccc7076e39d1/flang/test/Driver/linker-flags.f90#L5-L9 | 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 :) 


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

https://reviews.llvm.org/D126291