llvm dev - Mar 2017 - de-posixifying list tests?

Rafael, Filipe,

I am looking at the fixes you apply to sanitizer tests and they worry me.
(e.g. https://reviews.llvm.org/D31498)
The fixes are mostly mechanical and thus every single change looks safe,
but given the amount of changes there is large risk to cripple some of the
tests
in a way that they will stop detecting failures.

When I write a test for new functionality, I always verify that the test
fails w/o the implementation
of that functionality (i.e. that the test catches the failure). I hope most
of the tests are developed this way too. But when you modify the tests, I
bet you only verify that the passing test is still passing. Am I right? If
yes, this is not the way to modify tests, unfortunately.

So, my question is: did you consider modifying the environment to closer
match what already works well on posix, instead of massively changing
tests?

Thanks,

--kcc
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170330/3a974136/attachment-0001.html>

On 30 March 2017 at 11:38, Kostya Serebryany <kcc at google.com>
wrote:
> Rafael, Filipe,
>
> I am looking at the fixes you apply to sanitizer tests and they worry me.
> (e.g. https://reviews.llvm.org/D31498)
> The fixes are mostly mechanical and thus every single change looks safe,
> but given the amount of changes there is large risk to cripple some of the
> tests
> in a way that they will stop detecting failures.
>
> When I write a test for new functionality, I always verify that the test
> fails w/o the implementation
> of that functionality (i.e. that the test catches the failure). I hope most
> of the tests are developed this way too. But when you modify the tests, I
> bet you only verify that the passing test is still passing. Am I right? If
> yes, this is not the way to modify tests, unfortunately.
It is probably not as comprehensive and testing with the functionality
disabled, but I do tent to try to break the test locally. For example,
when changing from [] to count I will check that the test does catch
having the wrong count.
> So, my question is: did you consider modifying the environment to closer
> match what already works well on posix, instead of massively changing
tests?
Yes, unfortunately there is no good way to do that as far as I can
tell. Different posix shells on windows have different oddities and
they break from time to time (which is probably why we have an
internal shell in the first place).

Currently the only big feature that I am changing tests for is
avoiding a subshell. The rest I seem to be able to implement in the
internal shell. I have just started sending the shell patches up for
review.

The difference to the posix tests are fairly small. I have attached a
diff showing all that is left from current trunk.

Cheers,
Rafael
-------------- next part --------------
diff --git a/compiler-rt/test/asan/TestCases/Posix/closed-fds.cc
b/compiler-rt/test/asan/TestCases/Posix/closed-fds.cc
index b7bca26..75e3216a 100644
--- a/compiler-rt/test/asan/TestCases/Posix/closed-fds.cc
+++ b/compiler-rt/test/asan/TestCases/Posix/closed-fds.cc
@@ -2,7 +2,8 @@
 // symbolizer still works.
 
 // RUN: rm -f %t.log.*
-// RUN: %clangxx_asan -O0 %s -o %t 2>&1 &&
%env_asan_opts=log_path='"%t.log"':verbosity=2 not %run %t
2>&1
+// RUN: %clangxx_asan -O0 %s -o %t
+// RUN: %env_asan_opts=log_path='"%t.log"':verbosity=2 not
%run %t
 // RUN: FileCheck %s --check-prefix=CHECK-FILE < %t.log.*
 
 // FIXME: copy %t.log back from the device and re-enable on Android.
diff --git a/compiler-rt/test/asan/TestCases/Posix/coverage-fork-direct.cc
b/compiler-rt/test/asan/TestCases/Posix/coverage-fork-direct.cc
index c196719..dec7825 100644
--- a/compiler-rt/test/asan/TestCases/Posix/coverage-fork-direct.cc
+++ b/compiler-rt/test/asan/TestCases/Posix/coverage-fork-direct.cc
@@ -1,8 +1,10 @@
 // RUN: %clangxx_asan -fsanitize-coverage=func %s -o %t
 // RUN: rm -rf %T/coverage-fork-direct
 // RUN: mkdir -p %T/coverage-fork-direct && cd %T/coverage-fork-direct
-// RUN: (%env_asan_opts=coverage=1:coverage_direct=1:verbosity=1 %run %t; \
-// RUN:  %sancov rawunpack *.sancov.raw; %sancov print *.sancov) 2>&1
+// RUN: %env_asan_opts=coverage=1:coverage_direct=1:verbosity=1 %run %t >
%t.log 2>&1
+// RUN: %sancov rawunpack *.sancov.raw
+// RUN: %sancov print *.sancov >> %t.log 2>&1
+// RUN: FileCheck %s < %t.log
 //
 // XFAIL: android
 
@@ -34,5 +36,4 @@ int main(int argc, char **argv) {
 
 // CHECK-DAG: Child PID: [[ChildPID:[0-9]+]]
 // CHECK-DAG: Parent PID: [[ParentPID:[0-9]+]]
-// CHECK-DAG: read 3 PCs from {{.*}}.[[ParentPID]].sancov
-// CHECK-DAG: read 1 PCs from {{.*}}.[[ChildPID]].sancov
+// CHECK: read 3 64-bit PCs from {{.*}}.[[ParentPID]].sancov
diff --git a/compiler-rt/test/asan/TestCases/Posix/coverage-sandboxing.cc
b/compiler-rt/test/asan/TestCases/Posix/coverage-sandboxing.cc
index c4e6bc7..431dce1 100644
--- a/compiler-rt/test/asan/TestCases/Posix/coverage-sandboxing.cc
+++ b/compiler-rt/test/asan/TestCases/Posix/coverage-sandboxing.cc
@@ -12,9 +12,9 @@
 // RUN: %env_asan_opts=coverage=1:verbosity=1 %run %t a b 2>&1 |
FileCheck %s --check-prefix=CHECK-sandbox
 // RUN: %sancov unpack coverage_sandboxing_test.sancov.packed
 // RUN: cd ..
-// RUN: %sancov print vanilla/`basename %dynamiclib`*.sancov > vanilla.txt
-// RUN: %sancov print sandbox1/`basename %dynamiclib`*.sancov > sandbox1.txt
-// RUN: %sancov print sandbox2/`basename %dynamiclib`*.sancov > sandbox2.txt
+// RUN: %sancov print vanilla/%xdynamiclib_filename*.sancov > vanilla.txt
+// RUN: %sancov print sandbox1/%xdynamiclib_filename*.sancov > sandbox1.txt
+// RUN: %sancov print sandbox2/%xdynamiclib_filename*.sancov > sandbox2.txt
 // RUN: diff vanilla.txt sandbox1.txt
 // RUN: diff vanilla.txt sandbox2.txt
 // RUN: rm -r %T/coverage_sandboxing_test
diff --git a/compiler-rt/test/asan/TestCases/Posix/deep_call_stack.cc
b/compiler-rt/test/asan/TestCases/Posix/deep_call_stack.cc
index 18ba563..2d2b056 100644
--- a/compiler-rt/test/asan/TestCases/Posix/deep_call_stack.cc
+++ b/compiler-rt/test/asan/TestCases/Posix/deep_call_stack.cc
@@ -1,6 +1,7 @@
 // Check that UAR mode can handle very deep recusrion.
-// RUN: %clangxx_asan -O2 %s -o %t && \
-// RUN:   (ulimit -s 4096; %env_asan_opts=detect_stack_use_after_return=1 %run
%t) 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O2 %s -o %t
+// RUN: ulimit -s 4096
+// RUN: %env_asan_opts=detect_stack_use_after_return=1 %run %t 2>&1 |
FileCheck %s
 
 // Also check that use_sigaltstack+verbosity doesn't crash.
 // RUN:
%env_asan_opts=verbosity=1:use_sigaltstack=1:detect_stack_use_after_return=1
%run %t  | FileCheck %s

We could make lit tests more portable by unconditionally
setting execute_external to False in lit.common.cfg. This would use the lit
shell on all platforms, and make it impossible to accidentally write tests
that don't run on Windows from Posix.

We have a long term goal to do this anyway:
https://bugs.llvm.org//show_bug.cgi?id=5241 Want to be the first LLVM
project to do it?

On Thu, Mar 30, 2017 at 8:38 AM, Kostya Serebryany via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Rafael, Filipe,
>
> I am looking at the fixes you apply to sanitizer tests and they worry me.
> (e.g. https://reviews.llvm.org/D31498)
> The fixes are mostly mechanical and thus every single change looks safe,
> but given the amount of changes there is large risk to cripple some of the
> tests
> in a way that they will stop detecting failures.
>
> When I write a test for new functionality, I always verify that the test
> fails w/o the implementation
> of that functionality (i.e. that the test catches the failure). I hope
> most of the tests are developed this way too. But when you modify the
> tests, I bet you only verify that the passing test is still passing. Am I
> right? If yes, this is not the way to modify tests, unfortunately.
>
> So, my question is: did you consider modifying the environment to closer
> match what already works well on posix, instead of massively changing
> tests?
>
> Thanks,
>
> --kcc
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170330/086ee2f8/attachment.html>

On Thu, Mar 30, 2017 at 11:36 AM, Reid Kleckner <rnk at google.com> wrote:
> We could make lit tests more portable by unconditionally
> setting execute_external to False in lit.common.cfg. This would use the lit
> shell on all platforms, and make it impossible to accidentally write tests
> that don't run on Windows from Posix.
>
> We have a long term goal to do this anyway: https://bugs.llvm.org/
> /show_bug.cgi?id=5241 Want to be the first LLVM project to do it?
>
Yes, that would make total sense.

>
> On Thu, Mar 30, 2017 at 8:38 AM, Kostya Serebryany via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Rafael, Filipe,
>>
>> I am looking at the fixes you apply to sanitizer tests and they worry
me.
>> (e.g. https://reviews.llvm.org/D31498)
>> The fixes are mostly mechanical and thus every single change looks
safe,
>> but given the amount of changes there is large risk to cripple some of
>> the tests
>> in a way that they will stop detecting failures.
>>
>> When I write a test for new functionality, I always verify that the
test
>> fails w/o the implementation
>> of that functionality (i.e. that the test catches the failure). I hope
>> most of the tests are developed this way too. But when you modify the
>> tests, I bet you only verify that the passing test is still passing. Am
I
>> right? If yes, this is not the way to modify tests, unfortunately.
>>
>> So, my question is: did you consider modifying the environment to
closer
>> match what already works well on posix, instead of massively changing
>> tests?
>>
>> Thanks,
>>
>> --kcc
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170330/d9ab0f65/attachment.html>

On 30 March 2017 at 14:36, Reid Kleckner <rnk at google.com>
wrote:
> We could make lit tests more portable by unconditionally setting
> execute_external to False in lit.common.cfg. This would use the lit shell
on
> all platforms, and make it impossible to accidentally write tests that
don't
> run on Windows from Posix.
>
> We have a long term goal to do this anyway:
> https://bugs.llvm.org//show_bug.cgi?id=5241 Want to be the first LLVM
> project to do it?
Interesting. I will keep sending the patches for review. Of the things
I have seen used that I don't plan to implement right away is
subshell.

Cheers,
Rafael