Differential D42390
msan: Pass --disable-new-dtags for tests Authored by Hahnfeld on Jan 22 2018, 11:51 AM.
Details We really want to have DT_RPATH instead of DT_RUNPATH: When This currently happens with GNU gold and lld while GNU ld
Diff Detail Event TimelineComment Actions I'm not sure about this. Reverting to an old, deprecated feature is rarely the answer. One option is to unset LD_LIBRARY_PATH in tests. Comment Actions I use libc++ with the host compiler and it needs to be in LD_LIBRARY_PATH for all other tests to work. Comment Actions You should be able to handle this in lit. Take a look at how compiler-rt/test/asan/lit.cfg munges LD_LIBRARY_PATH, and do something similar in compiler-rt/test/msan/Unit/. Comment Actions I was half-way through implementing this and wanted to align tsan where we might have the same problem (for some reasons unknown to me the tests don't fail there?). However, setting environment variables like LD_LIBRARY_PATH in lit means that also the compiler will run in this environment: symbol lookup error: <...>/projects/compiler-rt/lib/tsan/libcxx_tsan_x86_64/lib/libc++.so.1: undefined symbol: __tsan_init This obviously works for tsan-binaries but not if some toolchain binaries are linked against libc++. Comment Actions I normally build on my mac, and haven't actually seen these errors. Can you tell me how to reproduce? Should I build/test on Linux? Comment Actions Right. That's why LD_LIBRARY_PATH is evil, as are environment variables in general. Comment Actions Unless I'm missing something, it should not be necessary if we add -Wl,-rpath flags to the right %clang substitutions. Comment Actions IIUC, the search order is:
So, DT_RUNPATH will not trump LD_LIBRARY_PATH unless setuid/setgid bits are set, which is not the case for these tests. Comment Actions Oh, sorry, I did not notice that you said "unset". Comment Actions No worries. Like Jonas said, it's an ordering problem, so if you could prepend the correct paths to LD_LIBRARY_PATH, it should work -- perhaps easier said than done. Again, I think this could be done on a test by test basis which lit should be able to handle. Comment Actions This deserves some longer explanation and it was getting a bit late yesterday: The error only comes up with some local changes implementing the approach with LD_LIBRARY_PATH. The "problem" is that I'm building with LLVM_ENABLE_LIBCXX, so bin/clang is linked against libc++. Additionally the "host" clang (that I'm using to build LLVM) was built the same way, so there already is a libc++.so in LD_LIBRARY_PATH. If some tests are linked with -Wl,-rpath and use DT_RUNPATH, the library in LD_LIBRARY_PATH takes precedence and the instrumented one is not used. If we however add the instrumented libc++ to LD_LIBRARAY_PATH, bin/clang will also use it which results in the symbol error mentioned above when building tsan tests. Yep, but as explained above we have the ordering issue both when compiling (where we don't want to have the instrumented libc++) and when executing the test (that's where we want the instrumented version!). That's why I wanted to use the "old" DT_RPATH which is evaluated before looking at LD_LIBRARY_PATH and is only set for the compiled test, not for bin/clang. If you are strongly against that, the only other possibility I see is setting LD_LIBRARY_PATH when executing the test, ie add a substitution like %load_instrumented_libcxx which evaluates to env LD_LIBRARY_PATH=.... This would avoid the problems when bin/clang also uses libc++. Comment Actions Thanks for the explanation. Since it's a problem with your particular environment, I think you should fix that instead. You can eliminate the need to export LD_LIBRARY_PATH to run your host compiler by creating a simple script and symlink it to clang and clang++, e.g., here's a simple example... $ cat run_clang.sh #!/bin/sh name=`basename $0` prefix=<host clang prefix> LD_LIBRARY_PATH=$prefix/lib $prefix/bin/$name $@ $ ln -s run_clang.sh clang $ ln -s run_clang.sh clang++ Then you can get rid of the global LD_LIBRARY_PATH, and run cmake like this CC=<path to run_clang.sh>/clang cmake ../llvm. No need to pass CXX since cmake will set it automatically based on CC. Comment Actions I strongly disagree: Environment variables are used like that in the wild. And the same thing will happen when libc++ is installed to /usr/lib one day in the future. Comment Actions Pretty sure /usr/lib gets searched last, i.e., DT_RUNPATH always wins unless LD_LIBRARY_PATH is set. As a general rule, only set LD_LIBRARY_PATH as a last resort, and if you have a special case, use a wrapper script. Comment Actions That's right, unless it's also in LD_LIBRARY_PATH
You are the first person to say that and in general the tests shouldn't fail just because someone needed to use the "last resort". Comment Actions But that's my point. You should avoid setting LD_LIBRARY_PATH unless you have no other choice.
Well, perhaps the second, i.e., s/evil/last resort/:
Comment Actions I understood this to be related to setting environment variables in the test. As the name suggest they are meant for the environment. Comment Actions I'll let @eugenis speak for himself, but in general LD_LIBRARY_PATH is not a good idea. A quick google search will provide a bunch of links to blogs and articles that make a much stronger case than I can. Comment Actions Okay, rethinking about this, we probably can't get rid of it in our environment because we use the modules system (http://modules.sourceforge.net/) which heavily relies on modifying environment variables. In general, would patches be acceptable (maybe in a different form) that solves this kind "problem"? Comment Actions I think unsetting LD_LIBRARY_PATH in the tests sounds reasonable -- many tests already do that. Comment Actions Which tests do that? AFAICS the asan tests you have been referring to add something to LD_LIBRARY_PATH... Comment Actions Perhaps I should have said manipulate/set, etc. The point is that is not unreasonable to change it. However, I would again suggest you get away from relying on magic environment variables. As far as I can tell, you are the only one having this issue. Comment Actions As I said we won't and most of HPC clusters use this way to install multiple compilers etc. It's probably just that I'm using an uncommon configuration to build LLVM (LLVM_ENABLE_LIBCXX + LLVM_ENABLE_LLD + clang host compiler linked against libc++ which is in LD_LIBRARY_PATH). Because I can't see a correct way to fix this in all cases, I'm abandoning this revision and will keep the patch locally. |