[stdlib] Avoid malloc_size on OpenBSD. #30560

3405691582 · 2020-03-22T05:00:12Z

malloc introspection is a platform feature that is unavailable on
OpenBSD. There is no workaround for the feature, so simply avoid calling
it in this case. It is assumed on OpenBSD that the tail allocator will
allocate the amount of memory requested of it, and nothing more.

3405691582 · 2020-03-22T05:07:32Z

cc @milseman who was working in this area recently

CodaFi

Could you please add a comment here explaining what you wrote in the PR's summary?

milseman · 2020-03-27T22:46:16Z

Array also calls malloc_size, and has for years. String just recently added this. I'm curious, how did this manifest for you? Is there an alternative we can do in the shim instead?

milseman · 2020-03-27T22:51:10Z

CC @Catfish-Man @jckarter

3405691582 · 2020-03-27T23:39:53Z

Could you please add a comment here explaining what you wrote in the PR's summary?

Done. I tweaked the language in the code comment to be a little more generic; who knows if there might be a porting effort by someone to another platform without malloc introspection.

Array also calls malloc_size, and has for years. String just recently added this. I'm curious, how did this manifest for you? Is there an alternative we can do in the shim instead?

There are three uses of malloc_size in Swift that are problematic:

ContiguousArrayBuffer.swift -- this is easy to fix, but this looks like it might be partially addressed in pr Adopt the new allocation/growth strategy in Array #30270, so it might just be easiest to wait for that.
ManagedBuffer.swift -- this one is more complicated to fix, because the allocation and the introspection call contexts are separated far apart. I have an unpublished commit to kludge around this, but it's a bit of a hack.
StringStorage.swift -- this pr.

I'm working on an OpenBSD port recently. It doesn't have malloc introspection, and introspection like this is something we can't kludge around, because knowing the allocation extents depends on introspecting the system allocator, so the only way around this is to track the size of the allocation in parallel with every allocation.

jckarter · 2020-03-30T17:20:30Z

It's going to be easy to forget about platforms like OpenBSD as we work on the standard library and refactor code, which might add new malloc_size checks in the future. It might be better for long term maintenance if we wrapped up all of the standard library's uses of malloc_size in a helper function that handles the case that it might be unavailable. We could add a _stdlib_has_malloc_size static inline function to the shims, and implement the helper like this:

internal func mallocSize(ptr: UnsafeRawPointer) -> UInt {
  if _stdlib_has_malloc_size() {
    return _stdlib_malloc_size(ptr)
  }
  return nil
}

_stdlib_malloc_size could then be stubbed out to abort on OSes that don't have it as well.
That way, the conditional only has to be in one place for any OSes that don't support malloc introspection.

3405691582 · 2020-03-30T17:27:05Z

I'm happy to try and make that refactor here, that sounds simple and straightforward enough in itself, but I still don't know how to apply it for ManagedBufferPointer. That doesn't mean we can't at least apply that scheme where it works for the moment.

jckarter · 2020-03-30T17:37:33Z

@Catfish-Man or @milseman would be more familiar with the specific code at hand, but in general, you ought to be able to replace existing direct calls to _stdlib_malloc_size(x) with mallocSize(x) ?? originalMallocArgument, since we should be able to assume we got at least as much memory as we'd asked for originally.

3405691582 · 2020-03-30T17:43:01Z

Sure, but the problem with ManagedBufferPointer is that the malloc_size call is divorced from the allocation (see _capacityInBytes), and ManagedBufferPointer asserts on "illegal stored properties".

Happy to hear further advice on this, though.

3405691582 · 2020-03-31T03:20:34Z

Adopted @jckarter's suggestion to introduce a new shim. Still would like to hear if @Catfish-Man or @milseman have advice.

Points of note:

as mentioned, ManagedBuffer.swift is not converted in this PR
mallocSize shim returns Optional<Int> and not Optional<UInt>, because the operators on pointers take Ints; the preexisting code also makes some assumptions based on _swift_stdlib_malloc_size returning an Int.
the catchall implementation of _swift_stdlib_malloc_size returns 0 instead of asserting; it might make things a little too complex to try and drag in the necessary dependencies to get runtime assertions.

compnerd · 2020-04-03T15:29:27Z

Thanks for this, I think that the shim'ed approach is definitely nicer.

compnerd · 2020-04-03T15:29:33Z

@swift-ci please test

swift-ci · 2020-04-03T15:32:13Z

Build failed
Swift Test Linux Platform
Git Sha - 3886ca245bb8bc5e82b352ed6383963229588347

swift-ci · 2020-04-03T15:32:39Z

Build failed
Swift Test OS X Platform
Git Sha - 3886ca245bb8bc5e82b352ed6383963229588347

compnerd · 2020-04-08T16:25:25Z

@swift-ci please test macOS platform

swift-ci · 2020-04-08T18:15:52Z

Build failed
Swift Test OS X Platform
Git Sha - f594ddb4439c73d53df83918ab95d37c4a463fdd

3405691582 · 2020-04-08T18:29:48Z

Looks like the OS X build failure is because swiftpm is being built with a platform Swift SDK for SDKROOT but with the Swift stdlib from HEAD + this PR, is that right? Is there something this PR is missing?

CodaFi · 2020-04-13T18:31:49Z

Lost the build failure

@swift-ci test macOS platform

swift-ci · 2020-04-13T20:34:08Z

Build failed
Swift Test OS X Platform
Git Sha - f594ddb4439c73d53df83918ab95d37c4a463fdd

benrimmington

Suggestions:

use __swift_bool if possible?

and update the copyright year in all files.

-// Copyright (c) 2014 - 20** Apple Inc. and the Swift project authors
+// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors

stdlib/public/SwiftShims/LibcShims.h

stdlib/public/core/Shims.swift

benrimmington · 2020-04-15T06:21:47Z

stdlib/public/SwiftShims/LibcShims.h

+#define HAS_MALLOC_SIZE 1
 static inline __swift_size_t _swift_stdlib_malloc_size(const void *ptr) {
 #if defined(__ANDROID__)
+#define HAS_MALLOC_SIZE 1


Does this #define HAS_MALLOC_SIZE twice for Android?

Also, should HAS_MALLOC_SIZE be renamed to _SWIFT_STDLIB_HAS_MALLOC_SIZE?

Good catch. Removed the Android #define.

I'm not sure a rename is necessary; giving it a full prefix suggests this is something with stronger scope than it actually has. The underscore prefix seems rather uncommon for stdlib as well, so I'm inclined to leave it as-is.

Suggestion: replace each #define HAS_MALLOC_SIZE with a function?

-#define HAS_MALLOC_SIZE 1 +static inline __swift_bool _swift_stdlib_has_malloc_size() { return true; }

-#define HAS_MALLOC_SIZE 0 +static inline __swift_bool _swift_stdlib_has_malloc_size() { return false; }

It's a lot more to copy and paste for the next new port, though.This suggests it would be far better to push the preprocessor conditionals into one definition of each function at the top level, but that would probably be better suited to do in a separate PR.

jckarter · 2020-04-15T22:39:59Z

@swift-ci Please test

swift-ci · 2020-04-15T22:42:31Z

Build failed
Swift Test OS X Platform
Git Sha - f594ddb4439c73d53df83918ab95d37c4a463fdd

swift-ci · 2020-04-15T22:42:39Z

Build failed
Swift Test Linux Platform
Git Sha - f594ddb4439c73d53df83918ab95d37c4a463fdd

compnerd · 2020-04-20T16:25:47Z

@swift-ci please test

swift-ci · 2020-04-20T17:37:20Z

Build failed
Swift Test OS X Platform
Git Sha - 03ffde0cecb3c576c500a9de4242ec8e1b3d222b

compnerd · 2020-04-26T18:01:07Z

@swift-ci please test macOS platform

swift-ci · 2020-04-26T19:45:27Z

Build failed
Swift Test OS X Platform
Git Sha - 03ffde0cecb3c576c500a9de4242ec8e1b3d222b

benrimmington · 2020-04-26T20:50:21Z

The macOS failure was:

dyld: Symbol not found: _$ss11_mallocSize12ofAllocationSiSgSV_tF
  Referenced from: /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/swift-test
  Expected in: /usr/lib/swift/libswiftCore.dylib

Does this mean that the system dylib is expected to contain the new _mallocSize(ofAllocation:) function, when building SwiftPM?

compnerd · 2020-04-27T14:46:31Z

Yes, the tests currently run against the system version of the core library. This is due to a system issue that’s been there for a while. Fixing that is blocked by the other build system work that exposes the issue in an addressable manner.

CC: @drexin

jckarter · 2020-04-27T17:36:22Z

Running tests against the OS runtime seems like a feature, not a bug, because we want to know if we accidentally add symbols to the standard library, since they'll be burdened by deployment target limitations because they aren't available in older OSes.

In this case, we need to mark that new function with the @_alwaysEmitIntoClient attribute, so that it's not part of the standard library ABI.

jckarter · 2020-04-27T17:37:55Z

stdlib/public/core/Shims.swift

@@ -36,3 +36,8 @@ internal var _fastEnumerationStorageMutationsTarget: CUnsignedLong = 0
 internal let _fastEnumerationStorageMutationsPtr =
  UnsafeMutablePointer<CUnsignedLong>(Builtin.addressof(&_fastEnumerationStorageMutationsTarget))
 #endif
+
+@usableFromInline


In addition to being @usableFromInline, this should also be @_alwaysEmitIntoClient, so that user code doesn't try to link against a nonexistent symbol in older Swift runtime dylibs.

Done.

Thanks, that's the piece of knowledge I was missing here.

malloc introspection is a platform feature that is unavailable on OpenBSD. There is no workaround for the feature, so we have to assume that allocations succeed in allocating exactly the amount of memory requested, and nothing more. Here a new mallocSize shim is introduced so the feature check for malloc introspection is pushed to the shims, rather than using os checks directly from Swift. Not every use of malloc_size has been converted yet; ManagedBuffer.swift still remains. However, this module requires special care to fix, which will be done separately.

3405691582 · 2020-04-28T00:10:55Z

Can someone please kick-off swift-ci on Mac OS for me?

jckarter · 2020-04-28T22:58:26Z

@swift-ci Please test

swift-ci · 2020-04-28T23:01:10Z

Build failed
Swift Test Linux Platform
Git Sha - 03ffde0cecb3c576c500a9de4242ec8e1b3d222b

swift-ci · 2020-04-28T23:02:36Z

Build failed
Swift Test OS X Platform
Git Sha - 03ffde0cecb3c576c500a9de4242ec8e1b3d222b

jckarter · 2020-04-29T15:18:23Z

@CodaFi This look good before I merge?

jckarter · 2020-04-29T18:02:16Z

Thanks @3405691582!

CodaFi requested changes Mar 27, 2020

View reviewed changes

CodaFi assigned milseman Mar 27, 2020

3405691582 force-pushed the OpenBSD_StringStorage_AvoidMallocSize branch from 3886ca2 to 538e2d3 Compare March 27, 2020 23:33

3405691582 force-pushed the OpenBSD_StringStorage_AvoidMallocSize branch 2 times, most recently from 09262ff to f594ddb Compare March 31, 2020 03:10

benrimmington reviewed Apr 15, 2020

View reviewed changes

stdlib/public/SwiftShims/LibcShims.h Outdated Show resolved Hide resolved

stdlib/public/core/Shims.swift Outdated Show resolved Hide resolved

3405691582 force-pushed the OpenBSD_StringStorage_AvoidMallocSize branch from f594ddb to 4b70859 Compare April 15, 2020 04:30

benrimmington reviewed Apr 15, 2020

View reviewed changes

3405691582 force-pushed the OpenBSD_StringStorage_AvoidMallocSize branch from 4b70859 to 03ffde0 Compare April 15, 2020 15:20

jckarter reviewed Apr 27, 2020

View reviewed changes

3405691582 force-pushed the OpenBSD_StringStorage_AvoidMallocSize branch from 03ffde0 to 7830028 Compare April 28, 2020 00:04

CodaFi approved these changes Apr 29, 2020

View reviewed changes

jckarter merged commit 9cdfb0e into swiftlang:master Apr 29, 2020

[stdlib] Avoid malloc_size on OpenBSD. #30560

[stdlib] Avoid malloc_size on OpenBSD. #30560

Uh oh!

Conversation

3405691582 commented Mar 22, 2020

Uh oh!

3405691582 commented Mar 22, 2020

Uh oh!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

milseman commented Mar 27, 2020

Uh oh!

milseman commented Mar 27, 2020

Uh oh!

3405691582 commented Mar 27, 2020

Uh oh!

jckarter commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

3405691582 commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jckarter commented Mar 30, 2020

Uh oh!

3405691582 commented Mar 30, 2020

Uh oh!

3405691582 commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd commented Apr 3, 2020

Uh oh!

compnerd commented Apr 3, 2020

Uh oh!

swift-ci commented Apr 3, 2020

Uh oh!

swift-ci commented Apr 3, 2020

Uh oh!

compnerd commented Apr 8, 2020

Uh oh!

swift-ci commented Apr 8, 2020

Uh oh!

3405691582 commented Apr 8, 2020

Uh oh!

CodaFi commented Apr 13, 2020

Uh oh!

swift-ci commented Apr 13, 2020

Uh oh!

benrimmington left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benrimmington Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

benrimmington Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

3405691582 Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

benrimmington Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

3405691582 Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

jckarter commented Apr 15, 2020

Uh oh!

swift-ci commented Apr 15, 2020

Uh oh!

swift-ci commented Apr 15, 2020

Uh oh!

compnerd commented Apr 20, 2020

Uh oh!

swift-ci commented Apr 20, 2020

Uh oh!

compnerd commented Apr 26, 2020

Uh oh!

jckarter commented Mar 30, 2020 •

edited

Loading

3405691582 commented Mar 30, 2020 •

edited

Loading

3405691582 commented Mar 31, 2020 •

edited

Loading