Page MenuHomePhabricator

[OpenMP] Add scaffolding for negative runtime tests
ClosedPublic

Authored by jdenny on Apr 21 2020, 7:42 AM.

Details

Summary

Without this patch, the openmp project's test suites do not appear to
have support for negative tests. However, D78170 needs to add a test
that an expected runtime failure occurs.

This patch makes not visible in all of the openmp project's test
suites. In all but libomptarget/test, it should be possible for a
test author to insert not before a use of the lit substitution for
running a test program. In libomptarget/test, that substitution is
target-specific, and its value is echo when the target is not
available. In that case, inserting not before a lit substitution
would expect an echo fail, so this patch instead defines a separate
lit substitution for expected runtime fails.

Diff Detail

Event Timeline

jdenny created this revision.Apr 21 2020, 7:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2020, 7:42 AM
This revision is now accepted and ready to land.Apr 21 2020, 9:32 AM

This patch makes not visible in all of the openmp project's test
suites. In all but libomptarget/test, it should be possible for a
test author to insert not before a use of the lit substitution for
running a test program.

I see no config.substitutions for the other test suites, so I guess it will try to use not from PATH? IIRC LLVM and Clang add a substitution from not to the full path (via use_default_substitutions, but that's probably not available for standalone builds). Doing so is a bit tricky because "not" may be part of the checked strings. LLVM does some funny \| \bnot\b regex (which I have yet to understand), so I leave it up to you if you want to dig into that. Alternatively, we can either provide %not (which makes it unambiguous) or just not wire it up for the other suites and adapt the commit message.

I see no config.substitutions for the other test suites, so I guess it will try to use not from PATH?

Ah, thanks for pointing that out. I wasn't being careful with those test suites because I'm not actually adding tests to them yet. No, not is not guaranteed to be in PATH.

IIRC LLVM and Clang add a substitution from not to the full path (via use_default_substitutions, but that's probably not available for standalone builds). Doing so is a bit tricky because "not" may be part of the checked strings.

What do you mean by "checked strings" here? Lit substitutions only happen on RUN lines, right?

LLVM does some funny \| \bnot\b regex (which I have yet to understand), so I leave it up to you if you want to dig into that.

I don't understand that either. That makes sense only when not stdin is a pipe. Grepping for RUN: not shows that's not always true. That's probably why not is also in PATH, but that makes the substitution seem redundant. When I remove the substitution (from lit/llvm/config.py), check-llvm behaves. I haven't yet run check-clang, etc.

Alternatively, we can either provide %not (which makes it unambiguous)

My only argument against that solution is that no other LLVM test suite does this. I grepped for RUN:.*%not to verify.

\bnot\b works in a quick example I just tried.

or just not wire it up for the other suites and adapt the commit message.

Maybe.

I just noticed another issue: ompt tests are skipped if FileCheck is not available. This is configured in openmp/runtime/test/lit.cfg. However, other openmp test suites that use FileCheck don't bother. Why is ompt different?

I just noticed another issue: ompt tests are skipped if FileCheck is not available. This is configured in openmp/runtime/test/lit.cfg. However, other openmp test suites that use FileCheck don't bother. Why is ompt different?

Probably historic reasons, the OMPT tests were the first to use FileCheck. If other tests use it, we should probably make it required for all suites.

jdenny updated this revision to Diff 259085.Apr 21 2020, 12:51 PM

Added the substitutions to actually make not available.

LLVM does some funny \| \bnot\b regex (which I have yet to understand), so I leave it up to you if you want to dig into that.

I don't understand that either. That makes sense only when not stdin is a pipe. Grepping for RUN: not shows that's not always true. That's probably why not is also in PATH, but that makes the substitution seem redundant. When I remove the substitution (from lit/llvm/config.py), check-llvm behaves. I haven't yet run check-clang, etc.

check-clang also behaves.

Alternatively, we can either provide %not (which makes it unambiguous)

My only argument against that solution is that no other LLVM test suite does this. I grepped for RUN:.*%not to verify.

\bnot\b works in a quick example I just tried.

or just not wire it up for the other suites and adapt the commit message.

Maybe.

Hopefully by providing the familiar not now, we'll encourage more negative tests. To that end, I went with the \bnot\b substitution.

I checked that it actually works this time by adding a RUN: not false in each openmp test suite. Of course, I didn't include that in the commit.

I'll wait a bit longer to push in case people have comments for this update.

I just noticed another issue: ompt tests are skipped if FileCheck is not available. This is configured in openmp/runtime/test/lit.cfg. However, other openmp test suites that use FileCheck don't bother. Why is ompt different?

Probably historic reasons, the OMPT tests were the first to use FileCheck.

Thanks.

If other tests use it, we should probably make it required for all suites.

Maybe in another patch.

Hahnfeld accepted this revision.Apr 21 2020, 1:10 PM

LGTM as well

This revision was automatically updated to reflect the committed changes.