This is an archive of the discontinued LLVM Phabricator instance.

[LLD] [COFF] Don't try to detect MSVC installations in mingw mode
ClosedPublic

Authored by mstorsjo on Feb 15 2023, 2:12 AM.

Details

Summary

In mingw mode, all linker paths are passed explicitly to the linker
by the compiler driver. Don't try to implicitly add linker paths
from the LIB environment variable or by detecting an MSVC
installation directory.

If the /winsysroot command line parameter is explicitly passed to
lld-link while /lldmingw is specified, it could be considered reasonable
to actually include those paths. However, modifying the code to
handle only the /winsysroot case but not the other ones, when the
mingw mode has been enabled, seems like much more code complexity
for a mostly hypothetical case.

Add a test for this when case when using LIB. (The code paths for
trying to detect an MSVC installation aren't really regression tested.)

Also fix an issue in the existing test for "Check that when /winsysroot
is specified, %LIB% is ignored.", where the LIB variable pointed
to a nonexistent directory, so the test would pass even if /winsysroot
wouldn't be specified.

Diff Detail

Event Timeline

mstorsjo created this revision.Feb 15 2023, 2:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2023, 2:12 AM
mstorsjo requested review of this revision.Feb 15 2023, 2:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2023, 2:12 AM
hans added inline comments.Feb 15 2023, 4:14 AM
lld/COFF/Driver.cpp
1515

i assume lld will warn about unused arguments if the user tries to use /winsysroot or /vctoolsdir in mingw mode?

mstorsjo added inline comments.Feb 15 2023, 4:33 AM
lld/COFF/Driver.cpp
1515

No, lld doesn't warn about unclaimed arguments.

hans added inline comments.Feb 15 2023, 9:56 AM
lld/COFF/Driver.cpp
1515

Hmm, should we make it warn in this case? Just silently ignoring /winsysroot seems like a bad experience.

mstorsjo added inline comments.Feb 15 2023, 2:50 PM
lld/COFF/Driver.cpp
1515

I guess it would be reasonable to print a warning here, if there are explicit command line arguments that we know we're not dealing with.

mstorsjo updated this revision to Diff 497817.Feb 15 2023, 3:01 PM

Added a warning if command line arguments are ignored.

mstorsjo added inline comments.Feb 15 2023, 3:03 PM
lld/COFF/Driver.cpp
1519

Having a negated if condition, followed by an else (i.e. a double negation), is a bit non-ideal, but I feel the main codepath (first the main case is presented, followed by the exception to it) is easier to follow this way.

hans accepted this revision.Feb 16 2023, 4:06 AM

lgtm

This revision is now accepted and ready to land.Feb 16 2023, 4:06 AM
mati865 accepted this revision.Feb 16 2023, 1:21 PM

LGTM

mstorsjo reopened this revision.Feb 17 2023, 1:29 AM

I had to revert this, as it broke one test in compiler-rt/test/profile/Windows/coverage-weak-lld.cpp, which relied on linking in an MSVC environment (finding the lib path from the environment) with -lldmingw to opt in to mingw style behaviours.

This revision is now accepted and ready to land.Feb 17 2023, 1:29 AM

Should that even be considered as supported?

Should that even be considered as supported?

That's a good question... The flag in itself unlocks a whole bunch of behaviours, some which can be considered useful, and some probably just as tolerable sideeffects (like adding list markers for __CTOR_LIST__ and such) - so in general I guess it's a bit iffy. I have mentioned the flag to various people throughout the years that they can try it out to see if it gives a behaviour they need though.

For some behaviours, there are individual flags that control the behaviours (like -stdcall-fixup or -auto-import) where the default behaviour depends on whether the mingw flag is set - but there are also lots of behaviours that aren't individually controllable.

I'm not sure what would be best here - add a more specific -tolerate-duplicate-weak-sybols option or something like that? Or add a more specific flag, always passed by default by the mingw driver frontend, e.g. -detect-msvc:no?

For context - I haven't heard about a specific case where this would make any difference, I just noticed the fact that this is done unconditionally - and there's a small risk that someone ends up relying on implicitly linking in things from a WinSDK they have lying around.

I'm not sure what would be best here - add a more specific -tolerate-duplicate-weak-sybols option or something like that? Or add a more specific flag, always passed by default by the mingw driver frontend, e.g. -detect-msvc:no?

Sorry if you have been waiting for me, this has slipped under my radar.
I don't know MSVC at all so I cannot provide any suggestions there but for MinGW any way that disables MSVC search by the default probably would be beneficial.

I had to revert this, as it broke one test in compiler-rt/test/profile/Windows/coverage-weak-lld.cpp, which relied on linking in an MSVC environment (finding the lib path from the environment) with -lldmingw to opt in to mingw style behaviours.

I was under impression lld-link should follow MSVC closely (so this behaviour would be surprising) but that might just a wrong impression from a bystander.

I'm not sure what would be best here - add a more specific -tolerate-duplicate-weak-sybols option or something like that? Or add a more specific flag, always passed by default by the mingw driver frontend, e.g. -detect-msvc:no?

Sorry if you have been waiting for me, this has slipped under my radar.

No problem - I wasn't waiting specifically for you, I just didn't have time to take much further action on this at the time, and it's not a high priority thing.

I don't know MSVC at all so I cannot provide any suggestions there but for MinGW any way that disables MSVC search by the default probably would be beneficial.

I had to revert this, as it broke one test in compiler-rt/test/profile/Windows/coverage-weak-lld.cpp, which relied on linking in an MSVC environment (finding the lib path from the environment) with -lldmingw to opt in to mingw style behaviours.

I was under impression lld-link should follow MSVC closely (so this behaviour would be surprising) but that might just a wrong impression from a bystander.

Well, the starting point is that it should follow MSVC style behaviours, but overall I do think lld-link offers a few extensions outside of what's possible in the regular MSVC ecosystem, and that's probably fine in itself. But using -lldmingw is indeed quite loosely specified...

mstorsjo updated this revision to Diff 557821.Oct 20 2023, 11:52 PM

Rebased on latest git main.

After https://github.com/llvm/llvm-project/pull/68077 and https://github.com/llvm/llvm-project/pull/69781 - the compiler-rt test that used -lldmingw in MSVC environments has been updated to use a more specific option; I'll go ahead and try to reland this soon.