Re: [PATCH] MdeModulePkg/FileExplorerLib: avoid packed struct for program data

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 11 Dec 2018 at 02:48, Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > Sent: Saturday, December 08, 2018 6:25 PM
> > To: edk2-devel@lists.01.org
> > Cc: Wang, Jian J; Wu, Hao A; Ni, Ruiyu; Ard Biesheuvel
> > Subject: [PATCH] MdeModulePkg/FileExplorerLib: avoid packed struct for
> > program data
> >
> > Struct packing is only necessary for data structures whose in-memory
> > representation is covered by the PI or UEFI specs, and may deviate
> > from the ordinary C rules for alignment.
> >
> > So in case of FileExplorerLib, this applies to the device path struct
> > only, and other structures used to carry program data should not be
> > packed, or we may end up with alignment faults on architectures such
> > as ARM, which don't permit load/store double or multiple instructions
> > to access memory locations that are not 32-bit aligned.
> >
> > E.g., the following call in FileExplorerLibConstructor()
> >
> >   InitializeListHead (&gFileExplorerPrivate.FsOptionMenu->Head);
> >
> > which is emitted as follows for 32-bit ARM/Thumb2 by Clang-5.0
> >
> >     3de0:       b510            push    {r4, lr}
> >     3de2:       4604            mov     r4, r0
> >     ...
> >     3de8:       e9c4 4400       strd    r4, r4, [r4]
> >     3dec:       bd10            pop     {r4, pc}
> >
> > will perform a double-word store on the first argument, passed in
> > register r0, assuming that the pointer type of the argument is
> > enough to guarantee that the value is suitably aligned.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  MdeModulePkg/Library/FileExplorerLib/FileExplorer.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.h
> > b/MdeModulePkg/Library/FileExplorerLib/FileExplorer.h
> > index bf1450dbd581..603185abe4b1 100644
> > --- a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.h
> > +++ b/MdeModulePkg/Library/FileExplorerLib/FileExplorer.h
> > @@ -51,6 +51,8 @@ typedef struct {
> >    EFI_DEVICE_PATH_PROTOCOL       End;
> >  } HII_VENDOR_DEVICE_PATH;
> >
> > +#pragma pack()
> > +
> >  typedef struct {
> >    EFI_HANDLE                        DeviceHandle;
> >    EFI_DEVICE_PATH_PROTOCOL          *DevicePath;
> > @@ -100,8 +102,6 @@ typedef struct {
> >
> >  #define FILE_EXPLORER_PRIVATE_FROM_THIS(a)  CR (a,
> > FILE_EXPLORER_CALLBACK_DATA, FeConfigAccess,
> > FILE_EXPLORER_CALLBACK_DATA_SIGNATURE)
> >
> > -#pragma pack()
> > -
>
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>
>
> Best Regards,
> Hao Wu
>

Thanks all

Pushed as  f0574a194c1e..765fb87c2b70