Hi Greg,
On Wed, Apr 16, 2014 at 4:58 PM, Greg Fitzgerald <garious at gmail.com>
wrote:
> > First of all, sorry for the late response (I'm in the process of
moving
> to California).
>
> Welcome!
>
>
> > We need to verify that simple command "clang -fsanitize=address
foo.cc"
> works
>
> Agreed. The test suite is useful in many different scenarios:
>
> 1) Verifying the integrated clang.
> 2) Verifying the integrated gcc.
> 3) Verifying the libraries during development.
>
> I'm working to improve #3 and in a way that does not interfere with #1
or
> #2.
>
(1) should be the part of (3). If you want to test the sanitizer runtiume
library "during development",
you should verify that it works with the Clang at hand. Sanitizer runtime
and the compiler are tightly
coupled, why would you want to test the former in isolation?
> > we add additional -L/path/to/compiler-rt/libs to all %clang
invocations
> in lit test-suite.
>
> Yes, that's the next patch I have for you. But adding the '-L'
flag
> is optional. If you want to use the test suite to verify an
> integrated clang or gcc, you disable that.
>
Modifying Clang driver for testing-only purposes, especially for testing in
a non-default configuration
doesn't sound good to me. Also, I still don't get why you would want
that
(see comment above).
> > 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?
>
> Clever, but I hope we can try to avoid the symlink hackery. Locally,
> I've renamed "%sim %t" to "%run %t" so it reads
quite nicely, IMHO.
> It was a bunch of work to go change all the tests, but I think it's
> the right approach. Each cross-compiled variation configures
"%run"
> from the command-line when invoking CMake. No hacks.
>
I will reply to that shortly.
>
> -Greg
> .
>
> On Tue, Apr 15, 2014 at 5:08 PM, Alexey Samsonov <samsonov at
google.com>
> wrote:
> > 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
>
--
Alexey Samsonov, Mountain View, CA
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140418/7c7041ba/attachment.html>