Use uninitialized dealloc in swift_deallocError

[zayass]

Current of implementation swift_deallocError produce assertion in code like this

import Foundation

func empty(_ error: Error?) {
}

func crash(_ error: NSError?) {
  empty(error)
}

crash(nil)

Assertion is here https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L678

assert(object->refCounts.isDeiniting());

When swift_deallocError called box have unowned count = 1, strong count = 0 and deiniting = false.

I am not sure is it correct way to avoid assertion, and would it work properlly in all cases. So would be cool if somebody looks into this.

[jrose-apple]

I don't think this is the correct fix, since the error is initialized. @mikeash, have we seen anything like this?

[zayass]

@jrose-apple As I understand swift_deallocError invoked only when error box is allocated but not filled with actual error so we should not call _destroyErrorObject just deallocate skipping deiniting.
This method is looks pretty close to what is needed if first assumption is correct.

Or what you mean "initialized"

[mikeash]

I wasn't aware of any problems with this, but this does look like the correct fix. The SWIFT_OBJC_INTEROP version of swift_deallocError is:

/// Deallocate an error object whose contained object has already been
/// destroyed.
void
swift::swift_deallocError(SwiftError *error, const Metadata *type) {
  object_dispose((id)error);
}

This fix adjusts the non-interop version to match. Assuming the ObjC version is correct (I imagine someone would have noticed by now if it weren't) this should be good.

[jrose-apple]

My mistake! Can you add a test for this, then, Anton? test/stdlib/ErrorBridged.swift seems to be a good place.

[mikeash]

Whoops, my poor Markdown skills resulted in most of the comment being hidden. Since it describes that the function is intended to deallocate an error object which has already been destroyed, that's kind of important!

[zayass]

@jrose-apple of course will do

[zayass]

@jrose-apple As I see test/stdlib/ErrorBridged.swift REQUIRES: objc_interop but this code instead executes only if interop disabled. Do you know some another good place for test? Or just create new test?

[jrose-apple]

Ah, right. Well, we can't test NSError specifically in the Swift repo because swift-corelibs-foundation isn't available in the Swift tests, but this seems like a problem that would occur with any Error, or maybe any class that conforms to Error (as opposed to the usual struct or enum). If that's the case, then adding it to test/stdlib/Error.swift is probably good.

[zayass]

problem that would occur with any Error

With any error marked as @objc. But @objc also require to import Foundation

[zayass]

I was wrong simple enum errors also produce assertions. But I am still not clear how to test it.
Just add code which leads to assertion to test/stdlib/Error.swift and if assert occurs test fail?

[mikeash]

The test should automatically fail on crash by default, so you should be able to make a simple test case that just triggers the failure case and returns. You can try running it with and without your fix to make sure it fails when it's supposed to.

[millenomi]

You can have perhaps a separate PR against swift-corelibs-Foundation to add a test there, which is where we plan to have bridging tests for non-ObjC bridging.

[zayass]

Looks like original issue already fixed in master by #18937
Compiler does not emit deallocError at all in this case.

Maybe still actual in some other cases but I don't them.

[jrose-apple]

cc @jckarter.

@mikeash, maybe we should just test this manually, by calling swift_deallocError directly? Or should it be an error to call it on an uninitialized buffer?

[mikeash]

It can't be called on a completely uninitialized buffer. It calls through to either object_destroy (when ObjC interop is enabled) or swift_deallocObject which requires that they be deinitialized/uninitialized but otherwise valid objects.

[zayass]

@mikeash Can you look into this?

Original code which leads to issue:

import Foundation

func empty(_ error: Error?) { }
func crash(_ error: NSError?) { empty(error) }

SIL generated by swift-DEVELOPMENT-SNAPSHOT-2018-08-26-a-ubuntu16.04:

sil hidden @$S4test5crashyy10Foundation7NSErrorCSgF : $@convention(thin) (@guaranteed Optional<NSError>) -> () {
// %0                                             // users: %5, %4, %1
bb0(%0 : $Optional<NSError>):
  debug_value %0 : $Optional<NSError>, let, name "error", argno 1 // id: %1
  %2 = alloc_existential_box $Error, $NSError     // users: %10, %6, %3
  %3 = project_existential_box $NSError in %2 : $Error // user: %9
  retain_value %0 : $Optional<NSError>            // id: %4
  switch_enum %0 : $Optional<NSError>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1 // id: %5

bb1:                                              // Preds: bb0
  dealloc_existential_box %2 : $Error, $NSError   // id: %6
  br bb4                                          // id: %7

// %8                                             // user: %9
bb2(%8 : $NSError):                               // Preds: bb0
  store %8 to %3 : $*NSError                      // id: %9
  %10 = enum $Optional<Error>, #Optional.some!enumelt.1, %2 : $Error // user: %11
  br bb3(%10 : $Optional<Error>)                  // id: %11

// %12                                            // users: %15, %14
bb3(%12 : $Optional<Error>):                      // Preds: bb4 bb2
  // function_ref empty(_:)
  %13 = function_ref @$S4test5emptyyys5Error_pSgF : $@convention(thin) (@guaranteed Optional<Error>) -> () // user: %14
  %14 = apply %13(%12) : $@convention(thin) (@guaranteed Optional<Error>) -> ()
  release_value %12 : $Optional<Error>            // id: %15
  %16 = tuple ()                                  // user: %17
  return %16 : $()                                // id: %17

bb4:                                              // Preds: bb1
  %18 = enum $Optional<Error>, #Optional.none!enumelt // user: %19
  br bb3(%18 : $Optional<Error>)                  // id: %19
} // end sil function '$S4test5crashyy10Foundation7NSErrorCSgF'

SIL generated by swift-DEVELOPMENT-SNAPSHOT-2018-09-04-a-ubuntu16.04:

// crash(_:)
sil hidden @$S4test5crashyy10Foundation7NSErrorCSgF : $@convention(thin) (@guaranteed Optional<NSError>) -> () {
// %0                                             // users: %3, %2, %1
bb0(%0 : $Optional<NSError>):
  debug_value %0 : $Optional<NSError>, let, name "error", argno 1 // id: %1
  retain_value %0 : $Optional<NSError>            // id: %2
  switch_enum %0 : $Optional<NSError>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb1 // id: %3

bb1:                                              // Preds: bb0
  br bb4                                          // id: %4

// %5                                             // user: %8
bb2(%5 : $NSError):                               // Preds: bb0
  %6 = alloc_existential_box $Error, $NSError     // users: %9, %7
  %7 = project_existential_box $NSError in %6 : $Error // user: %8
  store %5 to %7 : $*NSError                      // id: %8
  %9 = enum $Optional<Error>, #Optional.some!enumelt.1, %6 : $Error // user: %10
  br bb3(%9 : $Optional<Error>)                   // id: %10

// %11                                            // users: %14, %13
bb3(%11 : $Optional<Error>):                      // Preds: bb4 bb2
  // function_ref empty(_:)
  %12 = function_ref @$S4test5emptyyys5Error_pSgF : $@convention(thin) (@guaranteed Optional<Error>) -> () // user: %13
  %13 = apply %12(%11) : $@convention(thin) (@guaranteed Optional<Error>) -> ()
  release_value %11 : $Optional<Error>            // id: %14
  %15 = tuple ()                                  // user: %16
  return %15 : $()                                // id: %16

bb4:                                              // Preds: bb1
  %17 = enum $Optional<Error>, #Optional.none!enumelt // user: %18
  br bb3(%17 : $Optional<Error>)                  // id: %18
} // end sil function '$S4test5crashyy10Foundation7NSErrorCSgF'

As you can see alloc_existential_box moved into .some branch and dealloc_existential_box is eliminated. So no more issue in this case.

But I dont know is any other code can produce dealloc_existential_box for empty box

[jckarter]

I recently changed the order of operations for existential formations so that the box allocation occurs lazily. This fix still looks correct. We'd get similar codegen if a call throws while trying to initialize the box in-place, as in:

struct Foo<T>: Error { var x, y: T }
func foo<T>() throws -> Foo<T>
func makeError<T>(of: T.Type) -> Error {
  return try foo() as Foo<T>
}

[zayass]

@jckarter thanks for example

[zayass]

Test added. Can anyone trigger ci?

[jrose-apple]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - c59d755b004564e406551be9aa6d30625e837b9e

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - c59d755b004564e406551be9aa6d30625e837b9e

[jrose-apple]

All right, merging. Thanks, Anton!