much improved debug output of BootInformation + enum TagType #76

phip1611 · 2021-07-02T11:17:22Z

The current debug output of the BootInformation-struct is not optimal. It isn't nicely formatted (no spaces), doesn't include all relevant fields (like UEFI system table tag) and it outputs all ELF-sections, which can be pretty fast thousands of lines with pretty much useless output.

What do you think about my improvement?

POC with my multiboot2 Rust kernel in QEMU:

Multiboot2 Boot Information {
    start_address: 0x0000000000597000,
    end_address: 0x00000000005e9ae0,
    total_size: 0x0000000000052ae0,
    boot_loader_name_tag: "GRUB 2.04-1ubuntu44.2",
    command_line: "",
    memory_areas: None,
    module_tags (count): 0,
    elf_sections_tags (count): 5287,
    efi_32_ih: None,
    efi_64_ih: Some(
        EFIImageHandle64 {
            typ: 20,
            size: 16,
            pointer: 114517528,
        },
    ),
    efi_sdt_32_tag: None,
    efi_sdt_64_tag: Some(
        EFISdt64 {
            typ: 12,
            size: 16,
            pointer: 127852568,
        },
    ),
    efi_memory_map_tag: None,
}

IsaacWoods

Just a few thoughts to start; I think this is probably an improvement. I'm away for the weekend but am happy to review after I get back.

IsaacWoods · 2021-07-02T13:16:11Z

src/lib.rs

-                    area.size()
-                )?;
-            }
+        for _t in  self.module_tags() {


module_tags returns an ~impl Iterator so this could just be self.module_tags().count I think

IsaacWoods · 2021-07-02T13:16:24Z

src/lib.rs

-                    s.size(),
-                    s.flags().bits()
-                )?;
+            for _s in elf_sections_tag.sections() {


Same use of count could be here I think

ghost · 2021-07-03T15:54:49Z

src/lib.rs

+            .field("memory_areas", &self.memory_map_tag())
+            // so far, I didn't found a nice way to connect the iterator with ".field()" because
+            // the iterator isn't Debug
+            .field("module_tags (count)", &self.module_tags().count())


I would prefer an implementation of Debug for the iterator if possible. In my experience, having module tags printed helps debugging. Also, you could try mapping over the iterator to Debug each element?

I absolutely agree with you. The behaviour should be equal to Vec::iter() which also prints all elements when debug-formatted. But with the current implementation of the ModuleIter I can't achieve this because of different compile time errors, like lifetime and ownership problems :/

phip1611 · 2021-07-05T16:33:09Z

Hi @IsaacWoods @Caduser2020
I tried to satisfy your review proposals. Unfortunately, I don't find a easy solution to print all elements when the iterator of for example ModuleTags is debug-formatted... there are problems with lifetimes and/or ownership all the time :/

I tried it to solve it like this:

impl <'a> Debug for ModuleIter<'a> {
    fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
        let mut list = f.debug_list();
        self.iter.for_each(|ref e| {
            list.entry(e);
        });
        list.finish()
    }
}

but this results in compile errors...

self.iter.for_each(|ref e| {
   |         ^^^^^^^^^ move occurs because `self.iter` has type `TagIter<'_>`, which does not implement the `Copy` trait

IsaacWoods · 2021-07-05T17:31:08Z

Hmm, could you use by_ref to solve that? Like this (note, untested):

impl <'a> Debug for ModuleIter<'a> {
    fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
        let mut list = f.debug_list();
        self.iter.by_ref().for_each(|ref e| {
            list.entry(e);
        });
        list.finish()
    }
}

phip1611 · 2021-07-05T20:20:19Z

Nope, by_ref consumes &mut self which we don't have here.

IsaacWoods · 2021-07-05T21:46:33Z

Might as well take the easy way out and clone it then, which should work and is trivially cheap. Also note that by accessing self.iter, you're going to print all of the tags - ModuleIter implements Iterator itself and filters out the non-module tags. I've used the correct iterator in my Playground example.

phip1611 · 2021-07-06T11:18:49Z

Yes it worked with .clone() @IsaacWoods . In addition, I implemented an enum which helps us to get a nice name for each tag type. The enum is directly taken from the multiboot2 spec and I was really carefully with replacing existing numeric literals with enum variants. What do you think?

Example debug output:

    Multiboot2 Boot Information {
        start_address: 0x000000000068e000,
        end_address: 0x00000000006f6960,
        total_size: 0x0000000000068960,
        boot_loader_name_tag: "GRUB 2.04-1ubuntu44.2",
        command_line: "",
        memory_areas: None,
        module_tags: [
            Tag {
                typ: 21,
                typ (name): MultibootTagTypeLoadBaseAddr,
                size: 12,
            },
            Tag {
                typ: 1,
                typ (name): MultibootTagTypeCmdline,
                size: 9,
            },
            Tag {
                typ: 2,
                typ (name): MultibootTagTypeBootLoaderName,
                size: 30,
            },
            Tag {
                typ: 9,
                typ (name): MultibootTagTypeElfSections,
                size: 428180,
            },
            Tag {
                typ: 12,
                typ (name): MultibootTagTypeEfi64,
                size: 16,
            },
            Tag {
                typ: 14,
                typ (name): MultibootTagTypeAcpiOld,
                size: 28,
            },
            Tag {
                typ: 15,
                typ (name): MultibootTagTypeAcpiNew,
                size: 44,
            },
            Tag {
                typ: 18,
                typ (name): MultibootTagTypeEfiBs,
                size: 8,
            },
            Tag {
                typ: 20,
                typ (name): MultibootTagTypeEfi64Ih,
                size: 16,
            },
        ],
        elf_sections_tags (count): 6689,
        efi_32_ih: None,
        efi_64_ih: Some(
            EFIImageHandle64 {
                typ: 20,
                size: 16,
                pointer: 114517528,
            },
        ),
        efi_sdt_32_tag: None,
        efi_sdt_64_tag: Some(
            EFISdt64 {
                typ: 12,
                size: 16,
                pointer: 127852568,
            },
        ),
        efi_memory_map_tag: None,
    }

src/elf_sections.rs

IsaacWoods

Thanks for working on this - great changes overall I think, and I like the explicit enum for tag types. Just a few straggling changes left.

As one last suggestion, before we merge, it might be nice to squash into fewer commits that collect changes to specific areas together. Are you comfortable performing an interactive rebase or would you prefer me to do this?

src/header.rs

src/lib.rs

src/module.rs

phip1611 · 2021-07-07T08:03:32Z

As one last suggestion, before we merge, it might be nice to squash into fewer commits that collect changes to specific areas together. Are you comfortable performing an interactive rebase or would you prefer me to do this?

No problem, I'm well familiar with this.

I adjusted everything. Hope I didn't miss anything. @IsaacWoods

IsaacWoods · 2021-07-07T12:12:09Z

Great - thanks @phip1611 for this work! I'll merge this and get it published when I get the chance.

IsaacWoods reviewed Jul 2, 2021

View reviewed changes

ghost reviewed Jul 3, 2021

View reviewed changes

ghost reviewed Jul 6, 2021

View reviewed changes

src/elf_sections.rs Outdated Show resolved Hide resolved

IsaacWoods requested changes Jul 7, 2021

View reviewed changes

src/header.rs Show resolved Hide resolved

src/header.rs Outdated Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

src/module.rs Outdated Show resolved Hide resolved

much improved debug output of BootInformation

03f2322

phip1611 changed the title ~~much improved debug output of BootInformation~~ much improved debug output of BootInformation + enum TagType Jul 7, 2021

introduced enum TagType

993d443

IsaacWoods approved these changes Jul 7, 2021

View reviewed changes

IsaacWoods merged commit abfecec into rust-osdev:master Jul 7, 2021

phip1611 deleted the debug-format-improved branch July 7, 2021 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

much improved debug output of BootInformation + enum TagType #76

much improved debug output of BootInformation + enum TagType #76

phip1611 commented Jul 2, 2021

IsaacWoods left a comment

IsaacWoods Jul 2, 2021

IsaacWoods Jul 2, 2021

ghost Jul 3, 2021

phip1611 Jul 5, 2021 •

edited

phip1611 commented Jul 5, 2021 •

edited

IsaacWoods commented Jul 5, 2021

phip1611 commented Jul 5, 2021

IsaacWoods commented Jul 5, 2021

phip1611 commented Jul 6, 2021 •

edited

IsaacWoods left a comment

phip1611 commented Jul 7, 2021 •

edited

IsaacWoods commented Jul 7, 2021

much improved debug output of BootInformation + enum TagType #76

much improved debug output of BootInformation + enum TagType #76

Conversation

phip1611 commented Jul 2, 2021

IsaacWoods left a comment

Choose a reason for hiding this comment

IsaacWoods Jul 2, 2021

Choose a reason for hiding this comment

IsaacWoods Jul 2, 2021

Choose a reason for hiding this comment

ghost Jul 3, 2021

Choose a reason for hiding this comment

phip1611 Jul 5, 2021 • edited

Choose a reason for hiding this comment

phip1611 commented Jul 5, 2021 • edited

IsaacWoods commented Jul 5, 2021

phip1611 commented Jul 5, 2021

IsaacWoods commented Jul 5, 2021

phip1611 commented Jul 6, 2021 • edited

IsaacWoods left a comment

Choose a reason for hiding this comment

phip1611 commented Jul 7, 2021 • edited

IsaacWoods commented Jul 7, 2021

phip1611 Jul 5, 2021 •

edited

phip1611 commented Jul 5, 2021 •

edited

phip1611 commented Jul 6, 2021 •

edited

phip1611 commented Jul 7, 2021 •

edited