[Stdlib] Change SWIFT_RUNTIME_STDLIB_INTERNAL to not export the symbol. #19614

mikeash · 2018-09-28T21:00:47Z

The functions in LibcShims are used externally, some directly and some through @inlineable functions. These are changed to SWIFT_RUNTIME_STDLIB_SPI to better match their actual usage. Their names are also changed to add "_swift" to the front to match our naming conventions.

Three functions from SwiftObject.mm are changed to SPI and get a _swift prefix.

A few other support functions are also changed to SPI. They already had a prefix and look like they were meant to be SPI anyway. It was just hard to notice any mixup when they were #defined to the same thing.

rdar://problem/35863717

mikeash · 2018-09-28T21:00:58Z

@swift-ci please test

gparker42 · 2018-09-28T21:07:47Z

stdlib/public/Platform/Platform.swift

 }

 public func fcntl(
  _ fd: Int32,
  _ cmd: Int32,
  _ ptr: UnsafeMutableRawPointer
 ) -> Int32 {
-  return _stdlib_fcntlPtr(fd, cmd, ptr)
+  return _swift_swift_stdlib_fcntlPtr(fd, cmd, ptr)


This and a few other functions use the prefix _swift_swift_stdlib_. They should be _swift_stdlib_.

What the heck? The perils of trying to get something for people to look at before closing time on Friday, I guess.

gparker42 · 2018-09-28T21:20:04Z

Where do the libc functions get used externally? The entire set of libc shims exists only because libswiftCore can't import the libc's module directly because it would create a circular dependency. Other modules should use libc.

jckarter · 2018-09-28T21:28:09Z

It could be that we end up emitting them because @inlinable functions contain references to them. We never want the shim functions themselves to be emitted as ABI entry points, though. Can we mark them always_inline in the header? cc @jrose-apple

jckarter · 2018-09-28T21:29:03Z

Or, if always_inline is inappropriate, could we give them the standard C symbol name with an asm() attribute? Would that still cause problems with transitive imports?

jrose-apple · 2018-09-28T21:50:35Z

We can't make them always_inline because that implies having an inlinable body in the header, which would defeat the purpose.

asm is a little weirder: it looks like we translate asm to _silgen_name, and I don't trust that we'd get everything correct if a program has two functions with the same _silgen_name. If we held off on lowering from the C name to the linkage name until IRGen, instead of in SIL, I'd feel comfortable with it, but as is using asm seems like a bad idea.

swift-ci · 2018-09-28T22:12:04Z

Build failed
Swift Test Linux Platform
Git Sha - 89ac2564a787a61a9f986afafd305fa372ea9bf4

compnerd · 2018-10-01T16:51:05Z

@jckarter - please do remember to use __USER_LABEL_PREFIX__ rather than hardcoding it if you go down the asm label route.

mikeash · 2018-10-01T17:10:28Z

Many of the shims are dumb wrappers to avoid the circular dependency, but many wrap different underlying OS APIs in one unified function for other stuff to use. For example, malloc_size and random have different implementations for different targets.

We have a few cross-dylib references within the Swift libraries themselves. libswiftSwiftPrivate.dylib uses these:

__swift_stdlib_close
__swift_stdlib_read
__swift_stdlib_write

And libswiftDarwin.dylib uses these:

__swift_stdlib_fcntl
__swift_stdlib_getEnviron
__swift_stdlib_getErrno
__swift_stdlib_ioctl
__swift_stdlib_open
__swift_stdlib_setErrno
__swift_swift_stdlib_fcntlPtr
__swift_swift_stdlib_ioctlPtr
__swift_swift_stdlib_openat

A few of these functions are being exposed through @inlineable calls as well.

_stdlib_malloc_size is used in _ContiguousArrayBuffer.init(_uninitializedCount:minimumCapacity:), ManagedBuffer.capacityInBytes, and ManagedBuffer.capacity.

_stdlib_strlen is used in KeyPath._appendingKeyPaths.

_stdlib_random is used in SystemRandomNumberGenerator.

jckarter · 2018-10-01T17:28:38Z

The ones with nontrivial or nonportable logic in them probably shouldn't be in "shims"; that seems misleading. We probably need those entry points in the Darwin library to deal with variadic C interfaces, but since Darwin is not the standard library, do the circular dependency issues still matter there? Can we write the Darwin stubs as static inline wrappers around the standard C/POSIX entry points?

jrose-apple · 2018-10-01T17:31:08Z

I think the Darwin ones are holdovers, yes. They can probably be simple wrappers like the other overlay shims.

mikeash · 2018-10-01T18:41:44Z

random is the major outlier here, as it has a not-quite-trivial implementation for Windows, and a pretty complex implementation for non-Apple, non-Windows targets. That should probably move to its own file to reduce confusion and keep the "shims" simple. I'll do that and see if I can get rid of the cross-library references from libswiftDarwin.dylib.

mikeash · 2018-10-01T21:04:20Z

Coming along. I put random in its own file and put all the ones referenced by libswiftDarwin.dylib and libswiftSwiftPrivate.dylib into a separate header where they live as static inline functions. Now I just need to get malloc_size and strlen taken care of.

mikeash · 2018-10-01T21:05:35Z

@compnerd Can you check over the Windows side of my changes? I tried my best to keep it working but it's very likely I didn't get everything right.

jrose-apple · 2018-10-01T21:27:59Z

stdlib/public/SwiftShims/module.modulemap

@@ -19,6 +20,11 @@ module SwiftShims {
  export *
 }

+module SwiftShimsInline {


Maybe this should be "_SwiftPlatformOverlayShims", to follow the convention below?

Azoy · 2018-10-02T13:35:39Z

Shouldn’t some of these be marked API? AFAIK functions like _swift_stdlib_random aren’t used in overlay code, but rather @inlinable code like the description states. I know SPI and API both export, but it seems worthwhile marking them API if they are and leading with swift_ prefix.

mikeash · 2018-10-02T14:05:56Z

random does seem to fit the definition of API from Visibility.h.

mikeash · 2018-10-02T21:24:21Z

Updated with the latest. Didn't get to random yet. This moves a bunch of stuff around and makes the remaining functions in LibcShims.cpp INTERNAL. A bunch of others moved to static inline functions in the header. As many functions as possible are now in LibcOverlayShims.h which includes the relevant headers and calls the underlying functions without redeclaring them or messing around with custom typedefs.

A few tests had to be fixed up to build the Darwin mock overlay. They were getting away without doing that before, but this change exposed the problem.

The output for gen_swift_source.swift.response changed. The import Darwin line no longer produces key.is_system: 1. I'm not entirely sure why, and don't know if this is a correct change to the test, or an indication that something has gone wrong.

The functions in LibcShims are used externally, some directly and some through @inlineable functions. These are changed to SWIFT_RUNTIME_STDLIB_SPI to better match their actual usage. Their names are also changed to add "_swift" to the front to match our naming conventions. Three functions from SwiftObject.mm are changed to SPI and get a _swift prefix. A few other support functions are also changed to SPI. They already had a prefix and look like they were meant to be SPI anyway. It was just hard to notice any mixup when they were #defined to the same thing. rdar://problem/35863717

… and make them static inline. This avoids exposing them as exported symbols in libswiftCore.

…ine. Move LibcShimsInline.h to LibcOverlayShims.h for more consistent naming. Fix up several tests that needed the mock Darwin overlay built. Fix one SourceKit test that no longer produces is_system: 1 on an `import Darwin` line.

mikeash · 2018-10-03T14:17:05Z

Changed random to be API and rebased on top of the latest master. I think this is ready to go, unless there's more feedback. I do want to get this right, so that is welcome. 😄

mikeash · 2018-10-03T14:17:12Z

@swift-ci please test

swift-ci · 2018-10-03T14:19:18Z

Build failed
Swift Test OS X Platform
Git Sha - 89ac2564a787a61a9f986afafd305fa372ea9bf4

swift-ci · 2018-10-03T14:19:18Z

Build failed
Swift Test Linux Platform
Git Sha - 89ac2564a787a61a9f986afafd305fa372ea9bf4

…Random.h/cpp can use it.

…ings. Fix Linux implementation in Random.cpp to actually compile.

mikeash · 2018-10-03T18:43:27Z

@swift-ci please test

swift-ci · 2018-10-03T18:45:22Z

Build failed
Swift Test Linux Platform
Git Sha - c4d5db4

swift-ci · 2018-10-03T18:47:04Z

Build failed
Swift Test OS X Platform
Git Sha - c4d5db4

mikeash · 2018-10-04T12:50:40Z

@swift-ci please test

swift-ci · 2018-10-04T12:51:11Z

Build failed
Swift Test OS X Platform
Git Sha - 1e9741a

swift-ci · 2018-10-04T12:52:41Z

Build failed
Swift Test Linux Platform
Git Sha - 1e9741a

…ndation.

mikeash · 2018-10-04T17:21:35Z

@swift-ci please test

swift-ci · 2018-10-04T17:24:16Z

Build failed
Swift Test Linux Platform
Git Sha - 3f728fe6196c5ee37414ae4bed25627151c8aa27

swift-ci · 2018-10-04T17:25:19Z

Build failed
Swift Test OS X Platform
Git Sha - 3f728fe6196c5ee37414ae4bed25627151c8aa27

…iveC non-inlinable as it calls an internal function.

mikeash · 2018-10-04T20:29:17Z

@swift-ci please test

swift-ci · 2018-10-04T20:31:26Z

Build failed
Swift Test Linux Platform
Git Sha - a78a01a

swift-ci · 2018-10-04T20:33:05Z

Build failed
Swift Test OS X Platform
Git Sha - a78a01a

The changes in swiftlang#19614 require Darwin/Glibc to build first. This usually happened anyway (and thus the problem wasn't noticed for a while) but sometimes SwiftPrivate would win the race and the build would fail. rdar://problem/45624328

mikeash requested a review from gparker42 September 28, 2018 21:01

gparker42 reviewed Sep 28, 2018

View reviewed changes

mikeash force-pushed the no-internal-export branch from 89ac256 to bef6d96 Compare October 1, 2018 17:20

jrose-apple reviewed Oct 1, 2018

View reviewed changes

mikeash added 6 commits October 3, 2018 09:55

[Stdlib] Pull _swift_stdlib_random into a separate file.

6926fbd

[Stdlib] Move the shims used by Platform.swift into a separate header…

b267528

… and make them static inline. This avoids exposing them as exported symbols in libswiftCore.

[Stdlib] Move _swift_stdlib_read/write/close into LibcShimsInline.h.

ec25f92

[Stdlib] Change _swift_stdlib_random SPI to swift_stdlib_random API.

c4d5db4

mikeash force-pushed the no-internal-export branch from 5ccff97 to c4d5db4 Compare October 3, 2018 14:15

mikeash added 2 commits October 3, 2018 10:48

[Stdlib] Move the declaration of __swift_ssize_t to SwiftStddef.h so …

1e8c35d

…Random.h/cpp can use it.

[Stdlib] Clean up includes in LibcShims and Random.

5391abb

mikeash force-pushed the no-internal-export branch from c467bb8 to 5391abb Compare October 3, 2018 15:16

mikeash added 2 commits October 3, 2018 11:34

[Stdlib] Delete a stray ß in LibcShims.h

4113697

[Stdlib] Fix Linux detection for malloc_size. Silence const cast warn…

1e9741a

…ings. Fix Linux implementation in Random.cpp to actually compile.

[Stdlib] Make _swift_extractDynamicValue INTERNAL as it's used by Fou…

a78a01a

…ndation.

mikeash force-pushed the no-internal-export branch from 3f728fe to a78a01a Compare October 4, 2018 17:21

[Stdlib] Make the !_runtime(_ObjC) version of _bridgeAnythingToObject…

b05c9e9

…iveC non-inlinable as it calls an internal function.

mikeash merged commit f4db1dd into swiftlang:master Oct 5, 2018

mikeash mentioned this pull request Oct 31, 2018

[Stdlib] Add Darwin/Glibc as a dependency for SwiftPrivate. #20194

Merged

jrose-apple mentioned this pull request Sep 9, 2019

Minor simplification to FixedWidthInteger._random(using: &generator). #27056

Closed

[Stdlib] Change SWIFT_RUNTIME_STDLIB_INTERNAL to not export the symbol. #19614

[Stdlib] Change SWIFT_RUNTIME_STDLIB_INTERNAL to not export the symbol. #19614

Uh oh!

Conversation

mikeash commented Sep 28, 2018

Uh oh!

mikeash commented Sep 28, 2018

Uh oh!

gparker42 Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

mikeash Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

gparker42 commented Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jckarter commented Sep 28, 2018

Uh oh!

jrose-apple commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Sep 28, 2018

Uh oh!

compnerd commented Oct 1, 2018

Uh oh!

mikeash commented Oct 1, 2018

Uh oh!

jckarter commented Oct 1, 2018

Uh oh!

jrose-apple commented Oct 1, 2018

Uh oh!

mikeash commented Oct 1, 2018

Uh oh!

mikeash commented Oct 1, 2018

Uh oh!

mikeash commented Oct 1, 2018

Uh oh!

jrose-apple Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

Azoy commented Oct 2, 2018

Uh oh!

mikeash commented Oct 2, 2018

Uh oh!

mikeash commented Oct 2, 2018

Uh oh!

mikeash commented Oct 3, 2018

Uh oh!

mikeash commented Oct 3, 2018

Uh oh!

swift-ci commented Oct 3, 2018

Uh oh!

swift-ci commented Oct 3, 2018

Uh oh!

mikeash commented Oct 3, 2018

Uh oh!

swift-ci commented Oct 3, 2018

Uh oh!

swift-ci commented Oct 3, 2018

Uh oh!

mikeash commented Oct 4, 2018

Uh oh!

swift-ci commented Oct 4, 2018

Uh oh!

swift-ci commented Oct 4, 2018

Uh oh!

mikeash commented Oct 4, 2018

Uh oh!

swift-ci commented Oct 4, 2018

Uh oh!

swift-ci commented Oct 4, 2018

Uh oh!

mikeash commented Oct 4, 2018

Uh oh!

swift-ci commented Oct 4, 2018

Uh oh!

swift-ci commented Oct 4, 2018

Uh oh!

jrose-apple commented Sep 28, 2018 •

edited

Loading