Page MenuHomePhabricator

[CMake] Streamline code signing for debugserver #2
ClosedPublic

Authored by sgraenitz on Nov 28 2018, 11:34 AM.

Details

Summary

Major fixes after D54476 (use Diff1 as base for comparison to see only recent changes):

  • In standalone builds target directory for debugserver must be LLDB's bin, not LLVM's bin
  • Default identity for code signing must not force-override LLVM_CODESIGNING_IDENTITY globally

We have a lot of cases, make them explicit:

  • ID used for code signing (debugserver and in tests):
    • LLDB_CODESIGN_IDENTITY if set explicitly, or otherwise
    • LLVM_CODESIGNING_IDENTITY if set explicitly, or otherwise
    • lldb_codesign as the default
  • On Darwin we have a debugserver target that:
    1. we build and sign if:
      1. LLDB_NO_DEBUGSERVER=OFF (default) && LLDB_USE_SYSTEM_DEBUGSERVER=OFF (default) && code signing ID == lldb_codesign
    2. we copy from the system if:
      1. LLDB_USE_SYSTEM_DEBUGSERVER=ON and found on system (explicit case)
      2. code signing ID != lldb_codesign and found on system (fallback case)
    3. will not be available if
      1. LLDB_NO_DEBUGSERVER=ON
      2. code signing ID != lldb_codesign and not found on system
  • On other systems, the debugserver target is not defined, which is equivalent to [3A]

Common configurations on Darwin:

  • [1A] cmake -GNinja ../llvm builds debugserver from source and signs with lldb_codesign, no code signing for other binaries (prints status: lldb debugserver: /path/to/bin/debugserver)
  • [1A] cmake -GNinja -DLLVM_CODESIGNING_IDENTITY=- -DLLDB_CODESIGN_IDENTITY=lldb_codesign ../llvm builds debugserver from source and signs with lldb_codesign, ad-hoc code signing for other binaries (prints status: lldb debugserver: /path/to/bin/debugserver)
  • [2A] cmake -GNinja -DLLVM_CODESIGNING_IDENTITY=- -DLLDB_USE_SYSTEM_DEBUGSERVER=ON ../llvm copies debugserver from system, ad-hoc code signing for other binaries (prints status: Copy system debugserver from: /path/to/system/debugserver)
  • [2B] cmake -GNinja -DLLVM_CODESIGNING_IDENTITY=- ../llvm same, but prints additional warning: Cannot code sign debugserver with identity '-'. Will fall back to system's debugserver.
  • [3A] cmake -GNinja -DLLVM_CODESIGNING_IDENTITY=- -DLLDB_NO_DEBUGSERVER=ON ../llvm debugserver not available (prints status: lldb debugserver will not be available)

Diff Detail

Repository
rL LLVM

Event Timeline

sgraenitz created this revision.Nov 28 2018, 11:34 AM

Fix target directory for debugserver: it must not be LLVM_TOOLS_BINARY_DIR but LLVM_RUNTIME_OUTPUT_INTDIR. It's a difference in standalone builds of LLDB. LLVM_TOOLS_BINARY_DIR was wrong here as it points to the binary directory in the detached LLVM build tree. It should make no difference for in-tree builds.

Default identity for code signing must be lldb_codesign. Also remove the comment to deprecate LLDB_CODESIGN_IDENTITY in favor of LLVM_CODESIGNING_IDENTITY.

Move code sign settings to LLDBConfig.cmake

sgraenitz edited the summary of this revision. (Show Details)Nov 28 2018, 12:03 PM
sgraenitz added reviewers: JDevlieghere, beanz.
JDevlieghere added inline comments.Nov 28 2018, 1:39 PM
tools/debugserver/CMakeLists.txt
18 ↗(On Diff #175739)

Why do we need to define this option again for the debugserver? Why can't it use the one from LLDBConfig.cmake?

sgraenitz marked an inline comment as done.Nov 28 2018, 3:26 PM

Thanks for having a look.

tools/debugserver/CMakeLists.txt
18 ↗(On Diff #175739)

In a debugserver standalone build we don't include(LLDBConfig). It makes sense to me, because there is not much to configure when only building the debugserver.

sgraenitz marked an inline comment as not done.Nov 28 2018, 3:26 PM

In-tree builds look good now. I successfully built and tested with these configurations:

green dragon: xcrun cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=On -DLLVM_ENABLE_MODULES=On ../llvm
my default: xcrun cmake -GNinja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DLLVM_TARGETS_TO_BUILD="X86;ARM;AArch64" ../llvm

Had a short look at standalone builds. Including the test suite is currently not supported. With this config I could build the binaries successfully (with a quick-fix for debugserver D55032):

LLDB standalone: xcrun cmake -GNinja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DLLVM_TARGETS_TO_BUILD="X86;ARM;AArch64" -DLLVM_CONFIG=/path/to/llvm-build/llvm-config ../lldb
debugserver standalone: xcrun cmake -GNinja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DLLVM_TARGETS_TO_BUILD="X86;ARM;AArch64" -DLLVM_CONFIG=/path/to/llvm-build/bin/llvm-config ../lldb/tools/debugserver
JDevlieghere added inline comments.Nov 28 2018, 4:05 PM
tools/debugserver/CMakeLists.txt
18 ↗(On Diff #175739)

Makes sense. Maybe we could put this in the top level CMakeLists.txt?

sgraenitz added inline comments.
tools/debugserver/CMakeLists.txt
18 ↗(On Diff #175739)

This is the top level CMakeLists.txt for standalone debugserver :) See my previous comment about testing.

JDevlieghere accepted this revision.Dec 4 2018, 10:08 AM

LGTM. Thanks!

tools/debugserver/CMakeLists.txt
18 ↗(On Diff #175739)

Thanks. I'm not super excited about duplicating the variables but I understand why it's needed. I think long term it'd be great if we could extract some stuff form the top-level cmake file and just make it dispatch things, but that's beyond the scope of what you're doing right now.

This revision is now accepted and ready to land.Dec 4 2018, 10:08 AM
sgraenitz updated this revision to Diff 176949.Dec 6 2018, 4:17 AM

Avoid conflicts: updating diff for recent changes on master

sgraenitz updated this revision to Diff 177486.Dec 10 2018, 4:15 AM

Avoid force-overwriting of LLVM_CODESIGNING_IDENTITY

sgraenitz edited the summary of this revision. (Show Details)Dec 10 2018, 4:16 AM
sgraenitz added a reviewer: labath.

Sorry for editing this again, but D55328 brought up an important change request (see https://reviews.llvm.org/D55328#anchor-1322655) and I only realized this side-effect afterwards: adding LLDB to a LLVM build tree (all with default configurations), would have caused all binaries to be signed with the lldb_codesign identity. This is clearly not intended. The modified summary now outlines the identity handling in more detail. @labath thanks for bringing up your concerns, I did the change here and will adjust the other review.

sgraenitz edited the summary of this revision. (Show Details)Dec 10 2018, 4:34 AM
sgraenitz marked 9 inline comments as done.
sgraenitz added inline comments.
CMakeLists.txt
141 ↗(On Diff #177486)

If we have a debugserver target, tests should depend on it (either copy or build).

tools/debugserver/CMakeLists.txt
18 ↗(On Diff #175739)

Less duplication as LLDB_CODESIGN_IDENTITY went back to debugserver CMakeLists.txt

tools/debugserver/source/CMakeLists.txt
109 ↗(On Diff #177486)

New logic for code sign identity. Using a separate cache variable to avoid varying behavior on reconfigurations.

labath accepted this revision.Dec 10 2018, 7:55 AM

This looks much better. Thanks.

tools/debugserver/source/CMakeLists.txt
104 ↗(On Diff #177486)

Could you also mark this as internal, to avoid it showing up in the GUIs and such.

sgraenitz marked 2 inline comments as done.

Turn LLDB_CODESIGN_IDENTITY_USED variable from STRING into INTERNAL

This revision was automatically updated to reflect the committed changes.