search

wickwirew / Runtime

A Swift Runtime library for viewing type info, and the dynamic getting and setting of properties.
MIT License
1.08k stars 94 forks source link

On macOS `testClass` crashes in dealloc #5

Closed helje5 closed 5 years ago

helje5 commented 6 years ago

Looks like something is wrong/different w/ the RC here.

Test Case '-[RuntimeMacTests.FactoryTests testClass]' started.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x0000000108bfea2d libswiftCore.dylib`_swift_release_dealloc + 13
    frame #1: 0x0000000103d362d4 RuntimeMacTests`FactoryTests.testClass(self=0x000000010390e4a0) at FactoryTests.swift:58
    frame #2: 0x0000000103d368d5 RuntimeMacTests`@objc FactoryTests.testClass() at FactoryTests.swift:0
    frame #3: 0x00007fffb972f0cc CoreFoundation`__invoking___ + 140
    frame #4: 0x00007fffb972ef51 CoreFoundation`-[NSInvocation invoke] + 289
    frame #5: 0x000000010034eb36 XCTest`__24-[XCTestCase invokeTest]_block_invoke.272 + 50
wickwirew commented 6 years ago

May have to remove this feature or make it an iOS only thing. Was able to build classes without the objc runtime but not without memory leaks. ARC ignore the objects even when the retain counts are set.

helje5 commented 6 years ago

Maybe the layout on x86 is just different to the layout on ARM. And I think it would be OK to just throw the unableToBuildType on macOS/Linux. Though, maybe just make the whole function only available to iOS until is is ported, that is probably better.

You could also create an issue for this, maybe some other contributor steps in and does the macOS/Linux parts.

helje5 commented 6 years ago

I commented out the functionality in PR #6. If that is integrated, we could close this particular issue. (and maybe we would want to create new ones for macOS/Linux/x86 support).

wickwirew commented 6 years ago

Merged your PR. Would like to get this functionality in sometime. From the looks of it I don't know how possible it is though. Going to close this for now.

NSExceptional commented 5 years ago

Can we re-open this? The crash is still there, but we're just sort of ignoring it by not supporting macOS...

Per Joe's advice, we should be using swift_allocObject. I'm trying to look into this. Perhaps we could copy that into CRuntime somehow.

wickwirew commented 5 years ago

Reopened. If youre able to get it feel free to PR it! Id love to have this. Ill try to look into it as well

wickwirew commented 5 years ago

@NSExceptional Got it quick version of it working using swift_allocObject. Havent tested for memory leaks or anything but its a start.

In Factory.swift:

    func buildClass(type: Any.Type) throws -> Any {
        var md = ClassMetadata(type: type)
        let info = md.toTypeInfo()

        guard var value = _allocObject(
            type: type,
            requiredSize: md.pointer.pointee.classSize,
            requiredAlignmentMask: UInt32(md.alignment)) else {
                throw RuntimeError.unableToBuildType(type: type)
        }

        try withClassValuePointer(of: &value) { pointer in
            try setProperties(typeInfo: info, pointer: pointer)
        }

        return value
    }
NSExceptional commented 5 years ago

Neat! Can you share your definition for _allocObject as well? You just beat me to it, I was asking Joe how to export it to C

Edit: I'm actually getting an error thrown, it's not able to cast the resulting value to T in createInstance(), using this definition:

@_silgen_name("swift_allocObject")
func _allocObject(
    type: Any.Type,
    requiredSize: UInt32,
    requiredAlignmentMask: UInt32
) -> UnsafeRawPointer?
wickwirew commented 5 years ago

Here you go:

@_silgen_name("swift_allocObject")
func _allocObject(type: Any.Type,
                  requiredSize: UInt32,
                  requiredAlignmentMask: UInt32) -> AnyObject?
NSExceptional commented 5 years ago

Sweet! I can verify that it doesn't leak, the deconstructor is indeed called when the object goes out of scope. You should push the fix! noleak

wickwirew commented 5 years ago

I’ll be out for a few hours so if you have it working feel free to push a PR as well! If not I’ll do it later. swift_allocObject will also have to be moved to CRuntime as well if you do PR it

NSExceptional commented 5 years ago

Done and done:

https://github.com/wickwirew/CRuntime/pull/4 https://github.com/wickwirew/Runtime/pull/40

wickwirew commented 5 years ago

Merged! Thanks for adding that.