Skip to content

IRGen: properly handle deallocation of a boxed type #12717

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.

Sign up for GitHub

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 1 commit into from
Nov 2, 2017
Merged

IRGen: properly handle deallocation of a boxed type #12717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/IRGen/GenHeap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1541,6 +1541,7 @@ class FixedBoxTypeInfoBase : public BoxTypeInfo {
auto size = layout.emitSize(IGF.IGM);
auto alignMask = layout.emitAlignMask(IGF.IGM);

IGF.emitNativeSetDeallocating(box);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should SILGen be emitting a set_deallocating instruction somewhere instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is valid SIL:

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latter: #12744

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The former: #12745

emitDeallocateHeapObject(IGF, box, size, alignMask);
}

Expand Down
15 changes: 15 additions & 0 deletions test/IRGen/alloc_box.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// RUN: %target-swift-frontend -primary-file %s -emit-ir -o - | %FileCheck %s

func f() -> Bool? { return nil }

({
guard var b = f() else { return }
let c = { b = true }
_ = (b, c)
})()

// CHECK-LABEL: @_T09alloc_boxyycfU_
// CHECK: <label>:8:
// CHECK: call void @swift_setDeallocating
// CHECK: call void @swift_rt_swift_deallocObject