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

["Leif Lindholm" <leif@nuviainc.com>]

For clarity, I'm suggesting *I* take care of moving this into a
generic Intrinsics handling lib, and that HPE take care of including
and using it.

I'm also not suggesting we revert the CopyMem patch before that is
complete.

On Thu, Mar 12, 2020 at 14:44:52 +0000, Leif Lindholm wrote:
> And what would you propose we do the next time the RISC-V toolchain
> generates a memcpy call based on some other completely valid change to
> core code?
> 
> Doing this de-risks the RISC-V upstreaming effort.
> It's also a trivial move/rename opetation - the only question is where
> the library should live and be called.
> 
> /
>     Leif
> 
> On Thu, Mar 12, 2020 at 14:33:27 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
> > 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
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > > 
> > > >