Skip to content

[ModuleTrace] Track direct dependencies through #import and @_exported import #32738

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

Merged

Conversation

varungandhi-apple
Copy link
Contributor

@varungandhi-apple varungandhi-apple commented Jul 7, 2020

Still missing:

  • Test cases.
    • Test case(s) for SPI (maybe SPI change should be a separate PR?)
    • Test cases for overlay-overlay dependency hazards. (Current status: works on my machine)
    • Test cases for import chains. I really wish unit-testing this part of the compiler were easier lolsob. (Current status: I think it should work?)
  • Some need to break up last commit into smaller commits. Seems difficult, a lot of things depend on each other.
  • It's not clear that the depth-first traversal can be refactored out but maybe it's possible. This is fine. We can refactor later. There's also a bunch of special cases, don't want to block landing the fix on an elegant design.
  • It might be possible to factor out some of the logic in getABIDependenciesForSwiftModule. Seems ok.

Just opening the PR to get initial feedback on whether the approach looks right.

@varungandhi-apple
Copy link
Contributor Author

varungandhi-apple commented Jul 9, 2020

SPI PR separated out: #32771 (please ignore the first two commits here)

Copy link
Contributor

@beccadax beccadax left a comment

Choose a reason for hiding this comment

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

Sorry—I wrote a bunch of review comments a couple days ago, but it looks like I forgot to post them. You’ve made changes since, so I’m not sure how many are still applicable.

@varungandhi-apple varungandhi-apple marked this pull request as ready for review July 17, 2020 08:04
@varungandhi-apple
Copy link
Contributor Author

@swift-ci please smoke test

@varungandhi-apple varungandhi-apple changed the title WIP: Fix behavior of "isImportedDirectly" field [ModuleTrace] Track direct dependencies through ObjC #import and @_exported import Jul 17, 2020
@varungandhi-apple varungandhi-apple changed the title [ModuleTrace] Track direct dependencies through ObjC #import and @_exported import [ModuleTrace] Track direct dependencies through #import and @_exported import Jul 17, 2020
@shahmishal
Copy link
Member

@swift-ci please smoke test

@varungandhi-apple
Copy link
Contributor Author

Interesting! We found a slightly different cycle on Windows:

<unknown>:0: note: error: invariant violation: unexpected cycle in import graph!

loaded_module_trace_append (Swift) @ 0x222246b2db0

depends on Swift (Swift) @ 0x22222f02708

depends on SwiftShims (Clang) @ 0x222246b2150

depends on visualc (Clang) @ 0x222246b26a0

depends on Swift (Swift) @ 0x22222f02708

What is going on here? 🤔🤔🤔

@varungandhi-apple
Copy link
Contributor Author

  • Hamish pointed out the cycles involved in the stdlib may have to do with implicit private imports of the stdlib by Clang modules. 70f8c19
  • Robert pointed out that the Darwin (Swift) -> SwiftOverlayShims (Clang) -> Darwin (Swift) dependency is likely due to SwiftOverlayShims (Clang) depending on Darwin (Clang) but us automatically using the Swift module.

Trying to think about how we want to handle this properly...

@varungandhi-apple

This comment has been minimized.

@varungandhi-apple
Copy link
Contributor Author

Mostly just some cleanup/reorganization and more comments explaining why the different checks are necessary, removing a single one makes one or more of the tests fail. 😅

@varungandhi-apple

This comment has been minimized.

@varungandhi-apple

This comment has been minimized.

@varungandhi-apple varungandhi-apple changed the title [ModuleTrace] Track direct dependencies through #import and @_exported import [DNM] [ModuleTrace] Track direct dependencies through #import and @_exported import Jul 18, 2020
@varungandhi-apple
Copy link
Contributor Author

Current status is DNM because there is an issue building llbuildSwift, noticed in #32980. Looking into it.

@varungandhi-apple varungandhi-apple changed the title [DNM] [ModuleTrace] Track direct dependencies through #import and @_exported import [ModuleTrace] Track direct dependencies through #import and @_exported import Jul 21, 2020
@varungandhi-apple

This comment has been minimized.

@varungandhi-apple
Copy link
Contributor Author

varungandhi-apple commented Jul 21, 2020

So one of the issues was with iterator invalidation. I'm still not entirely sure why that was happening (I would've expected a temporary to be created which lasts the duration of the loop) but creating a separate copy of the set seems to fix the issue. From what I can tell, the set itself was not being modified, because we do check for module equality and those checks were passing.

@varungandhi-apple

This comment has been minimized.

@varungandhi-apple
Copy link
Contributor Author

Source compat suite was passing with copying code in #32980, just re-running it with latest changes without the copy. I did check locally that llbuildSwift and SwiftPM built fine.

@varungandhi-apple

This comment has been minimized.

2 similar comments
@varungandhi-apple

This comment has been minimized.

@varungandhi-apple

This comment has been minimized.

@varungandhi-apple

This comment has been minimized.

Overlay-overlay dependencies are incorrectly marked indirect when the
downstream overlay's underlying module imports the upstream overlay's
underlying module but the downstream overlay does not explicitly import
the upstream overlay.
@varungandhi-apple
Copy link
Contributor Author

Fixed #import -> #include to make Windows less sad.

@varungandhi-apple

This comment has been minimized.

We need to traverse the module dependency graph and track which modules expose
which other modules' ABIs, while making sure that we don't hit a loop while
trawling through Clang (sub)modules.

Fixes rdar://64993153.
@varungandhi-apple
Copy link
Contributor Author

Fixed #include -> #import to make macOS less sad.

@varungandhi-apple
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@beccadax beccadax left a comment

Choose a reason for hiding this comment

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

It looks like you may have some unfinished business at a few points. Otherwise, I think this looks okay.

@varungandhi-apple
Copy link
Contributor Author

Going to merge this, since fixing rdar://64993153 is kinda' pressing. Will submit follow-up PRs for the TODOs.

@varungandhi-apple varungandhi-apple merged commit 497e049 into swiftlang:master Jul 24, 2020
@varungandhi-apple varungandhi-apple deleted the vg-fix-indirect-dep branch July 24, 2020 21:15
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.

4 participants