On Mon, Oct 03 2016, James Clarke wrote: > Hi Neil, > >> On 2 Oct 2016, at 23:32, NeilBrown <neilb@xxxxxxxx> wrote: >> >>> On Thu, Sep 29 2016, James Clarke wrote: >>> >>> Since the MBR layout only has partition records as 2-byte aligned, the 32-bit >>> fields in them are not aligned. Thus, they cannot be accessed on some >>> architectures (such as SPARC) by using a "struct MBR_part_record *" pointer, >>> as the compiler can assume that the pointer is properly aligned. Instead, the >>> records must be accessed by going through the MBR struct itself every time. >> >> Weird.... >> >> Can you see if adding "__attribute__((packed))" to struct >> MBR_part_record also fixes the problem? > > That also works. When I wrote the patch initially, I wasn’t sure if it was a > "correct" fix, but having looked into it more I *believe* it is conformant. The > alignment of a packed struct is 1-byte, so, while the compiler may know that the > 32-bit fields are 8-byte aligned within the struct, the pointer to the struct > need not be aligned, and so the correct conservative code is generated. > >> It seems strange that the compiler lets you take a pointer, but then >> doesn't use it correctly. Maybe it is an inconsistency in the types. > > Yes, the type doesn’t include the provenance of the pointer, so in general the > compiler can’t know it came from a packed struct (although in this case not much > static analysis would be needed). See [1] and [2]. > >> I don't necessarily disagree with your fix, but I'd like to understand >> why the current code is wrong. > > Hopefully the links make it clearer. > > Regards, > James > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628 > [2] https://llvm.org/bugs/show_bug.cgi?id=22821 Thanks. It looks as though the type change I suggested would work, but probably isn't the best solution. Your patch is probably safest, though adding the __attribute__((packed)) as well wouldn't hurt. I'll leave it for Jes to decide what exactly to apply, but I can offer a Reviewed-by: NeilBrown <neilb@xxxxxxxx> for you patch. BTW I tried compiling mdadm with clang to see if my clang was new enough to give a warning (it isn't) but it found a few other things to give errors about ... I should post patches. Thanks, NeilBrown
Attachment:
signature.asc
Description: PGP signature
