Re: [patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx"

["Bi, Dandan" <dandan.bi@intel.com>]

Thanks Laszlo for the detailed description.
I will communicate will ECC owner to see whether ECC tool can be enhanced for these issues firstly. 
Thank you all.

Regards,
Dandan
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, April 13, 2018 1:51 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx"

On 04/12/18 18:47, Kinney, Michael D wrote:
> Laszlo,
> 
> I think I would rather see the ECC tool fixed.

I didn't dare suggest that, but I agree it's a superior solution. When I tried ECC last time, I was surprised how powerful it was, so if it recognized even this case, that would certainly fit its quality :)

Thanks!
Laszlo

> 
> Mike
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-
>> bounces@lists.01.org] On Behalf Of Laszlo Ersek
>> Sent: Thursday, April 12, 2018 2:34 AM
>> To: Bi, Dandan <dandan.bi@intel.com>; edk2- devel@lists.01.org
>> Cc: Dong, Eric <eric.dong@intel.com>
>> Subject: Re: [edk2] [patch] UefiCpuPkg/PiSmmCpuDxeSmm:
>> Add "extern" keyword for "gPatchxxx"
>>
>> Hello Dandan,
>>
>> On 04/12/18 10:50, Dandan Bi wrote:
>>> Background description:
>>> In SmmProfileInternal.h, ECC check tool report an
>> issue at line 103.
>>> Detailed ECC Error info:Variable definition appears
>> in header file.
>>> Include files should contain only public or only
>> private data and
>>> cannot contain code or define data variables
>>>
>>> ECC report similar issues in PiSmmCpuDxeSmm.h.
>>>
>>> Then we review all the new introduced "gPatchxxx",
>> since they have
>>> been defined in the nasm file, we can add "extern"
>> keyword for them
>>> in the C source or header files.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement
>> 1.1
>>> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
>>> ---
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h     | 8
>> ++++----
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 2
>> +-
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c     | 6
>> +++---
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c      | 4
>> ++--
>>>  4 files changed, 10 insertions(+), 10 deletions(-)
>>
>> This is a bug (a false positive) in the ECC tool. The following
>> declaration:
>>
>>> X86_ASSEMBLY_PATCH_LABEL            gPatchSmmCr0;
>>
>> does not declare an *object* (a variable). Instead, it declares a
>> *function* (and not a pointer to a function!), because (from
>> "MdePkg/Include/Library/BaseLib.h"):
>>
>>> ///
>>> /// Type definition for representing labels in NASM
>> source code that allow for
>>> /// the patching of immediate operands of IA32 and
>> X64 instructions.
>>> ///
>>> /// While the type is technically defined as a
>> function type (note: not a
>>> /// pointer-to-function type), such labels in NASM
>> source code never stand for
>>> /// actual functions, and identifiers declared with
>> this function type should
>>> /// never be called. This is also why the EFIAPI
>> calling convention specifier
>>> /// is missing from the typedef, and why the typedef
>> does not follow the usual
>>> /// edk2 coding style for function (or pointer-to-
>> function) typedefs. The VOID
>>> /// return type and the VOID argument list are merely
>> artifacts.
>>> ///
>>> typedef VOID (X86_ASSEMBLY_PATCH_LABEL) (VOID);
>>
>> That is, when you see
>>
>>> X86_ASSEMBLY_PATCH_LABEL            gPatchSmmCr0;
>>
>> That is identical to the following function
>> declaration:
>>
>>> VOID gPatchSmmCr0 (VOID);
>>
>> Now, the ISO C99 standard says:
>>
>>> 6.2.2 Linkages of identifiers
>>>
>>> [...]
>>>
>>>   5 If the declaration of an identifier for a
>> function has no
>>>     storage-class specifier, its linkage is
>> determined exactly as if
>>>     it were declared with the storage-class specifier
>> /extern/. [...]
>>
>> Thus, the report from ECC is a false positive.
>>
>> I don't mind the patch (the changes don't make any difference at the 
>> C-language level, see the spec above); however, the commit message 
>> should be 100% clear that the patch works around a limitation with 
>> the ECC tool.
>>
>> Can you please submit v2 with an updated commit message?
>>
>> Thanks!
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel