Skip to content
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.

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

On macOS testClass crashes in dealloc #5

Closed
helje5 opened this issue Dec 12, 2017 · 13 comments
Closed

On macOS testClass crashes in dealloc #5

helje5 opened this issue Dec 12, 2017 · 13 comments

Comments

@helje5
Copy link
Contributor

helje5 commented Dec 12, 2017

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
Copy link
Owner

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
Copy link
Contributor Author

helje5 commented Dec 13, 2017

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
Copy link
Contributor Author

helje5 commented Dec 13, 2017

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
Copy link
Owner

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
Copy link
Contributor

NSExceptional commented Apr 10, 2019

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 wickwirew reopened this Apr 10, 2019
@wickwirew
Copy link
Owner

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
Copy link
Owner

wickwirew commented Apr 10, 2019

@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
Copy link
Contributor

NSExceptional commented Apr 10, 2019

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
Copy link
Owner

Here you go:

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

@NSExceptional
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor

Done and done:

wickwirew/CRuntime#4
#40

wickwirew added a commit that referenced this issue Apr 11, 2019
Use swift_allocObject instead of Objc runtime, fix #5
@wickwirew
Copy link
Owner

Merged! Thanks for adding that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants