Skip to content

Remove _silgen_name from all Apple overlays except Dispatch and Foundation #5309

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
merged 10 commits into from
Oct 19, 2016

Conversation

jrose-apple
Copy link
Contributor

@jrose-apple jrose-apple commented Oct 15, 2016

Steps towards getting rid of _silgen_name altogether, which is generally unsafe and not something we're interested in supporting in its current form. I left out Dispatch and Foundation for now because those have code owners active in the Swift project. The "platform" overlay (Darwin/Glibc) has also been exempted for now because I didn't want to poke at any low-level import dependencies.

There are a few downsides to all this: these headers get installed along with the shims even though they're only used to build the overlays, and only on Apple platforms. I or someone else should go back later and

  1. Split these new shim headers into their own folders with their own module maps.
  2. Add separate CMake targets for each one, for better dependencies.
  3. Don't install the headers on irrelevant platforms.

Overall, though, I think this is a safety and simplicity win.

This does require a dummy protocol for now; hopefully we can take it out
later.
Loading a Clang module eagerly brings in overlays for anything it re-exports,
but this is a problem for these new "shim header" modules, which generally
import the underlying module for an overlay and are in turn imported by the
overlay. That means that when we try to import an overlay, we'll end up with
a circular reference before it's done loading all its dependencies. Break the
cycle by not exporting anything from these modules, which are mostly just an
implementation detail anyway.
These get imported by Swift, so "Objective-C" would be more appropriate.
For now, though, just let it fall back to C mode.
@jrose-apple
Copy link
Contributor Author

@moiseev, @slavapestov, can you review these changes?

@swift-ci Please test macOS platform

@swift-ci Please smoke test Linux platform

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test macOS platform

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - dce51ed
Test requested by - @jrose-apple

@modocache
Copy link
Contributor

The DebugInfo/bbentry-location.swift failure has been blocking me all day. :(

@CodaFi
Copy link
Contributor

CodaFi commented Oct 17, 2016

Let's see if #5325 got it, then.

@swift-ci please test OS X platform.

@jckarter
Copy link
Contributor

Thanks for trying to clean this up! Maybe we should also hide _silgen_name behind a flag so that it's harder for people to try to add more uses of it outside of the stdlib (and maybe Foundation, which is pretty tied up in the runtime too).

@jrose-apple
Copy link
Contributor Author

That's my ultimate goal (and I want the flag to be -parse-stdlib, ideally), but we've still got a bit more to do before that. (In particular, IIRC it's used by LLDB and playgrounds to inject symbols or make identifiable SIL functions.)

@jckarter
Copy link
Contributor

Cool. LLDB and Playgrounds shouldn't be too much of a problem; the former already has its own special frontend mode, and the Playground logger directly manipulates ASTs, so doesn't need to be subjected to parser-level restrictions.

@jrose-apple
Copy link
Contributor Author

Max talked to me in person a few days ago, and Slava left behind an emoji, so I'm going to assume this is okay to merge and possibly undergo refinement later. Retesting just to be safe.

@swift-ci Please test macOS

@jrose-apple jrose-apple merged commit 222811a into swiftlang:master Oct 19, 2016
@jrose-apple jrose-apple deleted the overlays-and-silgen_name branch October 19, 2016 16:21
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.

5 participants