[LLVMdev] Building sanitizers for Android

Tue Apr 15 17:08:18 PDT 2014

Hi Greg,

First of all, sorry for the late response (I'm in the process of moving to
California).

On Fri, Apr 4, 2014 at 6:13 PM, Greg Fitzgerald <garious at gmail.com> wrote:

> Alexey,
>
> >> Some good news, the drivers (both gcc and clang) allow us to put the
> >> '-L' parameters after the '-l' parameters.
>
> I made these changes locally and it went really well.  The patch to
> clang is quite small and only one unit-test needed updating.  In
> compiler-rt, I updated the flags passed to clang to include a
> '-L${COMPILER_RT_BINARY_DIR}/lib' and
> '-I${COMPILER_RT_BINARY_DIR}/include' flag.  All tests pass except for
> the default blacklist tests. I had to move those into the clang test
> suite.
>

I've looked at the patch to Clang. I'll try to explain why I'm opposed to
this approach.

You are making the test suite configuration more and more dependent on the
specific
version of the specific Compiler (ToT Clang), while recently we were moving
in the opposite direction.
You even hardcode the knowledge of how the sanitizer headers and sanitizer
libraries are used (by adding -I and -L flags).

I treat sanitizer test-suite as toolchain tests, not a library tests. We
need to verify that simple command
"clang -fsanitize=address foo.cc" works, and produce a binary that works as
expected. As an illustration, suppose
the following sequence of events
1) we land your changes to Clang driver, so that it uses -lasan
-L/path/to/clang/resource/dir
2) we add additional -L/path/to/compiler-rt/libs to all %clang invocations
in lit test-suite.
3) someone accidentally breaks the Clang dirver, and now it uses -L/*wrong/*
path/to/clang/resource/dir
We will not be able to detect this error. The test-suite is green, but the
functionality is broken.

IMO we should just deal with the fact that sanitizer test-suite depends not
only on sanitizer libraries themselves,
but also on LLVM/Clang libs and tools.

>
> As for cross-compilation and test ARM Linux, I needed to update all
> tests to include '%sim' before invoking cross-compiled executables.
> For x86, the variable is empty.  For cross-compiling, it's a
> user-configured ${COMPILER_RT_EMULATOR}, which for ARM Linux, is QEMU.
>  There are currently a handful of test failures.  If you'd like, I can
> XFAIL them and commit this stuff sooner than later.
>

This looks familiar to what Evgeniy did for running lit test suite on
Android. This is now possible
w/o modifying any RUN-lines, see the hacky scripts in
compiler-rt/test/asan/android_commands/
folder. Can you use the similar approach for QEMU?

> Do you want these patches?
>
> -Greg
>
>
> On Fri, Apr 4, 2014 at 10:22 AM, Greg Fitzgerald <garious at gmail.com>
> wrote:
> >>>> in a standalone mode, then "make check-compiler-rt" in that tree
> >>>> wouldn't rebuild runtimes, and instead assume that they are provided
> by toolchain
> >> What is wrong with that approach?
> >
> > Not wrong, just inconsistent.  I'm trying to test the
> > just-built-runtime before running the 'install' step.  That's the way
> > all the other projects work, "all check-all install" in that order.
> > In other words, "build, verify, commit".
> >
> >
> >> And, again, we have to ensure that -L/path/to/new/asan provided by
> >> user "wins" over -L/path/to/clang/resource/dir, and we have strong
> >> reasons to put the latter as early in the link line as possible.
> >
> > Some good news, the drivers (both gcc and clang) allow us to put the
> > '-L' parameters after the '-l' parameters.  So we can put -lasan at
> > the start of the parameter list to get the desired link order and -L
> > at the end so that the user-specified runtime is always chosen.
> >
> > -Greg
> >
> >
> > On Fri, Apr 4, 2014 at 5:57 AM, Alexey Samsonov <samsonov at google.com>
> wrote:
> >>
> >> On Thu, Apr 3, 2014 at 9:23 PM, Greg Fitzgerald <garious at gmail.com>
> wrote:
> >>>
> >>> > I don't think it's a good idea to let user hijack
> >>> > the driver and stuff in custom version of
> >>> > ASan runtime instead the one installed/built
> >>> > with compiler :)
> >>>
> >>> I'm okay with it.  This is open source.  If someone wants to put the
> >>> sanitizers on a shorter release cycle than the full toolchain, who are
> we to
> >>> hold them back?
> >>
> >>
> >> They can always rebuild the fresh ASan from source and replace the ASan
> >> runtime in toolchain with the new one.
> >>
> >>>
> >>>
> >>> > What do you mean by "invoking the test-suite  directly"?
> >>>
> >>> In Clang's CMakeLists, add_subdirectory() of compiler-rt's 'test'
> >>> directory.  I suggested we do this earlier, because of clang's
> hardcoded
> >>> dependency on the path to ASan.  Instead, I think a better solution is
> to
> >>> break the hardcoded dependency, allowing the test suite to point clang
> to
> >>> compiler-rt's local copy of ASan using a '-L' parameter.
> >>
> >>
> >> I don't believe it's needed anywhere except in the testing
> infrastructure
> >> you suggest. And, again, we have to ensure that -L/path/to/new/asan
> provided
> >> by user "wins" over -L/path/to/clang/resource/dir, and we have strong
> >> reasons to put the latter as early in the link line as possible. That
> said,
> >> ....
> >>
> >>>
> >>>
> >>> Greg
> >>>
> >>>
> >>> On Thursday, April 3, 2014, Alexey Samsonov <samsonov at google.com>
> wrote:
> >>>>
> >>>>
> >>>> On Thu, Apr 3, 2014 at 5:04 AM, Greg Fitzgerald <garious at gmail.com>
> >>>> wrote:
> >>>>>
> >>>>> > we would still want to use compiler-rt test-suite in a standalone
> >>>>> > mode, to test fully built/installed toolchains,
> >>>>> and even GCC.
> >>>>>
> >>>>> Sounds good.
> >>>>>
> >>>>>
> >>>>> > Clang driver links the static xsan runtimes from a hardcoded
> >>>>> > paths in Clang resource directory, and doesn't add flags like
> >>>>> > "-lasan -L/path/to/clang/resource/dir". I find this behavior
> >>>>> > reasonable.
> >>>>>
> >>>>> Can we change the flags to "-lasan -L/path/to/clang/resource/dir"?
>  If
> >>>>> we make that change,
> >>>>
> >>>>
> >>>> I don't think it's a good idea to let user hijack the driver and
> stuff in
> >>>> custom version of
> >>>> ASan runtime instead the one installed/built with compiler :)
> >>>>
> >>>>>
> >>>>> I can stop suggesting that the clang build should
> >>>>> invoke the compiler-rt test suite directly.  :-)
> >>>>
> >>>>
> >>>> I haven't yet fully understood your complaints.
> >>
> >>
> >>
> >>>>
> >>>> Note that if you build/configure compiler-rt
> >>>> in a standalone mode, then "make check-compiler-rt" in that tree
> wouldn't
> >>>> rebuild runtimes,
> >>>> and instead assume that they are provided by toolchain (that is, in
> >>>> standalone mode runtimes and
> >>>> test suites are effectively independent).
> >>
> >>
> >> ^^^^
> >> What is wrong with that approach?
> >>
> >>
> >>>>
> >>>> What do you mean by "invoking the test-suite directly"?
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> > Do you want to give it a try?
> >>>>
> >>>> I'll take a crack it, sure.
> >>>>
> >>>>
> >>>> By the way, locally, I now have just over half the ASan test suite
> >>>> passing ARM-Linux via QEMU.  It's been really convenient to build
> >>>> compiler-rt in standalone mode.  The edit-compile-run cycle is much
> >>>> shorter this way.  Thanks for your help in making this work!
> >>>>
> >>>> -Greg
> >>>>
> >>>>
> >>>>
> >>>> On Wed, Apr 2, 2014 at 7:56 AM, Alexey Samsonov <samsonov at google.com>
> >>>> wrote:
> >>>> > Hi Greg,
> >>>> >
> >>>> > On Wed, Apr 2, 2014 at 2:50 AM, Greg Fitzgerald <garious at gmail.com>
> >>>> > wrote:
> >>>> >>
> >>>> >> Alexey, Evgeniy,
> >>>> >>
> >>>> >> I propose the following steps to unify multi-arch support in
> >>>> >> compiler-rt:
> >>>> >>
> >>>> >> 1) The compiler-rt test suite adds
> "-L${CMAKE_CURRENT_BINARY_DIR}/lib"
> >>>> >> to its 'clang' variables.  This way we can test the sanitizers
> without
> >>>> >> installing any libs to the just-built-clang install directory.
> >>>> >
> >>>> >
> >>>> > This is possible, but please keep in mind that:
> >>>> > 1) we would still want to use compiler-rt test-suite in a standalone
> >>>> > mode,
> >>>> > to test fully built/installed toolchains,
> >>>> > and even GCC.
> >>>> > 2) Adding -L... wouldn't work: Clang driver links the static xsan
> >>>> > runtimes
> >>>> > from a hardcoded paths in Clang resource directory,
> >>>> > and doesn't add flags like "-lasan -L/path/to/clang/resource/dir". I
> >>>> > find
> >>>> > this behavior reasonable.
> >>>> >
> >>>> > I don't see a problem with the current approach - we can make "run
> >>>> > sanitizer
> >>>> > test suite" command in the
> >>>> > top-level build tree depend on "build/install compiler-rt
> >>>> > ExternalProject"
> >>>> > steps (see how it's done currently).
> >>>> >
> >>>> >>
> >>>> >>
> >>>> >> 2) The "clang/runtime" build calls ExternalProject once for each
> arch
> >>>> >> it needs of compiler-rt.  So once to create x86_64 libs and once
> for
> >>>> >> i386 libs.
> >>>> >>
> >>>> >> 3) The compiler-rt build drops the ${arch} suffix from its libs,
> and
> >>>> >> the "clang/runtime" build uses CMAKE_INSTALL_PREFIX to control
> where
> >>>> >> the compiler-rt libs go.
> >>>> >>
> >>>> >>
> >>>> >>
> "CMAKE_INSTALL_PREFIX=${CMAKE_CURRENT_BINARY_DIR}/lib/clang/${CLANG_VERSION}/x86_64-linux"
> >>>> >>
> >>>> >>
> >>>> >> 4) Remove multi-arch support from the compiler-rt build.  Instead,
> >>>> >> declare compiler-rt as an "any-arch" build, configured with:
> >>>> >>     CMAKE_CXX_COMPILER (defaults to just-built-clang)
> >>>> >>     CMAKE_CXX_FLAGS (defaults to llvm's default target)
> >>>> >>     COMPILER_RT_TEST_COMPILER (defaults to CMAKE_CXX_COMPILER)
> >>>> >>     COMPILER_RT_TEST_FLAGS (defaults to CMAKE_CXX_FLAGS)
> >>>> >
> >>>> >
> >>>> > Unfortunately, this could be problematic for Mac - we use "universal
> >>>> > binaries" there - a single static/dynamic runtime
> >>>> > which contains runtimes for all the possible architectures. We might
> >>>> > work
> >>>> > around this, however, by adding a custom
> >>>> > Mac-specific step that would merge all single-arch binaries into a
> >>>> > multi-arch one - I think this is fine as long as we
> >>>> > greatly simplify the rest of the ugly build system.
> >>>> >
> >>>> > Do you want to give it a try?
> >>>> >
> >>>> >>
> >>>> >>
> >>>> >> Thoughts?
> >>>> >>
> >>>> >> Thanks,
> >>>> >> Greg
> >>>> >>
> >>>> >> On Tue, Apr 1, 2014 at 12:58 AM, Evgeniy Stepanov
> >>>> >> <eugeni.stepanov at gmail.com> wrote:
> >>>> >> > It does sound like Android is better suited for "honest"
> >>>> >> > cross-compilation, rather than "build compiler-rt for all
> targets we
> >>>> >> > can find" model.
> >>>> >> >
> >>>> >> > I'm still not convinced that we must require the "ninja install"
> >>>> >> > step.
> >>>> >> > Could we just "ninja clang" and then build the second stage
> against
> >>>> >> > the first stage build directory? Will this "find_package" thing
> not
> >>>> >> > work that way, or do you mean it as a way to copy target asan
> >>>> >> > runtime
> >>>> >> > where the first stage compiler will find it (or probably both)?
> >>>> >> >
> >>>> >> > On Mon, Mar 31, 2014 at 6:46 PM, Alexey Samsonov
> >>>> >> > <samsonov at google.com>
> >>>> >> > wrote:
> >>>> >
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> Alexey Samsonov, MSK
> >>
> >>
> >>
> >>
> >> --
> >> Alexey Samsonov, MSK
>

-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140415/0178affe/attachment.html>