This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Always build debugserver on Darwin and allow tests to use the system's one
ClosedPublic

Authored by sgraenitz on Jul 16 2019, 9:33 AM.

Details

Summary

We can always build debugserver, but we can't always sign it to be useable for testing. LLDB_USE_SYSTEM_DEBUGSERVER should only tell whether or not the system debugserver should be used for testing.
The old behavior complicated the logic around debugserver a lot. The new logic sorts out most of it.

Please note that this patch is in early stage and needs some more testing. It should not affect platfroms other than Darwin. It builds on Davide's approach to validate the code-signing identity at configuration time.

What do you think?

Diff Detail

Repository
rL LLVM

Event Timeline

sgraenitz created this revision.Jul 16 2019, 9:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 9:33 AM
sgraenitz updated this revision to Diff 210124.Jul 16 2019, 9:47 AM

Minor fixes

sgraenitz marked 3 inline comments as done.Jul 16 2019, 9:53 AM
sgraenitz added inline comments.
lldb/CMakeLists.txt
94 ↗(On Diff #210124)

The target will always exist now. The dependency would now depend on LLDB_USE_SYSTEM_DEBUGSERVER and set in lldb/test/CMakeLists.txt.

lldb/tools/debugserver/source/CMakeLists.txt
86 ↗(On Diff #210124)

The option may move to LLDBConfig.cmake? Actually, it affects not only the debugserver target, but also test configuration.

258 ↗(On Diff #210124)

Removing the if() ... endif() would bloat the diff even more. Would do it in a follow-up NFC commit.

sgraenitz marked an inline comment as not done.Jul 16 2019, 9:54 AM

Don't pass ON/OFF for LLDB_SERVER_IS_DEBUGSERVER as it becomes as #define

I think that this is a good change overall to do but the name LLDB_USE_SYSTEM_DEBUGSERVER is kind of a confusing to me. The name itself doesn't really convey to me that the system debugserver is going to be used for just testing. Before this change, that flag indicated that the system debugserver was going to be used for everything. Maybe you could change it to something like LLDB_TEST_WITH_SYSTEM_DEBUGSERVER?

That's a fair point. However, changing the name of the parameter would force us to update quite a number of bot configurations, caches and documentation. So if it's "acceptable" I would be in favor to keep LLDB_USE_SYSTEM_DEBUGSERVER.
Also it's kind-of in line with the (internal) LLDB_CAN_USE_DEBUGSERVER and LLDB_CAN_USE_LLDB_SERVER variables.

compnerd added inline comments.Jul 17 2019, 2:16 PM
lldb/cmake/modules/AddLLDB.cmake
292 ↗(On Diff #210357)

Can you add a comment explaining that you want to strip leading whitespace? Alternatively, if its trailing whitespace, please use OUTPUT_STRIP_TRAILING_WHITESPACE in the execute_process on L288 please.

293 ↗(On Diff #210357)

I think that subdir is a misleading name. This is the path to debugserver, subdir should be LLDB.framework/Resources. I don't really have an opinion on renaming the variable or composing the variable, that choice is yours :-)

lldb/tools/debugserver/source/CMakeLists.txt
258 ↗(On Diff #210124)

Sure, although ignoring whitespace also is helpful :-)

lldb/unittests/CMakeLists.txt
83 ↗(On Diff #210357)

Shouldn't debugserver not be available always? It doesn't matter if it isn't being used. Furthermore, elision from the distribution targets will also prevent the unnecessary build so there is no need to worry about the default builds being longer than necessary.

lldb/unittests/tools/lldb-server/CMakeLists.txt
16 ↗(On Diff #210357)

Why is this being checked in lldb-server?

20 ↗(On Diff #210357)

Should this not be debugserver?

sgraenitz updated this revision to Diff 210448.Jul 17 2019, 3:54 PM
sgraenitz marked 9 inline comments as done.

Address Saleem's feedback

Thanks for taking a look.

lldb/cmake/modules/AddLLDB.cmake
292 ↗(On Diff #210357)

It's been there before and I think it may strip a trailing newline. Went with the execute_process variant.

lldb/unittests/CMakeLists.txt
83 ↗(On Diff #210357)

Shouldn't debugserver not be available always?

This is the directory for debugserver unittests.

lldb/unittests/tools/lldb-server/CMakeLists.txt
16 ↗(On Diff #210357)

It sets LLDB_SERVER_IS_DEBUGSERVER

I did some more testing and from my point of view this seems to work. If it looks acceptable to you, I would like to land it tomorrow (CEST) before the 9.0 branch cut.

JDevlieghere accepted this revision.Jul 17 2019, 4:38 PM
This revision is now accepted and ready to land.Jul 17 2019, 4:38 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 6:30 AM
xiaobai added inline comments.Jul 18 2019, 5:05 PM
lldb/trunk/tools/debugserver/source/CMakeLists.txt
50

Could we actually preserve LLDB_NO_DEBUGSERVER? After pulling this patch down into my tree and modifying my caches, I realize that I never really change or test debugserver and would like to avoid building it at all.

sgraenitz marked 2 inline comments as done.Jul 19 2019, 3:44 AM
sgraenitz added inline comments.
lldb/trunk/tools/debugserver/source/CMakeLists.txt
50

This should be equivalent to -DLLDB_TOOL_DEBUGSERVER_BUILD=OFF

sgraenitz marked an inline comment as done.Jul 19 2019, 3:46 AM
sgraenitz added inline comments.
lldb/trunk/tools/debugserver/source/CMakeLists.txt
50

Ok this is a different issue. I guess we should align these:

if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
  add_subdirectory(darwin-debug)
  add_subdirectory(debugserver)
endif()

if (LLDB_CAN_USE_LLDB_SERVER)
  add_lldb_tool_subdirectory(lldb-server)
endif()
sgraenitz marked 2 inline comments as done.Jul 19 2019, 7:55 AM
sgraenitz added inline comments.
lldb/trunk/tools/debugserver/source/CMakeLists.txt
50
sgraenitz marked an inline comment as done.Jul 22 2019, 1:56 AM