This is an archive of the discontinued LLVM Phabricator instance.

[clang] Make the driver not diagnose errors on nonexistent linker inputs
ClosedPublic

Authored by thakis on Sep 10 2021, 12:00 PM.

Details

Summary

When nonexistent linker inputs are passed to the driver, the linker
now errors out, instead of the compiler. If the linker does not run,
clang now emits a "warning: linker input unused" instead of an error
for nonexistent files.

The motivation for this change is that I noticed that
clang-cl /winsysroot sysroot main.cc ole32.lib emitted a
"ole32.lib not found" error, even though the linker finds it just fine when
I run clang-cl /winsysroot sysroot main.cc /link ole32.lib.

The same problem occurs if running clang-cl main.cc ole32.lib in a
non-MSVC shell.

The problem is that DiagnoseInputExistence() only looked for libs in %LIB%,
but MSVCToolChain uses much more involved techniques.

For this particular problem, we could make DiagnoseInputExistence() ask
the toolchain to see if it can find a .lib file, but in general the
driver can't know what the linker will do to find files, so it shouldn't
try. For example, if we implement PR24616, lld-link will look in the
registry to determine a good default for %LIB% if it isn't set.

This is less or a problem for the gcc driver, since .a paths there are
either passed via -l flags (which honor -L), or via a qualified path
(that doesn't honor -L) -- but for example ld.lld's --chroot flag
can also trigger this problem. Without this patch,
clang -fuse-ld=lld -Wl,--chroot,some/dir /file.o will complain that
/file.o doesn't exist, even though
clang -fuse-ld=lld -Wl,--chroot,some/dir -Wl,/file.o succeeds just fine.

This implements rnk's suggestion on the old bug PR27234.

Diff Detail

Event Timeline

thakis requested review of this revision.Sep 10 2021, 12:00 PM
thakis created this revision.
thakis added inline comments.Sep 10 2021, 12:16 PM
clang/lib/Driver/Driver.cpp
2173

This means that typo suggestion now works even if you pass /link. Before this patch, it didn't:

% out/gn/bin/clang-cl /Brepo -### clang/test/Driver/unknown-arg.c /link
clang: error: no such file or directory: '/Brepo'; did you mean '/Brepro'?

I suppose this means that before, we unintentionally and silently passed flags that clang-cl didn't know to link.exe even if they were in front of /link! …and we _still_ do this for flags that aren't within editing distance of 1 of existing clang-cl flags:

% out/gn/bin/clang-cl clang/test/Driver/unknown-arg.c /asdfasdf -fuse-ld=lld /link
lld-link: error: could not open '/asdfasdf': No such file or directory

Now we even do this if there's no /link flag:

% out/gn/bin/clang-cl clang/test/Driver/unknown-arg.c /libpath:foo -fuse-ld=lld
# fine

That's kind of weird! With - we do something better:

% out/gn/bin/clang-cl clang/test/Driver/unknown-arg.c -libpath:foo -fuse-ld=lld
clang: warning: unknown argument ignored in clang-cl: '-libpath:foo' [-Wunknown-argument]

It's the old "things starting with '/' might be path, or might be flag" thing again, and as the --chroot example in this comment shows, it actually might be a path even if no file exists at that particular path. Hrm.

thakis added inline comments.Sep 10 2021, 12:23 PM
clang/lib/Driver/Driver.cpp
2173

Maybe we should tack on a && !(IsCLMode() && Value.startswith("/")) here for that? (lld-)link.exe don't have a feature like -chroot as far as I know. It's a bit heuristic-y, but probably better than not doing it?

hans added a comment.Sep 10 2021, 12:26 PM

Hmm. As you worried yourself in https://bugs.llvm.org/show_bug.cgi?id=27234#c3 we might be getting a worse user experience in some cases. For example:

$ clang x.o
clang: error: no such file or directory: 'x.o'
clang: error: no input files

$ clang -Wl,x.o
/usr/bin/ld: cannot find x.o: No such file or directory
clang: error: linker command failed with exit code 1 (use -v to see invocation)

We would now generate the latter message. Which doesn't really look much worse to me. I guess we might have wasted time by doing some compiles before invoking the linker, though..

GCC matches the current Clang behaviour though:

$ gcc x.o
gcc: error: x.o: No such file or directory
gcc: fatal error: no input files

I wonder if there are deeper reasons for GCC and Clang's current behaviour than early diagnosis and slightly nicer error messages.

Since cl.exe seems to match the behaviour your patch is switching to, maybe we should only do this in clang-cl mode?

clang/lib/Driver/Driver.cpp
2141

Sounds hacky, who wrote that? Oh, I see :-)

2165

you mean "If we don't end up invoking the *linker*"?

2173

Hmm, yeah it sounds reasonable.

thakis updated this revision to Diff 371987.Sep 10 2021, 12:32 PM
thakis marked 2 inline comments as done.
  • driver -> linker in comment
  • don't ignore flags in clang-cl mode that start with a '/'

I wonder if there are deeper reasons for GCC and Clang's current behaviour than early diagnosis and slightly nicer error messages.

One easy way to find out is land this and if it breaks something, add a comment and a test for the next person :)

Since cl.exe seems to match the behaviour your patch is switching to, maybe we should only do this in clang-cl mode?

Maybe! But the current behavior is arguably wrong for the clang -fuse-ld=lld -Wl,-chroot,some/path /foo.o case, so maybe we should try changing both for starters and then row back if needed?

clang/lib/Driver/Driver.cpp
2165

Err, yes.

thakis updated this revision to Diff 372016.Sep 10 2021, 2:47 PM
thakis edited the summary of this revision. (Show Details)

flang test update (?!)

hans accepted this revision.Sep 11 2021, 10:30 AM

I wonder if there are deeper reasons for GCC and Clang's current behaviour than early diagnosis and slightly nicer error messages.

One easy way to find out is land this and if it breaks something, add a comment and a test for the next person :)

Since cl.exe seems to match the behaviour your patch is switching to, maybe we should only do this in clang-cl mode?

Maybe! But the current behavior is arguably wrong for the clang -fuse-ld=lld -Wl,-chroot,some/path /foo.o case, so maybe we should try changing both for starters and then row back if needed?

Sounds good to me.

This revision is now accepted and ready to land.Sep 11 2021, 10:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2021, 5:58 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
phosek added a subscriber: phosek.Sep 13 2021, 9:55 AM

Clang :: Driver/cl.c started failing on our Windows bots after this change landed. Here is the error output is, would it be possible to take a look and revert the change if needed?

Hm, it passes on my win box and on the one running the presubmit test. Do you know if there's anything special about your bot? Any idea how I could reproduce this? The output is error: command failed with exit status: True.

Does it fail consistently? Or was it a one-time flake?

saghir added a subscriber: saghir.Sep 13 2021, 2:51 PM

Hi, this change also breaks Power PC bots:

  1. https://lab.llvm.org/buildbot/#/builders/19/builds/6451/steps/24/logs/stdio
  2. https://lab.llvm.org/buildbot/#/builders/18/builds/2443/steps/23/logs/stdio

It fails consistently on our bots. Can you please take a look?

Thanks!

Does it fail consistently? Or was it a one-time flake?

It's been failing consistently. It might be related to our toolchain setting lld as the default linker: https://github.com/llvm/llvm-project/blob/ce6d512015737643df794a2790fa9627f795ea7f/clang/cmake/caches/Fuchsia-stage2.cmake#L36

Hi, this change also breaks Power PC bots:

  1. https://lab.llvm.org/buildbot/#/builders/19/builds/6451/steps/24/logs/stdio
  2. https://lab.llvm.org/buildbot/#/builders/18/builds/2443/steps/23/logs/stdio

It fails consistently on our bots. Can you please take a look?

Thanks!

That's a more interesting failure. It fails because:

clang-13: error: /wd4146: 'linker' input unused [-Werror,-Wunused-command-line-argument]
clang-13: error: /wd4291: 'linker' input unused [-Werror,-Wunused-command-line-argument]
clang-13: error: /wd4391: 'linker' input unused [-Werror,-Wunused-command-line-argument]
clang-13: error: /wd4722: 'linker' input unused [-Werror,-Wunused-command-line-argument]
clang-13: error: /wd4800: 'linker' input unused [-Werror,-Wunused-command-line-argument]

compiler-rt\cmake\config-ix.cmake does check_cxx_compiler_flag(/wd4800 COMPILER_RT_HAS_WD4800_FLAG) etc. I suppose this patch does have the effect of making flags only warnings if no linking is happening:

>out\gn\bin\clang-cl --driver-mode=gcc /wd1245 -c main.cc
clang-cl: warning: /wd1245: 'linker' input unused [-Wunused-command-line-argument]

That breaks check_cxx_compiler_flag() since it doesn't do its checks with -Werror. I guess that's an argument to keeping this an error for the gcc-style driver. On the other hand, it's a bit weird to use cmake:check_cxx_compiler_flag to check for MSVC-style flags since MSVC itself only emits a warning on unknown flags. (On the third hand, this behavior between gcc and cl seems to be used to detect cl in practice :) ).

Just using -wd... instead of /wd... would inside check_cxx_compiler_flag would make things go, and we don't use this often currently:

>git grep check_.*compiler_flag(\/
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/GR COMPILER_RT_HAS_GR_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/GS COMPILER_RT_HAS_GS_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/MT COMPILER_RT_HAS_MT_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/Oy COMPILER_RT_HAS_Oy_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/Zi COMPILER_RT_HAS_Zi_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/W4 COMPILER_RT_HAS_W4_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/WX COMPILER_RT_HAS_WX_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/wd4146 COMPILER_RT_HAS_WD4146_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/wd4291 COMPILER_RT_HAS_WD4291_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/wd4221 COMPILER_RT_HAS_WD4221_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/wd4391 COMPILER_RT_HAS_WD4391_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/wd4722 COMPILER_RT_HAS_WD4722_FLAG)
compiler-rt/cmake/config-ix.cmake:check_cxx_compiler_flag(/wd4800 COMPILER_RT_HAS_WD4800_FLAG)
openmp/runtime/cmake/config-ix.cmake:    check_cxx_compiler_flag(/EHsc LIBOMP_HAVE_EHSC_FLAG)
openmp/runtime/cmake/config-ix.cmake:    check_cxx_compiler_flag(/GS LIBOMP_HAVE_GS_FLAG)
openmp/runtime/cmake/config-ix.cmake:    check_cxx_compiler_flag(/Oy- LIBOMP_HAVE_Oy__FLAG)
openmp/runtime/cmake/config-ix.cmake:    check_cxx_compiler_flag(/arch:SSE2 LIBOMP_HAVE_ARCH_SSE2_FLAG)
openmp/runtime/cmake/config-ix.cmake:    check_cxx_compiler_flag(/Qsafeseh LIBOMP_HAVE_QSAFESEH_FLAG)
openmp/runtime/cmake/config-ix.cmake:  check_cxx_compiler_flag(/Qlong_double LIBOMP_HAVE_LONG_DOUBLE_FLAG)
openmp/runtime/cmake/config-ix.cmake:  check_cxx_compiler_flag(/Qdiag-disable:177 LIBOMP_HAVE_DIAG_DISABLE_177_FLAG)
openmp/runtime/cmake/config-ix.cmake:  check_cxx_compiler_flag(/Qinline-min-size=1 LIBOMP_HAVE_INLINE_MIN_SIZE_FLAG)

But I think it's better to undo this for the gcc-style driver instead. I'll land a change for that.

thakis added a comment.EditedSep 13 2021, 3:42 PM

Does it fail consistently? Or was it a one-time flake?

It's been failing consistently. It might be related to our toolchain setting lld as the default linker: https://github.com/llvm/llvm-project/blob/ce6d512015737643df794a2790fa9627f795ea7f/clang/cmake/caches/Fuchsia-stage2.cmake#L36

I use lld on my machines too, so that's not it.

Hi, this change also breaks Power PC bots:

  1. https://lab.llvm.org/buildbot/#/builders/19/builds/6451/steps/24/logs/stdio
  2. https://lab.llvm.org/buildbot/#/builders/18/builds/2443/steps/23/logs/stdio

It fails consistently on our bots. Can you please take a look?

Thanks!

This will hopefully be better after b7bac5a172e51ed065b3b4dc64cc2d8831e8081c.

Hi, this change also breaks Power PC bots:

  1. https://lab.llvm.org/buildbot/#/builders/19/builds/6451/steps/24/logs/stdio
  2. https://lab.llvm.org/buildbot/#/builders/18/builds/2443/steps/23/logs/stdio

It fails consistently on our bots. Can you please take a look?

Thanks!

This will hopefully be better after b7bac5a172e51ed065b3b4dc64cc2d8831e8081c.

Yes, my local build succeeded. One of the bots is green already, and the other one is building but should be fine too. Thanks for the prompt fix!

Hi, this change also breaks Power PC bots:

  1. https://lab.llvm.org/buildbot/#/builders/19/builds/6451/steps/24/logs/stdio
  2. https://lab.llvm.org/buildbot/#/builders/18/builds/2443/steps/23/logs/stdio

It fails consistently on our bots. Can you please take a look?

Thanks!

This will hopefully be better after b7bac5a172e51ed065b3b4dc64cc2d8831e8081c.

Yes, my local build succeeded. One of the bots is green already, and the other one is building but should be fine too. Thanks for the prompt fix!

Our builders went back to green as well, thanks!