Panic on irq_handler overwrite

[Lurk]

Fixes #50

In this PR we introduce struct IrqHandler.

struct IrqHandler {
    callback: Box<dyn FnMut() + Send + Sync>,
    attached: bool,
}

Right now there is only one ancillary field - attached, which allows an "overwrite" check. But potentially opens the way to build something like /proc/interrupts.

[michalfita]

That's simple solution that works. From my perspective, however, a person working on embedded systems it's 1024/2048 bytes in cache (larger on 64 bit systems¹) just to mark if we already assigned interrupt handler - quite a waste IMVHO. Some static bit array, but none of crates delivering this datatype stabilized for const generics yet.

My idea (sorry, I didn't have time to work on it) was to check, if the pointer in the Box<> refers to empty_irq_callback address. What I'm not sure about is how to check the address of the function used by closure held in Box.

The alternative method is to hold array of references to trait (that way we remove the need for Box<dyn ...> and use generic type to hold closure with the IRQ# statically. But that idea is a lot of work - beauty of it is complete removal of indirection and dependency on dynamically allocated memory.

Right now there is only one ancillary field

Interrupt vectors have to be maximally short and fit into cache, so any interrupt handler is dispatched without delay.

---

¹ While Rust's size of bool is 1 byte, structures have to be aligned (if if not on x86_64 probably, then definitely on ARM)

[Lurk]

Hi @michalfita,
thank you for the input.

I was thinking about a static bit array, but then we will need to warp it in the SpinLock, right?

I don`t know why I did not tried this before:

type Callback = Box<dyn FnMut()>;

fn cmp(left: &Callback, right: &Callback) -> bool {
    return left.as_ref() as *const _ == right.as_ref() as *const _;
}

fn callback() {}

fn main() {
    let box1_with_callback1: Callback = Box::new(callback);
    let box2_with_callback1: Callback = Box::new(callback);
    let box1_with_callback2: Callback = Box::new(|| {});
    let box1_with_callback3: Callback = Box::new(|| {});

    assert!(cmp(&box1_with_callback1, &box2_with_callback1));
    assert!(!cmp(&box1_with_callback1, &box1_with_callback2));
    assert!(!cmp(&box1_with_callback2, &box1_with_callback3));
}

But this works perfectly.

I kind of get the Box removal idea, but just kind of :)

[michalfita]

I kind of get the Box removal idea, but just kind of :)

This just rough idea, not exercised yet.

Thanks, for checking the idea about comparing addresses.

[Lurk]

Okay, on the second thought this:

    let box1_with_callback1:  Box<dyn FnMut()> = Box::new(callback);
    let box2_with_callback1:  Box<dyn FnMut()> = Box::new(callback);

are not the same callbacks anymore because each Box::new is creating a new copy on the heap. And addresses of those copies are not the same.

so this:

box1_with_callback1.as_ref() as *const _ == box2_with_callback1.as_ref() as *const _

It should never be true. But it is, and I do not understand how :)

UPD

So, box1_with_callback1.as_ref() as *const _ is not a pointer. It is a fat pointer. We are comparing not pointers to the data (function in our case) but pointers to vtable. Which is unstable, and Clippy really hates this:

error: comparing trait object pointers compares a non-unique vtable address
  --> src/main.rs:22:5
   |
22 |     box1_with_callback1.as_ref() as *const _ == box2_with_callback1.as_ref() as *const _;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: consider extracting and comparing data pointers only
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#vtable_address_comparisons

We can do one more conversion and get a pointer to the function box1_with_callback1.as_ref() as *const _ as *const(), which does not help because of Box.

If you have any idea where I should look - I am all ears :)

[nuta]

Thanks for the detailed explanation! I think comparing pointers is no longer a good way. How about using BitMap (available in kerla_utils crate) to track attached IRQ numbers instead?

static ATTACHED_IRQS: SpinLock<BitMap<32 /* = 256 / 8 */>> = SpinLock::new(BitMap::zeroed());

[michalfita]

box1_with_callback1.as_ref() as *const _ == box2_with_callback1.as_ref() as *const _
It should never be true. But it is, and I do not understand how :)

My understanding of AsRef implementation for Box<T> is that it returns reference to T. But there still might be some extra going on...

So, box1_with_callback1.as_ref() as *const _ is not a pointer. It is a fat pointer. We are comparing not pointers to the data (function in our case) but pointers to vtable. Which is unstable [...]

According to https://stackoverflow.com/questions/66673989/how-to-get-raw-pointer-of-box-without-consuming-it the box have to be explicitly dereferenced first - makes sense to me.

How about using BitMap (available in kerla_utils crate) to track attached IRQ numbers instead?
static ATTACHED_IRQS: SpinLock<BitMap<32 /* = 256 / 8 */>> = SpinLock::new(BitMap::zeroed());

That approach is seem to be good as well.

[Lurk]

My understanding of AsRef implementation for Box<T> is that it returns reference to T. But there still might be some extra going on...

It will return a reference to T but not the "original" one. According to the docs, Box::new is
Allocates memory on the heap and then places x into it.
So AsRef will return the reference to T, but this reference will be to allocated by Box memory on the heap.

According to https://stackoverflow.com/questions/66673989/how-to-get-raw-pointer-of-box-without-consuming-it the box have to be explicitly dereferenced first - makes sense to me.

Since Box does not store information about the value, just the copy of a value, we will get a copy, not the original value, when we dereference that Box.

fn test(){}

fn main() {
    let a = Box::new(test as fn());
    let b = Box::new(test as fn());
    
    println!("pointer to a {:p}", &a);
    println!("pointer to b {:p}", &b);
    println!("derefernced a {:p}", &*a);
    println!("derefernced b {:p}", &*b);
}

produces:

pointer to a 0x7ffc87094128
pointer to b 0x7ffc87094130
derefernced a 0x55fa220919d0
derefernced b 0x55fa220919f0

I do not see how we can check callback equality while boxing it.

How about using BitMap (available in kerla_utils crate) to track attached IRQ numbers instead?

static ATTACHED_IRQS: SpinLock<BitMap<32 /* = 256 / 8 */>> = SpinLock::new(BitMap::zeroed());

it looks like this is the only way for now that everybody agrees :)

[michalfita]

Looks good to me.

[nuta]

:+1

[nuta]

Thanks for the improvement, @Lurk!