IRGen: properly handle deallocation of a boxed type

[compnerd]

When a boxed type is destroyed, ensure that the object is marked as
deiniting first. This was caught by an assertion in the runtime.

Addresses SR-6268 | rdar://problem/35296541

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

[compnerd]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - b2c3a4802956ef28d8b877351d4d9403fa8ca774

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - b2c3a4802956ef28d8b877351d4d9403fa8ca774

[compnerd]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - b2c3a4802956ef28d8b877351d4d9403fa8ca774

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - b2c3a4802956ef28d8b877351d4d9403fa8ca774

[compnerd]

@gparker42 Im not sure if you or someone else is the best person for reviewing this

[gparker42]

No, you need someone more familiar with SIL's semantics. @jckarter? @gottesmm?

[aschwaighofer]

We call boxTI.deallocate() from the dealloc_box SIL instruction. It assumes that the box object is uninitialized.

emitDeallocateHeapObject assumes that an object is in the deallocating stage -- we need to mark it as such. Under a normal lifetime of an object this is done by the ultimate release. Since we forgo the ultimate release here (the object is unitialized) marking it as deallocating (setting the IsDeallocating bit) is correct.

This LGTM.

I believe we also need this call inside the NonFixedBoxTypeInfo deallocate method because swift_deallocBox also directly calls swift_deallocObject.

[jckarter]

Should SILGen be emitting a set_deallocating instruction somewhere instead?

[aschwaighofer]

I believe this is valid SIL:

%0 = alloc_box $FooBox // Allocate an uninitialized box
dealloc_box %0 // Deallocate the box assuming it is unitialized

[compnerd]

@aschwaighofer I think that we want to do the NonFixedBoxTypeInfo in a separate change (happy to take that on, though need to figure out a test case for that), but I believe that you are right that this needs to be mirrored there as well.

@jckarter that was exactly my question when I started investigating this. Given a quick skim of the SIL langref, I came to the conclusion that the sample that @aschwaighofer posted was desired and that we should handle this by emitting the call in the deallocate handling.

Thanks for the review @aschwaighofer! Going to merge this for now, and follow up with a change for the NonFixedBoxTypeInfo case.

[aschwaighofer]

@jckarter pointed out that calling a runtime function is overhead.
We only check this in assert builds. Can you conditionalize the call to IGF.emitNativeSetDeallocating(box); on assert builds?

[compnerd]

I agree with @gparker42 that the assertions are invaluable debugging assets. So, I was looking at this a bit further for the NonFixedBoxTypeInfo. I think my understanding might be too weak to come up with a proper test case for that. I was trying to play with returning an optional protocol instead of a concrete type (AIUI which should give us a non-fixed sized boxed type), however, that seems to work properly. I believe that the reasoning for that is that we are constructing the indirect destruction, which will correctly mark up the Deiniting bit on the object. I think that the problem is as @jckarter pointed out: we are directly calling swift_deallocObject, and that does not guarantee that the object is in the right state for the tear down. Doing the forced deallocation as a flag sounds like a reasonable approach to avoiding the extra runtime call. At that point, this seems like it may tread into the ABI breaking territory, so we should figure this out and implement it before the 5.0 release.

[aschwaighofer]

Here is an example for NonFixedTypes.

protocol Initializable {
  init()
}

func f2<T: Initializable>(_ x: T) -> T? { return nil }

func c<T: Initializable>(_ x: T) {

({
  guard var b = f2(x) else { return }
  let c = { b = T() }
  _ = (b, c)
})()

}
extension Bool : Initializable {
  init() {
    self = true
  }
}
c(true)

[aschwaighofer]

I checked and it is necessary to set the bit i.e. the solution to just skip the check is not valid.

If we don't set the bit we end up on the swift_unownedRelease path in swift_deallocObject:

  if (object->refCounts.canBeFreedNow()) {
    // object state DEINITING -> DEAD
    SWIFT_RT_ENTRY_CALL(swift_slowDealloc)
         (object, allocatedSize,
          allocatedAlignMask);
  } else {
    // object state DEINITING -> DEINITED
HERE->    SWIFT_RT_ENTRY_CALL(swift_unownedRelease)(object);
  }

It needs class metadata:

void swift::swift_unownedRelease(HeapObject *object)
    SWIFT_CC(RegisterPreservingCC_IMPL) {
  SWIFT_RT_TRACK_INVOCATION(object, swift_unownedRelease);
  if (!isValidPointerForNativeRetain(object))
    return;

  // Only class objects can be unowned-retained and unowned-released.
  assert(object->metadata->isClassObject());
  assert(static_cast<const ClassMetadata*>(object->metadata)->isTypeMetadata());
  
  if (object->refCounts.decrementUnownedShouldFree(1)) {
    auto classMetadata = static_cast<const ClassMetadata*>(object->metadata);
    
    SWIFT_RT_ENTRY_CALL(swift_slowDealloc)
        (object, classMetadata->getInstanceSize(),
         classMetadata->getInstanceAlignMask());
  }
}

We should just add an entry point that is a combination of swift_setDeallocating and swift_deallocObject and call it instead of emitDeallocateHeapObject.

swift_deallocBox should call object->refCounts.decrementFromOne (equivalent to calling swift_setDeallocting).

[aschwaighofer]

The latter: #12744

[aschwaighofer]

The former: #12745