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

multiboot2: properly type DST tags #134

Merged
merged 2 commits into from
May 19, 2023
Merged

multiboot2: properly type DST tags #134

merged 2 commits into from
May 19, 2023

Conversation

phip1611
Copy link
Collaborator

This PR introduces a proper DST typing of Multiboot2 tags that are DSTs. This includes all tags that have a dynamically size, usually used for a null-terminated string at the end of a tag.

@phip1611 phip1611 self-assigned this Apr 28, 2023
@phip1611 phip1611 marked this pull request as draft April 28, 2023 09:53
@phip1611 phip1611 mentioned this pull request Apr 28, 2023
@phip1611 phip1611 force-pushed the dst-tags branch 7 times, most recently from e1e04a1 to 16a1e50 Compare May 3, 2023 13:47
@phip1611 phip1611 marked this pull request as ready for review May 3, 2023 13:48
multiboot2/src/lib.rs Outdated Show resolved Hide resolved
multiboot2/src/lib.rs Outdated Show resolved Hide resolved
// All sized tags automatically have a Pointee implementation where
// Pointee::Metadata is (). Hence, the TagTrait is implemented automatically for
// all tags that are sized.
impl<T: Pointee<Metadata = usize>> TagTrait for T {
Copy link
Collaborator Author

@phip1611 phip1611 May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @nicholasbishop,
May I ask you for help, please? That's a tough one, I think. I want to have default implementations for both cases but the compiler tells me the following

image

However, I'd assume that both implementations are distinct, as Metadata has a different specialization..

I have to cover both cases, as at another place in the code, I do the following:

 unsafe fn from_base_tag<'a>(tag: &Tag) -> &'a Self {
        let ptr = tag as *const _ as *const ();
        // second parameter is either of type `()` or `usize`
        let ptr = ptr_meta::from_raw_parts(ptr, Self::dst_size(tag));
        &*ptr
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly the advice in https://stackoverflow.com/questions/40392524 would help, although I had some difficulty following it.

I'm not quite clear on whether a blanket impl makes sense here anyway though. Do I have it right that all tag types must start with a 32-bit ID followed by a 32-bit size field? The blanket impl would mean that TagTrait is implemented for things like () and u32 which seems wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just found out that there is an open issue for my use case: rust-lang/rust#20400

Do I have it right that all tag types must start with a 32-bit ID followed by a 32-bit size field

Yes

The blanket impl would mean that TagTrait is implemented for things like () and u32 which seems wrong.

I do want to have TagTrait implemented for all tags, sized and unsized ones. This is required as I need to create sized as well as unsized tag references from the same method:

// Self::dst_size() either returns `()` for sized types or a value of type `usize` for DSTs
let ptr = ptr_meta::from_raw_parts(ptr, Self::dst_size(tag));

Copy link
Collaborator Author

@phip1611 phip1611 May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a chat with a colleque. After looking at rust-lang/rust#20400 and rust-lang/rfcs#1672 (comment), I came to the conclusion that the solution without 4a6964e is good enough.

Do you have maybe any other thoughts or another idea for a solution? The only problem with my solution without 4a6964e is that I have to repeat

impl crate::TagTrait for BootLoaderNameTag {
    fn dst_size(base_tag: &Tag) -> usize {
        // The size of the sized portion of the bootloader name tag.
        let tag_base_size = 8;
        assert!(base_tag.size >= 8);
        base_tag.size as usize - tag_base_size
    }
}

for every dynamically sized tag. But this only affects three tags. So I think, the solution is good enough..

@phip1611
Copy link
Collaborator Author

@YtvwlD any further thoughts on this?

This commit transforms a few tags where applicable to DSTs. This better
fits into the Rust type system and makes a few things easier, such as
parsing the cmdline string from the command line tag.

Depending on how users used the tags, this change is not even breaking.
Additionally, there is now a public trait which allows custom tag users
to also benefit from DSTs.
@phip1611