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

Panic with error parameter should be set if the method returns NO when trying to serialize MTLBinaryArchive #653

Open
chyyran opened this issue Sep 19, 2024 · 6 comments
Labels
A-framework Affects the framework crates and the translator for them A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates bug Something isn't working

Comments

@chyyran
Copy link

chyyran commented Sep 19, 2024

I'm trying to serialize a MTLBinaryArchive to a file inside the application bundle caches directory.

   unsafe {
            binary_archive.addFunctionWithDescriptor_library_error(&fragment_desc, &fragment)?;
            binary_archive.addFunctionWithDescriptor_library_error(&vertex_desc, &vertex)?;
        }

        unsafe {
            let manager = NSFileManager::defaultManager();
            let application_support =manager
                .URLsForDirectory_inDomains(NSSearchPathDirectory::NSCachesDirectory, NSSearchPathDomainMask::NSUserDomainMask);

            let metal_lib = application_support.first().unwrap().URLByAppendingPathComponent(ns_string!("librashader.metallib"))
                .unwrap();
            eprintln!("{:?}", metal_lib);
            if let Err(e) = binary_archive.serializeToURL_error(&metal_lib) {
                eprintln!("{:?}", e.localizedDescription());
            }
        }

This is panicking unexpectedly with objc2 generated code

URL { __superclass: file:///Users/chyyran/Library/Containers/red.snowflakepow.librashader-objctest/Data/Library/Caches/librashader.metallib }
thread '<unnamed>' panicked at /Users/chyyran/.cargo/registry/src/index.crates.io-6f17d22bba15001f/objc2-metal-0.2.2/src/generated/MTLBinaryArchive.rs:79:1:
error parameter should be set if the method returns NO
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::option::expect_failed
   3: core::option::Option<T>::expect
             at /rustc/adf8d168af9334a8bf940824fcf4207d01e05ae5/library/core/src/option.rs:928:21
   4: objc2::__macro_helpers::msg_send::encountered_error
             at /Users/chyyran/.cargo/registry/src/index.crates.io-6f17d22bba15001f/objc2-0.5.2/src/__macro_helpers/msg_send.rs:153:5
   5: objc2::__macro_helpers::msg_send::MsgSend::send_message_error
             at /Users/chyyran/.cargo/registry/src/index.crates.io-6f17d22bba15001f/objc2-0.5.2/src/__macro_helpers/msg_send.rs:95:26
   6: objc2_metal::generated::__MTLBinaryArchive::MTLBinaryArchive::serializeToURL_error

How I'm debugging this is a little complex so it might just be easier to give repro instructions.

$ git clone https://github.com/SnowflakePowered/librashader
$ git checkout metal-shader-cache
$ git submodule update --init
$ cd librashader-runtime-mtl 
$ cargo test triangle -- --test-threads=1

This won't actually give you the full unwind message though due to C-unwind ABI changes, but I wrapped it in catch_unwind in the repro branch (https://github.com/SnowflakePowered/librashader/tree/metal-shader-cache) to get more of that out. I got this backtrace via a very finicky debug setup with C ABI bindings and running within XCode so it's too complicated to set up compared to just the cargo test.

@madsmtm madsmtm added bug Something isn't working A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates A-framework Affects the framework crates and the translator for them labels Sep 19, 2024
@madsmtm
Copy link
Owner

madsmtm commented Sep 19, 2024

error parameter should be set if the method returns NO

Huh, that's very odd, and contrary to the documented behaviour of the function - the header says:

@method serializeToURL:error:
@abstract Write the contents of a MTLBinaryArchive to a file.
@discussion Persisting the archive to a file allows opening the archive on a subsequent instance of the app, making available the contents without recompiling.
@param url The file URL to which to write the file
@param error If the function fails, this will be set to describe the failure. This can be (but is not required to be) an error from the MTLBinaryArchiveDomain domain. Other possible errors can be file access or I/O related.
@return Whether or not the writing the file succeeded.

Emphasis mine, I cannot read this in any other way than that the error is set if the method fails (and that's also the expected behaviour in general, see #276 for more details).

So I'd argue that this is an Apple bug, but I guess I'll have a look at what Swift does in this case.

This won't actually give you the full unwind message though due to C-unwind ABI changes

Yeah, see also #539, should hopefully be a bit better with the next version of objc2.

@madsmtm
Copy link
Owner

madsmtm commented Sep 19, 2024

Smaller reproducer:

use objc2::rc::Retained;
use objc2_foundation::{ns_string, NSURL};
use objc2_metal::{
    MTLBinaryArchive, MTLBinaryArchiveDescriptor, MTLCreateSystemDefaultDevice, MTLDevice,
};

#[link(name = "CoreGraphics", kind = "framework")]
extern "C" {}

fn main() {
    unsafe {
        let device = Retained::from_raw(MTLCreateSystemDefaultDevice()).unwrap();
        let binary_archive = device
            .newBinaryArchiveWithDescriptor_error(&MTLBinaryArchiveDescriptor::new())
            .unwrap();

        let metal_lib = NSURL::URLWithString(ns_string!("file://test.metallib")).unwrap();
        if let Err(e) = binary_archive.serializeToURL_error(&metal_lib) {
            eprintln!("{:?}", e.localizedDescription());
        }
    }
}

@madsmtm
Copy link
Owner

madsmtm commented Sep 19, 2024

Equivalent Swift:

import Foundation
import Metal
import CoreGraphics

let device = MTLCreateSystemDefaultDevice()!
let binary_archive = try! device.makeBinaryArchive(descriptor: MTLBinaryArchiveDescriptor())

let metal_lib = NSURL(string: "file://test.metallib")!
do {
    try binary_archive.serialize(to: metal_lib as URL)
} catch {
    print(error)
}

Seems like they return a Foundation._GenericObjCError.nilError.

@madsmtm
Copy link
Owner

madsmtm commented Sep 19, 2024

@madsmtm
Copy link
Owner

madsmtm commented Sep 19, 2024

Hmm, wondering how we can do the same, without defining a whole new NSError subclass? Maybe we use a custom "__objc2" NSErrorDomain, a custom status code, and a useful description/debug description? Might also be useful for objc2::exception::catch which currently has to awkwardly handle nil exceptions.

Although all of that's still fallible in OOM situations, so we'll still have to panic/abort somewhere.

@madsmtm
Copy link
Owner

madsmtm commented Sep 19, 2024

And the error should of course be cached.

@madsmtm madsmtm added this to the objc2 v0.6 milestone Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-framework Affects the framework crates and the translator for them A-objc2 Affects the `objc2`, `objc2-exception-helper` and/or `objc2-encode` crates bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants