repr(C) is necessary
le-jzr opened this issue · 7 comments
Currently, several structures use #[repr(packed)]. This works merely by coincidence, since packed is just a modifier, not a standalone representation (equivalent to #[repr(rust, packed)]). Correct annotation should be #[repr(C, packed)].
Even better would be to fix the padding by fixing structure fields, since packed exists to allow misaligned field accesses (i.e. special code in place of every load and store), which is totally unnecessary for aligned multiboot data.
Some discussion about the alignment of ElfSectionsTag: 0010836#commitcomment-21738992
Most important bit:
Hmmm, this seems wrong... Are we absolutely sure the structure layout is correct? As is, 64b fields in the ELF64 section table are misaligned, which feels like a very wrong thing to do.
In general, I agree that we should replace all instances of #[repr(packed)] with #[repr(C, packed)], especially since struct field reordering optimizations are available in nightly now.
If it turns out that the structure layout is really what GRUB2 gives us currently (note that the layout as specified in the Multiboot specification doesn't have this issue), then I'd strongly suggest we just ignore the ElfSectionsTag entirely for ELF64, for safety reasons. Or provide a way to use it safely (as in, copy elsewhere).
Is it possible that grub actually gives the info in u32 fields even for ELF64? I can't check it right now.
As most of all the #[repr(packed) instances has been replaced with #[repr(C, packed)] I think this issue is solved. If there is another issue. Like with the commitcomment linked earlier, open another issue thread. If you think otherwise, just comment on this thread. If there is no input for a few days, then I'll close the issue.
I see a few structs that are still #[repr(packed)]. I'll make a pull request to fix those.
Pull request is ready to be merged. I'm going to close this. Reopen if necessary.