Page MenuHomePhabricator

[CMake] Check that a certificate for lldb is present at build time.
ClosedPublic

Authored by davide on Mon, Jun 24, 4:46 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

davide created this revision.Mon, Jun 24, 4:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jun 24, 4:46 PM
Herald added a subscriber: mgorny. · View Herald Transcript
This revision is now accepted and ready to land.Mon, Jun 24, 4:46 PM

rdar://problem/52078735

JDevlieghere requested changes to this revision.Mon, Jun 24, 5:52 PM

On second thought, let's check that LLDB_CODESIGN_IDENTITY equals lldb_codesign before doing this check.

This revision now requires changes to proceed.Mon, Jun 24, 5:52 PM
labath added a subscriber: labath.Mon, Jun 24, 11:42 PM

This code should go to tools/debugserver/source/CMakeLists.txt so that it is next to the code which performs the actual code signing. Doing that will make it easier to keep it in sync with changes to code signing, as well as make it obvious that it is not in sync with them right now (there's a pretty complex interaction of various cmake options (LLDB_CODESIGN_IDENTITY, LLVM_CODESIGNING_IDENTITY, LLDB_USE_SYSTEM_DEBUGSERVER, etc.) which affects code signing, and this code is ignoring all of those)...

On second thought, let's check that LLDB_CODESIGN_IDENTITY equals lldb_codesign before doing this check.

This question isn't important but I'm kind of curious: Does it have to be called lldb_codesign? Could you have an arbitrary identity and then sign with that, assuming the cert exists, or does debugserver expect a cert with that name exactly?

lldb/CMakeLists.txt
77 ↗(On Diff #206330)

I think CMAKE_SYSTEM_NAME MATCHES "Darwin" is redundant because debugserver should only be a target if you're running on Darwin, per the logic in tools/CMakeLists.txt.

This code should go to tools/debugserver/source/CMakeLists.txt so that it is next to the code which performs the actual code signing. Doing that will make it easier to keep it in sync with changes to code signing, as well as make it obvious that it is not in sync with them right now (there's a pretty complex interaction of various cmake options (LLDB_CODESIGN_IDENTITY, LLVM_CODESIGNING_IDENTITY, LLDB_USE_SYSTEM_DEBUGSERVER, etc.) which affects code signing, and this code is ignoring all of those)...

+1

On second thought, let's check that LLDB_CODESIGN_IDENTITY equals lldb_codesign before doing this check.

This question isn't important but I'm kind of curious: Does it have to be called lldb_codesign? Could you have an arbitrary identity and then sign with that, assuming the cert exists, or does debugserver expect a cert with that name exactly?

The name doesn't matter.

In D63745#1556941, @labath wrote:
This code should go to tools/debugserver/source/CMakeLists.txt

+1

More precisely, this is the condition where you want to do your check:
https://github.com/llvm/llvm-project/blob/287f0403e310/lldb/tools/debugserver/source/CMakeLists.txt#L131

You don't need if(CMAKE_SYSTEM_NAME MATCHES "Darwin" AND TARGET debugserver) there.
Use ${LLDB_CODESIGN_IDENTITY_USED} instead of lldb_codesign.
Error message could recommend -DLLDB_USE_SYSTEM_DEBUGSERVER=ON and/or -DLLDB_NO_DEBUGSERVER=ON

This code should go to tools/debugserver/source/CMakeLists.txt so that it is next to the code which performs the actual code signing. Doing that will make it easier to keep it in sync with changes to code signing, as well as make it obvious that it is not in sync with them right now (there's a pretty complex interaction of various cmake options (LLDB_CODESIGN_IDENTITY, LLVM_CODESIGNING_IDENTITY, LLDB_USE_SYSTEM_DEBUGSERVER, etc.) which affects code signing, and this code is ignoring all of those)...

New patch coming, thanks!

davide updated this revision to Diff 206475.Tue, Jun 25, 9:55 AM

Address feedback from many.

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Jun 25, 10:13 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jun 25, 10:13 AM