This is an archive of the discontinued LLVM Phabricator instance.

[LIT] Added an option to llvm-lit to emit the necessary test coverage data, divided per test case
ClosedPublic

Authored by xgupta on Jul 1 2023, 3:38 AM.

Details

Summary

This patch is the first part of https://llvm.org/OpenProjects.html#llvm_patch_coverage.

We have first define a new variable LLVM_TEST_COVERAGE which when set, pass --emit-coverage option to
llvm-lit which will help in setting a unique value to LLVM_PROFILE_FILE for each RUN. So for example
coverage data for test case llvm/test/Analysis/AliasSet/memtransfer.ll will be emitted as
build/test/Analysis/AliasSet/memtransfer.profraw

Diff Detail

Event Timeline

xgupta created this revision.Jul 1 2023, 3:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2023, 3:38 AM
Herald added a subscriber: delcypher. · View Herald Transcript
xgupta updated this revision to Diff 536522.Jul 1 2023, 3:45 AM

minor update

xgupta published this revision for review.Jul 1 2023, 3:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2023, 3:48 AM

Please also add a test of lit itself (see llvm/utils/lit/tests). I think something using --emit-coverage on a test that echoes $LLVM_PROFILE_FILE should be enough.

llvm/utils/lit/lit/TestRunner.py
1141–1143

This call seems redundant

llvm/utils/lit/lit/cl_arguments.py
188

Since this option doesn't actually affect whether the tests emit coverage we should probably name it something more descriptive, e.g. --per-test-coverage or something to that regard.

xgupta updated this revision to Diff 536547.Jul 1 2023, 12:02 PM
xgupta marked 2 inline comments as done.

Address comments
Need to fix test case [WIP]

xgupta updated this revision to Diff 536569.Jul 1 2023, 11:32 PM

Update test case [WIP]

hnrklssn added inline comments.Jul 2 2023, 5:31 AM
llvm/utils/lit/lit/TestRunner.py
1150

This looks like it might remove other existing env vars. Could you add a test case checking that other variables remain also?

xgupta updated this revision to Diff 536984.Jul 4 2023, 1:18 AM

Update to avoid overwriting env

xgupta added a comment.Jul 4 2023, 1:23 AM

Please also add a test of lit itself (see llvm/utils/lit/tests). I think something using --emit-coverage on a test that echoes $LLVM_PROFILE_FILE should be enough.

Hi Henrik,

Currently, I am getting LLVM_PROFILE_FILE = None instead of LVM_PROFILE_FILE = per-test-coverage.profraw so the test case is failing. And I can't use .ll test case as the opt and other tools should not have a dependency to lit test cases.

Can you suggest if this is the right way to have test case?

llvm/utils/lit/lit/TestRunner.py
1150

I have updated the code that will not overwrite env.

hnrklssn added inline comments.Jul 4 2023, 2:27 AM
llvm/utils/lit/tests/Inputs/per-test-coverage/lit.cfg
8

Instead of fetching the environment variable here (which seems like it's probably happening before the test is executed?) as part of the test framework, try running a script that reads the env var on its own

xgupta updated this revision to Diff 537060.Jul 4 2023, 5:09 AM

Update test case [WIP]

xgupta added inline comments.Jul 4 2023, 5:12 AM
llvm/utils/lit/tests/Inputs/per-test-coverage/lit.cfg
8

Tried with a script per-test-coverage.py but not working it prints None, same for other env variables. Also same thing happen with .sh script attached in the patch as wip.

xgupta added inline comments.Jul 4 2023, 5:19 AM
llvm/utils/lit/tests/Inputs/per-test-coverage/lit.cfg
8

But the patch is working as expected I could see -

llvm/llvm-upstream/build $ bin/llvm-lit -a -v --per-test-coverage  ../llvm/test/Analysis/AliasSet

llvm/llvm-upstream/build  $ find .  -type f -name "*.profraw" | grep -i ".profraw$"  
./test/Analysis/AliasSet/unknown-inst-tracking.profraw
./test/Analysis/AliasSet/memtransfer.profraw
./test/Analysis/AliasSet/argmemonly.profraw
./test/Analysis/AliasSet/saturation.profraw
./test/Analysis/AliasSet/guards.profraw
./test/Analysis/AliasSet/memset.profraw
./test/Analysis/AliasSet/intrinsics.profraw
delcypher added inline comments.Jul 4 2023, 3:51 PM
llvm/docs/CommandGuide/lit.rst
193

You may want to rephrase this. Lit can (and is) used outside of the llvm project so the <build>/test directory isn't guaranteed to be the name of a test directory. Instead I think you're saying that the coverage data files will be located in the directory that config.test_exec_root is set to in a test suite.

A concrete example under the llvm-project itself would be clang. It's lit shell tests are usually located in <build>/tools/clang/test which is different to the path you give above.

llvm/utils/lit/lit/LitConfig.py
91

Consider adding a @property getter and a setter (see maxIndividualTestTime) so that when setting the data it can be validated.

llvm/utils/lit/lit/TestRunner.py
1149

How is this unique to every RUN line? A test case could have multiple run lines and I don't see how that would be handled here.

@xgupta Could you add a test case where a lit.cfg file sets config.per_test_coverage directly? A test suite may want to set it there rather than on the command line.

delcypher added inline comments.Jul 4 2023, 4:13 PM
llvm/docs/CMake.rst
378

This documentation seems inaccurate right now. Reading the implementation, enabling this option doesn't actually cause any coverage to be collected. I suspect you will do this in later patches but I would suggest you update the documentation when the feature actually works.

Nits:

  • LLVM_TEST_COVERAGE is a vague name. LLVM_INDIVIDUAL_TEST_COVERAGE might be more specific.
  • Consider being more specific about where the code coverage data is written (e.g. under the config.test_exec_root directory.
  • Documentation doesn't typically use the phrase "This allows you to...". Instead consider writing " This allows code coverage analysis of each individual test case"

Super nit:

  • Missing fullstop (period) at the end of the last sentence.
xgupta updated this revision to Diff 538148.Jul 7 2023, 7:56 AM
xgupta marked 5 inline comments as done.

Address comments (Test case is not passing yet)

xgupta added a comment.Jul 7 2023, 7:59 AM

@xgupta Could you add a test case where a lit.cfg file sets config.per_test_coverage directly? A test suite may want to set it there rather than on the command line.

I have added one test case but it is not passing, I am investigating that.

llvm/docs/CMake.rst
378

This documentation seems inaccurate right now. Reading the implementation, enabling this option doesn't actually cause any coverage to be collected. I suspect you will do this in later patches but I would suggest you update the documentation when the feature actually works.

Yeah, I was thinking of future name, agree, updated now, Thanks.

llvm/docs/CommandGuide/lit.rst
193

Thanks for pointing it, updated.

llvm/utils/lit/lit/TestRunner.py
1149

I updated the documentation to say it is set for each test file.

xgupta updated this revision to Diff 540458.Jul 14 2023, 9:39 AM

Update TestRunner.py for each RUN rahter than per test case ( Testcase WIP)

llvm/utils/lit/lit/LitConfig.py
91

I added that but thinking about the use of that property as "echo_all_commands" don't have it. Can you please clarify?

llvm/utils/lit/lit/TestRunner.py
1149

Kept the documentation for each RUN and updated the code in that regard.

xgupta updated this revision to Diff 541007.Jul 17 2023, 6:57 AM
xgupta marked an inline comment as done.

Put export LLVM_PROFILE_FILE={llvm_profile_file} before the RUN command execute.

xgupta updated this revision to Diff 541008.Jul 17 2023, 7:00 AM

Minor update

xgupta updated this revision to Diff 542510.Jul 20 2023, 7:17 AM

remove test case

xgupta added a comment.EditedJul 20 2023, 7:30 AM

Hi,

writing test case in llvm/utils/lit/tests seems difficult, as when we run them, there is no .script file generated as in case with other llvm or clang test cases. So the patch is not working there and the env variable is not setting. I am thinking if we can submit in this way without testcase since lit code is very stable so chances of breaking something is less.

shivam@linux ~/.llvm/llvm-upstream/build (arcpatch-D154280) $ bin/llvm-lit -a -v --per-test-coverage  ../clang/test/SemaCXX/bool.cpp

llvm-lit: /home/shivam/.llvm/llvm-upstream/llvm/utils/lit/lit/llvm/config.py:484: note: using clang: /home/shivam/.llvm/llvm-upstream/build/bin/clang
-- Testing: 1 tests, 1 workers --
PASS: Clang :: SemaCXX/bool.cpp (1 of 1)
Script:
--
export LLVM_PROFILE_FILE=bool0.profraw && : 'RUN: at line 1';   /home/shivam/.llvm/llvm-upstream/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-upstream/build/lib/clang/17/include -nostdsysteminc -std=c++14 -fsyntax-only -verify=expected,precxx17 -Wno-constant-conversion /home/shivam/.llvm/llvm-upstream/clang/test/SemaCXX/bool.cpp
export LLVM_PROFILE_FILE=bool1.profraw && : 'RUN: at line 2';   /home/shivam/.llvm/llvm-upstream/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-upstream/build/lib/clang/17/include -nostdsysteminc -std=c++14 -fsyntax-only -verify=expected,precxx17 -Wno-constant-conversion -Wno-deprecated -Wdeprecated-increment-bool /home/shivam/.llvm/llvm-upstream/clang/test/SemaCXX/bool.cpp
export LLVM_PROFILE_FILE=bool2.profraw && : 'RUN: at line 3';   /home/shivam/.llvm/llvm-upstream/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-upstream/build/lib/clang/17/include -nostdsysteminc -std=c++2b -fsyntax-only -verify=expected,cxx17 -Wno-constant-conversion -Wno-deprecated -Wdeprecated-increment-bool /home/shivam/.llvm/llvm-upstream/clang/test/SemaCXX/bool.cpp
--
Exit Code: 0


********************

Testing Time: 0.76s
  Passed: 1
shivam@linux ~/.llvm/llvm-upstream/build (arcpatch-D154280) $ find . -type f -name "*.profraw" | grep -i "profraw"

./default.profraw
./tools/clang/test/SemaCXX/bool1.profraw
./tools/clang/test/SemaCXX/bool0.profraw
./tools/clang/test/SemaCXX/bool2.profraw

Hi,

writing test case in llvm/utils/lit/tests seems difficult, as when we run them, there is no .script file generated as in case with other llvm or clang test cases. So the patch is not working there and the env variable is not setting. I am thinking if we can submit in this way without testcase since lit code is very stable so chances of breaking something is less.

shivam@linux ~/.llvm/llvm-upstream/build (arcpatch-D154280) $ bin/llvm-lit -a -v --per-test-coverage  ../clang/test/SemaCXX/bool.cpp

llvm-lit: /home/shivam/.llvm/llvm-upstream/llvm/utils/lit/lit/llvm/config.py:484: note: using clang: /home/shivam/.llvm/llvm-upstream/build/bin/clang
-- Testing: 1 tests, 1 workers --
PASS: Clang :: SemaCXX/bool.cpp (1 of 1)
Script:
--
export LLVM_PROFILE_FILE=bool0.profraw && : 'RUN: at line 1';   /home/shivam/.llvm/llvm-upstream/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-upstream/build/lib/clang/17/include -nostdsysteminc -std=c++14 -fsyntax-only -verify=expected,precxx17 -Wno-constant-conversion /home/shivam/.llvm/llvm-upstream/clang/test/SemaCXX/bool.cpp
export LLVM_PROFILE_FILE=bool1.profraw && : 'RUN: at line 2';   /home/shivam/.llvm/llvm-upstream/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-upstream/build/lib/clang/17/include -nostdsysteminc -std=c++14 -fsyntax-only -verify=expected,precxx17 -Wno-constant-conversion -Wno-deprecated -Wdeprecated-increment-bool /home/shivam/.llvm/llvm-upstream/clang/test/SemaCXX/bool.cpp
export LLVM_PROFILE_FILE=bool2.profraw && : 'RUN: at line 3';   /home/shivam/.llvm/llvm-upstream/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-upstream/build/lib/clang/17/include -nostdsysteminc -std=c++2b -fsyntax-only -verify=expected,cxx17 -Wno-constant-conversion -Wno-deprecated -Wdeprecated-increment-bool /home/shivam/.llvm/llvm-upstream/clang/test/SemaCXX/bool.cpp
--
Exit Code: 0


********************

Testing Time: 0.76s
  Passed: 1
shivam@linux ~/.llvm/llvm-upstream/build (arcpatch-D154280) $ find . -type f -name "*.profraw" | grep -i "profraw"

./default.profraw
./tools/clang/test/SemaCXX/bool1.profraw
./tools/clang/test/SemaCXX/bool0.profraw
./tools/clang/test/SemaCXX/bool2.profraw

It sounds like it’s because the test is not using external ShTest. Later it might be worth looking into supporting internal lit shell, but I haven’t checked what that would imply yet.

xgupta updated this revision to Diff 542776.Jul 20 2023, 11:58 PM

Added test cases

This revision is now accepted and ready to land.Jul 21 2023, 2:22 AM
This revision was landed with ongoing or failed builds.Jul 21 2023, 2:53 AM
This revision was automatically updated to reflect the committed changes.
DavidSpickett added inline comments.
llvm/utils/lit/lit/TestRunner.py
1087

This appears to get set but never used. Or I am missing something later in the code, if that's the case, consider making that more obvious e.g. declaring it outside the for loop.

Side note: I have been wanting to try some things where we run only the tests we think might fail after a given patch, and this could be very useful there. Thank you for working on this.

DavidSpickett added inline comments.Jul 21 2023, 8:28 AM
llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py
9

If I understand correctly, LLVM_PROFILE_FILE should be set by lit itself. So this test could be more robust if it did env -u LLVM_PROFILE_FILE python.... The new value for LLVM_PROFILE_FILE should be redefined by lit as part of the test.

DavidSpickett added inline comments.Jul 21 2023, 8:34 AM
llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py
9

Actually ignore me, doing that would remove the value set by LIT because lit is the one that runs the RUN line.

xgupta reopened this revision.Jul 25 2023, 7:20 AM
This revision is now accepted and ready to land.Jul 25 2023, 7:20 AM
xgupta updated this revision to Diff 543962.Jul 25 2023, 7:22 AM

update python to %{python} in test cases

xgupta marked 2 inline comments as done.Jul 25 2023, 7:25 AM
xgupta added inline comments.
llvm/utils/lit/lit/TestRunner.py
1087

Actually, this variable is being set for each RUN line in a test case so it is in the loop.

DavidSpickett added inline comments.Jul 25 2023, 7:50 AM
llvm/utils/lit/lit/TestRunner.py
1087

That explains it being in the loop, but where is env actually used?

It makes a copy of the current environment, and sets a new value in it:

env = dict(test.config.environment)  # Create a copy of the environment
env["LLVM_PROFILE_FILE"] = llvm_profile_file

However it doesn't use env again, instead it sets llvm_profile_file directly:

commands[j] = f"export LLVM_PROFILE_FILE={llvm_profile_file} && {commands[j]}"
coverage_index += 1

Which seems correct, so is env actually unused?

If it's used later outside the loop it would be whatever the last env set, which would likely create some strange results.

xgupta updated this revision to Diff 544038.Jul 25 2023, 10:40 AM
xgupta marked an inline comment as done.

Updated lit.cfg of test cases for %{python}

xgupta updated this revision to Diff 544206.Jul 25 2023, 10:13 PM

Don't set env variable

xgupta marked an inline comment as done.Jul 25 2023, 10:17 PM
xgupta added inline comments.
llvm/utils/lit/lit/TestRunner.py
1087

Thanks, @DavidSpickett, yes you are correct, we are not using env variable in this code.
Actually that we were using it before when setting LLVM_PROFILE_FILE for each file (.script), but now it should be for each RUN line so use a different approach.

Were you able to reproduce the issue and check it was fixed, or do you need me to try it again to confirm?

xgupta marked an inline comment as done.Jul 26 2023, 1:39 AM

Were you able to reproduce the issue and check it was fixed, or do you need me to try it again to confirm?

Yes, I reproduce it on docker and checked it was fixed by replacing python to %{python} in test cases (Actually there is only python3, not python so it was failing).
I am waiting for pre-merge checks to be completed and then will reland it.

This revision was landed with ongoing or failed builds.Jul 26 2023, 4:19 AM
This revision was automatically updated to reflect the committed changes.