This is an archive of the discontinued LLVM Phabricator instance.

Differential D98196

[lldb] Add missing debugserver/lldb-server dependencies to check-lldb
ClosedPublic

Authored by teemperor on Mar 8 2021, 9:45 AM.

Details

Reviewers
JDevlieghere
labath
Commits
rGaada8984e617: [lldb] Add missing debugserver dependency to check-lldb
Summary

D96202 removes the test dependencies on debugserver/lldb-server which is causing a bunch of
tests to fail on a clean build.

This patch re-adds those dependencies. I decided to not add the dependencies in the test/API
folder as we actually need those dependencies in all kinds of tests (we have shell, API and a few unit
tests such as the LLDBServerTests that depend on these binaries).

Diff Detail

Unit TestsFailed

TimeTest
40 msx64 debian > LLVM.Transforms/PhaseOrdering::reassociate-after-unroll.ll
70 msx64 windows > LLVM.Transforms/PhaseOrdering::reassociate-after-unroll.ll

Event Timeline

teemperor created this revision.Mar 8 2021, 9:45 AM
Herald added a subscriber: mgorny. · View Herald TranscriptMar 8 2021, 9:45 AM
teemperor requested review of this revision.Mar 8 2021, 9:45 AM
teemperor mentioned this in D96202: [lldb/test] Automatically find debug servers to test.
Harbormaster completed remote builds in B92693: Diff 329052.Mar 8 2021, 2:23 PM
JDevlieghere accepted this revision.Mar 8 2021, 4:42 PM

LGTM

lldb/test/CMakeLists.txt
48

I was worried about the situation where we tell LLDB to test with the system debugserver, but unless I misremember the target won't be there when that's the case, so this is perfect.

This revision is now accepted and ready to land.Mar 8 2021, 4:42 PM
labath accepted this revision.Mar 8 2021, 11:39 PM
labath added inline comments.
lldb/test/CMakeLists.txt
48

test/API/CMakeLists.txt will create a "debugserver" target in that case (in order to copy it into the build dir). However, I think this is exactly what we want to happen here.

50–52

It seems this is already present on line 59.

JDevlieghere added inline comments.Mar 9 2021, 1:27 PM
lldb/test/CMakeLists.txt
48

So I did misremember, but the result is the same. Nice :-)

teemperor added inline comments.Mar 11 2021, 6:55 AM
lldb/test/CMakeLists.txt
50–52

Good point, I'll just add the dependency there then.

This revision was landed with ongoing or failed builds.Mar 11 2021, 6:56 AM
Closed by commit rGaada8984e617: [lldb] Add missing debugserver dependency to check-lldb (authored by teemperor). · Explain Why
This revision was automatically updated to reflect the committed changes.
teemperor added a commit: rGaada8984e617: [lldb] Add missing debugserver dependency to check-lldb.
Herald added a subscriber: lldb-commits. · View Herald TranscriptMar 11 2021, 6:56 AM

Revision Contents

PathSize
lldb/
test/
9 lines

Diff 329052

lldb/test/CMakeLists.txt

Show All 34 Lines
set_target_properties(lldb-test-deps PROPERTIES FOLDER "lldb misc") set_target_properties(lldb-test-deps PROPERTIES FOLDER "lldb misc")
function(add_lldb_test_dependency) function(add_lldb_test_dependency)
foreach(dependency ${ARGN}) foreach(dependency ${ARGN})
add_dependencies(lldb-test-deps ${dependency}) add_dependencies(lldb-test-deps ${dependency})
endforeach() endforeach()
endfunction(add_lldb_test_dependency) endfunction(add_lldb_test_dependency)
# lldb itself and lldb-test is an hard dependency for the testsuites. # lldb itselt, lldb-test and the different debugservers are an hard dependency
# for the testsuites.
add_lldb_test_dependency(lldb) add_lldb_test_dependency(lldb)
add_lldb_test_dependency(lldb-test) add_lldb_test_dependency(lldb-test)
if(TARGET debugserver)
add_lldb_test_dependency(debugserver)
JDevlieghereUnsubmitted
Not Done
ReplyInline Actions

I was worried about the situation where we tell LLDB to test with the system debugserver, but unless I misremember the target won't be there when that's the case, so this is perfect.

JDevlieghere: I was worried about the situation where we tell LLDB to test with the system debugserver, but…
labathUnsubmitted
Not Done
ReplyInline Actions

test/API/CMakeLists.txt will create a "debugserver" target in that case (in order to copy it into the build dir). However, I think this is exactly what we want to happen here.

labath: test/API/CMakeLists.txt will create a "debugserver" target in that case (in order to copy it…
JDevlieghereUnsubmitted
Not Done
ReplyInline Actions

So I did misremember, but the result is the same. Nice :-)

JDevlieghere: So I did misremember, but the result is the same. Nice :-)
endif()
if(TARGET lldb-server)
add_lldb_test_dependency(lldb-server)
endif()
labathUnsubmitted
Not Done
ReplyInline Actions

It seems this is already present on line 59.

labath: It seems this is already present on line 59.
teemperorAuthorUnsubmitted
Done
ReplyInline Actions

Good point, I'll just add the dependency there then.

teemperor: Good point, I'll just add the dependency there then.
# On Darwin, darwin-debug is an hard dependency for the testsuites. # On Darwin, darwin-debug is an hard dependency for the testsuites.
if (CMAKE_SYSTEM_NAME MATCHES "Darwin") if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
add_lldb_test_dependency(darwin-debug) add_lldb_test_dependency(darwin-debug)
endif() endif()
if(TARGET lldb-server) if(TARGET lldb-server)
add_lldb_test_dependency(lldb-server) add_lldb_test_dependency(lldb-server)
▲ Show 20 Lines • Show All 198 Lines • Show Last 20 Lines