Page MenuHomePhabricator

[RFC][test] Adapt debug-info lit framework for more general purposes - part 1
Needs ReviewPublic

Authored by jhenderson on Jan 25 2021, 3:33 AM.

Details

Summary

Discussion thread: https://lists.llvm.org/pipermail/llvm-dev/2021-January/148048.html

Move debuginfo-test into a subdirectory of a new top-level directory, called cross-project-tests. The new name replaces "debuginfo-test" as an LLVM project enabled via LLVM_ENABLE_PROJECTS.

Depends on D101982.

Diff Detail

Event Timeline

jhenderson created this revision.Jan 25 2021, 3:33 AM
jhenderson requested review of this revision.Jan 25 2021, 3:33 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
jhenderson edited the summary of this revision. (Show Details)Jan 25 2021, 6:01 AM
grimar added a subscriber: grimar.Jan 28 2021, 12:01 AM
jhenderson updated this revision to Diff 321758.Feb 5 2021, 7:33 AM
jhenderson retitled this revision from [WIP][RFC][test] Add lit framework for cross-project test suite to [WIP][RFC][test] Adapt debug-info lit framework for more general purposes.

Replace new testsuite with adaptation of the existing debuginfo-test testsuite.

jhenderson retitled this revision from [WIP][RFC][test] Adapt debug-info lit framework for more general purposes to [RFC][test] Adapt debug-info lit framework for more general purposes - part 1.
jhenderson edited the summary of this revision. (Show Details)

I've iterated on this a few times locally, and feel like the easiest thing to do is to make the debuginfo-test directory a subdirectory of some new top-level directory. Further changes are to follow to make the whole thing a little bit more self-consistent.

I consider this now to be in a good state to review.

Ping? (Also all the patches that depend on this)

jhenderson updated this revision to Diff 328488.Mar 5 2021, 5:45 AM

Rebase and ping!

Could someone please take a look at this patch series? It's been open for 6 weeks with no comment on this or 3 of the 4 other patches in the series.

We have a need for this mechanism downstream, and will shortly fallback to doing something like this locally, but it would be far preferable to do this upstream.

How does LLVM_ENABLE_PROJECTS=cross-project-tests will work if only configure a subset of the projects? Or will it requires all the projects to be configured?

How does LLVM_ENABLE_PROJECTS=cross-project-tests will work if only configure a subset of the projects? Or will it requires all the projects to be configured?

By the end of the patch series, the aim is to have tests marked as UNSUPPORTED if certain tools cannot be found, possibly due to them not being in LLVM_ENABLE_PROJECTS. If they are in LLVM_ENABLE_PROJECTS and cannot be found, the tests will fail.

The existing rules already mark some tests as UNSUPPORTED if lldb is not found in the environment somewhere (possibly because it is built due to being in LLVM_ENABLE_PROJECTS, or possibly because it is installed on the machine).

D96511 (the fourth patch in this series) will mark tests as UNSUPPORTED if clang can't be found and isn't in LLVM_ENABLED_PROJECTS. The configuration matrix in this case would be:

  • clang in "LLVM_ENABLED_PROJECTS" and clang not found - error for missing tool.
  • clang in "LLVM_ENABLED_PROJECTS" and clang found using existing rules (either in build output or via environment variables) - tests run.
  • clang not in "LLVM_ENABLED_PROJECTS" and clang not found - debuginfo_tests (and any other tests added in the future with REQUIRES: clang) will be marked as UNSUPPORTED.

In D96510 (the third patch), I add LLD to the list of tools these tests look for, if it is in LLVM_ENABLE_PROJECTS, so that tests can be added that use REQUIRES: lld. This is subtly different to how the other tools work (the executable won't be searched for in the build paths if it is not in the LLVM_ENABLE_PROJECTS), but probably should be discussed in D96510.

How does LLVM_ENABLE_PROJECTS=cross-project-tests will work if only configure a subset of the projects? Or will it requires all the projects to be configured?

By the end of the patch series, the aim is to have tests marked as UNSUPPORTED if certain tools cannot be found, possibly due to them not being in LLVM_ENABLE_PROJECTS. If they are in LLVM_ENABLE_PROJECTS and cannot be found, the tests will fail.

The existing rules already mark some tests as UNSUPPORTED if lldb is not found in the environment somewhere (possibly because it is built due to being in LLVM_ENABLE_PROJECTS, or possibly because it is installed on the machine).

D96511 (the fourth patch in this series) will mark tests as UNSUPPORTED if clang can't be found and isn't in LLVM_ENABLED_PROJECTS. The configuration matrix in this case would be:

  • clang in "LLVM_ENABLED_PROJECTS" and clang not found - error for missing tool.

Out of curiosity, any particular scenarios where you expect this ^ to come up. (does this configuration require the user to manually ensure the clang subproject was built before running the tests? So this error would be telling the user to go and run a separate build target before running the test target? If that's the case, it'd be good to have the dependencies added automatically if possible - in which case when would this error turn up? Only if there's some internal build system failure (akin to someone deleting a file out from underneath the build system - as unlikely as a .o file (under the purview of the build system) being missing from a linker invocation))

How does LLVM_ENABLE_PROJECTS=cross-project-tests will work if only configure a subset of the projects? Or will it requires all the projects to be configured?

By the end of the patch series, the aim is to have tests marked as UNSUPPORTED if certain tools cannot be found, possibly due to them not being in LLVM_ENABLE_PROJECTS. If they are in LLVM_ENABLE_PROJECTS and cannot be found, the tests will fail.

The existing rules already mark some tests as UNSUPPORTED if lldb is not found in the environment somewhere (possibly because it is built due to being in LLVM_ENABLE_PROJECTS, or possibly because it is installed on the machine).

D96511 (the fourth patch in this series) will mark tests as UNSUPPORTED if clang can't be found and isn't in LLVM_ENABLED_PROJECTS. The configuration matrix in this case would be:

  • clang in "LLVM_ENABLED_PROJECTS" and clang not found - error for missing tool.

Out of curiosity, any particular scenarios where you expect this ^ to come up. (does this configuration require the user to manually ensure the clang subproject was built before running the tests? So this error would be telling the user to go and run a separate build target before running the test target? If that's the case, it'd be good to have the dependencies added automatically if possible - in which case when would this error turn up? Only if there's some internal build system failure (akin to someone deleting a file out from underneath the build system - as unlikely as a .o file (under the purview of the build system) being missing from a linker invocation))

Just to be clear, clang (and later lld in a future change) is already in the list of dependencies for check-debuginfo, so the dependency is there, if you run check-debuginfo to run the tests. The use case I've got in mind is where someone didn't use check-* to build the test dependencies. I routinely run subsets of the lit tests without running a check-* target, because I often am working on individual tools or tests, and there's no need for me to build the rest of LLVM (I build the one executable, say llvm-readelf, and that's about it). If a user happens to have cleared out their build directory beforehand and then try to run the tests in this way, lit will emit the error about a missing tool rather than the test itself failing, so you get quicker and clearer feedback.

either in build output or via environment variables

This "magic" seems somehow fragile to me: if I have an old lldb installed and I run the cross-project tests with clang+lld+flang configured, it may run the lldb test and incorrectly fails ninja check-all because I only intended to test clang+lld+flang that I'm building/testing in the repo. Can this be controlled by a flag instead so there is an explicit opt-in to use a prebuilt tool? (-DCROSS_PROJECT_LLDB_PATH=<path> straw man).

either in build output or via environment variables

This "magic" seems somehow fragile to me: if I have an old lldb installed and I run the cross-project tests with clang+lld+flang configured, it may run the lldb test and incorrectly fails ninja check-all because I only intended to test clang+lld+flang that I'm building/testing in the repo. Can this be controlled by a flag instead so there is an explicit opt-in to use a prebuilt tool? (-DCROSS_PROJECT_LLDB_PATH=<path> straw man).

Yeah, I was in two minds about it. I followed the approach I did because discussing with others, it's sometimes desired to run some of the tests (I think the ones to do with dexter) with any old compiler, because how it gets compiled is somewhat unimportant.

That being said, I don't think I'm changing anything with regards to LLDB at least. I'd be happy to change the behaviour so that you have to opt-in deliberately to using tools that aren't part of the explicit dependencies, if they can be found, but the obvious solution might impact other projects outside this area, so probably deserves a wider discussion on llvm-dev, if you think it's worth proceeding with that. I'll take a look and see what I can come up with.

either in build output or via environment variables

This "magic" seems somehow fragile to me: if I have an old lldb installed and I run the cross-project tests with clang+lld+flang configured, it may run the lldb test and incorrectly fails ninja check-all because I only intended to test clang+lld+flang that I'm building/testing in the repo. Can this be controlled by a flag instead so there is an explicit opt-in to use a prebuilt tool? (-DCROSS_PROJECT_LLDB_PATH=<path> straw man).

Yeah, I was in two minds about it. I followed the approach I did because discussing with others, it's sometimes desired to run some of the tests (I think the ones to do with dexter) with any old compiler, because how it gets compiled is somewhat unimportant.

That being said, I don't think I'm changing anything with regards to LLDB at least. I'd be happy to change the behaviour so that you have to opt-in deliberately to using tools that aren't part of the explicit dependencies, if they can be found, but the obvious solution might impact other projects outside this area, so probably deserves a wider discussion on llvm-dev, if you think it's worth proceeding with that. I'll take a look and see what I can come up with.

I went back over the lit code. The behaviour of use_clang, use_lld and use_llvm_tool is as I thought: all three already look for the tool in the PATH. The build directory is first, so if the tool has just been built, it will already use that. Otherwise, it will find an installed tool. That appears to be by design. I guess people could opt-out of using preinstalled tools by omitting the installation directories from the PATH when running lit. Do we need something more than that?

mehdi_amini added a comment.EditedMar 18 2021, 12:17 PM

Do we need something more than that?

Yes: that may be how the debuginfo-tests was setup, but that seems like a no-go to me for generalizing this into a "cross-projects" that intends to serve the monorepo, for the reasons I mentioned above. I may be interested in some cross-project test in the future but unlikely to want to test lldb. Yet lldb is in /usr/bin/ on my Mac and this isn't a good "default" to use this for running the tests here IMO, especially when I don't intend this (and I obviously can't remove /usr/bin/ from my path without unintended consequences)

Do we need something more than that?

Yes: that may be how the debuginfo-tests was setup, but that seems like a no-go to me for generalizing this into a "cross-projects" that intends to serve the monorepo, for the reasons I mentioned above. I may be interested in some cross-project test in the future but unlikely to want to test lldb. Yet lldb is in /usr/bin/ on my Mac and this isn't a good "default" to use this for running the tests here IMO, especially when I don't intend this (and I obviously can't remove /usr/bin/ from my path without unintended consequences)

Perhaps Apple folks who implemented the original (@aprantl and @JDevlieghere I believe) can weigh in on their intended (& realized/actual) use cases for this functionality - though I agree that probably an explicit opt in to "I want to test with whatever binaries are in my PATH rather than just-built tools in this checkout" would be desirable here.

Perhaps Apple folks who implemented the original (@aprantl and @JDevlieghere I believe) can weigh in on their intended (& realized/actual) use cases for this functionality - though I agree that probably an explicit opt in to "I want to test with whatever binaries are in my PATH rather than just-built tools in this checkout" would be desirable here.

This is more incidental than intentional behavior. The debuginfo-tests originally tested that the "system" /usr/bin/gdb would could debug clang-generated code. At some point I generalized the testsuite to also support LLDB, and kept the use-the-system-debugger behavior. In a monorepo world it makes much more sense to make this behavior configurable, but I would appreciate an option to test against an LLDB that is installed somewhere in the system and not just the latest version from the monorepo, so we can test some basic level backwards compatibility.

Perhaps Apple folks who implemented the original (@aprantl and @JDevlieghere I believe) can weigh in on their intended (& realized/actual) use cases for this functionality - though I agree that probably an explicit opt in to "I want to test with whatever binaries are in my PATH rather than just-built tools in this checkout" would be desirable here.

This is more incidental than intentional behavior. The debuginfo-tests originally tested that the "system" /usr/bin/gdb would could debug clang-generated code. At some point I generalized the testsuite to also support LLDB, and kept the use-the-system-debugger behavior. In a monorepo world it makes much more sense to make this behavior configurable, but I would appreciate an option to test against an LLDB that is installed somewhere in the system and not just the latest version from the monorepo, so we can test some basic level backwards compatibility.

Okay, all sounds reasonable to me. I think there's a more general problem that the lit tests can use tools in the PATH, outside the build directory, if the tool hasn't been built (this is based on reading the lit source code - I need to confirm this in other test directories). I think we can resolve all of this with a new CMake variable, something like LLVM_USE_INSTALLED_TOOLS_FOR_TESTS (other name suggestions welcome). I'll take a look at this at some point in the coming days and post something on the mailing list to get more people's attention.

penzn added a subscriber: penzn.May 4 2021, 11:53 AM

TL;DR: I think we can turn off looking for tools on the PATH when use_llvm_tool is used. This covers the use-cases in the debuginfo-tests/cross-project-tests area. Support already exists (but needs improving) for overriding using the just-built tool for these cases.

Full details:

I've dug into how tools are picked up. The exact behaviour depends on the tool in question. The first thing that's important to note is that for most tools, when you see an invocation like RUN: llvm-objcopy ..., the "llvm-objcopy" is substituted for the full path to the tool found in build directory. This means the tool won't be found on the PATH. The list of tools is defined in config files, e.g. https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/test/lit.cfg.py#L146.

A limited number of places use use_llvm_tool to find the executable. This is used by the debuginfo-tests to find LLDB, clang++, clang-cl and indirectly clang itself. There are various similar uses in other places throughout the project testsuites. use_llvm_tool allows you to specify an environment variable which points to the tool to use. In the debuginfo-tests, you can use the environment variable "CLANG" to specify the location of the clang executable. Unfortunately, it also specifies the location of LLDB (I think this is a bug)! If the environment variable is not specified, the tool is found on the PATH. The build directory is at the front of the PATH. This means that if the tool isn't in the build directory (i.e. a user hasn't built it), it will use the installed tool from the PATH.

Given the environment variable option, I think we could just disable using tools from the PATH where use_llvm_tool has been used. Note that we can't completely disable the use of PATH for all cases, because system tools like rm/cp etc need to be available.

jhenderson updated this revision to Diff 343339.May 6 2021, 3:06 AM
jhenderson edited the summary of this revision. (Show Details)

Rebased on D101982. That patch now allows us to override the tool used with an environment variable. I'm still looking at preventing the tools being picked up from the PATH.

I'm still looking at preventing the tools being picked up from the PATH.

I dug into this. The behaviour is not specific to the debuginfo-tests/cross-project-test testsuite. Rather, any tool that is found via use_llvm_tool will use this process. Primarily, this means any tool that calls use_clang/use_lld will fallback to the PATH version if none is found in the build directory. Looking back at the history, for clang this has been the behaviour for at least 12 years (see https://github.com/llvm/llvm-project/commit/be4253a0caf6f011b2af12803db9cf980f48084e), quite possibly longer. Changing the behaviour to no longer rely on the PATH could impact various tests beyond this testsuite, in clangd and lldb as well as in the clang and lld testsuites themselves.

I'm not convinced we need the behaviour to use the PATH version now. With the CLANG and LLDB environment variables allowing to override which tool is used, and a similar patch for LLD planned for later today, I think we can probably remove the functionality. I've posted https://lists.llvm.org/pipermail/llvm-dev/2021-May/150421.html to discuss this further. Either way, I think the decision that the email chain will hopefully lead to shouldn't hold up proceeding with this patch series. Could I therefore ask people to resume reivewing this series, please?

and a similar patch for LLD planned for later today

This patch is up for review as D101995.

I'm not convinced we need the behaviour to use the PATH version now. With the CLANG and LLDB environment variables allowing to override which tool is used, and a similar patch for LLD planned for later today, I think we can probably remove the functionality. I've posted https://lists.llvm.org/pipermail/llvm-dev/2021-May/150421.html to discuss this further. Either way, I think the decision that the email chain will hopefully lead to shouldn't hold up proceeding with this patch series. Could I therefore ask people to resume reivewing this series, please?

Quick update on this: D101982 fixes the environment variable for LLDB to be 'LLDB' not 'CLANG', so that it is now possible to specify that executable as requested by @aprantl. I have dropped the other new environment variables, as there has been no specific request for them, but can easily readd them, should the need arise. I will upload a new review, likely later today, to try disabling the wider PATH usage for these tools, but it is a somewhat independent item now.

Ping #5!!

The rest of the patches in this series have been approved. It's just this patch that needs looking at. There's no particular need to look at most of the files that were in the debuginfo-test folder, as they've simply been moved. I'd recommend reviewing the changes outside that folder, plus the CMake files.

Seems like there are only a few line changed other than the renaming? What kind of review is needed here? Just an agreement on the principle of moving these? I think it should be someone having a stake in the debug-info tests to give some sort of approval here.