On Mon, Oct 03 2016, James Clarke wrote:
Hi Neil,
quoted
On 2 Oct 2016, at 23:32, NeilBrown [off-list ref] wrote:
quoted
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.
quoted
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].
quoted
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 [off-list ref]
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