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

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

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
> > > >
> > > >
> > > >
> > > >
> > >
> > > 
> > >