Page MenuHomePhabricator

[CMake] Streamline code signing for debugserver
ClosedPublic

Authored by sgraenitz on Nov 13 2018, 7:40 AM.

Details

Summary

Use llvm_codesign to sign debugserver with entitlements. Handle reconfigurations correctly. We have a lot of cases, make them explicit:

(1) build and sign debugserver, if all conditions apply:

  • LLDB_NO_DEBUGSERVER=OFF (default)
  • On Darwin: LLDB_USE_SYSTEM_DEBUGSERVER=OFF (default)
  • On Darwin: LLVM_CODESIGNING_IDENTITY == lldb_codesign

(2) use system debugserver, if on Darwin and any of:

  • LLDB_USE_SYSTEM_DEBUGSERVER=ON and debugserver found on system (explicit case)
  • LLVM_CODESIGNING_IDENTITY != lldb_codesign and debugserver found on system (fallback case)

(3) debugserver will not be available, in case of:

  • LLDB_NO_DEBUGSERVER=ON
  • On Darwin: LLVM_CODESIGNING_IDENTITY != lldb_codesign and debugserver not found on system

(4) error state, in case of:

  • LLDB_USE_SYSTEM_DEBUGSERVER=ON and debugserver not found on system
  • LLDB_USE_SYSTEM_DEBUGSERVER=ON and LLDB_NO_DEBUGSERVER=ON

Diff Detail

Repository
rL LLVM

Event Timeline

sgraenitz created this revision.Nov 13 2018, 7:40 AM
JDevlieghere added inline comments.Nov 13 2018, 10:18 AM
tools/debugserver/source/CMakeLists.txt
101 ↗(On Diff #173854)

Should we emit and error if these variables are in an inconsistent state, e.g. when both are set?

118 ↗(On Diff #173854)

Why did you make this variable name lowercase? Does that have any semantic meaning?

xiaobai added inline comments.Nov 13 2018, 11:08 AM
tools/debugserver/source/CMakeLists.txt
128–133 ↗(On Diff #173854)

I'm pretty sure that if you do not copy debugserver it will still find the one from your ${lldb_framework_dir} when you run lldb anyway. Do you know why we copy it over?

sgraenitz added inline comments.Nov 13 2018, 11:57 AM
tools/debugserver/source/CMakeLists.txt
101 ↗(On Diff #173854)

Yes, could do that.

118 ↗(On Diff #173854)

LLVM tends to use lowercase names for local/non-cached variables, e.g. update_src_props in:
https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/AddLLVM.cmake

I didn't find any guidelines on conventions for CMake, but I thought it's a good idea. It's a little surprising to mix in such changes to patches that also have semantic changes. Maybe I should continue to use uppercase names for now and add a subsequent patch (with a proper title) that does the change for the whole file. What do you think?

128–133 ↗(On Diff #173854)

Yes this is what I mean with "old behaviour" here. DEBUGSERVER_PATH still points to the original system binary and gets passed to tests, e.g. in test/CMakeLists.txt above.

JDevlieghere added inline comments.Nov 13 2018, 12:39 PM
tools/debugserver/source/CMakeLists.txt
118 ↗(On Diff #173854)

Cool, I wasn't aware of the convention. I think either is fine, whatever you prefer.

sgraenitz updated this revision to Diff 174053.Nov 14 2018, 9:11 AM
sgraenitz marked 4 inline comments as done.

debugserver cannot be ad-hoc code signed; handle entitlements&identity also in standalone debugserver, add feasibility checks, polishing

sgraenitz marked 2 inline comments as not done.Nov 14 2018, 9:13 AM
sgraenitz added inline comments.
tools/debugserver/source/CMakeLists.txt
128–133 ↗(On Diff #173854)

Do you know why we copy it over?

It was introduced with https://reviews.llvm.org/rL325068 and the commit message points out, that this makes the built debugger functional on Darwin when compiling without code signing. In order to use debugserver on Darwin, it cannot be ad-hoc code signed, but needs a "real" certificate (as documented in docs/code-signing.txt).

Got in touch with Vedant and it seems, that DEBUGSERVER_PATH could also point to the copied version. Did that.

sgraenitz edited the summary of this revision. (Show Details)Nov 14 2018, 9:14 AM
sgraenitz added a subscriber: vsk.Nov 14 2018, 9:24 AM
labath added a subscriber: labath.Nov 15 2018, 9:09 AM
labath added inline comments.
tools/debugserver/source/CMakeLists.txt
101 ↗(On Diff #173854)

Alternatively, we could make this a single tri-state variable. LLDB_DEBUGSERVER_MODE=OFF|SYSTEM|TREE or something ?

sgraenitz marked an inline comment as not done.
sgraenitz edited the summary of this revision. (Show Details)

Handle reconfigurations correctly; fix configuration messages; add note for generator expressions

Improve description for options LLDB_NO_DEBUGSERVER and LLDB_USE_SYSTEM_DEBUGSERVER

sgraenitz edited the summary of this revision. (Show Details)Nov 15 2018, 11:26 AM

Last revision changed a lot, but I think support for reconfigurations is worth it.
The current state would be acceptable from my side now -- if I didn't miss anything! :-)

tools/debugserver/source/CMakeLists.txt
159 ↗(On Diff #174254)

This is not a regression. Just added a verbose comment to remember once we look at this.

190 ↗(On Diff #174254)

This is resetting DEBUGSERVER_PATH and SKIP_TEST_DEBUGSERVER cached variables for the 3 non-error cases (see updated summary). It should be reconfiguration-safe. All manual testing on Darwin passed.

101 ↗(On Diff #173854)

Yes possible, but I hope it's not necessary. It would be great to keep existing names for user-definable options where possible. Otherwise it breaks bots, etc.

JDevlieghere accepted this revision.Nov 15 2018, 5:15 PM

Thanks for working on this, Stefan. This LGTM.

This revision is now accepted and ready to land.Nov 15 2018, 5:15 PM

Set global LLVM_CODESIGNING_IDENTITY from LLDB_CODESIGN_IDENTITY (if given).
Pass through ENTITLEMENTS from add_lldb_executable to add_llvm_executable.

My change for lldb-server (https://reviews.llvm.org/D54444) became obsolete, but two small pieces of it are required in general/here.
Added them post-approval with the update above. It's the two additions to cmake/modules/AddLLDB.cmake and CMakeLists.txt. Will leave this open for a little longer and commit next week.

JDevlieghere accepted this revision.Nov 16 2018, 11:04 AM
This revision was automatically updated to reflect the committed changes.