From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.151; helo=mga17.intel.com; envelope-from=michael.d.kinney@intel.com; receiver=edk2-devel@lists.01.org Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9F0B922729604 for ; Thu, 12 Apr 2018 09:47:19 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Apr 2018 09:47:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,442,1517904000"; d="scan'208";a="219936702" Received: from orsmsx108.amr.corp.intel.com ([10.22.240.6]) by fmsmga006.fm.intel.com with ESMTP; 12 Apr 2018 09:47:18 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.55]) by ORSMSX108.amr.corp.intel.com ([169.254.2.171]) with mapi id 14.03.0319.002; Thu, 12 Apr 2018 09:47:18 -0700 From: "Kinney, Michael D" To: Laszlo Ersek , "Bi, Dandan" , "edk2-devel@lists.01.org" , "Kinney, Michael D" CC: "Dong, Eric" Thread-Topic: [edk2] [patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx" Thread-Index: AQHT0jtpEJeu79bhiEaiSVsDAyFsNqP9U6cAgAADfJA= Date: Thu, 12 Apr 2018 16:47:17 +0000 Message-ID: References: <20180412085014.107784-1-dandan.bi@intel.com> <7dd6846f-fe34-ad7b-ae20-dc7d55ef84f5@redhat.com> In-Reply-To: <7dd6846f-fe34-ad7b-ae20-dc7d55ef84f5@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.22.254.139] MIME-Version: 1.0 Subject: Re: [patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx" X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Apr 2018 16:47:19 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, I think I would rather see the ECC tool fixed. 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 ; edk2- > devel@lists.01.org > Cc: Dong, Eric > Subject: Re: [edk2] [patch] UefiCpuPkg/PiSmmCpuDxeSmm: > Add "extern" keyword for "gPatchxxx" >=20 > Hello Dandan, >=20 > 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 > > Cc: Laszlo Ersek > > Contributed-under: TianoCore Contribution Agreement > 1.1 > > Signed-off-by: Dandan Bi > > --- > > 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(-) >=20 > This is a bug (a false positive) in the ECC tool. The > following > declaration: >=20 > > X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr0; >=20 > 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"): >=20 > > /// > > /// 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); >=20 > That is, when you see >=20 > > X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr0; >=20 > That is identical to the following function > declaration: >=20 > > VOID gPatchSmmCr0 (VOID); >=20 > Now, the ISO C99 standard says: >=20 > > 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/. [...] >=20 > Thus, the report from ECC is a false positive. >=20 > 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. >=20 > Can you please submit v2 with an updated commit > message? >=20 > Thanks! > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel