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

["Laszlo Ersek" <lersek@redhat.com>]

On 03/12/20 15:44, 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?

We could choose to enable the intrinsics library for RISC-V at that point.

IIUC, the CreateDeviceManagerForm() code in question did break an edk2
rule ("don't use structure assignment") *prior* to commit 64a228f5f893.
The rule violation was in commit 32465d9ae7ee; RISC-V only exposed it.
This doesn't seem uncharted territory.

Thanks
Laszlo

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