rust-osdev/acpi

AML: Suggestion - Use smart pointer instead of [u8]

rw-vanc opened this issue · 2 comments

It's very difficult to debug AML code and the AML parser due to lack of information about where an error occurs. I suggest using smart pointers instead of [u8] for the buffer type, and reporting the relative position of the cursor when an error occurs. This will allow the developer to find the offending byte much more easily.

I definitely agree this could be improved. In the past, I've got pretty far with just printing the next dozen or so bytes and seeing what isn't parsing, but it would be nicer to have more context.

I'm also guessing that this would be an iteration of the solution you've suggested in #169? If so I think doing it by tracking the offset through the slice as we go will be a more accurate solution.

I don't think we need a smart pointer, as fundamentally we will be operating on a &[u8], but I might be misunderstanding what you mean. How about replacing our current parser 'input' type (which is currently &[u8]) with a wrapper type with an offset into the overall buffer which is added to as we go through the slice?

Edit: although I've just had a look at our current parser code and I agree this looks simplest to do, because of the way we return new_input slices out of the parser, to do with pointer arithmetic under the surface. I wonder if we can have a wrapper type that wraps the entire buffer, but implements indexing by range etc. so we can keep most of our current code looking like it operates on a raw slice? This could be a fairly large upheaval, but is probably the cleanest solution in the long run.

I have an implementation with this:

pub struct AmlStream<'a> {
    source: &'a [u8],
    buf: &'a [u8],
}

There was a lot of copy/paste and removal of &'s to make it work, but it seems to work fine. However, I think I will change source to usize, as it is never used except for debugging. The implementation of Debug is genuinely helpful, although I would like your thoughts on what it should look like.

The content of functions is copied into a Vec, I convert the Vec into an AmlStream at the time the function gets executed.

My IDE is complaining that I need an implementation of PartialEq for [u8; 4] and [_; 0] with AmlStream (for the test macros) but the compiler doesn't complain. I can't figure out the syntax for PartialEq<u8; N>, so if you can point me at something, that would be helpful. [Edit: cargo test is failing because of this, so fixing it is mandatory.]

I hope to submit a PR this week.