Page MenuHomePhabricator

[CMake] Revised LLDB.framework builds
AcceptedPublic

Authored by sgraenitz on Wed, Dec 5, 7:48 AM.

Details

Summary

Add features to LLDB CMake builds that have so far only been available in Xcode. Clean up a few inconveniences and prepare further improvements.

Options:

  • LLDB_FRAMEWORK_BUILD_DIR determines target directory (in build-tree)
  • LLDB_FRAMEWORK_INSTALL_DIR only determines target directory in install-tree
  • LLVM_EXTERNALIZE_DEBUGINFO allows externalized debug info (dSYM on Darwin, emitted to bin)
  • LLDB_FRAMEWORK_TOOLS determines which executables will be copied to the framework's Resources (dropped symlinking, removed INCLUDE_IN_SUITE, removed dummy targets)

Other changes:

  • clean up add_lldb_executable()
  • include LLDBFramework.cmake from source/API/CMakeLists.txt
  • use *.plist.in files, which are typical for CMake and independent from Xcode
  • add clang headers to the framework bundle

Event Timeline

sgraenitz created this revision.Wed, Dec 5, 7:48 AM

I like seeing all of the cmake modifications for the LLDB.framework. Are we planning on trying to get rid of the Xcode project at some point soon and use the auto generated one made by cmake?

sgraenitz marked an inline comment as done.Wed, Dec 5, 9:05 AM

I am still running some remaining tests, but please let me know what you think about it in the meantime. In case you want to try this patch yourself, please checkout together with the related revisions (see Stack).

cmake/modules/LLDBConfig.cmake
67

designated

friss added a subscriber: friss.Wed, Dec 5, 9:13 AM

I like seeing all of the cmake modifications for the LLDB.framework. Are we planning on trying to get rid of the Xcode project at some point soon and use the auto generated one made by cmake?

While there are no immediate plans to do so, we want to be able to do it and thus we need feature parity. Maintaining the Xcode project over a variety of branches is really expensive.

sgraenitz updated this revision to Diff 176954.Thu, Dec 6, 4:48 AM

Avoid conflicts: updating diff for recent changes on master

sgraenitz updated this revision to Diff 176998.Thu, Dec 6, 9:48 AM

Copy Clang headers to LLDB.framework without an intermediate staging directory. This approach seems much simpler and it avoids errors from an ordering issue at configuration time (preventing Clang headers from being copied to the staging directory).

sgraenitz updated this revision to Diff 177003.Thu, Dec 6, 10:07 AM
sgraenitz marked an inline comment as not done.

LLDB.framework requires an in-tree build

This looks fine to me. I don't know much about frameworks, but I think it makes things cleaner by grouping all the framework-related code together.

It's not introduced in this patch, but the part that struck me as a hack has the force-overwriting of LLVM_CODESIGNING_IDENTITY. Is there any way that could be removed?

Thanks for taking a look!

It's not introduced in this patch, but the part that struck me as a hack has the force-overwriting of LLVM_CODESIGNING_IDENTITY. Is there any way that could be removed?

I agree, this is unfortunate. The problem is that we only have a single global setting for the identity in LLVM and we want lldb_codesign as the default for LLDB. We can only change this default if we use a cache script or pass it to CMake explicitly, but at the time we arrive in LLDBConfig it's set already. So to make it work out-of-the-box we need the force-overwrite.

Actually, I considered changing the llvm_codesign function to accept an override value for the identity and pass it through lldb_add_executable/llvm_add_executable per target. In the end the use-case didn't really seem worth the amount of change. Also, overriding a global LLVM_CODESIGNING_IDENTITY with a default value might be very confusing. I considered a LLDB-specific version as well (D54352), but with the amount of code duplication @beanz was definitely right that this is not a good way either.

It's not introduced in this patch, but the part that struck me as a hack has the force-overwriting of LLVM_CODESIGNING_IDENTITY. Is there any way that could be removed?

I agree, this is unfortunate. The problem is that we only have a single global setting for the identity in LLVM and we want lldb_codesign as the default for LLDB.

Now that I think about it again, I could at least print a warning if LLVM_CODESIGNING_IDENTITY is not empty when force-overwriting. What do you think?

labath added a comment.Fri, Dec 7, 7:28 AM

Printing the warning would definitely be nice.

Making the identity overridable via llvm_codesign arguments also sounds reasonable if we want to have the flexibility of signing things with different identities.

If we don't need that flexibility, can't we just have the users set the LLVM variable instead of the LLDB one?

Working on it..

.. if we want to have the flexibility of signing things with different identities.

Not sure if it's a good idea to sign with different identities within one build. Could become confusing?

If we don't need that flexibility, can't we just have the users set the LLVM variable instead of the LLDB one?

Well, then we always had to pass the LLVM variable explicitly in order to achieve the default behavior. For the LLDB one we can control the default value.

sgraenitz updated this revision to Diff 177229.Fri, Dec 7, 8:54 AM

Improve handling of LLDB_CODESIGN_IDENTITY vs. LLVM_CODESIGNING_IDENTITY

sgraenitz updated this revision to Diff 177231.Fri, Dec 7, 8:56 AM

Finally fix that typo

sgraenitz marked an inline comment as done.Fri, Dec 7, 9:06 AM
sgraenitz added inline comments.
cmake/modules/LLDBConfig.cmake
64

@labath Could this be a way to phase out LLDB_CODESIGN_IDENTITY and move to the LLVM one?

  • An explicitly set LLDB value takes precedence
  • Warn if a non-empty LLVM value is overwritten
  • Default to lldb_codesign if none is set explicitly
sgraenitz marked an inline comment as done.Fri, Dec 7, 9:20 AM
sgraenitz added inline comments.
cmake/modules/LLDBConfig.cmake
64

Not sure if it is too intrusive.

labath added inline comments.Fri, Dec 7, 10:40 AM
cmake/modules/LLDBConfig.cmake
64

You say "phase out", but it's not clear to me how would that happen. What would be the next step here? I don't think llvm will ever get lldb_codesign as the default value for LLVM_CODESIGN_IDENTITY.

Given that we seem to have two parts of the project with different default policies on code signing, I think the cleanest approach would be to have code signing identity overridable on a per-call basis.

You might not even need to modify add_llvm_executable for this to work. I think you might be able to just locally do set(LLVM_CODESIGN_IDENTITY ${LLDB_CODESIGN_IDENTITY}) in the scope of add_lldb_executable and the signing code should pick that up instead of the cache variable (the cmake scope rules always confuse me, but I think this how it works).

sgraenitz marked an inline comment as done and an inline comment as not done.

Move code sign changes to D55328, minor improvements

aprantl added inline comments.Mon, Dec 10, 10:22 AM
resources/LLDB-Info.plist.in
18

Just out of curiosity: Why this? Is this expected to be replaced after code signing?

sgraenitz marked 3 inline comments as done.Mon, Dec 10, 10:23 AM
sgraenitz added inline comments.
cmake/modules/LLDBConfig.cmake
64

You say "phase out", but it's not clear to me how would that happen.

Yes sorry, that was dump phrasing. Explained in more detail in D55013.

sgraenitz marked 3 inline comments as done.Mon, Dec 10, 10:28 AM
sgraenitz added inline comments.
resources/LLDB-Info.plist.in
18

Good question. Seems to be a weird representation for something like default or none. It's in all the plists.

The bundle signature in new Xcode projects is initialized as ????. You can probably leave it at that, but if you want your own code, you need to register it: http://developer.apple.com/datatype/index.html

https://stackoverflow.com/questions/1875912

aprantl added inline comments.Thu, Dec 13, 11:02 AM
cmake/modules/LLDBConfig.cmake
275

We should keep this warning, but just say that in-tree builds are not supported. There is no bot testing this and we don't want to bother with keeping this config work.

aprantl accepted this revision.Thu, Dec 13, 11:02 AM

Let's give this a try.

This revision is now accepted and ready to land.Thu, Dec 13, 11:02 AM

Please be sure to watch all LLDB bots (green dragon and lab.llvm.org:8011) closely when landing this change.

JDevlieghere accepted this revision.Thu, Dec 13, 11:33 AM

Sorry for the late review, I had a look originally but didn't have any comments. LGTM!

sgraenitz marked an inline comment as done.Tue, Dec 18, 9:33 AM

Thanks everyone for your patience with the review. I will spare us the potential merge conflicts before Christmas and land this after the holidays. Cheers