[ubsan][test] Be more specific than "error:" with implicit-check-not

[rsundahl]

Summary

The string "error:" is too general and reports false positive from system
messages that can occur on stderr when using network devices for testing.
The string "runtime error:" is sufficiently unique for these connection
errors to pass through w/o a false positive.

rdar://100564373

[thetruestblue]

LGTM

[MaskRay]

The string "error:" is too general and reports false positive from system messages that can occur on stderr when using network devices for testing.

This is a bit opaque. Do you run tests remotely?

--implicit-check-not=error: is used a lot, also in other test directories, so I need to understand it better.

[rsundahl]

In D139230#3967991, @MaskRay wrote:

The string "error:" is too general and reports false positive from system messages that can occur on stderr when using network devices for testing.

This is a bit opaque. Do you run tests remotely?

--implicit-check-not=error: is used a lot, also in other test directories, so I need to understand it better.

Yes, this is from a remote connected device. I'll stipulate that it's not a general solution to the problem. It would be a good thing to embrace connected devices and to be resilient to dropped connections, retries, latency and reconnects (which I think that we should do). That said, it's a much bigger lift and it makes sense to do so in an RFC so we do it right. In defense of this change, I would say that it is to address a real issue now and that I wouldn't add other work-arounds to connection noise because the sources are infinite. I am really only asking to be slightly more discriminating in the pattern that we do not want to see. The specificity of the regex is arbitrary in the sense that it is meant to reject a particular pattern that is _known_ to be possible and not the universe of strings that could possibly have the pattern "error: " contained within it. You are correct that there are other tests that look for the same string but only the ones that are contained in compiler-rt are susceptible to connection noise. There are approximately 15 tests that use the pattern, all in ubsan. I'd be happy to refine those tests as well if you agree. Again, this is only increasing specificity to avoid false negatives.

[rsundahl]

Added additional ubsan tests that would also benefit from additional specificity.

[rsundahl]

Rebased to get current.

[MaskRay]

In D139230#3979232, @rsundahl wrote:

In D139230#3967991, @MaskRay wrote:

The string "error:" is too general and reports false positive from system messages that can occur on stderr when using network devices for testing.

This is a bit opaque. Do you run tests remotely?

--implicit-check-not=error: is used a lot, also in other test directories, so I need to understand it better.

Yes, this is from a remote connected device. I'll stipulate that it's not a general solution to the problem. It would be a good thing to embrace connected devices and to be resilient to dropped connections, retries, latency and reconnects (which I think that we should do). That said, it's a much bigger lift and it makes sense to do so in an RFC so we do it right. In defense of this change, I would say that it is to address a real issue now and that I wouldn't add other work-arounds to connection noise because the sources are infinite. I am really only asking to be slightly more discriminating in the pattern that we do not want to see. The specificity of the regex is arbitrary in the sense that it is meant to reject a particular pattern that is _known_ to be possible and not the universe of strings that could possibly have the pattern "error: " contained within it. You are correct that there are other tests that look for the same string but only the ones that are contained in compiler-rt are susceptible to connection noise. There are approximately 15 tests that use the pattern, all in ubsan. I'd be happy to refine those tests as well if you agree. Again, this is only increasing specificity to avoid false negatives.

--implicit-check-not=error: is very common among llvm-project, though no longer existent in compiler-rt after this patch.
A casual reader might ask why compiler-rt is different and needs the s/error:/runtime error:/ treatment...

As you acknowledged, this is a bigger lift. Have you tried sending an RFC?

compiler-rt/test/ubsan/TestCases/Pointer/align-assume-attribute-alloc_align-on-function.cpp

8 ↗

(On Diff #482669)

"runtime runtime error:" this seems wrong

[rsundahl]

In D139230#4003336, @MaskRay wrote:

In D139230#3979232, @rsundahl wrote:

In D139230#3967991, @MaskRay wrote:

The string "error:" is too general and reports false positive from system messages that can occur on stderr when using network devices for testing.

This is a bit opaque. Do you run tests remotely?

--implicit-check-not=error: is used a lot, also in other test directories, so I need to understand it better.

Yes, this is from a remote connected device. I'll stipulate that it's not a general solution to the problem. It would be a good thing to embrace connected devices and to be resilient to dropped connections, retries, latency and reconnects (which I think that we should do). That said, it's a much bigger lift and it makes sense to do so in an RFC so we do it right. In defense of this change, I would say that it is to address a real issue now and that I wouldn't add other work-arounds to connection noise because the sources are infinite. I am really only asking to be slightly more discriminating in the pattern that we do not want to see. The specificity of the regex is arbitrary in the sense that it is meant to reject a particular pattern that is _known_ to be possible and not the universe of strings that could possibly have the pattern "error: " contained within it. You are correct that there are other tests that look for the same string but only the ones that are contained in compiler-rt are susceptible to connection noise. There are approximately 15 tests that use the pattern, all in ubsan. I'd be happy to refine those tests as well if you agree. Again, this is only increasing specificity to avoid false negatives.

--implicit-check-not=error: is very common among llvm-project, though no longer existent in compiler-rt after this patch.
A casual reader might ask why compiler-rt is different and needs the s/error:/runtime error:/ treatment...

The extra specificity is not required for tests that are not (potentially) actually tested on a connected device. The other uses of just "error: " are done in a more controlled environment in which it would be extremely rare to see spurious output on stderr (or stdin) and the strings that are expected and unexpected by the tests are completely reproducible. In the case of compiler-rt, in the presence of a potentially connected device upon which the test is run, there is reason to be more specific when including or excluding strings that could be tainted with arbitrary connection noise in order to be better tolerant of that noise.

As you acknowledged, this is a bigger lift. Have you tried sending an RFC?

I have been considering this and I think it's a good idea. It's on my backlog to investigate whether or not anybody else is currently testing on devices and/or connected devices and evaluate whether we should rather be doing this upstream instead.

compiler-rt/test/ubsan/TestCases/Pointer/align-assume-attribute-alloc_align-on-function.cpp

8 ↗

(On Diff #482669)

Very definitely wrong. Thank you @MaskRay. (Fixed by https://reviews.llvm.org/D140321)

compiler-rt/test/ubsan/TestCases/Pointer/align-assume-attribute-align_value-on-paramvar.cpp

-// RUN: %clang   -x c   -fsanitize=alignment -O0 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
-// RUN: %clang   -x c   -fsanitize=alignment -O1 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
-// RUN: %clang   -x c   -fsanitize=alignment -O2 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
-// RUN: %clang   -x c   -fsanitize=alignment -O3 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
-
-// RUN: %clang   -x c++ -fsanitize=alignment -O0 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
-// RUN: %clang   -x c++ -fsanitize=alignment -O1 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
-// RUN: %clang   -x c++ -fsanitize=alignment -O2 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
-// RUN: %clang   -x c++ -fsanitize=alignment -O3 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
+// RUN: %clang   -x c   -fsanitize=alignment -O0 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="runtime error:"
+// RUN: %clang   -x c   -fsanitize=alignment -O1 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="runtime error:"
+// RUN: %clang   -x c   -fsanitize=alignment -O2 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="runtime error:"
+// RUN: %clang   -x c   -fsanitize=alignment -O3 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="runtime error:"
+
+// RUN: %clang   -x c++ -fsanitize=alignment -O0 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="runtime error:"
+// RUN: %clang   -x c++ -fsanitize=alignment -O1 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="runtime error:"
+// RUN: %clang   -x c++ -fsanitize=alignment -O2 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="runtime error:"
+// RUN: %clang   -x c++ -fsanitize=alignment -O3 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="runtime error:"
 
 #include <stdlib.h>
 
 char *passthrough(__attribute__((align_value(0x8000))) char *x) {
   return x;
 }
 
 int main(int argc, char* argv[]) {