Enable LLDB tests in Linux pre-merge CI #94208
Conversation
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-lldb Author: Vlad Serebrennikov (Endilll) ChangesFull diff: https://github.com/llvm/llvm-project/pull/94208.diff 2 Files Affected:
diff --git a/.ci/generate-buildkite-pipeline-premerge b/.ci/generate-buildkite-pipeline-premerge
index 033ab804b165e..a9972f235ff46 100755
--- a/.ci/generate-buildkite-pipeline-premerge
+++ b/.ci/generate-buildkite-pipeline-premerge
@@ -153,7 +153,6 @@ function exclude-linux() {
for project in ${projects}; do
case ${project} in
cross-project-tests) ;; # tests failing
- lldb) ;; # tests failing
openmp) ;; # https://github.com/google/llvm-premerge-checks/issues/410
*)
echo "${project}"
@@ -170,7 +169,6 @@ function exclude-windows() {
compiler-rt) ;; # tests taking too long
openmp) ;; # TODO: having trouble with the Perl installation
libc) ;; # no Windows support
- lldb) ;; # tests failing
bolt) ;; # tests are not supported yet
*)
echo "${project}"
@@ -213,7 +211,7 @@ function check-targets() {
echo "check-unwind"
;;
lldb)
- echo "check-all" # TODO: check-lldb may not include all the LLDB tests?
+ echo "check-lldb" # TODO: check-lldb may not include all the LLDB tests?
;;
pstl)
echo "check-all"
diff --git a/clang/examples/PrintFunctionNames/PrintFunctionNames.cpp b/clang/examples/PrintFunctionNames/PrintFunctionNames.cpp
index 6509a6440e12d..b2b785b87c25c 100644
--- a/clang/examples/PrintFunctionNames/PrintFunctionNames.cpp
+++ b/clang/examples/PrintFunctionNames/PrintFunctionNames.cpp
@@ -72,7 +72,7 @@ class PrintFunctionsConsumer : public ASTConsumer {
*sema.LateParsedTemplateMap.find(FD)->second;
sema.LateTemplateParser(sema.OpaqueParser, LPT);
llvm::errs() << "late-parsed-decl: \"" << FD->getNameAsString() << "\"\n";
- }
+ }
}
};
|
|
Worth a shot i guess. Though it could be somewhat disruptive if we do in fact have flaky tests. I don't think we need to run all tests. Should we just run the tests for the LLDB C++ language plugin (i.e., the part of LLDB that invokes Clang and the JIT). And then we could expand the set of tests from there later? CC @adrian-prantl @JDevlieghere |
In the bash script that currently handles this, "targets that we need to check if lldb is tested" is expressible, but "targets that we need to check if lldb is tested on clang changes" is not, and I consider it already too complicated for a bash script. |
|
First Windows CI results: LLDB unit tests do not even build. |
Ah i see, could you point me to that part of the bash script? |
|
@Michael137 This is for Linux. Windows works the same way just below those lines: llvm-project/.ci/generate-buildkite-pipeline-premerge Lines 253 to 255 in 9a7bd8a |
Ah, so what would the issue be with just adding more specific |
Those more specific targets will be used instead of |
|
Linux results are ready. |
This is due to #92083, it's been reverted now. |
|
And I would bet that a majority of the unresolved are due to: You can either install it with Also there is |
I'll do that, but I think LLDB should follow the example of MLIR here, so that pipeline scripts don't have hard-coded dependencies for individual projects: llvm-project/.ci/monolithic-linux.sh Line 41 in cee6e81 llvm-project/mlir/python/requirements.txt Lines 1 to 3 in a58dd0e |
|
@DavidSpickett I made the changes you suggested. Let's see how this Linux run fares. |
Linaro's Windows bot uses clang-cl, but I do remember something like this there too. I will do a local build with msvc and see what I can do. |
|
|
Linux is now passing in 30 minutes. It's not clear how much overhead LLDB actually added, because other Linux builds on Clang changes are anywhere between 22 and 28 minutes. |
|
This is awesome, thank you for working on this! |
|
Tagging @slydiman @vvereschaka |
Possibly! I'm talking about relying on implementation details of a project and I think the textual representation of LLVM IR isn't necessarily an implementation detail of LLVM. But if the change was to an implementation detail and LLVM had a need to make the change, it's on the Clang community to react to that change.
I'm not trying to suggest that Clang can change anything it wants and lldb needs to just deal with it. I'm saying that Clang needs the ability to change our internal implementation details without requiring a PR author to update downstream projects relying on those implementation details. We should work together to reduce the amount of burden between both projects of course, but if a downstream project is expecting a very specific wording for a diagnostic message, Clang developers should not have their (correct) changes reverted because they updated the diagnostic wording and it broke a downstream test. Would it be useful for us to have a conference call to discuss this in more detail to make sure we're all clear and comfortable? |
|
I had a nice chat with @ldionne and @nikic today and we touched on the topic of precommit CI for lldb being triggered from Clang, and that resulted in a change of opinion and perhaps a better way for me to phrase what I'm after and how it relates to this PR. I think that what we ultimately need is improved coordination between projects when a change has wider-reaching impacts. If a change to an implementation detail in Clang breaks an lldb test, we don't want to land the change and disrupt the community. But the developer making the change may not be able to fix the issues outside of Clang (lack of expertise, time constraints, etc) and we don't want them to walk away from the PR as a result. I think what would be a better situation would be for 1) the issue to be noticed during a precommit CI run, 2) the PR author to try to address the issues themselves, 3) the impacted project to be willing to step in and help out if needed (discuss solutions, offer to push changes to the PR to solve the issue, etc). Basically, everyone working together to try to get the patch landed with as little disruption or overhead as possible. (3) comes with a fair amount of friction though because it's not always obvious who to reach out to, and (particularly for new contributors) it can be awkward to reach out to strangers to impose on them. So I wonder if it would be possible (or make sense) to have a precommit CI failure to automatically add the relevant PR subscribers/reviewers group to the PR. e.g., if there's a PR with a If we had something like that, then I think we could manually exclude the handful of sometimes flaky tests but otherwise add lldb precommit CI to Clang's pipeline. |
|
I would normally have no issue making the changes needed to other projects, and I mostly try to do that. It's a bit concerning when the changes needed are quite trivial, for example just changing diagnostics expectations, but you have no other way to test that. I think the lldb pre-commit CI would at least help here, for trivial changes, we can keep trying to fix and verifying it by pushing to the PR, no changes needed on developer setup side. |
Can you elaborate on what specifically you find hard? Building should be fairly straightforward, besides Python and SWIG, which are required to run the tests, most of our dependencies are optional. Unlike the compiler, the debugger is (1) tied to the system it's running on, and (2) an interactive tool, so maybe that's what you're referring to? Either way, if there are concrete ways we can make LLDB easier to work on, I'd love to hear your suggestions! |
This was about a year ago, and I remember @erichkeane had similar issues, but the main points:
AFAIK, on windows I would run into this issue where running the LLDB test suite would cause hundreds of frozen terminal windows to spawn, and that was quite difficult to recover from without rebooting. I eventually thought that was too dangerous to run on windows and managed to get it working on linux virtual machine. |
I think it would be reasonable to have the CI add the last (or last few) git author(s) for the failing test, to help investigate the issue on the lldb side. |
Windows support is definitely lacking which probably means a bunch of tests don't run there. We do have one buildbot that runs on Windows so I have a hard time believing it's totally broken. The last runs are consistently green and based on the number of tests it seems to still cover a remarkable subset of the test suite.
This simply isn't true. Xcode is a multi-config build system and we have a bot that ensures that doesn't break. Maybe there's something different about MSVC?
I really sympathize with this one. Most of that's boils down to how much we have to rely on the OS and other tools (the compiler, the linker, etc) for our integration tests. Having more tests written as unit tests or regression tests using |
|
We have configured few buildbots for cross lldb tests with Windows x86_64, Linux x86_64 hosts and Linux Aarch64 targets. Our main setup is Windows host and currently it is 100% green. We still have a number of local patches. The biggest changes are related to Makefile.rules to build API tests with different tools and hosts and linking the correct libraries on Linux host. We also adapted Shell tests to run on a remote target. We will prepare PRs to mainline soon. We have fugured out that currently lldb does not support CXX ABI 2. Few tests are marked as XFAIL with unclear reasons and strange conditions, so we got few XPASS tests. Most failed API tests on Windows hosts were related to path separators. Currently SBFileSpec (FileSpec wrapper for python) does not have a constructor to pass tripple or style, so it is impossible to create a POSIX path in API tests on the Windows host. But FileSpec stores a normalized path and the path style is completelly ignored inside lldb code. |
The page I linked about this deprecated variable talks about poor support in "Ninja Multi-Config" specifically, which is what I use, but implies it can be used in Xcode. |
|
@AaronBallman @JDevlieghere @Michael137 @adrian-prantl @DavidSpickett @mizvekov I updated the PR to test LLDB on Clang changes. I hope we can start gathering approvals to get this PR moving. |
| @@ -72,7 +72,7 @@ class PrintFunctionsConsumer : public ASTConsumer { | |||
| *sema.LateParsedTemplateMap.find(FD)->second; | |||
| sema.LateTemplateParser(sema.OpaqueParser, LPT); | |||
| llvm::errs() << "late-parsed-decl: \"" << FD->getNameAsString() << "\"\n"; | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Unrelated whitespace change?
There was a problem hiding this comment.
I need to make some changes in Clang for testing purposes. This will be removed before merging.
|
I have no objections in theory, but in practice I remain concerned about the flakiness of lldb's tests for the impacts on Clang reviewers. There are timeout failures, but they might be limited to Windows and thus not a problem with your PR (https://lab.llvm.org/buildbot/#/builders/219/builds/11819 and But then there are failures like: or https://lab.llvm.org/buildbot/#/builders/68/builds/75698 -- different assertion, but like above, seems to be spurious and unrelated to changes in the changeset, such as https://lab.llvm.org/buildbot/#/builders/68/builds/75666 Perhaps one way forward is for lldb tests to run on lldb changes for a few months before enabling them on Clang changes? Then the lldb folks can either report back that things are in pretty good shape from their experience, or we'll maybe have a better idea of how to proceed. That said, if others aren't as concerned about test flakiness, I won't block progress on this PR. |
|
From my side as an author of the PR, I want to see Clang and LLDB code owners agree on the way forward, then I'll implement it if I can. |
|
FYI in the libc++ CI we've been running the LLDB tests for a few months now and I haven't seen a single unrelated failure. |
Thanks, that's really useful information! |
Do note that libc++ CI only runs a subset of the LLDB tests. Only the ones that are impacted by libc++; these tests are not flakey (or I haven't seen them be at least). We could take the same approach for clang if we find the whole test-suite to be too flakey i suppose |
|
@Endilll This looks good, thanks! How will this affect test capacity? Right now, the Linux bots are lagging, while the Windows bot is breezing through. This is the opposite of the usual. Are we under-provisioned on Linux CI resources? How much worse will it get when we add this extra workload? |
Evidence is anecdotal, but we're looking at about 4 more minutes on Linux CI. |
This patch removes LLDB from a list of projects that are excluded from building and testing on pre-merge CI on Linux. Windows environment needs to be prepared in order to test LLDB (llvm#94208 (comment)), but we don't have enough maintenance resources to do that at the moment. Because LLDB has been in the list of projects that need to be tested on Clang changes, this PR make this happen on Linux. This seems to be the consensus in the discussion of this PR. Signed-off-by: Hafidz Muzakky <[email protected]>
This is a temporary measure to alleviate Linux pre-commit CI waiting times that started snowballing [recently](https://discourse.llvm.org/t/long-wait-for-linux-presubmit-testing/79547/5). My [initial estimate](#94208 (comment)) of 4 additional minutes spent per built seems to be in the right ballpark, but looks like that was the last straw to break camel's back. It seems that CI load got past the tipping point, and now it's not able to burn through the queue over the night on workdays. I don't intend to overthrow the consensus we reached in #94208, but it shouldn't come at the expense of the whole LLVM community. I'll enable this back as soon as we have news that we got more capacity for Linux pre-commit CI.
This patch removes LLDB from a list of projects that are excluded from building and testing on pre-merge CI on Linux.
Windows environment needs to be prepared in order to test LLDB (#94208 (comment)), but we don't have enough maintenance resources to do that at the moment.
Because LLDB has been in the list of projects that need to be tested on Clang changes, this PR make this happen on Linux. This seems to be the consensus in the discussion of this PR.