Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment

["Abner Chang" <abner.chang@hpe.com>]

I don't prefer to do this at this moment, Leif. I have no problem if we create a BZ for this and create BaseCompilerIntrinsicsLib in MdeModulePkg, but please don't bind it with RISC-V EDK2 port. You know my concern pretty well, we can't hold those RISC-V patches on hands like forever and address the code structure optimization. We can still work on BaseCompilerIntrinsicsLib but not part of this RISC-V submission. We can implement RISC-V variant if necessary after RISC-V edk2 get in edk2 master.

Abner

> -----Original Message-----
> From: Leif Lindholm [mailto:leif@nuviainc.com]
> Sent: Thursday, March 12, 2020 10:03 PM
> To: Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>
> Cc: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>;
> afish@apple.com; michael.d.kinney@intel.com; pete@akeo.ie; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> instead of GUID assignment
> 
> +Ard, Laszlo.
> 
> I think it would make sense to move it to MdeModulePkg (or MdePkg) and
> rename it BaseCompilerIntrinsicsLib (it *is* a BASE library).
> 
> As I alluded in my reply to Ray - x86 also have this problem, but to a lesser
> extent, and ended up creating library functions to call instead of using plain C
> for certain operations.
> Which was probably the right decision when it was restricted to a very few
> corner cases.
> 
> /
>     Leif
> 
> On Thu, Mar 12, 2020 at 13:24:30 +0000, Schaefer, Daniel (DualStudy) wrote:
> > Hi Leif,
> >
> > you're right. If I revert my commit and include
> >   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > without making any changes to it, the build succeeds.
> >
> > What do others think? (cc Michael, Pete, Andrew, Ard, who have made
> > changes to this module) Is this a big hack or should we use it in RISC-V, too
> and move the module to MdeModulePkg?
> > Why isn't this a problem on x86? Was it fine on Itanium?
> >
> > - Daniel
> >
> > ________________________________
> > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > <devel@edk2.groups.io><mailto:devel@edk2.groups.io> on behalf of Leif
> > Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > Sent: Thursday, March 12, 2020 11:55
> > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; Schaefer, Daniel
> > (DualStudy)
> <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > Cc: Chang, Abner (HPS SW/FW Technologist)
> > <abner.chang@hpe.com><mailto:abner.chang@hpe.com>; Chen, Gilbert
> > <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>; Dandan Bi
> > <dandan.bi@intel.com><mailto:dandan.bi@intel.com>; Eric Dong
> > <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem
> > instead of GUID assignment
> >
> > Hi Daniel,
> >
> > There is nothing wrong with this patch that just went in (and I should
> > have called out sooner if I wanted to stop it), but I think a better
> > solution is to implement a RISC-V variant of
> > ArmPkg/Library/CompilerIntrinsicsLib/.
> >
> > It is perfectly valid for the compiler to generate memcpy calls in
> > response to struct operations that are perfectly valid C.
> >
> > In fact, we could consider moving the ArmPkg one over into
> > MdeModulePkg. I have a feeling that including a
> >   NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
> > in your current build would be an alternative solution to your
> > compilation error.
> >
> > /
> >     Leif
> >
> > On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote:
> > > GCC translates a simple assignment to memcpy, which EDKII doesn't
> provide.
> > > See:
> > > INVALID URI REMOVED
> 2Darch
> > > ive.com_edk2-2Ddevel-
> 40lists.01.org_msg11928.html&d=DwIBAg&c=C5b8zRQ
> > >
> O1miGmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3
> E&m=wjlf
> > >
> QYZsXfc5WWDFmywLOEYlCKvrpYrnaXMIpJcLK7U&s=426yv7VvgHTtgtYaR0f0X
> NiaEA
> > > LgqZkxektjywEwPco&e=
> > >
> > > REF:INVALID URI REMOVED
> > > anocore.org_show-5Fbug.cgi-3Fid-
> 3D2547&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2
> > >
> LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=wjlfQYZsX
> fc5WWD
> > > FmywLOEYlCKvrpYrnaXMIpJcLK7U&s=PjC_mIwh0GhNy5np2h7K8l4l-
> pLLBWt1FRuLB
> > > UbULpc&e=
> > >
> > > Signed-off-by: Daniel Schaefer
> > > <daniel.schaefer@hpe.com><mailto:daniel.schaefer@hpe.com>
> > > Cc: Abner Chang
> <abner.chang@hpe.com><mailto:abner.chang@hpe.com>
> > > Cc: Gilbert Chen
> <gilbert.chen@hpe.com><mailto:gilbert.chen@hpe.com>
> > > Cc: Leif Lindholm <leif@nuviainc.com><mailto:leif@nuviainc.com>
> > > Cc: Dandan Bi <dandan.bi@intel.com><mailto:dandan.bi@intel.com>
> > > Cc: Eric Dong <eric.dong@intel.com><mailto:eric.dong@intel.com>
> > > ---
> > >
> > > Notes:
> > >     v2:
> > >       - Use CopyMem instead of CopyGuid [Dandan]
> > >
> > >  MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git
> a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > index 5cc527679a78..0540e6fa8a44 100644
> > > --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c
> > > @@ -619,7 +619,7 @@ CreateDeviceManagerForm(
> > >          TokenHelp = HiiSetString (HiiHandle, 0, String, NULL);
> > >          FreePool (String);
> > >
> > > -        FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid;
> > > +        CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid,
> > > + sizeof (EFI_GUID));
> > >
> > >          //
> > >          // Network device process
> > > --
> > > 2.25.0
> > >
> > >
> > >
> > >
> >
> > 
> >