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.

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

Fix lldb when using experimental_mixed_language_library #1911

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brentleyjones
Copy link
Collaborator

@brentleyjones brentleyjones commented Mar 17, 2023

This applies a similar fix that MobileNativeFoundation/rules_xcodeproj@d40173d did, for the same reason.

@brentleyjones
Copy link
Collaborator Author

I probably have to make a couple more tweaks, but the key thing is that only the objc custom modulemap ends up downstream -Xcc flags. I'll add a test around this behavior as well.

@brentleyjones
Copy link
Collaborator Author

@thii Do you know of an example where this breaks?

@brentleyjones
Copy link
Collaborator Author

I think I'll also try to have a single umbrella modulemap created instead (one that doesn't depend on the swift_library, but still has the generated header entry), as that would solve this problem as well: https://github.com/buildbuddy-io/rules_xcodeproj/blob/main/docs/faq.md#why-do-some-of-my-swift_librarys-compile-twice-in-bwx-mode

@thii
Copy link
Member

thii commented Mar 17, 2023

I think umbrella_module_map is the correct one, otherwise an objc_library won't be able to see the interface of the underlying Swift module. (I forgot to add an example to check it.)

On why lldb breaks, I think it's because the generated Swift header always have this kind of import statement:

#import <MyMixedModule/MyMixedModule.h>

and if you don't have codebase structure that makes this work, or proper -I flags, or header maps, then lldb won't be able to find MyMixedModule.h.

Edit: Obj-C umbrella header import statement in Swift generated header, not Swift generated header import statement.

@brentleyjones
Copy link
Collaborator Author

I'll try the single module map method instead then Monday.

@thii
Copy link
Member

thii commented Mar 17, 2023

Edit: It was the Obj-C umbrella header import statement in the Swift generated header, not the Swift generated header import statement within itself.

Updated the above comment as well.

@brentleyjones
Copy link
Collaborator Author

The lldb issue is because later swift_library's get the umbrella modulemap in -fmodule-map-file=, while the inner one gets the non-umbrella version, and lldb collects all -Xcc flags down the dependency tree, leading to modulemap redefinition errors.

I think the proper fix is that we need two module maps to get propagated:

  • MODULE.modulemap, with just the ObjC stuff
  • MODULE.swift.modulemap, with just the MODULE.swift Swift sutff

And that way both are seen and depended on down stream.

I don't get how modulemaps are used with Bazel compilation though, as I couldn't get a dependent objc_library to see the modulemap created by this macro. So I think someone else will have to take on this fix.

@thii
Copy link
Member

thii commented Mar 24, 2023

IIRC it was done like that so that we didn't have to change our import statements during the migration to Bazel. For example @import MixedModule; would also import the Swift interface of the mixed module. But if you only care about building with Bazel then this change makes sense.

Now thinking about it again, I guess this might have been the reason of many of our lldb issues in the past.

@brentleyjones
Copy link
Collaborator Author

Does this change work for you too @chiragramani?

@brentleyjones brentleyjones changed the title Fix lldb when using experimental_mixed_language_library. Fix lldb when using experimental_mixed_language_library Mar 24, 2023
@chiragramani
Copy link
Contributor

Does this change work for you too @chiragramani?

We have a slightly different implementation, but I think the root cause you highlighted would also fix our case. I will check and let you know. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants