Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clang: messes up its search path if a prefixed gcc is on the PATH #11495

Open
jeremyd2019 opened this issue Apr 14, 2022 · 27 comments · Fixed by llvm/llvm-project#76949
Open

Comments

@jeremyd2019
Copy link
Member

This has cropped up a few times now (#10762, and a couple of times on Discord). After #10634, the 'triplet' directory (ex, /clang64/x86_64-w64-mingw32) no longer exists, and after #10651 (comment) mingw-w64-clang no longer installs gcc aliases, so clang now will prefer 'adopting' the paths of an unrelated prefixed gcc (ex, x86_64-w64-mingw32-gcc) if it finds one on the PATH. This has happened if users have put both /clang64/bin and /mingw64/bin on the path, or if they have some other mingw distribution installed and have path type inherit set.

https://github.com/llvm/llvm-project/blob/234678fbf9cf05c232221bb8253ed658507f3b49/clang/lib/Driver/ToolChains/MinGW.cpp#L413-L427 is the logic responsible.

@mstorsjo thoughts on how to improve this situation? Should we carry a local patch to not look for a gcc on the PATH?

@jeremyd2019 jeremyd2019 changed the title clang: messes up its search directories if a prefixed gcc is on the path clang: messes up its search path if a prefixed gcc is on the PATH Apr 14, 2022
rkitover added a commit to rkitover/windows-alt-sshd-msys2 that referenced this issue Apr 14, 2022
It breaks the compiler on CLANG64 if another gcc is installed, for
example the Chocolatey mingw package.

See: msys2/MINGW-packages#11495

Signed-off-by: Rafael Kitover <rkitover@gmail.com>
@mstorsjo
Copy link
Contributor

This has cropped up a few times now (#10762, and a couple of times on Discord). After #10634, the 'triplet' directory (ex, /clang64/x86_64-w64-mingw32) no longer exists, and after #10651 (comment) mingw-w64-clang no longer installs gcc aliases, so clang now will prefer 'adopting' the paths of an unrelated prefixed gcc (ex, x86_64-w64-mingw32-gcc) if it finds one on the PATH. This has happened if users have put both /clang64/bin and /mingw64/bin on the path, or if they have some other mingw distribution installed and have path type inherit set.

https://github.com/llvm/llvm-project/blob/234678fbf9cf05c232221bb8253ed658507f3b49/clang/lib/Driver/ToolChains/MinGW.cpp#L413-L427 is the logic responsible.

@mstorsjo thoughts on how to improve this situation? Should we carry a local patch to not look for a gcc on the PATH?

Thanks for the great summary of the situation! This is indeed a tricky situation... Carrying a local patch to tweak this behaviour works of course, but if one of the most major distributions of the tool needs to tweak the tool defaults, those defaults aren't great...

I wonder if there's a better way of formulating the logic here... Does it make sense to try to distinguish between whether clang is cross compiling or not? If cross compiling (I'm not sure if there's a good definition of that within clang - maybe checking getDefaultTargetTriple() and whether the target OS of that is mingw?), one really shouldn't be looking in <bin>/.. but one really should need a triple prefix. Or put another way; if we're not cross compiling, I guess we should raise the priority of <bin>/.. over looking for a gcc elsewhere. Or perhaps we don't need to think about cross compilation, but if we'd just check if <bin>/.. looks like a valid sysroot for the target. Should we check for whether e.g. lib/libkernel32.a exists there? Hmm. Perhaps it's ok to pick that over a different gcc, if such a file exists and we'd keep the cross compilation check (default target os is mingw, default arch matches the current target arch)?

@mati865
Copy link
Collaborator

mati865 commented Apr 14, 2022

I think I'd prefer looking for mingw-w64 header/library in default paths.
That would allow one day to have LLVM based mingw-w64 toolchain in path (without GCC aliases) and still be able to use GCC.

For us the quickest workaround would be disabling GCC detection or making it first check *-cc and later *-gcc.

@mstorsjo
Copy link
Contributor

I think I'd prefer looking for mingw-w64 header/library in default paths.

Can you clarify a bit more in detail what you mean here?

That would allow one day to have LLVM based mingw-w64 toolchain in path (without GCC aliases) and still be able to use GCC.

For us the quickest workaround would be disabling GCC detection or making it first check *-cc and later *-gcc.

If you want to do it as a local patch, then yeah, that's probably simplest. For upstreaming, I'm not sure about checking for *-cc - right now, I guess it checks for *-gcc to detect a cross compiler installed elsewhere. In a default cross gcc install, there's no *-cc, only *-gcc. So that wouldn't work as a different way of achieving what is done now - and thus might be harder to argue for being the right thing to do, even if it would fix the msys2 case.

(About that, I wonder if I in llvm-mingw also should reduce the number of aliases I install. I guess it makes sense to install unprefixed cc, c99, c11, but if a regular cross gcc doesn't install triple prefixed versions of them, perhaps I could cut down on the number of frontend tools a little?)

@mati865
Copy link
Collaborator

mati865 commented Apr 15, 2022

I think I'd prefer looking for mingw-w64 header/library in default paths.

Can you clarify a bit more in detail what you mean here?

I was thinking about CMAKE_INSTALL_PREFIX but now I realize it would only work when installing LLVM via package manager.
So probing ../include/_mingw.h or some library would be better.

@revelator
Copy link
Contributor

probably best to look for libgcc.a since _mingw.h is used by both it might not be enough.

@royqh1979
Copy link

royqh1979 commented Apr 27, 2022

I found that clang's official windows binary use "-target x86_64-pc-windows-gnu" to tell clang use MinGW GCC's library. (By default it will use vc's library)

So why don't we just let mingw-w64-clang-x86_64-clang's user to use that option explicitly to do cross compile? (Since mingw-w64-x86_64-clang will do cross compile by default.)

@revelator
Copy link
Contributor

clarifying i ment because both compilers use the mingw-w64 api's it might not be enough to destinguish between them.

@royqh1979
Copy link

I mean, since mingw-w64-clang-x86_64-clang is distributed with it's own library, it should use that library by default, unless explicitly told to use other compiler's library.

@revelator
Copy link
Contributor

i thought that was the default ?!? atleast it used to be but there have been so many changes to clang that im having a hard time following it.

@revelator
Copy link
Contributor

btw my comment was pointed at this comment.

I was thinking about CMAKE_INSTALL_PREFIX but now I realize it would only work when installing LLVM via package manager.
So probing ../include/_mingw.h or some library would be better.

but since both gcc and clang use the same api (though clang uses the ucrt version) i ment that it might not be enough to distinguish between them looking for _mingw.h instead looking for libgcc.a or libgcc_s.a which might but is also a bit harder to do.

@revelator
Copy link
Contributor

maybe parse gcc -dumpversion ? might do the trick.

@royqh1979
Copy link

i thought that was the default ?!? atleast it used to be but there have been so many changes to clang that im having a hard time following it.
i thought that was the default ?!? atleast it used to be but there have been so many changes to clang that im having a hard time following it.

Then what's the purpose of the whole mingw-w64-clang-x86_64-* things? If mingw-w64-clang-x86_64-clang is not meant to be used as a standalone compiler, what's the difference of it and mingw-w64-x86_64-clang?

@revelator
Copy link
Contributor

i think we are discussing two different things ? previous versions of the mingw-w64-clang package had a patch that allowed putting a flag for linking with the clang runtimes but defaulted to the gcc runtime if it was not used, but if i remember correctly this patch was ditched after we got clang working as a standalone compiler. so kinda the opposite but maybe something similar can be used ?.

@mati865
Copy link
Collaborator

mati865 commented Apr 28, 2022

btw my comment was pointed at this comment.

I was thinking about CMAKE_INSTALL_PREFIX but now I realize it would only work when installing LLVM via package manager.
So probing ../include/_mingw.h or some library would be better.

but since both gcc and clang use the same api (though clang uses the ucrt version) i ment that it might not be enough to distinguish between them looking for _mingw.h instead looking for libgcc.a or libgcc_s.a which might but is also a bit harder to do.

Huh?
We want to know if Clang is shipped with mingw-w64 before we start searching for GCC. Searching for mingw-w64 headers/libs is the best solution here.

@revelator
Copy link
Contributor

mistake on my part then, i was thinking it related to which runtime it linked to.

@ferdnyc
Copy link
Contributor

ferdnyc commented Jun 3, 2022

I don't pretend to fully understand this issue (either the causes or the proposed solutions), but is it the reason (or, related to the reason) why some clang64 packages are installing [what purport to be] mingw32 binaries ?

I was looking at /clang64/bin/ from a MinGW-Clang64 shell, and was surprised to find:

$ ls /clang64/bin/x86_64*
/clang64/bin/x86_64-w64-mingw32-agrep.exe
/clang64/bin/x86_64-w64-mingw32-c++.exe
/clang64/bin/x86_64-w64-mingw32-captoinfo.exe
/clang64/bin/x86_64-w64-mingw32-cc.exe
/clang64/bin/x86_64-w64-mingw32-clang++.exe
/clang64/bin/x86_64-w64-mingw32-clang.exe
/clang64/bin/x86_64-w64-mingw32-clear.exe
/clang64/bin/x86_64-w64-mingw32-deflatehd.exe
/clang64/bin/x86_64-w64-mingw32-g++.exe
/clang64/bin/x86_64-w64-mingw32-gcc.exe
/clang64/bin/x86_64-w64-mingw32-inflatehd.exe
/clang64/bin/x86_64-w64-mingw32-infocmp.exe
/clang64/bin/x86_64-w64-mingw32-infotocap.exe
/clang64/bin/x86_64-w64-mingw32-pkg-config.exe
/clang64/bin/x86_64-w64-mingw32-pkgconf.exe
/clang64/bin/x86_64-w64-mingw32-reset.exe
/clang64/bin/x86_64-w64-mingw32-tabs.exe
/clang64/bin/x86_64-w64-mingw32-tic.exe
/clang64/bin/x86_64-w64-mingw32-toe.exe
/clang64/bin/x86_64-w64-mingw32-tput.exe
/clang64/bin/x86_64-w64-mingw32-tset.exe

Coming from a half-dozen different packages:

$ pacman -Qo /clang64/bin/x86_64-w64-mingw32-*.exe
/clang64/bin/x86_64-w64-mingw32-agrep.exe is owned by mingw-w64-clang-x86_64-libtre-git r128.6fb7206-2
/clang64/bin/x86_64-w64-mingw32-c++.exe is owned by mingw-w64-clang-x86_64-clang 14.0.3-1
/clang64/bin/x86_64-w64-mingw32-captoinfo.exe is owned by mingw-w64-clang-x86_64-ncurses 6.3-5
/clang64/bin/x86_64-w64-mingw32-cc.exe is owned by mingw-w64-clang-x86_64-clang 14.0.3-1
/clang64/bin/x86_64-w64-mingw32-clang++.exe is owned by mingw-w64-clang-x86_64-clang 14.0.3-1
/clang64/bin/x86_64-w64-mingw32-clang.exe is owned by mingw-w64-clang-x86_64-clang 14.0.3-1
/clang64/bin/x86_64-w64-mingw32-clear.exe is owned by mingw-w64-clang-x86_64-ncurses 6.3-5
/clang64/bin/x86_64-w64-mingw32-deflatehd.exe is owned by mingw-w64-clang-x86_64-nghttp2 1.47.0-1
/clang64/bin/x86_64-w64-mingw32-g++.exe is owned by mingw-w64-clang-x86_64-gcc-compat 14.0.3-1
/clang64/bin/x86_64-w64-mingw32-gcc.exe is owned by mingw-w64-clang-x86_64-gcc-compat 14.0.3-1
/clang64/bin/x86_64-w64-mingw32-inflatehd.exe is owned by mingw-w64-clang-x86_64-nghttp2 1.47.0-1
/clang64/bin/x86_64-w64-mingw32-infocmp.exe is owned by mingw-w64-clang-x86_64-ncurses 6.3-5
/clang64/bin/x86_64-w64-mingw32-infotocap.exe is owned by mingw-w64-clang-x86_64-ncurses 6.3-5
/clang64/bin/x86_64-w64-mingw32-pkg-config.exe is owned by mingw-w64-clang-x86_64-pkgconf 1.8.0-2
/clang64/bin/x86_64-w64-mingw32-pkgconf.exe is owned by mingw-w64-clang-x86_64-pkgconf 1.8.0-2
/clang64/bin/x86_64-w64-mingw32-reset.exe is owned by mingw-w64-clang-x86_64-ncurses 6.3-5
/clang64/bin/x86_64-w64-mingw32-tabs.exe is owned by mingw-w64-clang-x86_64-ncurses 6.3-5
/clang64/bin/x86_64-w64-mingw32-tic.exe is owned by mingw-w64-clang-x86_64-ncurses 6.3-5
/clang64/bin/x86_64-w64-mingw32-toe.exe is owned by mingw-w64-clang-x86_64-ncurses 6.3-5
/clang64/bin/x86_64-w64-mingw32-tput.exe is owned by mingw-w64-clang-x86_64-ncurses 6.3-5
/clang64/bin/x86_64-w64-mingw32-tset.exe is owned by mingw-w64-clang-x86_64-ncurses 6.3-5

The only one I examined in more detail, x86_64-w64-mingw32-clang.exe, is exactly the same size as clang.exe, and I assume is just a copy of the same binary?

@mati865
Copy link
Collaborator

mati865 commented Jun 3, 2022

The only one I examined in more detail, x86_64-w64-mingw32-clang.exe, is exactly the same size as clang.exe, and I assume is just a copy of the same binary?

Yes.

I don't pretend to fully understand this issue (either the causes or the proposed solutions), but is it the reason (or, related to the reason) why some clang64 packages are installing [what purport to be] mingw32 binaries ?

It's entirely unrelated to this issue. <arch>-w64-mingw32-<bin> is the target triple for compilers using mingw-w64, it is used mostly when cross compiling from Linux to mingw-w64 but some build systems prefer it rather than bare <bin>.

The issue is Clang searches for gcc in PATH, if it finds let's say /usr/bin/gcc then everything breaks. Installing mingw-w64-clang-<arch>-gcc-compat workarounds this problem because Clang will find gcc binary next to itself.

@Hyderman
Copy link

Hyderman commented Oct 3, 2022

The issue is Clang searches for gcc in PATH, if it finds let's say /usr/bin/gcc then everything breaks. Installing mingw-w64-clang--gcc-compat workarounds this problem because Clang will find gcc binary next to itself.

It solves the problem for Clang but clangd is always broken.

@MehdiChinoune
Copy link
Collaborator

This commit could solve the issue https://reviews.llvm.org/rG02b25bd904b5, Maybe we could backport it!

@ferdnyc
Copy link
Contributor

ferdnyc commented Dec 17, 2022

@MehdiChinoune

This commit could solve the issue https://reviews.llvm.org/rG02b25bd904b5, Maybe we could backport it!

Agreed, that does sound promising. Rather than backporting, a good first step might be to just build a newer Clang (one that includes that commit), to determine whether it really does make things better. If the answer is "yes", then a backport into the packaged version probably makes sense.

Also, I notice that the only tests added for that change were to the test/Driver/Inputs/mingw_fedora_tree/, despite there also being trees like test/Driver/Inputs/{mingw_mingw_builds_tree,mingw_msys2_tree}/ that could probably benefit from tests of the target/host binary resolution logic — either (or both) before and after that change.

That says to me that this issue (as experienced in MSYS2) isn't really on the Clang team's radar — if that commit does actually fix/improve things for us as well, effectively it's purely by accident.

Perhaps it's time to attempt to upstream (or re-upstream) this bug, rather than carrying local patches to fix it?

(Which, if we have to do that... as @mstorsjo pointed out, that isn't really a great look. For them! I'd think — well, I'd hope — that they'd be interested in addressing this upstream, as well. And the existence of a test/Driver/Inputs/mingw_msys2_tree/ directory at least implies that they consider it within their sphere of influence to "get things right" on the MSYS2 platform(s).)

@ferdnyc
Copy link
Contributor

ferdnyc commented Dec 17, 2022

HAHAHA! I just noticed that @mstorsjo is the author of the commit in question. 🤣 So I guess ignore my comments about the issue being on their radar, at least.

@mati865
Copy link
Collaborator

mati865 commented Dec 17, 2022

I don't know what makes you think that commit would help here.

Agreed, that does sound promising. Rather than backporting, a good first step might be to just build a newer Clang (one that includes that commit), to determine whether it really does make things better.

It's by far easier to backport the commit and test rather than building from trunk.

@mstorsjo
Copy link
Contributor

I made a PoC of a commit that should fix this (and the duplicate issue #19279, CC @huangqinjin).

See mstorsjo/llvm-project@mingw-install-base-sysroot - does someone want to try to apply this on the msys2 clang and retry the experiments? (I've tested it in a modified llvm-mingw environment where I've tried to trigger this issue.)

@mati865
Copy link
Collaborator

mati865 commented Jan 2, 2024

Without patch and with *-gcc-compat package removed:

$ PATH=$PATH:/d/msys64/mingw64/bin

$ which clang
/clang64/bin/clang

$ which gcc
/d/msys64/mingw64/bin/gcc

$ clang -E -Wp,-v -xc /dev/null
clang -cc1 version 17.0.6 based upon LLVM 17.0.6 default target x86_64-w64-windows-gnu
ignoring nonexistent directory "D:/msys64/mingw64/x86_64-w64-mingw32/include"
ignoring nonexistent directory "D:/msys64/mingw64/x86_64-w64-mingw32/usr/include"
#include "..." search starts here:
#include <...> search starts here:
 C:/msys64/clang64/lib/clang/17/include
 D:/msys64/mingw64/include
End of search list.
# 1 "nul"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 398 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "nul" 2

After patching:

$ clang -E -Wp,-v -xc /dev/null
clang -cc1 version 17.0.6 based upon LLVM 17.0.6 default target x86_64-w64-windows-gnu
ignoring nonexistent directory "C:/msys64/clang64/x86_64-w64-mingw32/include"
ignoring nonexistent directory "C:/msys64/clang64/x86_64-w64-mingw32/usr/include"
#include "..." search starts here:
#include <...> search starts here:
 C:/msys64/clang64/lib/clang/17/include
 C:/msys64/clang64/include
End of search list.
# 1 "nul"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 398 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "nul" 2

@mstorsjo
Copy link
Contributor

mstorsjo commented Jan 4, 2024

Without patch and with *-gcc-compat package removed:

$ PATH=$PATH:/d/msys64/mingw64/bin

$ which clang
/clang64/bin/clang

$ which gcc
/d/msys64/mingw64/bin/gcc

$ clang -E -Wp,-v -xc /dev/null
#include <...> search starts here:
 C:/msys64/clang64/lib/clang/17/include
 D:/msys64/mingw64/include
End of search list.

After patching:

$ clang -E -Wp,-v -xc /dev/null
#include <...> search starts here:
 C:/msys64/clang64/lib/clang/17/include
 C:/msys64/clang64/include
End of search list.

Thanks for testing! That looks like it works exactly as intended; I'll try to get this change upstreamed then.

@mstorsjo
Copy link
Contributor

mstorsjo commented Jan 4, 2024

Thanks for testing! That looks like it works exactly as intended; I'll try to get this change upstreamed then.

Posted a PR at llvm/llvm-project#76949.

mstorsjo added a commit to mstorsjo/llvm-project that referenced this issue Jan 5, 2024
…a proper mingw sysroot

This fixes uses of the MSYS2 clang64 environment compilers, if
another set of GCC based compilers are available further back
in PATH (which may be explicitly added, or inherited unintentionally
from other software installed).

(The issue in the clang64 environment can be worked around somewhat
by installing *-gcc-compat packages which present aliases named
<triple>-gcc within the clang64 environment as well.)

This fixes msys2/MINGW-packages#11495
and msys2/MINGW-packages#19279.
mstorsjo added a commit to llvm/llvm-project that referenced this issue Jan 7, 2024
…a proper mingw sysroot (#76949)

This fixes uses of the MSYS2 clang64 environment compilers, if another
set of GCC based compilers are available further back in PATH (which may
be explicitly added, or inherited unintentionally from other software
installed).

(The issue in the clang64 environment can be worked around somewhat by
installing *-gcc-compat packages which present aliases named
<triple>-gcc within the clang64 environment as well.)

This fixes msys2/MINGW-packages#11495 and
msys2/MINGW-packages#19279.
@mstorsjo
Copy link
Contributor

Thanks for testing! That looks like it works exactly as intended; I'll try to get this change upstreamed then.

Posted a PR at llvm/llvm-project#76949.

FWIW, the fix for this landed in upstream Clang in mstorsjo/llvm-project@c671870.

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this issue Jan 28, 2024
…a proper mingw sysroot (llvm#76949)

This fixes uses of the MSYS2 clang64 environment compilers, if another
set of GCC based compilers are available further back in PATH (which may
be explicitly added, or inherited unintentionally from other software
installed).

(The issue in the clang64 environment can be worked around somewhat by
installing *-gcc-compat packages which present aliases named
<triple>-gcc within the clang64 environment as well.)

This fixes msys2/MINGW-packages#11495 and
msys2/MINGW-packages#19279.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants