> This is a public interface. But ASan runtime (and test-suite) strongly depends on > the instrumentation pass in Clang.How do you feel about adding a runtime init check of a version number defined by asan_interface.h? -Greg On Tue, Apr 22, 2014 at 11:16 AM, Alexey Samsonov <samsonov at google.com> wrote:> > On Tue, Apr 22, 2014 at 10:43 AM, Greg Fitzgerald <garious at gmail.com> wrote: >> >> Sorry for the slow replies. I'm out on vacation this week. >> >> >> Alexey wrote: >> > If you want to test the sanitizer runtiume library "during development", >> > you should verify that it works with the Clang at hand. >> >> I want to test an implementation of libraries, not that clang links a >> library in its install directory. We only need one clang test for the >> latter (not 100) and that test already exists in the clang test suite. > > > No. The test in the clang test suite only calls "clang -fsanitize=address > -###" and matches > the command Clang will call linker with. It doesn't verify that the link > will succeed, or that > the resulting binary would run and produce expected output. > >> >> >> >> > Sanitizer runtime and the compiler are tightly >> > coupled, why would you want to test the former in isolation? >> >> They aren't that tightly-coupled though. There's an interface and an >> implementation. That interface rarely changes relative to the number >> of changes to the implementation. >> >> >> https://github.com/llvm-mirror/compiler-rt/commits/master/include/sanitizer > > > This is a public interface. But ASan runtime (and test-suite) strongly > depends on the instrumentation > pass in Clang. The latter can define hidden experimental flags we are > testing. Instrumentation pass > and compiler-rt library depend on each other. There were _several_ changes > in instrumentation pass > last week, most of which required a corresponding change in compiler-rt: > > https://github.com/llvm-mirror/llvm/commits/master/lib/Transforms/Instrumentation/AddressSanitizer.cpp > >> >> >> https://github.com/llvm-mirror/compiler-rt/commits/master/lib > > >> >> >> >> >> > Modifying Clang driver for testing-only purposes, especially for testing >> > in a non-default >> > configuration doesn't sound good to me. >> >> Agreed and that's not what's happening. The optional part is adding a >> -L flag to the invocation of clang within the compiler-rt test suite. >> If using the monolithic build, those flags are not added and clang >> finds the libs via its hardcoded '-L' flag. >> >> >> Yury wrote: >> > It's not that easy. Some tests require passing environment variables >> >> As Evgeniy mentioned, it's a short enough list of variables that you >> can whitelist them. LD_PRELOAD, LD_LIBRARY_PATH, ASAN_OPTIONS, etc. >> >> >> Evgeniy wrote: >> > Greg, do you copy binaries to the device on %clang or on %run? >> >> %run. Can you point me to a case where you needed to override %clang for >> that? >> >> Thanks, >> Greg >> >> On Mon, Apr 21, 2014 at 12:54 AM, Evgeniy Stepanov <eugenis at google.com> >> wrote: >> > Both %run and my symlink approach add certain (yet undocumented) >> > requirements on the tests, but they should be 100% robust if those >> > requirements are followed. >> > Greg, do you copy binaries to the device on %clang or on %run? The >> > latter would miss shared libraries that are not executed directly. >> > Environment can be recreated on the device, my script attempts to do >> > it (for several whitelisted variables). >> > We don't preserve ulimit setting, I modified one or two tests to not >> > rely on that. >> > >> > I don't mind switching to a combined approach - copy to device on >> > %clang, and replace symlink hacks with %run. >> > >> > >> > >> > On Mon, Apr 21, 2014 at 9:09 AM, Yury Gribov <y.gribov at samsung.com> >> > wrote: >> >>> We considered adding "%run" to all binary invocations, >> >>> but dropped this idea. I don't remember the details, but IIRC %run is >> >>> just not general enough. >> >> >> >> >> >> IMHO this is where simplicity of lit approach starts to fail - >> >> important >> >> information (environment variables, dependent shared libs, expected >> >> test >> >> status, etc.) is buried inside arbitrarily complex runstrings. >> >> >> >> -Y > > > > > -- > Alexey Samsonov, Mountain View, CA
On Tue, Apr 22, 2014 at 12:22 PM, Greg Fitzgerald <garious at gmail.com> wrote:> > This is a public interface. But ASan runtime (and test-suite) strongly > depends on > > the instrumentation pass in Clang. > > How do you feel about adding a runtime init check of a version number > defined by asan_interface.h? >We sometimes bump __asan_init_vX function name for important incompatible changes, that is, use a link-time check. Not sure why we would need to maintain an ASan version in public header.> > -Greg > > > On Tue, Apr 22, 2014 at 11:16 AM, Alexey Samsonov <samsonov at google.com> > wrote: > > > > On Tue, Apr 22, 2014 at 10:43 AM, Greg Fitzgerald <garious at gmail.com> > wrote: > >> > >> Sorry for the slow replies. I'm out on vacation this week. > >> > >> > >> Alexey wrote: > >> > If you want to test the sanitizer runtiume library "during > development", > >> > you should verify that it works with the Clang at hand. > >> > >> I want to test an implementation of libraries, not that clang links a > >> library in its install directory. We only need one clang test for the > >> latter (not 100) and that test already exists in the clang test suite. > > > > > > No. The test in the clang test suite only calls "clang -fsanitize=address > > -###" and matches > > the command Clang will call linker with. It doesn't verify that the link > > will succeed, or that > > the resulting binary would run and produce expected output. > > > >> > >> > >> > >> > Sanitizer runtime and the compiler are tightly > >> > coupled, why would you want to test the former in isolation? > >> > >> They aren't that tightly-coupled though. There's an interface and an > >> implementation. That interface rarely changes relative to the number > >> of changes to the implementation. > >> > >> > >> > https://github.com/llvm-mirror/compiler-rt/commits/master/include/sanitizer > > > > > > This is a public interface. But ASan runtime (and test-suite) strongly > > depends on the instrumentation > > pass in Clang. The latter can define hidden experimental flags we are > > testing. Instrumentation pass > > and compiler-rt library depend on each other. There were _several_ > changes > > in instrumentation pass > > last week, most of which required a corresponding change in compiler-rt: > > > > > https://github.com/llvm-mirror/llvm/commits/master/lib/Transforms/Instrumentation/AddressSanitizer.cpp > > > >> > >> > >> https://github.com/llvm-mirror/compiler-rt/commits/master/lib > > > > > >> > >> > >> > >> > >> > Modifying Clang driver for testing-only purposes, especially for > testing > >> > in a non-default > >> > configuration doesn't sound good to me. > >> > >> Agreed and that's not what's happening. The optional part is adding a > >> -L flag to the invocation of clang within the compiler-rt test suite. > >> If using the monolithic build, those flags are not added and clang > >> finds the libs via its hardcoded '-L' flag. > >> > >> > >> Yury wrote: > >> > It's not that easy. Some tests require passing environment variables > >> > >> As Evgeniy mentioned, it's a short enough list of variables that you > >> can whitelist them. LD_PRELOAD, LD_LIBRARY_PATH, ASAN_OPTIONS, etc. > >> > >> > >> Evgeniy wrote: > >> > Greg, do you copy binaries to the device on %clang or on %run? > >> > >> %run. Can you point me to a case where you needed to override %clang > for > >> that? > >> > >> Thanks, > >> Greg > >> > >> On Mon, Apr 21, 2014 at 12:54 AM, Evgeniy Stepanov <eugenis at google.com> > >> wrote: > >> > Both %run and my symlink approach add certain (yet undocumented) > >> > requirements on the tests, but they should be 100% robust if those > >> > requirements are followed. > >> > Greg, do you copy binaries to the device on %clang or on %run? The > >> > latter would miss shared libraries that are not executed directly. > >> > Environment can be recreated on the device, my script attempts to do > >> > it (for several whitelisted variables). > >> > We don't preserve ulimit setting, I modified one or two tests to not > >> > rely on that. > >> > > >> > I don't mind switching to a combined approach - copy to device on > >> > %clang, and replace symlink hacks with %run. > >> > > >> > > >> > > >> > On Mon, Apr 21, 2014 at 9:09 AM, Yury Gribov <y.gribov at samsung.com> > >> > wrote: > >> >>> We considered adding "%run" to all binary invocations, > >> >>> but dropped this idea. I don't remember the details, but IIRC %run > is > >> >>> just not general enough. > >> >> > >> >> > >> >> IMHO this is where simplicity of lit approach starts to fail - > >> >> important > >> >> information (environment variables, dependent shared libs, expected > >> >> test > >> >> status, etc.) is buried inside arbitrarily complex runstrings. > >> >> > >> >> -Y > > > > > > > > > > -- > > Alexey Samsonov, Mountain View, CA >-- Alexey Samsonov, Mountain View, CA -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140422/9c6bf10b/attachment.html>
Thanks for your feedback Alexey. I've decided to abandon my effort to include the sanitizer lit tests in the compiler-rt component. They needs to be part of the clang component because the clang-generated instrumentation is nontrivial. -Greg On Tue, Apr 22, 2014 at 12:39 PM, Alexey Samsonov <samsonov at google.com> wrote:> > On Tue, Apr 22, 2014 at 12:22 PM, Greg Fitzgerald <garious at gmail.com> wrote: >> >> > This is a public interface. But ASan runtime (and test-suite) strongly >> > depends on >> > the instrumentation pass in Clang. >> >> How do you feel about adding a runtime init check of a version number >> defined by asan_interface.h? > > > We sometimes bump __asan_init_vX function name for important incompatible > changes, that is, use > a link-time check. Not sure why we would need to maintain an ASan version in > public header. > >> >> >> -Greg >> >> >> On Tue, Apr 22, 2014 at 11:16 AM, Alexey Samsonov <samsonov at google.com> >> wrote: >> > >> > On Tue, Apr 22, 2014 at 10:43 AM, Greg Fitzgerald <garious at gmail.com> >> > wrote: >> >> >> >> Sorry for the slow replies. I'm out on vacation this week. >> >> >> >> >> >> Alexey wrote: >> >> > If you want to test the sanitizer runtiume library "during >> >> > development", >> >> > you should verify that it works with the Clang at hand. >> >> >> >> I want to test an implementation of libraries, not that clang links a >> >> library in its install directory. We only need one clang test for the >> >> latter (not 100) and that test already exists in the clang test suite. >> > >> > >> > No. The test in the clang test suite only calls "clang >> > -fsanitize=address >> > -###" and matches >> > the command Clang will call linker with. It doesn't verify that the link >> > will succeed, or that >> > the resulting binary would run and produce expected output. >> > >> >> >> >> >> >> >> >> > Sanitizer runtime and the compiler are tightly >> >> > coupled, why would you want to test the former in isolation? >> >> >> >> They aren't that tightly-coupled though. There's an interface and an >> >> implementation. That interface rarely changes relative to the number >> >> of changes to the implementation. >> >> >> >> >> >> >> >> https://github.com/llvm-mirror/compiler-rt/commits/master/include/sanitizer >> > >> > >> > This is a public interface. But ASan runtime (and test-suite) strongly >> > depends on the instrumentation >> > pass in Clang. The latter can define hidden experimental flags we are >> > testing. Instrumentation pass >> > and compiler-rt library depend on each other. There were _several_ >> > changes >> > in instrumentation pass >> > last week, most of which required a corresponding change in compiler-rt: >> > >> > >> > https://github.com/llvm-mirror/llvm/commits/master/lib/Transforms/Instrumentation/AddressSanitizer.cpp >> > >> >> >> >> >> >> https://github.com/llvm-mirror/compiler-rt/commits/master/lib >> > >> > >> >> >> >> >> >> >> >> >> >> > Modifying Clang driver for testing-only purposes, especially for >> >> > testing >> >> > in a non-default >> >> > configuration doesn't sound good to me. >> >> >> >> Agreed and that's not what's happening. The optional part is adding a >> >> -L flag to the invocation of clang within the compiler-rt test suite. >> >> If using the monolithic build, those flags are not added and clang >> >> finds the libs via its hardcoded '-L' flag. >> >> >> >> >> >> Yury wrote: >> >> > It's not that easy. Some tests require passing environment variables >> >> >> >> As Evgeniy mentioned, it's a short enough list of variables that you >> >> can whitelist them. LD_PRELOAD, LD_LIBRARY_PATH, ASAN_OPTIONS, etc. >> >> >> >> >> >> Evgeniy wrote: >> >> > Greg, do you copy binaries to the device on %clang or on %run? >> >> >> >> %run. Can you point me to a case where you needed to override %clang >> >> for >> >> that? >> >> >> >> Thanks, >> >> Greg >> >> >> >> On Mon, Apr 21, 2014 at 12:54 AM, Evgeniy Stepanov <eugenis at google.com> >> >> wrote: >> >> > Both %run and my symlink approach add certain (yet undocumented) >> >> > requirements on the tests, but they should be 100% robust if those >> >> > requirements are followed. >> >> > Greg, do you copy binaries to the device on %clang or on %run? The >> >> > latter would miss shared libraries that are not executed directly. >> >> > Environment can be recreated on the device, my script attempts to do >> >> > it (for several whitelisted variables). >> >> > We don't preserve ulimit setting, I modified one or two tests to not >> >> > rely on that. >> >> > >> >> > I don't mind switching to a combined approach - copy to device on >> >> > %clang, and replace symlink hacks with %run. >> >> > >> >> > >> >> > >> >> > On Mon, Apr 21, 2014 at 9:09 AM, Yury Gribov <y.gribov at samsung.com> >> >> > wrote: >> >> >>> We considered adding "%run" to all binary invocations, >> >> >>> but dropped this idea. I don't remember the details, but IIRC %run >> >> >>> is >> >> >>> just not general enough. >> >> >> >> >> >> >> >> >> IMHO this is where simplicity of lit approach starts to fail - >> >> >> important >> >> >> information (environment variables, dependent shared libs, expected >> >> >> test >> >> >> status, etc.) is buried inside arbitrarily complex runstrings. >> >> >> >> >> >> -Y >> > >> > >> > >> > >> > -- >> > Alexey Samsonov, Mountain View, CA > > > > > -- > Alexey Samsonov, Mountain View, CA