Always, no, never... forget to check your references

[bitjammer]

for weak semantics, that is!

94a9c51 made some changes to loading
weak references by adding some information in the lower bits with
respect to locking. These bits need to be masked out when performing a
load, such as when we want to get the metadata pointer for a class
instance. This normally works fine when going through normal codegen
methods.

When the runtime function swift_ClassMirror_subscript gets the offset
of one of its stored properties, it immediately packages it into the
rather ad-hoc existential container, known as just Mirror in the
runtime. However, the weak reference isn't aligned! It has bit 1 set.
We weren't loading the weak reference here as we would during
normal SILGen, such as with a weak_load instruction. Simulate that here
by calling swift_weakLoadStrong before putting it into the Mirror
container, which also clears the lower bits.

rdar://problem/27475034

[bitjammer]

I still need to update the tests and add a new test, but @rjmccall does this look like the right approach here?

[cc @dabrahams and @jckarter with whom I spoke about this before)

[rjmccall]

Doesn't this create a directness inconsistency? That is, doesn't reflect() expect to get a T*, not a T-that-happens-to-be-a-pointer?

[bitjammer]

Yeah, it stores an existential at a given out address. I think this was working because I was setting a weak property that happened to also be a global, i.e.:

public class Person {
  weak var parent: Person?
  let x = 0xAABBCCDDEE
}
let p = Person()
let p2 = Person()
p.parent = p2
let child = Swift._reflect(p)[0].1.value
print(child)

works, but

public class Person {
  weak var parent: Person?
  let x = 0xAABBCCDDEE
}
let p = Person()
p.parent = Person() // <-- Not a global
let child = _reflect(p)[0].1.value
print(child)

does not.

[bitjammer]

Hah, @rjmccall points out that p.parent = Person() won't hold onto parent, because it's weak. Wow, duh.

[bitjammer]

I think the indirectness is correct. The actual return does the right assignment here:

  new (outString) String(getFieldName(Clas->getDescription()->Class.FieldNames, i));
  // 'owner' is consumed by this call.
  new (outMirror) Mirror(reflect(owner, fieldData, fieldType.getType()));

And the implementation of the subscript operation itself looks to be correct in the debugger to me.

[rjmccall]

Lots of comments made in person:

• There's a parallel path for structs that also needs to be fixed; please extract out a common function to do this.
• You should make sure this works correctly for unowned(safe) and unowned(unsafe) references, too. It's okay to assume for now that struct/class fields can't be indirect.
• Mirror definitely has an invariant of holding a T* that this violates.
• Loading is the right thing to do, but we then need to use the MagicMirror constructor that makes a box to hold the loaded value.
• To construct a complete value, you'll potentially also need to copy in protocol witness tables if the property has existential or optional-existential type.
• That MagicMirror constructor doesn't care about the enclosing owner, but we're required to consume it, so you need to unownedRelease it when we're using that constructor.
• You need to use unknownWeakLoadStrong (and unknownUnownedLoadStrong, and unknownRetain(*fieldPtr)).
• Cases to test:
  • property of Swift class type holding a Swift class instance
  • property of ObjC-compatible class type holding a Swift class instance
  • property of ObjC-compatible class type holding an ObjC class instance
  • property of class-bound existential type holding a Swift class instance
  • property of class-bound existential type holding an ObjC class instance

[bitjammer]

Thanks John. I'm going to just address the weak case in this pull request and reconcile all of the other reference storage kinds in a a follow-up pull request to break up the patch a little bit.

[bitjammer]

Sigh. Just found another crash with structs in swift_MagicMirrorData_value. It tries to retain a non-aligned pointer in the default initializeBufferWithCopy.

[bitjammer]

@swift-ci Please test

[bitjammer]

@swift-ci Please smoke test

[bitjammer]

@swift-ci Please smoke test

[bitjammer]

/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-master/swift/stdlib/public/runtime/Reflection.mm:419:8: error: variable has incomplete type 'void'
  auto strongValue = swift_unknownWeakLoadStrong(weakField);
       ^

Wha? Why is this a void function on Linux?

[bitjammer]

@swift-ci Please smoke test

[CodaFi]

@swift-ci please smoke test.

[bitjammer]

@swift-ci Please smoke test

[bitjammer]

@swift-ci Please smoke test.

[bitjammer]

All tests passed, looks like an infrastructural issue with the packaging step. I'll merge now.