<div dir="ltr"><div class="gmail_extra">Hi Greg,</div><div class="gmail_extra"><br></div><div class="gmail_extra">First of all, sorry for the late response (I'm in the process of moving to California).</div><div class="gmail_extra">
<br><div class="gmail_quote">On Fri, Apr 4, 2014 at 6:13 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Alexey,<br>
<div class=""><br>
>> Some good news, the drivers (both gcc and clang) allow us to put the<br>
>> '-L' parameters after the '-l' parameters.<br>
<br>
</div>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></blockquote><div><br></div><div>I've looked at the patch to Clang. I'll try to explain why I'm opposed to this approach.</div><div><br></div><div>You are making the test suite configuration more and more dependent on the specific</div>
<div>version of the specific Compiler (ToT Clang), while recently we were moving in the opposite direction.</div><div>You even hardcode the knowledge of how the sanitizer headers and sanitizer libraries are used (by adding -I and -L flags).</div>
<div><br></div><div>I treat sanitizer test-suite as toolchain tests, not a library tests. We need to verify that simple command</div><div>"clang -fsanitize=address foo.cc" works, and produce a binary that works as expected. As an illustration, suppose</div>
<div>the following sequence of events</div><div>1) we land your changes to Clang driver, so that it uses -lasan -L/path/to/clang/resource/dir</div><div>2) we add additional -L/path/to/compiler-rt/libs to all %clang invocations in lit test-suite.</div>
<div>3) someone accidentally breaks the Clang dirver, and now it uses -L/<b>wrong/</b>path/to/clang/resource/dir</div><div>We will not be able to detect this error. The test-suite is green, but the functionality is broken.<br>
</div><div><br></div><div>IMO we should just deal with the fact that sanitizer test-suite depends not only on sanitizer libraries themselves,</div><div>but also on LLVM/Clang libs and tools.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<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></blockquote><div><br></div><div>This looks familiar to what Evgeniy did for running lit test suite on Android. This is now possible</div><div>w/o modifying any RUN-lines, see the hacky scripts in compiler-rt/test/asan/android_commands/</div>
<div>folder. Can you use the similar approach for QEMU?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Do you want these patches?<br>
<span class=""><font color="#888888"><br>
-Greg<br>
</font></span><div class=""><div class="h5"><br>
<br>
On Fri, Apr 4, 2014 at 10:22 AM, Greg Fitzgerald <<a href="mailto:garious@gmail.com">garious@gmail.com</a>> 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 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>> wrote:<br>
>><br>
>> On Thu, Apr 3, 2014 at 9:23 PM, Greg Fitzgerald <<a href="mailto:garious@gmail.com">garious@gmail.com</a>> 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 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 hardcoded<br>
>>> dependency on the path to ASan. Instead, I think a better solution is to<br>
>>> break the hardcoded dependency, allowing the test suite to point clang 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 infrastructure<br>
>> you suggest. And, again, we have to ensure that -L/path/to/new/asan 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 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>> 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"? 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 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 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 "-L${CMAKE_CURRENT_BINARY_DIR}/lib"<br>
>>>> >> to its 'clang' variables. This way we can test the sanitizers 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 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". 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 arch<br>
>>>> >> it needs of compiler-rt. So once to create x86_64 libs and once for<br>
>>>> >> i386 libs.<br>
>>>> >><br>
>>>> >> 3) The compiler-rt build drops the ${arch} suffix from its libs, and<br>
>>>> >> the "clang/runtime" build uses CMAKE_INSTALL_PREFIX to control where<br>
>>>> >> the compiler-rt libs go.<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 "universal<br>
>>>> > binaries" there - a single static/dynamic runtime<br>
>>>> > which contains runtimes for all the possible architectures. We 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 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 against<br>
>>>> >> > the first stage build directory? Will this "find_package" thing 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>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div>
</div></div>