Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Strip debug symbols when copying frameworks using "carthage copy-frameworks" #2361

Merged
merged 1 commit into from Mar 5, 2018
Merged

Strip debug symbols when copying frameworks using "carthage copy-frameworks" #2361

merged 1 commit into from Mar 5, 2018

Conversation

abrindam
Copy link

@abrindam abrindam commented Mar 1, 2018

Strip debug symbols when copying frameworks using "carthage copy-frameworks" if COPY_PHASE_STRIP is enabled.

Now "carthage copy-frameworks" behaves the same as vanilla Xcode copy task as far as debug symbols go.

See PR #2334 for full discussion on why this is a good thing.

@abrindam
Copy link
Author

abrindam commented Mar 1, 2018

Well I totally forgot the tests. Dealing with that now...

tmspzz
tmspzz reviewed Mar 1, 2018
View reviewed changes
Source/CarthageKit/Xcode.swift
public func stripDebugSymbols(_ frameworkURL: URL) -> SignalProducer<(), CarthageError> {
return SignalProducer<URL, CarthageError> { () -> Result<URL, CarthageError> in binaryURL(frameworkURL) }
.flatMap(.merge) { binaryURL -> SignalProducer<TaskEvent<Data>, CarthageError> in
let stripTask = Task("/usr/bin/xcrun", arguments: [ "strip", "-S","-o", binaryURL.path, binaryURL.path])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is not -x needed?

 -S     Remove the debugging symbol table entries (those created by the -g option to cc(1) and other
              compilers).
 -x     Remove all local symbols (saving only global symbols).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to figure out what flags to pass, I reverse-engineered Xcode by replacing the strip binary on my system with one that logs all the arguments passed to it. Xcode passes this flag and only this flag when using a vanilla copy. So I figured we should be doing what Xcode does for symmetry.

I am by far not an expert on the intricacies of stripping symbols, so I'm happy to listen to alternatives however.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further info... there is a setting STRIP_STYLE which seems to affect this. Default for me was "Debugging Symbols", but "Non-Global Symbols" seems to be what brings in "-x".

The documentation for this property seems to indicate it applies only to the linked product and not to strip on copy, but I'm suspicious that this is not the case. Will investigate further.

Copy link
Member

@tmspzz tmspzz Mar 1, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe then it should respect STRIP_STYLE ? I'm not an expert either so I guess that respecting the style is safe bet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decision was made to leave -S only.

See:

@mdiep
Copy link
Member

mdiep commented Mar 1, 2018

The original intention in #2334 was to keep symbols in the .dSYM, but not the the binary within the framework, correct?

If so, can we remove the symbols from the binary during build? Since copy-frameworks is run frequently, it's best to keep it as quick as possible.

@tmspzz
Copy link
Member

tmspzz commented Mar 1, 2018 •
edited

Good point.

My bad I indicated copy-frameworks as the correct place. Sorry @abrindam :(

However @mdiep this leaves more flexibility to the end users since they can decide whether to strip or not via COPY_PHASE_STRIP. Also, I'm guessing once the frameworks are stripped this is a no-op.

@abrindam
Copy link
Author

abrindam commented Mar 1, 2018

No problem folks. I raised this very question during my long post here: #2280 (comment)

I received a single reply indicating a preference for keeping the debug symbols in the built binaries because it maximized developer flexibility (they could then choose whether or not to strip, rather than forcing stripping).

Also keep in mind this is the behavior of the default Xcode copy operation you can add in Build Phases. So if you weren't using Carthage, this would be the behavior anyways. So I'm still a fan of this because I think copy-frameworks should always be a strict superset of the vanilla copy operation (aka copy-frameworks should do more, but never less). Since Xcode is willing to do this on every build, it seems reasonable it can't be that expensive.

Any thoughts on this line of thinking?

@tmspzz
Copy link
Member

tmspzz commented Mar 1, 2018

Last word is on @mdiep :)

I'm of your same opinion @abrindam . With debug symbols in the binaries I think debugging would also be easier (or am I wrong?)

@abrindam
Copy link
Author

abrindam commented Mar 1, 2018

I think an ideal setup would be to have COPY_PHASE_STRIP off for debug and on for release. Then there is absolutely no performance penalty for the usual developer iteration.

Not sure what most people do however...

@tmspzz
Copy link
Member

tmspzz commented Mar 1, 2018 •
edited

The default for a project is set to NO for COPY_PHASE_STRIP

@mdiep
Copy link
Member

mdiep commented Mar 2, 2018

I think an ideal setup would be to have COPY_PHASE_STRIP off for debug and on for release. Then there is absolutely no performance penalty for the usual developer iteration.

I like that. 👍

@tmspzz
Copy link
Member

tmspzz commented Mar 2, 2018

@abrindam I think we're good here then. The only change that I would like to see is to respsect the STRIP_STYLE

mdiep
mdiep requested changes Mar 2, 2018
View reviewed changes
Source/CarthageKit/Xcode.swift Outdated
/// There are many suggestions on how to do this but no one single
/// accepted way. This seems to work best:
/// https://lists.apple.com/archives/unix-porting/2006/Feb/msg00021.html
public func packageContainsDebugSymbols(_ packageURL: URL) -> SignalProducer<Bool, CarthageError> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is only used in the tests. Let's move it there.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdiep Agree and my first attempt had it there - the one problem was I need to user binaryURL() which is private to Xcode.swift.

I could make binaryURL() public, or I could just copy binaryURL() into the test. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see an issue with making it public or internal

@abrindam
Copy link
Author

abrindam commented Mar 5, 2018

All good on the unit test changes, I will move the function and make the helper function public.

On the STRIP_STYLE business, bad news. I've confirmed that vanilla copy is NOT affected by STRIP_STYLE - by once again replacing the strip binary with my own which observes what arguments are passed to it.

I might have reached this conclusion faster if I noticed the "nice name" in Xcode for COPY_PHASE_STRIP is "Strip Debug Symbols During Copy" (i.e. it mentions debug symbols explicitly). D'oh!

This actually makes sense though. In my case, the default for my project for "Strip Style" was "All Symbols". This would be disastrous for a dynamic framework as global symbols are needed to use the dylib (I think/I'm pretty sure).

To further confound things, I can't find a single source that explains why you should NOT strip non-global symbols along with debug symbols. So I think optimality-wise, it would be ideal to do strip -S -x as suggested by @blender and strip both debug and non-global symbols, but this would cause the behavior of carthage copy-frameworks to diverge from the vanilla copy (which does strip -S only).

Apple strikes again! Thoughts?

@tmspzz
Copy link
Member

tmspzz commented Mar 5, 2018 •
edited

Let's stick with the minimal subset then which is -S thus respecting COPY_PHASE_STRIP friendly name "Strip Debug Symbols During Copy" . I'm wondering then what STRIP_STYLE applies too.

I leave this here for further/future references: https://www.objc.io/issues/6-build-tools/mach-o-executables/

@abrindam
Copy link
Author

abrindam commented Mar 5, 2018

Ah, should have been clearer about that. STRIP_STYLE only applies to the actual target executable of the project you're building. In more formal terms, it affects the strip style of the stripping controlled by STRIP_INSTALLED_PRODUCT.

So when I log out the calls to strip across a build, I get something like this:

strip -S /Development/MyApp/Carthage/Build/Mac/FrameworkA.framework/Versions/A/FrameworkA -o /Development/MyApp/DerivedData/MyApp/Build/Products/Debug/MyApp.app/Contents/Frameworks/FrameworkA.framework/Versions/A/FrameworkA
strip -S /Development/MyApp/Carthage/Build/Mac/FrameworkB.framework/Versions/A/FrameworkB -o /Development/MyApp/DerivedData/MyApp/Build/Products/Debug/MyApp.app/Contents/Frameworks/FrameworkB.framework/Versions/A/FrameworkB
strip -S /Development/MyApp/ThirdParty/FrameworkC.framework/Versions/A/FrameworkC -o /Development/MyApp/DerivedData/MyApp/Build/Products/Debug/MyApp.app/Contents/Frameworks/FrameworkC.framework/Versions/A/FrameworkC
strip /Development/MyApp/DerivedData/MyApp/Build/Products/Debug/MyApp.app/Contents/MacOS/MyApp

This is with STRIP_STYLE to be "All Symbols". As you can see, all the Carthage frameworks still got "-S" while the final binary got no flag (which apparently equivalent to "-u -r").

Strip debug symbols when copying frameworks using "carthage copy-fram…
0492ec6 
…eworks" if COPY_PHASE_STRIP is enabled.

Now "carthage copy-frameworks" behaves the same as vanilla Xcode copy task as far as debug symbols go.
@abrindam
Copy link
Author

abrindam commented Mar 5, 2018

OK, I've made the changes to the unit tests, and now that's we've had that entertaining diversion on strip style, assuming the CI gods don't curse me, I think we should be good to go here.

@mdiep mdiep merged commit 8e6b603 into Carthage:master Mar 5, 2018
@mdiep
Copy link
Member

mdiep commented Mar 5, 2018

Thanks @abrindam!

@rudedogg rudedogg mentioned this pull request Mar 11, 2018
Closed
@jdhealy jdhealy mentioned this pull request Jun 13, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdiep mdiep mdiep approved these changes

@tmspzz tmspzz tmspzz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@abrindam @mdiep @tmspzz