public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>,
	"Bi, Dandan" <dandan.bi@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Dong, Eric" <eric.dong@intel.com>
Subject: Re: [patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx"
Date: Thu, 12 Apr 2018 19:51:21 +0200	[thread overview]
Message-ID: <8ddbdeb8-57c8-1352-1ae7-c2a0047a35a3@redhat.com> (raw)
In-Reply-To: <E92EE9817A31E24EB0585FDF735412F5B89CC7B7@ORSMSX113.amr.corp.intel.com>

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



  reply	other threads:[~2018-04-12 17:51 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-12  8:50 [patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx" Dandan Bi
2018-04-12  9:34 ` Laszlo Ersek
2018-04-12 16:47   ` Kinney, Michael D
2018-04-12 17:51     ` Laszlo Ersek [this message]
2018-04-13  1:33       ` Bi, Dandan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8ddbdeb8-57c8-1352-1ae7-c2a0047a35a3@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox