<div dir="ltr"><div class="gmail_extra">Hi Greg,</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 16, 2014 at 4:58 PM, Greg Fitzgerald <span dir="ltr"><<a href="mailto:garious@gmail.com" target="_blank">garious@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">> First of all, sorry for the late response (I'm in the process of moving to California).<br>
<br>
</div>Welcome!<br>
<div class=""><br>
<br>
> We need to verify that simple command "clang -fsanitize=address foo.cc" works<br>
<br>
</div>Agreed. The test suite is useful in many different scenarios:<br>
<br>
1) Verifying the integrated clang.<br>
2) Verifying the integrated gcc.<br>
3) Verifying the libraries during development.<br>
<br>
I'm working to improve #3 and in a way that does not interfere with #1 or #2.<br></blockquote><div><br></div><div>(1) should be the part of (3). If you want to test the sanitizer runtiume library "during development",</div>
<div>you should verify that it works with the Clang at hand. Sanitizer runtime and the compiler are tightly</div><div>coupled, why would you want to test the former in isolation?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
> we add additional -L/path/to/compiler-rt/libs to all %clang invocations in lit test-suite.<br>
<br>
</div>Yes, that's the next patch I have for you. But adding the '-L' flag<br>
is optional. If you want to use the test suite to verify an<br>
integrated clang or gcc, you disable that.<br></blockquote><div><br></div><div>Modifying Clang driver for testing-only purposes, especially for testing in a non-default configuration</div><div>doesn't sound good to me. Also, I still don't get why you would want that (see comment above).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
<br>
> This looks familiar to what Evgeniy did for running lit test suite on<br>
> Android. This is now possible w/o modifying any RUN-lines, see<br>
> the hacky scripts in compiler-rt/test/asan/android_commands/folder.<br>
> Can you use the similar approach for QEMU?<br>
<br>
</div>Clever, but I hope we can try to avoid the symlink hackery. Locally,<br>
I've renamed "%sim %t" to "%run %t" so it reads quite nicely, IMHO.<br>
It was a bunch of work to go change all the tests, but I think it's<br>
the right approach. Each cross-compiled variation configures "%run"<br>
from the command-line when invoking CMake. No hacks.<br></blockquote><div><br></div><div>I will reply to that shortly.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
-Greg<br>
.<br>
<div class="HOEnZb"><div class="h5"><br>
On Tue, Apr 15, 2014 at 5:08 PM, Alexey Samsonov <<a href="mailto:samsonov@google.com">samsonov@google.com</a>> wrote:<br>
> Hi Greg,<br>
><br>
> First of all, sorry for the late response (I'm in the process of moving to<br>
> California).<br>
><br>
> On Fri, Apr 4, 2014 at 6:13 PM, Greg Fitzgerald <<a href="mailto:garious@gmail.com">garious@gmail.com</a>> wrote:<br>
>><br>
>> Alexey,<br>
>><br>
>> >> Some good news, the drivers (both gcc and clang) allow us to put the<br>
>> >> '-L' parameters after the '-l' parameters.<br>
>><br>
>> I made these changes locally and it went really well. The patch to<br>
>> clang is quite small and only one unit-test needed updating. In<br>
>> compiler-rt, I updated the flags passed to clang to include a<br>
>> '-L${COMPILER_RT_BINARY_DIR}/lib' and<br>
>> '-I${COMPILER_RT_BINARY_DIR}/include' flag. All tests pass except for<br>
>> the default blacklist tests. I had to move those into the clang test<br>
>> suite.<br>
><br>
><br>
> I've looked at the patch to Clang. I'll try to explain why I'm opposed to<br>
> this approach.<br>
><br>
> You are making the test suite configuration more and more dependent on the<br>
> specific<br>
> version of the specific Compiler (ToT Clang), while recently we were moving<br>
> in the opposite direction.<br>
> You even hardcode the knowledge of how the sanitizer headers and sanitizer<br>
> libraries are used (by adding -I and -L flags).<br>
><br>
> I treat sanitizer test-suite as toolchain tests, not a library tests. We<br>
> need to verify that simple command<br>
> "clang -fsanitize=address foo.cc" works, and produce a binary that works as<br>
> expected. As an illustration, suppose<br>
> the following sequence of events<br>
> 1) we land your changes to Clang driver, so that it uses -lasan<br>
> -L/path/to/clang/resource/dir<br>
> 2) we add additional -L/path/to/compiler-rt/libs to all %clang invocations<br>
> in lit test-suite.<br>
> 3) someone accidentally breaks the Clang dirver, and now it uses<br>
> -L/wrong/path/to/clang/resource/dir<br>
> We will not be able to detect this error. The test-suite is green, but the<br>
> functionality is broken.<br>
><br>
> IMO we should just deal with the fact that sanitizer test-suite depends not<br>
> only on sanitizer libraries themselves,<br>
> but also on LLVM/Clang libs and tools.<br>
><br>
>><br>
>><br>
>> As for cross-compilation and test ARM Linux, I needed to update all<br>
>> tests to include '%sim' before invoking cross-compiled executables.<br>
>> For x86, the variable is empty. For cross-compiling, it's a<br>
>> user-configured ${COMPILER_RT_EMULATOR}, which for ARM Linux, is QEMU.<br>
>> There are currently a handful of test failures. If you'd like, I can<br>
>> XFAIL them and commit this stuff sooner than later.<br>
><br>
><br>
> This looks familiar to what Evgeniy did for running lit test suite on<br>
> Android. This is now possible<br>
> w/o modifying any RUN-lines, see the hacky scripts in<br>
> compiler-rt/test/asan/android_commands/<br>
> folder. Can you use the similar approach for QEMU?<br>
><br>
>><br>
>> Do you want these patches?<br>
>><br>
>> -Greg<br>
>><br>
>><br>
>> On Fri, Apr 4, 2014 at 10:22 AM, Greg Fitzgerald <<a href="mailto:garious@gmail.com">garious@gmail.com</a>><br>
>> wrote:<br>
>> >>>> in a standalone mode, then "make check-compiler-rt" in that tree<br>
>> >>>> wouldn't rebuild runtimes, and instead assume that they are provided<br>
>> >>>> by toolchain<br>
>> >> What is wrong with that approach?<br>
>> ><br>
>> > Not wrong, just inconsistent. I'm trying to test the<br>
>> > just-built-runtime before running the 'install' step. That's the way<br>
>> > all the other projects work, "all check-all install" in that order.<br>
>> > In other words, "build, verify, commit".<br>
>> ><br>
>> ><br>
>> >> And, again, we have to ensure that -L/path/to/new/asan provided by<br>
>> >> user "wins" over -L/path/to/clang/resource/dir, and we have strong<br>
>> >> reasons to put the latter as early in the link line as possible.<br>
>> ><br>
>> > Some good news, the drivers (both gcc and clang) allow us to put the<br>
>> > '-L' parameters after the '-l' parameters. So we can put -lasan at<br>
>> > the start of the parameter list to get the desired link order and -L<br>
>> > at the end so that the user-specified runtime is always chosen.<br>
>> ><br>
>> > -Greg<br>
>> ><br>
>> ><br>
>> > On Fri, Apr 4, 2014 at 5:57 AM, Alexey Samsonov <<a href="mailto:samsonov@google.com">samsonov@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Thu, Apr 3, 2014 at 9:23 PM, Greg Fitzgerald <<a href="mailto:garious@gmail.com">garious@gmail.com</a>><br>
>> >> wrote:<br>
>> >>><br>
>> >>> > I don't think it's a good idea to let user hijack<br>
>> >>> > the driver and stuff in custom version of<br>
>> >>> > ASan runtime instead the one installed/built<br>
>> >>> > with compiler :)<br>
>> >>><br>
>> >>> I'm okay with it. This is open source. If someone wants to put the<br>
>> >>> sanitizers on a shorter release cycle than the full toolchain, who are<br>
>> >>> we to<br>
>> >>> hold them back?<br>
>> >><br>
>> >><br>
>> >> They can always rebuild the fresh ASan from source and replace the ASan<br>
>> >> runtime in toolchain with the new one.<br>
>> >><br>
>> >>><br>
>> >>><br>
>> >>> > What do you mean by "invoking the test-suite directly"?<br>
>> >>><br>
>> >>> In Clang's CMakeLists, add_subdirectory() of compiler-rt's 'test'<br>
>> >>> directory. I suggested we do this earlier, because of clang's<br>
>> >>> hardcoded<br>
>> >>> dependency on the path to ASan. Instead, I think a better solution is<br>
>> >>> to<br>
>> >>> break the hardcoded dependency, allowing the test suite to point clang<br>
>> >>> to<br>
>> >>> compiler-rt's local copy of ASan using a '-L' parameter.<br>
>> >><br>
>> >><br>
>> >> I don't believe it's needed anywhere except in the testing<br>
>> >> infrastructure<br>
>> >> you suggest. And, again, we have to ensure that -L/path/to/new/asan<br>
>> >> provided<br>
>> >> by user "wins" over -L/path/to/clang/resource/dir, and we have strong<br>
>> >> reasons to put the latter as early in the link line as possible. That<br>
>> >> said,<br>
>> >> ....<br>
>> >><br>
>> >>><br>
>> >>><br>
>> >>> Greg<br>
>> >>><br>
>> >>><br>
>> >>> On Thursday, April 3, 2014, Alexey Samsonov <<a href="mailto:samsonov@google.com">samsonov@google.com</a>><br>
>> >>> wrote:<br>
>> >>>><br>
>> >>>><br>
>> >>>> On Thu, Apr 3, 2014 at 5:04 AM, Greg Fitzgerald <<a href="mailto:garious@gmail.com">garious@gmail.com</a>><br>
>> >>>> wrote:<br>
>> >>>>><br>
>> >>>>> > we would still want to use compiler-rt test-suite in a standalone<br>
>> >>>>> > mode, to test fully built/installed toolchains,<br>
>> >>>>> and even GCC.<br>
>> >>>>><br>
>> >>>>> Sounds good.<br>
>> >>>>><br>
>> >>>>><br>
>> >>>>> > Clang driver links the static xsan runtimes from a hardcoded<br>
>> >>>>> > paths in Clang resource directory, and doesn't add flags like<br>
>> >>>>> > "-lasan -L/path/to/clang/resource/dir". I find this behavior<br>
>> >>>>> > reasonable.<br>
>> >>>>><br>
>> >>>>> Can we change the flags to "-lasan -L/path/to/clang/resource/dir"?<br>
>> >>>>> If<br>
>> >>>>> we make that change,<br>
>> >>>><br>
>> >>>><br>
>> >>>> I don't think it's a good idea to let user hijack the driver and<br>
>> >>>> stuff in<br>
>> >>>> custom version of<br>
>> >>>> ASan runtime instead the one installed/built with compiler :)<br>
>> >>>><br>
>> >>>>><br>
>> >>>>> I can stop suggesting that the clang build should<br>
>> >>>>> invoke the compiler-rt test suite directly. :-)<br>
>> >>>><br>
>> >>>><br>
>> >>>> I haven't yet fully understood your complaints.<br>
>> >><br>
>> >><br>
>> >><br>
>> >>>><br>
>> >>>> Note that if you build/configure compiler-rt<br>
>> >>>> in a standalone mode, then "make check-compiler-rt" in that tree<br>
>> >>>> wouldn't<br>
>> >>>> rebuild runtimes,<br>
>> >>>> and instead assume that they are provided by toolchain (that is, in<br>
>> >>>> standalone mode runtimes and<br>
>> >>>> test suites are effectively independent).<br>
>> >><br>
>> >><br>
>> >> ^^^^<br>
>> >> What is wrong with that approach?<br>
>> >><br>
>> >><br>
>> >>>><br>
>> >>>> What do you mean by "invoking the test-suite directly"?<br>
>> >>>><br>
>> >>>><br>
>> >>>><br>
>> >>>><br>
>> >>>> > Do you want to give it a try?<br>
>> >>>><br>
>> >>>> I'll take a crack it, sure.<br>
>> >>>><br>
>> >>>><br>
>> >>>> By the way, locally, I now have just over half the ASan test suite<br>
>> >>>> passing ARM-Linux via QEMU. It's been really convenient to build<br>
>> >>>> compiler-rt in standalone mode. The edit-compile-run cycle is much<br>
>> >>>> shorter this way. Thanks for your help in making this work!<br>
>> >>>><br>
>> >>>> -Greg<br>
>> >>>><br>
>> >>>><br>
>> >>>><br>
>> >>>> On Wed, Apr 2, 2014 at 7:56 AM, Alexey Samsonov <<a href="mailto:samsonov@google.com">samsonov@google.com</a>><br>
>> >>>> wrote:<br>
>> >>>> > Hi Greg,<br>
>> >>>> ><br>
>> >>>> > On Wed, Apr 2, 2014 at 2:50 AM, Greg Fitzgerald <<a href="mailto:garious@gmail.com">garious@gmail.com</a>><br>
>> >>>> > wrote:<br>
>> >>>> >><br>
>> >>>> >> Alexey, Evgeniy,<br>
>> >>>> >><br>
>> >>>> >> I propose the following steps to unify multi-arch support in<br>
>> >>>> >> compiler-rt:<br>
>> >>>> >><br>
>> >>>> >> 1) The compiler-rt test suite adds<br>
>> >>>> >> "-L${CMAKE_CURRENT_BINARY_DIR}/lib"<br>
>> >>>> >> to its 'clang' variables. This way we can test the sanitizers<br>
>> >>>> >> without<br>
>> >>>> >> installing any libs to the just-built-clang install directory.<br>
>> >>>> ><br>
>> >>>> ><br>
>> >>>> > This is possible, but please keep in mind that:<br>
>> >>>> > 1) we would still want to use compiler-rt test-suite in a<br>
>> >>>> > standalone<br>
>> >>>> > mode,<br>
>> >>>> > to test fully built/installed toolchains,<br>
>> >>>> > and even GCC.<br>
>> >>>> > 2) Adding -L... wouldn't work: Clang driver links the static xsan<br>
>> >>>> > runtimes<br>
>> >>>> > from a hardcoded paths in Clang resource directory,<br>
>> >>>> > and doesn't add flags like "-lasan -L/path/to/clang/resource/dir".<br>
>> >>>> > I<br>
>> >>>> > find<br>
>> >>>> > this behavior reasonable.<br>
>> >>>> ><br>
>> >>>> > I don't see a problem with the current approach - we can make "run<br>
>> >>>> > sanitizer<br>
>> >>>> > test suite" command in the<br>
>> >>>> > top-level build tree depend on "build/install compiler-rt<br>
>> >>>> > ExternalProject"<br>
>> >>>> > steps (see how it's done currently).<br>
>> >>>> ><br>
>> >>>> >><br>
>> >>>> >><br>
>> >>>> >> 2) The "clang/runtime" build calls ExternalProject once for each<br>
>> >>>> >> arch<br>
>> >>>> >> it needs of compiler-rt. So once to create x86_64 libs and once<br>
>> >>>> >> for<br>
>> >>>> >> i386 libs.<br>
>> >>>> >><br>
>> >>>> >> 3) The compiler-rt build drops the ${arch} suffix from its libs,<br>
>> >>>> >> and<br>
>> >>>> >> the "clang/runtime" build uses CMAKE_INSTALL_PREFIX to control<br>
>> >>>> >> where<br>
>> >>>> >> the compiler-rt libs go.<br>
>> >>>> >><br>
>> >>>> >><br>
>> >>>> >><br>
>> >>>> >> "CMAKE_INSTALL_PREFIX=${CMAKE_CURRENT_BINARY_DIR}/lib/clang/${CLANG_VERSION}/x86_64-linux"<br>
>> >>>> >><br>
>> >>>> >><br>
>> >>>> >> 4) Remove multi-arch support from the compiler-rt build. Instead,<br>
>> >>>> >> declare compiler-rt as an "any-arch" build, configured with:<br>
>> >>>> >> CMAKE_CXX_COMPILER (defaults to just-built-clang)<br>
>> >>>> >> CMAKE_CXX_FLAGS (defaults to llvm's default target)<br>
>> >>>> >> COMPILER_RT_TEST_COMPILER (defaults to CMAKE_CXX_COMPILER)<br>
>> >>>> >> COMPILER_RT_TEST_FLAGS (defaults to CMAKE_CXX_FLAGS)<br>
>> >>>> ><br>
>> >>>> ><br>
>> >>>> > Unfortunately, this could be problematic for Mac - we use<br>
>> >>>> > "universal<br>
>> >>>> > binaries" there - a single static/dynamic runtime<br>
>> >>>> > which contains runtimes for all the possible architectures. We<br>
>> >>>> > might<br>
>> >>>> > work<br>
>> >>>> > around this, however, by adding a custom<br>
>> >>>> > Mac-specific step that would merge all single-arch binaries into a<br>
>> >>>> > multi-arch one - I think this is fine as long as we<br>
>> >>>> > greatly simplify the rest of the ugly build system.<br>
>> >>>> ><br>
>> >>>> > Do you want to give it a try?<br>
>> >>>> ><br>
>> >>>> >><br>
>> >>>> >><br>
>> >>>> >> Thoughts?<br>
>> >>>> >><br>
>> >>>> >> Thanks,<br>
>> >>>> >> Greg<br>
>> >>>> >><br>
>> >>>> >> On Tue, Apr 1, 2014 at 12:58 AM, Evgeniy Stepanov<br>
>> >>>> >> <<a href="mailto:eugeni.stepanov@gmail.com">eugeni.stepanov@gmail.com</a>> wrote:<br>
>> >>>> >> > It does sound like Android is better suited for "honest"<br>
>> >>>> >> > cross-compilation, rather than "build compiler-rt for all<br>
>> >>>> >> > targets we<br>
>> >>>> >> > can find" model.<br>
>> >>>> >> ><br>
>> >>>> >> > I'm still not convinced that we must require the "ninja install"<br>
>> >>>> >> > step.<br>
>> >>>> >> > Could we just "ninja clang" and then build the second stage<br>
>> >>>> >> > against<br>
>> >>>> >> > the first stage build directory? Will this "find_package" thing<br>
>> >>>> >> > not<br>
>> >>>> >> > work that way, or do you mean it as a way to copy target asan<br>
>> >>>> >> > runtime<br>
>> >>>> >> > where the first stage compiler will find it (or probably both)?<br>
>> >>>> >> ><br>
>> >>>> >> > On Mon, Mar 31, 2014 at 6:46 PM, Alexey Samsonov<br>
>> >>>> >> > <<a href="mailto:samsonov@google.com">samsonov@google.com</a>><br>
>> >>>> >> > wrote:<br>
>> >>>> ><br>
>> >>>><br>
>> >>>><br>
>> >>>><br>
>> >>>><br>
>> >>>> --<br>
>> >>>> Alexey Samsonov, MSK<br>
>> >><br>
>> >><br>
>> >><br>
>> >><br>
>> >> --<br>
>> >> Alexey Samsonov, MSK<br>
><br>
><br>
><br>
><br>
> --<br>
> Alexey Samsonov, MSK<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><div>Alexey Samsonov, Mountain View, CA</div></div>
</div></div>