Re: [PATCH edk2-platforms 09/27] Silicon/NXP: Add headers for SoC-specific i.MX packages to use

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Dec 04, 2018 at 10:33:23AM +0100, Ard Biesheuvel wrote:
> >>>>> +#pragma pack(push, 1)
> >>>> 
> >>>> I don't see this #pragma making any difference to the structs below,
> >>>> can it be dropped?
> >>> 
> >>> The pragma pack is defensive. Without it, we rely on the compiler
> >>> packing structures by default and this may not happen on 64 bit
> >>> compiles.
> >> 
> >> I understand that is what the pragma does. My comment was because all of the
> >> variables in the below structs are naturally aligned.
> >> The reason I dislike its use when effectively a no-op, is that it makes it look like it
> >> it isn't a no-op.
> >> 
> >> If it covers a larger set of structs, some of which require the packed attribute I'm
> >> OK with that. But I'm not a fan of adding it "just in case" without contemplating
> >> the statement's (lack of) effect.
> >> 
> >> Regards,
> >> 
> >> Leif
> >> 
> > 
> > Makes sense. I am checking to make sure this pragma wasn't
> > included due to some observed compiler behavior on our end, since
> > this header is also used on our ARM64 work.
> > Will remove it once confirmed it is safe.
> 
> I’d prefer to keep the pragma. It doesn’t only remove padding, it
> also informs the compiler that the whole struct may appear at any
> unaligned offset.
> On 32 bit ARM, this means the compiler will
> refrain from using load/store double or load/store multiple to
> access the contents when dereferencing a pointer to such a struct,
> which would result in a crash otherwise.

OK, if it is a real concern that the structs themselves may appear
unaligned in memory, please ignore this feedback.

/
    Leif