Page MenuHomePhabricator

[UpdateCCTestChecks] Support --check-globals
ClosedPublic

Authored by jdenny on Jun 22 2021, 7:25 AM.

Details

Summary

This option is already supported by update_test_checks.py, but it can
also be useful in update_cc_test_checks.py. For example, I'd like to
use it in OpenMP offload codegen tests to check global variables like
.offload_maptypes*.

Diff Detail

Event Timeline

jdenny requested review of this revision.Jun 22 2021, 7:25 AM
jdenny created this revision.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 22 2021, 7:25 AM
Herald added a subscriber: sstefan1. · View Herald Transcript
jdoerfert accepted this revision.Jun 22 2021, 7:44 AM

LG, cool :)

This revision is now accepted and ready to land.Jun 22 2021, 7:44 AM
jdenny updated this revision to Diff 353765.Jun 22 2021, 1:28 PM

Fixed the reported test failure. (Sorry, I had accidentally put that fix in a later patch even though the associated test is in this patch.)

LG, cool :)

Thanks for the reviews.

arichardson accepted this revision.Jun 22 2021, 3:19 PM

Thanks for working on this! We have some tests downstream that check globals and currently have to use // UTC_ARGS: --disable to manually retain them.

The other update script tests compare to an expected output file instead of using Filecheck directives. For larger tests the separate files are a lot more readable, whereas it doesn't really matter for this test.

clang/test/utils/update_cc_test_checks/check-globals.test
35

Running lit to verify that the output is valid could be something we might want to do for the other update script tests. But I guess using the scripts to generate tests that are checked it is usually sufficient.

llvm/utils/update_cc_test_checks.py
357

I feel like this line could do with a comment since it's not immediately obvious why it's needed as part of this commit.

Thanks for working on this!

Thanks for the review.

We have some tests downstream that check globals and currently have to use // UTC_ARGS: --disable to manually retain them.

Does that mean you manually maintain FileCheck directives there?

The other update script tests compare to an expected output file instead of using Filecheck directives. For larger tests the separate files are a lot more readable, whereas it doesn't really matter for this test.

I believe the only reason I used FileCheck directives in this case is because I didn't want to hard-code expected metadata and attributes, so I wanted regexes. However, I don't have a lot of experience writing checks for such constructs, so maybe hard-coding those is not brittle at all. If so, I'd be happy to switch to expected output files, which I agree are more readable.

clang/test/utils/update_cc_test_checks/check-globals.test
35

I don't know a general rule for when it's worthwhile and when it's not.

In this patch, I felt it was worthwhile because it was easy to think the directives looked fine without realizing declarations were in the wrong order (that happened to me at one point during development). I'm not sure someone modifying update_cc_test_checks.py and running this test suite is necessarily going to know which tests in other test suites should be re-generated using update_cc_test_checks.py and re-run to be sure all is well.

On the other hand, I think I could be convinced it's not worthwhile in my later patches in this series.

llvm/utils/update_cc_test_checks.py
357

Sure, I'll work on that.

ggeorgakoudis added a comment.EditedJun 23 2021, 6:53 AM

LGTM too!

jdenny updated this revision to Diff 353998.Jun 23 2021, 8:59 AM
jdenny marked an inline comment as done.

Applied arichardson's suggestion: comment on exclusion of separator comments.

Rebased.

LGTM too!

Thanks.

This revision was landed with ongoing or failed builds.Jun 25 2021, 10:18 AM
This revision was automatically updated to reflect the committed changes.

Oh I see this assumes that lit.site.cfg.py is below clang_obj_root. I guess that's my problem, looking…

Oh I see this assumes that lit.site.cfg.py is below clang_obj_root. I guess that's my problem, looking…

Let me know if the patch isn't general enough, or if I can help in some other way.

Thanks.

I landed a fix attempt for my thing in fda790fbfa0cba426d5e3787429a51e09ec64c6d .

I landed a fix attempt for my thing in fda790fbfa0cba426d5e3787429a51e09ec64c6d .

Thanks!

Another thing: https://reviews.llvm.org/harbormaster/unit/view/789318/

# command stderr:
/var/lib/buildkite-agent/builds/llvm-project/clang/test/utils/update_cc_test_checks/check-globals.test:83:10: error: LIT-RUN: expected string not found in input
LIT-RUN: PASS: {{.*}} norm.c

Input was:
<<<<<<
            1: lit.py: /var/lib/buildkite-agent/builds/llvm-project/llvm/utils/lit/lit/llvm/config.py:436: note: using clang: /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang 
            2: -- Testing: 2 tests, 1 workers -- 
            3: PASS: update_cc_test_checks.py example :: norm.c (1 of 2) 
            4: PASS: update_cc_test_checks.py example :: igf.c (2 of 2)

Looks like the test expects order igf.c, norm.c while the output is the other way round. Should this just use LIT-RUN-DAG so that the order doesn't matter, or should we add a sorted() somewhere that the output is deterministic?

Another thing: https://reviews.llvm.org/harbormaster/unit/view/789318/

# command stderr:
/var/lib/buildkite-agent/builds/llvm-project/clang/test/utils/update_cc_test_checks/check-globals.test:83:10: error: LIT-RUN: expected string not found in input
LIT-RUN: PASS: {{.*}} norm.c

Input was:
<<<<<<
            1: lit.py: /var/lib/buildkite-agent/builds/llvm-project/llvm/utils/lit/lit/llvm/config.py:436: note: using clang: /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang 
            2: -- Testing: 2 tests, 1 workers -- 
            3: PASS: update_cc_test_checks.py example :: norm.c (1 of 2) 
            4: PASS: update_cc_test_checks.py example :: igf.c (2 of 2)

Looks like the test expects order igf.c, norm.c while the output is the other way round. Should this just use LIT-RUN-DAG so that the order doesn't matter, or should we add a sorted() somewhere that the output is deterministic?

I pushed cc60fa2685bd to fix that.

It looks like this patch breaks the utils/update_cc_test_checks/check-globals.test on stand-alone builds of clang, because it hard codes lit to lit.py in the llvm source tree. I think it should using the lit passed to -DLLVM_EXTERNAL_LIT= (if that option was specified).

It looks like this patch breaks the utils/update_cc_test_checks/check-globals.test on stand-alone builds of clang, because it hard codes lit to lit.py in the llvm source tree. I think it should using the lit passed to -DLLVM_EXTERNAL_LIT= (if that option was specified).

This patch fixes the failure for me: D105873.