Page MenuHomePhabricator

[lldb/test] Automatically find debug servers to test
ClosedPublic

Authored by labath on Feb 6 2021, 6:21 AM.

Details

Summary

Our test configuration logic assumes that the tests can be run either
with debugserver or with lldb-server. This is not entirely correct,
since lldb server has two "personalities" (platform server and debug
server) and debugserver is only a replacement for the latter.

A consequence of this is that it's not possible to test the platform
behavior of lldb-server on macos, as it is not possible to get a hold of
the lldb-server binary.

One solution to that would be to duplicate the server configuration
logic to be able to specify both executables. However, that seems
excessively redundant.

A well-behaved lldb should be able to find the debug server on its own,
and testing lldb with a different (lldb-|debug)server does not seem very
useful (even in the out-of-tree debugserver setup, we copy the server
into the build tree to make it appear "real").

Therefore, this patch deletes the configuration altogether and changes
the low-level server retrieval functions to be able to both lldb-server
and debugserver paths. They do this by consulting the "support
executable" directory of the lldb under test.

Diff Detail

Event Timeline

labath created this revision.Feb 6 2021, 6:21 AM
labath requested review of this revision.Feb 6 2021, 6:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2021, 6:21 AM
labath added inline comments.Feb 6 2021, 6:26 AM
lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
75–79

@rupprecht : I'm adding you mainly because my last attempt to change this function broke some google integration stuff.

JDevlieghere accepted this revision.Feb 8 2021, 12:51 PM

This will break my "run the tests against an Xcode install", but it seems like I should be able to work around that looking for debugserver in the LLDB.framework in get_debugserver_exe. Overall this is a pretty nice cleanup so seems like a fair trade-off. LGTM.

This revision is now accepted and ready to land.Feb 8 2021, 12:51 PM
labath added a comment.Feb 9 2021, 2:04 AM

This will break my "run the tests against an Xcode install", but it seems like I should be able to work around that looking for debugserver in the LLDB.framework in get_debugserver_exe. Overall this is a pretty nice cleanup so seems like a fair trade-off. LGTM.

What exactly is that use case? Do you want to run the tests with the debugserver that comes with that xcode install? If that's the case, then I would expect this to just work, as lldb knows that in a framework build, the "support executable directory" is located inside the framework: https://github.com/llvm/llvm-project/blob/main/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm#L138

This will break my "run the tests against an Xcode install", but it seems like I should be able to work around that looking for debugserver in the LLDB.framework in get_debugserver_exe. Overall this is a pretty nice cleanup so seems like a fair trade-off. LGTM.

What exactly is that use case? Do you want to run the tests with the debugserver that comes with that xcode install? If that's the case, then I would expect this to just work, as lldb knows that in a framework build, the "support executable directory" is located inside the framework: https://github.com/llvm/llvm-project/blob/main/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm#L138

Even better, that's exactly the logic I had in mind, but I was going to add it in dotest based on the --framework argument. Cool, cool, cool.

rupprecht added inline comments.Feb 10 2021, 9:34 AM
lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
75–79

Thanks, patching this in now! (Sorry for the delay, I was out a couple days)

What exactly is that use case? Do you want to run the tests with the debugserver that comes with that xcode install? If that's the case, then I would expect this to just work, as lldb knows that in a framework build, the "support executable directory" is located inside the framework: https://github.com/llvm/llvm-project/blob/main/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm#L138

Even better, that's exactly the logic I had in mind, but I was going to add it in dotest based on the --framework argument. Cool, cool, cool.

Cool, I'm glad that this works out.

lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
75–79

Sure, np. Let me know how it works out.

rupprecht accepted this revision.Feb 10 2021, 12:28 PM
rupprecht added inline comments.
lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
75–79

Everything I've tried to throw this against seems to be working. I'm not sure exactly what the problem was before though. Seems fine to land and I can take a closer look if something fails later. Thanks for the heads up!

labath added inline comments.Feb 11 2021, 5:24 AM
lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
75–79

Thanks for checking it out.

The previous patch simply removed the LLDB_DEBUGSERVER_PATH thingy, and left the original logic logic in place (which was calculating the lldb server location based on realpath(3) of the lldb binary, which does not work well inside google).

This revision was automatically updated to reflect the committed changes.

It looks like this change resulted in a failing test on the Windows LLDB bot:

http://lab.llvm.org:8011/#/builders/83/builds/3602

It seems like one of the tests is trying to re-use the same log names within a single run and they are still being used by the previous instance from the same test run. Can you have a look?

It looks like this change resulted in a failing test on the Windows LLDB bot:

http://lab.llvm.org:8011/#/builders/83/builds/3602

It seems like one of the tests is trying to re-use the same log names within a single run and they are still being used by the previous instance from the same test run. Can you have a look?

I don't understand yet what is going on, but it does seem like this patch is to blame. I'll revert while I try to understand this.

goncharov added inline comments.
lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
75–76

hi @labath ! Sorry for ignorance but what is the reason to remove this resolution through env? For me (due to complicated build system) lldb-server is located in completely different location and "which lldb-server" finds nothing. cc @rupprecht

labath added inline comments.Feb 22 2021, 12:20 PM
lldb/packages/Python/lldbsuite/test/tools/lldb-server/lldbgdbserverutils.py
75–76

The main reason is that the variable was overloaded to mean both the path to the debugserver and lldb-server binaries (which meant that it was not possible to find both), though one could also argue this environment variable override is a gross hack in itself.

Where does GetLLDBPath(lldb.ePathTypeSupportExecutableDir) point to in your case? Could it be fixed (maybe via the new SharedLibraryDirectoryHelper (D96779)), or lldb-server moved so that the result is correct. If the support executable directory is not correct, then you're likely to run into other problems besides failing tests.

teemperor added inline comments.
lldb/test/API/CMakeLists.txt
115

I believe this removed line is causing us to have missing dependencies. I just did a clean build of LLDB and it turns out that debugserver is no longer a dependency of check-lldb, which causes a bunch of weird test failures such as:

********************
FAIL: lldb-unit :: tools/lldb-server/tests/./LLDBServerTests/StandardStartupTest.TestStopReplyContainsThreadPcs (2268 of 2282)
******************** TEST 'lldb-unit :: tools/lldb-server/tests/./LLDBServerTests/StandardStartupTest.TestStopReplyContainsThreadPcs' FAILED ********************
Note: Google Test filter = StandardStartupTest.TestStopReplyContainsThreadPcs
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from StandardStartupTest
[ RUN      ] StandardStartupTest.TestStopReplyContainsThreadPcs
/Users/teemperor/3llvm/llvm-project/lldb/unittests/tools/lldb-server/tests/TestBase.h:49: Failure
Value of: llvm::detail::TakeExpected(ClientOr)
Expected: succeeded
  Actual: failed  (executable doesn't exist: '/Users/teemperor/3llvm/rel/bin/debugserver')
[  FAILED  ] StandardStartupTest.TestStopReplyContainsThreadPcs (5 ms)
[----------] 1 test from StandardStartupTest (5 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (5 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] StandardStartupTest.TestStopReplyContainsThreadPcs

 1 FAILED TEST

I haven't bisected yet what exactly did remove the debugserver dependency but I'm pretty sure it's this patch (due to being landed recently and me not finding any other place where we added this dependency)

labath added inline comments.Mar 8 2021, 8:26 AM
lldb/test/API/CMakeLists.txt
115

Thanks for letting me know, and sorry about the breakage. Your analysis sounds believable.
I guess this should be replaced by something like

if(TARGET debugserver)
  add_lldb_test_dependency(debugserver)
endif()
if(TARGET lldb-server)
  add_lldb_test_dependency(lldb-server)
endif()

I can do that tomorrow.

teemperor added inline comments.Mar 8 2021, 8:27 AM
lldb/test/API/CMakeLists.txt
115

I'm still in office for a while so I can do it now :)