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.d...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Dandan Bi <dandan...@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
