This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rsundahl on Dec 2 2022, 1:48 PM.

Details

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

Diff Detail

Event Timeline

rsundahl created this revision.Dec 2 2022, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2022, 1:48 PM
Herald added a subscriber: Enna1. · View Herald Transcript
rsundahl requested review of this revision.Dec 2 2022, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2022, 1:48 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
This revision is now accepted and ready to land.Dec 2 2022, 3:41 PM
MaskRay added a subscriber: MaskRay.Dec 2 2022, 4:29 PM

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 added a comment.EditedDec 7 2022, 12:32 PM

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 updated this revision to Diff 481069.Dec 7 2022, 2:28 PM

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

rsundahl updated this revision to Diff 482559.Dec 13 2022, 10:54 AM

Rebased to get current.

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

"runtime runtime error:" this seems wrong

rsundahl marked an inline comment as done.Dec 19 2022, 10:48 AM

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

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