Page MenuHomePhabricator

[lit][compiler-rt] add multi-cfgd test suite discovery
Needs ReviewPublic

Authored by ychen on Feb 4 2020, 10:49 AM.

Details

Summary

Currently llvm-lit /path/to/src does not work for many compiler-rt tests which is not developer-friendly.

The reason for this is compiler-rt tests (asan, ubsan etc..) are really multi-config'd tests that have configurations for different architectures and dynamic/static linking. Currently we generate one TestSuite for each configuration at build directory side which could be invoked like llvm-lit /path/to/build/config-linux-dynamic. We could not do llvm-lit /path/to/src for all configurations because in LIT, class TestSuite has only one config object.

This patch

  • make class TestSuite own more than one TestConfig and update clients accordingly.
  • a multi-config'd TestSuite is recognized by text files with suffix '.multi.cfg'. The '.multi.cfg' file contents is <test suite name> /path/to/cfg_dir1/lit.site.cfg /path/to/cfg_dir2/lit.site.cfg ...
  • currently there exists testSuiteCache to avoid reloading same TestConfig twice when exec dir is in tree.(See llvm\utils\lit\tests\Inputs\exec-discovery-in-tree for examples). This patch add a testConfigCache to solve the same issue since class TestSuite is able to own multiple TestConfig now.
  • changes in utils/lit/lit/main.py handles the printing of discovered test-suite properly.
  • this is needed by D73981

Diff Detail

Event Timeline

ychen created this revision.Feb 4 2020, 10:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2020, 10:49 AM
ychen edited the summary of this revision. (Show Details)Feb 4 2020, 11:05 AM

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

yln added a comment.Feb 6 2020, 10:01 AM

Hi @ychen,

the code changes look fine. My understanding of your patch is that it allows test suites to be subdivided into parts that can be configured differently.
Before proceeding, can you please explain which problem you are trying to solve and why the existing facilities don't fit your needs?

Thanks!

ychen added a comment.EditedFeb 6 2020, 11:08 AM
In D73980#1862032, @yln wrote:

Hi @ychen,

the code changes look fine. My understanding of your patch is that it allows test suites to be subdivided into parts that can be configured differently.
Before proceeding, can you please explain which problem you are trying to solve and why the existing facilities don't fit your needs?

Thanks!

Hi @yln, this is mostly for compiler-rt where one test could be tested in many configurations. Without this patch, llvm-lit /path/to/test/src/ does not work since the config_map maps test source code to only one configuration currently. This patch adds a third test suite marker file (lit.site.multi.cfg), config_map could map to and invoke all the test configs applicable to the test.

D73981 shows the way it is used in the CMake file. I also updated the lit.rst doc.

ychen added a comment.Feb 20 2020, 3:19 PM

Hello, friendly ping for some attention here. I've run LIT unit test and most of the regression tests in the monorepo. I didn't see any regressions.

delcypher requested changes to this revision.Feb 23 2020, 6:43 PM

@ychen I need convincing that this feature is actually needed. Today the problem of multiple configurations is solved at the CMake level in compiler-rt where we have CMake generate multiple config directories that point to the same source tree. Each config is generated differently depending on the use case. Why isn't that sufficient for the problem you are trying to solve.

The description of this change is also very incomplete.

This revision now requires changes to proceed.Feb 23 2020, 6:43 PM
ychen added a comment.Feb 23 2020, 8:14 PM

@ychen I need convincing that this feature is actually needed. Today the problem of multiple configurations is solved at the CMake level in compiler-rt where we have CMake generate multiple config directories that point to the same source tree. Each config is generated differently depending on the use case. Why isn't that sufficient for the problem you are trying to solve.

The description of this change is also very incomplete.

Hi @delcypher, thanks for chime in. This patch is a pre-requisite of D73981, to solve the problem/inconvenience of not being able to do llvm-lit /path/to/compiler-rt/x.test. I will update the description.

ychen updated this revision to Diff 261951.May 4 2020, 3:56 PM
  • rebase
ychen retitled this revision from [lit] add multi-cfgd test suite discovery to [lit][compiler-rt] add multi-cfgd test suite discovery.May 4 2020, 3:57 PM
ychen edited the summary of this revision. (Show Details)
ychen edited the summary of this revision. (Show Details)May 4 2020, 3:58 PM

hello, I'm still looking to get this in.

rnk removed a reviewer: rnk.May 5 2020, 4:42 PM
rnk added a subscriber: rnk.

-self, load shedding

rnk removed a subscriber: rnk.May 19 2020, 11:25 AM
delcypher requested changes to this revision.May 20 2020, 12:49 PM

I've done another pass.

llvm/docs/CommandGuide/lit.rst
65

Is the second

:file:`{NAME}.site.multi.cfg`

supposed to be

:file:`lit.site.multi.cfg`

?

254

This format doesn't seem like to it would handle future changes very well. Could this be a more structured format, e.g. JSON?

llvm/utils/lit/CMakeLists.txt
11 ↗(On Diff #261951)

This might not work on Windows due to the slashes.

I don't like that we're doing something special here just for this test. This means that the test doesn't even exist if someone tries to run the tests in the source tree.
Can't we have the multicfg format support relative paths so

  • that additional copying is unnecessary
  • The test gets executed if lit is tested in the source tree.
llvm/utils/lit/lit/discovery.py
84–107

Parsing of the file format probably belongs in its own function.

133

If you're going to rename ts perhaps we should use a more helpful name, e.g. testConfig.

248

Why is this here? It deserves a comment at the very least.

llvm/utils/lit/lit/reports.py
13

This change is an API break. Is anyone using the old by_suite_and_test_path from their lit configuration files?

This revision now requires changes to proceed.May 20 2020, 12:49 PM

@yln Any opinions on this? You touch lit much more than I do these days.

ychen marked 9 inline comments as done.May 25 2020, 5:01 PM
ychen added inline comments.
llvm/docs/CommandGuide/lit.rst
65

Yes.

llvm/utils/lit/CMakeLists.txt
11 ↗(On Diff #261951)
  • Added relative path support
  • Removed the code here: the test works like others in the lit/test directory
llvm/utils/lit/lit/discovery.py
248

I should have removed this. It was intended to address the same issue the above code is addressing now. It could've been removed because when if sub_ts.configs[0] in ts.configs is true, if sub_ts is ts would also be true during the recursive call of getTestsInSuite (line 248).

if sub_ts is ts:
    continue
llvm/utils/lit/lit/reports.py
13

It seems no in-tree/lnt/test-suite/zorg clients are using this.

ychen updated this revision to Diff 266089.May 25 2020, 5:09 PM
ychen marked 2 inline comments as done.
  • support relative path in multicfg file
  • use json for multicfg file
  • update lit.rst
  • address comments

Gentle ping.

Definitely agree that this change is on the large side. For the most part, it just removes the assumption that class TestSuite only has one config. It is not a design change or anything. I'll follow up quickly if there are any issues.

MaskRay added a subscriber: MaskRay.EditedJun 10 2020, 10:35 AM

The description seems long and I haven't tried understanding what this patch does, but still wanted to make one comment.

If /tmp/ReleaseA is a build directory, for a test file compiler-rt/test/profile/gcov-basic.c, you can run /tmp/ReleaseA/bin/llvm-lit /tmp/ReleaseA/projects/compiler-rt/test/profile/Profile-x86_64/gcov-basic.c

(I create a shell alias for /tmp/ReleaseA/bin/llvm-lit)

yln resigned from this revision.Jul 22 2021, 4:21 PM