fix panic_handler

[publicqi]

#![no_std]
use pinocchio::{
    account_info::AccountInfo, default_allocator, default_panic_handler, program_entrypoint,
    pubkey::Pubkey, ProgramResult,
};

program_entrypoint!(process_instruction);
default_allocator!();
default_panic_handler!();

fn process_instruction(_: &Pubkey, _: &[AccountInfo], _: &[u8]) -> ProgramResult {
    Ok(())
}

[package]
name = "pinocchio-test"
version = "0.1.0"
edition = "2021"

[lib]
crate-type = ["cdylib", "lib"]

[dependencies]
pinocchio = { version = "0.7.1" }

[profile.release]
# strip = "symbols"
lto = true
opt-level = 3

This code doesn't compile now. Error message is

error: `#[panic_handler]` function required, but not found

[febo]

Did you try compiling without being #![no_std]? I think when you link the std, this change might conflict.

[febo]

Perhaps instead of modifying the existing macro, we need to add one specific for no_std:

#[macro_export]
macro_rules! nostd_panic_handler {
    () => {
        /// A panic handler for `no_std`.
        #[cfg(not(test))]
        #[no_mangle]
        #[panic_handler]
        fn custom_panic(info: &core::panic::PanicInfo<'_>) -> ! {
            if let Some(location) = info.location() {
                $crate::log::sol_log(location.file());
            }
            // Panic reporting.
            $crate::log::sol_log("** PANICKED **");
            // Never returns.
            loop {}
        }
    };
}

[publicqi]

yeah that sounds good.

[publicqi]

i had a wrong understanding about how panic works in sbf and now it's corrected:

std:
when a panic happens, rust runtime will call

rust_begin_unwind (std generted)
    std::sys::pal::sbf::panic (sbf compiler generated)
        custom_panic (pinocchio)
        abort syscall (sbf compiler generated)

no_std:

rust_begin_unwind (pinocchio)

so when using std, we define custom_panic as a hook and abort will be called eventually. but in no_std, we will need to manually implement rust_begin_unwind (or i should rephrase it "[panic_handler] will be compiled to rust_begin_unwind").

[febo]

Let's remove the #[panic_handler] attribute from here. The default_panic_handler will be used when you import pinocchio as a no_std but your program links against the std.

[publicqi]

so four cases here:

• if program is std, they'll need to manually specify features = ["std"] in their Cargo.toml. in that case custom-panic (using msg) will be used.
• if a program is no_std, rustc compiler will ask them to specify a [panic_handler], which is what handler doing.
• if a program is no_std but specifying features = ["std"], this should be undefined behavior (e.g. format in pinocchio won't work anyways)
• if a program is std while not specifying features = ["std"], this is kind of shady.

to conclude,

• with this [panic_handler], we discourage the fourth case as std will generate a builtin [panic_handler] and they will conflict.
• without this [panic_handler], people cannot use entrypoint!. they need to use program_entrypoint! and add a [panic_handler] themself.

[febo]

Few comments:

• in the first case, you could still have pinocchio as "no_std" even though your program is std. That would mean pinocchio won't use anything from the std but your program might.
• third case probably will fail compilation, since std won't be available.
• fourth case is not an ideal scenario, but it could happen quite often. The benefit of having pinocchio as a "no_std" is that it won't bring all the format! stuff automatically (reduces the binary size), but you might still have a library in your program that requires std.

[febo]

Let's add a specific opt-in nostd panic handler:

#[macro_export]
macro_rules! nostd_panic_handler {
    () => {
        /// A panic handler for `no_std`.
        #[cfg(not(test))]
        #[no_mangle]
        #[panic_handler]
        fn handler(info: &core::panic::PanicInfo<'_>) -> ! {
            if let Some(location) = info.location() {
                unsafe {
                    $crate::syscalls::sol_panic_(
                        location.file().as_ptr(),
                        location.file().len() as u64,
                        location.line() as u64,
                        location.column() as u64,
                    )
                }
            } else {
                $crate::log::sol_log("** PANICKED **");
                unsafe { $crate::syscalls::abort() }
            }
        }
    };
}

[febo]

When your program is no_std:

program_entrypoint!(...);
no_allocator!();
nostd_panic_handler!();

When your program links against the std, then you can use either of the default_panic_handler - it will depend on whether you are importing pinocchio as no_std or not:

entrypoint(...);

[publicqi]

When your program links against the std, then you can use either of the default_panic_handler - it will depend on whether you are importing pinocchio as no_std or not:

the functionality difference between the two is only CU diff and format? can we just deprecate the std custom-panic?

[febo]

When your program links against the std, then you can use either of the default_panic_handler - it will depend on whether you are importing pinocchio as no_std or not:

the functionality difference between the two is only CU diff and format? can we just deprecate the std custom-panic?

Soon we can deprecate it. At the moment the one with the format! is able to give the dynamic panic message, while the "no_std" variation of the default_panic_handler does not.

In rust 1.84, fmt::Arguments::as_str will be stabilized (see rust-lang/rust#132511), then we can have a single one that does not require std.

We will be able to use:

if let Some(Some(mm)) = info.message().map(|mes| mes.as_str()) {
     let mes = mm.as_bytes();
     unsafe {
          $crate::syscalls::sol_log_(mes.as_ptr(), mes.len() as u64);
     }
}

[febo]

@publicqi Wondering if you had a chance to look into my last comment. I am preparing a release and ideally would like to include this fix.

[publicqi]

yes. i'll fix it

[publicqi]

@febo ready for review. updated some docs too so hopefully it's clear enough.

[publicqi]

force pushed to clean the messy commits

[febo]

force pushed to clean the messy commits

Looks great – just a nit: let's remove the feature = "custom-panic references, they might have sneaked in when you rebased.

[publicqi]

good catch. fixed.

[febo]

One more thing, I think the changes to the syscalls module are also missing.

[publicqi]

ahh yes...

[publicqi]

now should be good. i rebased the wrong main branch.

https://github.com/anza-xyz/pinocchio/compare/main..98245d1c44979e03a8924943bcd8916c6822ca4a

[febo]

Looks great – thanks!