Authored by rnk on Jul 31 2019, 2:58 PM.
Tags
• Restricted Project
• Restricted Project
Subscribers

# Details

Reviewers
 russell.gallop • thakis pcc hans
Commits
Summary

Prior to this change, for a few compiler-rt libraries such as ubsan and
the profile library, Clang would embed "-defaultlib:path/to/rt-arch.lib"
into the .drective section of every object compiled with
-finstr-profile-generate or -fsanitize=ubsan as appropriate.

These paths assume that the link step will run from the same working
directory as the compile step. There is also evidence that sometimes the
paths become absolute, such as when clang is run from a different drive
letter from the current working directory. This is fragile, and I'd like
to get away from having paths embedded in the object if possible. Long
ago it was suggested that we use this for ASan, and apparently I felt
the same way back then:
https://reviews.llvm.org/D4428#56536

This is also consistent with how all other autolinking usage works for
PS4, Mac, and Windows: they all use basenames, not paths.

To keep things working for people using the standard GCC driver
library search path when it calls the linker. This is enough to make
check-ubsan pass, and seems like a generally good thing.

Users that invoke the linker directly (most clang-cl users) will have to
their build system. I'm not sure where I can document this. Ideally I'd
also do it in the MSBuild files, but I can't figure out where they go.

# Unit TestsFailedView All

TimeTest
Command: ['/usr/bin/clang++', '-o', '/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libunwind/test/Output/frameheadercache_test.pass.cpp.o', '-x', 'c++', '/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/test/frameheadercache_test.pass.cpp', '-c', '-v', '-ftemplate-depth=270', '-Werror=thread-safety', '-DLIBUNWIND_NO_TIMER', '-funwind-tables', '-std=c++2a', '-I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/include', '-D__STDC_FORMAT_MACROS', '-D__STDC_LIMIT_MACROS', '-D__STDC_CONSTANT_MACROS', '-I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/../libcxx/test/support', '-DLIBCXX_FILESYSTEM_STATIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/std/input.output/filesystems/Inputs/static_test_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libunwind/test/filesystem/Output/dynamic_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_HELPER="/usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/../libcxx/test/support/filesystem_dynamic_test_helper.py"', '-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER', '-Wall', '-Wextra', '-Werror', '-Wuser-defined-warnings', '-Wshadow', '-Wno-unused-command-line-argument', '-Wno-attributes', '-Wno-pessimizing-move', '-Wno-c++11-extensions', '-Wno-user-defined-literals', '-Wno-noexcept-type', '-Wsign-compare', '-Wunused-variable', '-Wunused-parameter', '-Wunreachable-code', '-c'] Exit Code: 1 Standard Error:

### Event Timeline

rnk created this revision.Jul 31 2019, 2:58 PM
Herald added a project: Restricted Project. Jul 31 2019, 2:58 PM
pcc added a comment.Jul 31 2019, 3:25 PM

The idea behind embedding the path was that these objects are usually not distributed, so things usually just work.

Is there some other way that we could arrange for the linker to search the sanitizer library directory automatically? Maybe we could add the path to one of the files in llvm/tools/msbuild? I guess that wouldn't solve the problem for command-line users but maybe that's enough.

rnk added a comment.Aug 1 2019, 11:11 AM

I'll look into that. I also noticed that check-ubsan fails. I think we should also change clang's driver to add this libpath when it invokes the linker, so that this works transparently when using the GCC-style driver.

I kind of like the current behavior since it's consistent with how resource dirs are passed to cc1, and how FILE expands if you give a relative work to clang, and how debug info paths look if you pass relative paths to clang.

rnk retitled this revision from Use library basenames when autolinking on Windows to [Windows] Autolink with basenames and add libdir to libpath.Sep 26 2019, 2:58 PM
rnk edited the summary of this revision. (Show Details)
rnk added a comment.Sep 26 2019, 3:03 PM

I happened to notice that I felt the same way about this in 2014 that I do today:
https://reviews.llvm.org/D4428#56536
That makes me feel like I should keep pushing for this change, even though so far people don't seem enthusiastic about it. :)

I changed the driver to add the library path to the link line when the driver is used to link. This fixes the vanilla GCC-style driver usage in the ubsan test suite.

Obviously, most users of clang-cl don't use clang-cl to link, so they won't get this behavior, but in the absence of documentation, it shows how things are intended to work. Chromium already happens to put clang's library directory on the link line, so everything should work for them.

Does this seem like reasonable behavior for now? I'd modify the msbuild integration myself to make this stuff work out of the box, but I honestly don't know how to test it or change it. The other thing worth checking is the clang PGO self-host on Windows. This has the potential to break that, and the fix would be to add a linker flag in LLVM's cmake.

If you strongly feel that this is the right direction, go for it, you're code owner here :)

To me, this feels like a regression. This change has no benefit that I can see (at least none that anybody's asked for that I'm aware of – then again I read llvm's bugzilla less than you do), and the drawback that folks have to explicitly pass /libpath:\path\to\clang\lib\clang\$changing_version\lib\windows to the linker, which to me is a pretty poor experience. I'd prefer if we used this qualified path for all runtime libs, so that users would never have to use this libpath. (We currently use a qualified path for libprofile but not asan and the like iirc.) The other thing worth checking is the clang PGO self-host on Windows. This has the potential to break that, and the fix would be to add a linker flag in LLVM's cmake. This does indeed break PGO self-host with lld-link (applied on top of r373200): <...>\bin\lld-link.exe /nologo utils\not\CMakeFiles\not.dir\not.cpp.obj utils\not\CMakeFiles\not.dir\__\__\resources\windows_version_resource.rc.res /out:bin\not.exe /implib:lib\not.lib /pdb:bin\not.pdb /version:0.0 /machine:x64 -fuse-ld=lld /STACK:10000000 /INCREMENTAL:NO /subsystem:console lib\LLVMSupport.lib psapi.lib shell32.lib ole32.lib uuid.lib advapi32.lib delayimp.lib -delayload:shell32.dll -delayload:ole32.dll lib\LLVMDemangle.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:bin\not.exe.manifest lld-link: warning: ignoring unknown argument '-fuse-ld=lld' lld-link: error: could not open 'clang_rt.profile-x86_64.lib': no such file or directory So at the very least this change would need that path adding. I agree with Nico that having to add a path dependent on the LLVM version sounds like a pain. Is it possible for the compiler to embed a /libpath as well as the dependent lib? That goes back to having a path embedded, though you could override it if required so could be an improvement over things at the moment. It sounds to me like: ...folks have to explicitly pass /libpath:\path\to\clang\lib\clang\$changing_version\lib\windows to the linker, which to me is a pretty poor experience

and

I'd like to get away from having paths embedded in the object if possible.

are hard to reconcile.

It may be possible to have the Windows installer add the path to LIB environment variable but that would rely on having run the installer, and could cause problems if you have multiple versions of LLVM around.

pcc added a comment.Sep 30 2019, 10:36 AM

Another option would be to let the behaviour be controlled by /Brepro, so that the default behaviour would be to embed the path, while /Brepro builds would only embed the basename.

/Brepro seems orthogonal to me. If you only pass relative paths and -no-canonical-prefixes then the embedded path is relative and doesn't impeded determinism.

Another slightly related thread, regarding libs from the clang resource dir and how they are specified to the linker (regarding the builtins library): https://reviews.llvm.org/D51440

rnk updated this revision to Diff 223128.Oct 3 2019, 6:03 PM
• Fix PGO build
Herald added a project: Restricted Project. Oct 3 2019, 6:03 PM
rnk added a comment.Oct 3 2019, 6:26 PM

Another slightly related thread, regarding libs from the clang resource dir and how they are specified to the linker (regarding the builtins library): https://reviews.llvm.org/D51440

I support that effort. :)

This does indeed break PGO self-host with lld-link (applied on top of r373200):

<...>\bin\lld-link.exe /nologo utils\not\CMakeFiles\not.dir\not.cpp.obj utils\not\CMakeFiles\not.dir\__\__\resources\windows_version_resource.rc.res /out:bin\not.exe /implib:lib\not.lib /pdb:bin\not.pdb /version:0.0 /machine:x64 -fuse-ld=lld /STACK:10000000 /INCREMENTAL:NO /subsystem:console lib\LLVMSupport.lib psapi.lib shell32.lib ole32.lib uuid.lib advapi32.lib delayimp.lib -delayload:shell32.dll -delayload:ole32.dll lib\LLVMDemangle.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:bin\not.exe.manifest
lld-link: warning: ignoring unknown argument '-fuse-ld=lld'
lld-link: error: could not open 'clang_rt.profile-x86_64.lib': no such file or directory

So at the very least this change would need that path adding.

I added a fragment to HandleLLVMOptions.cmake to ask clang for its resource directory and add it to the search path when compiling with instrumentation or sanitizers. I think it's not so bad, and we don't have to write down the version.

I agree with Nico that having to add a path dependent on the LLVM version sounds like a pain.

Is it possible for the compiler to embed a /libpath as well as the dependent lib? That goes back to having a path embedded, though you could override it if required so could be an improvement over things at the moment.

Nope, I tried it, got this:

t.obj : warning LNK4229: invalid directive '/libpath:foo' encountered; ignored

It sounds to me like:

...folks have to explicitly pass /libpath:\path\to\clang\lib\clang\$changing_version\lib\windows to the linker, which to me is a pretty poor experience and I'd like to get away from having paths embedded in the object if possible. are hard to reconcile. True. It may be possible to have the Windows installer add the path to LIB environment variable but that would rely on having run the installer, and could cause problems if you have multiple versions of LLVM around. Well, and I wouldn't want to follow Microsoft down the route of increasing dependence on the environment. If you strongly feel that this is the right direction, go for it, you're code owner here :) I think I do feel strongly about it, but I don't want to be too pushy. :) To me, this feels like a regression. This change has no benefit that I can see (at least none that anybody's asked for that I'm aware of – then again I read llvm's bugzilla less than you do), and the drawback that folks have to explicitly pass /libpath:\path\to\clang\lib\clang\$changing_version\lib\windows to the linker, which to me is a pretty poor experience. I'd prefer if we used this qualified path for all runtime libs, so that users would never have to use this libpath. (We currently use a qualified path for libprofile but not asan and the like iirc.)

libgcc follows a similar model, FWIW, it lives in a compiler-version-dependent path. The difference just happens to be that, on this particular platform, there's no compiler driver running the link to paper over this detail for the user.

LLVM has a lot of runtime libraries these days:

• profile (pgo & coverage)
• asan, ubsan, msan, tsan, sancov
• libfuzzer
• openmp
• libc++
• parallel stl

It doesn't seem so bad to document that, if you want to use LLVM instrumentation tools, you have to add one library search path. So, speaking of documentation, we don't really have good holistic instructions for how to use clang-cl. Where is the most obvious place to put this? The main one I know about and haven't done yet is the release notes.

rnk added a comment.Oct 3 2019, 6:30 PM

One last idea is that we could teach LLD to automatically add this directory to library search path, but then users who link with Visual C++ (not many anymore) will run into this as a corner case issue.

rnk updated this revision to Diff 232975.Dec 9 2019, 5:11 PM
• rebase
rnk added a comment.Feb 26 2020, 3:32 PM

I ran into this issue again, and would like to move forward with this. I'll rebase it.

rnk updated this revision to Diff 250068.Mar 12 2020, 3:10 PM
• rebase, ping
rnk added a reviewer: hans.Mar 12 2020, 3:11 PM

I believe folks are in agreement about this direction, but I'm waiting for someone to mark the revision accepted. Let me know if that's not the case.

hans accepted this revision.Apr 23 2020, 4:17 AM

I agree with the sentiment that it's annoying when clang doesn't just work out of the box. But it will still do, except for asan and profile info etc, for which I think it's reasonable to add to the documentation what the user needs to do. I think the place to do that is https://clang.llvm.org/docs/UsersManual.html#clang-cl (and a release note). It would be great if the doc includes the error message one would get when the link fails, to make it easy to find through web search.

One last idea is that we could teach LLD to automatically add this directory to library search path, but then users who link with Visual C++ (not many anymore) will run into this as a corner case issue.

I thought about that too, and I think it's a good idea. Having the linker in a toolchain know about the runtime libs seems pretty reasonable I think?

clang/docs/ReleaseNotes.rst
63

Could this be expanded to a more explain-like-I'm-five style example?

This revision is now accepted and ready to land.Apr 23 2020, 4:17 AM
rnk updated this revision to Diff 259751.Apr 23 2020, 4:12 PM
• Lots of documentation words
hans added a comment.Apr 24 2020, 12:11 AM

The docs look great, thanks!

This revision was automatically updated to reflect the committed changes.