From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 9BDB221E95E15 for ; Wed, 30 Aug 2017 05:46:21 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Aug 2017 05:49:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,448,1498546800"; d="scan'208";a="895517454" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by FMSMGA003.fm.intel.com with ESMTP; 30 Aug 2017 05:49:01 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 30 Aug 2017 05:49:01 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 30 Aug 2017 05:49:01 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.117]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002; Wed, 30 Aug 2017 20:48:59 +0800 From: "Ni, Ruiyu" To: "Wu, Hao A" , "Kinney, Michael D" , Ard Biesheuvel CC: "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH] SourceLevelDebugPkg/DebugAgentCommon: Use global variable for revision Thread-Index: AQHTIMgMxOz9gHTpREOVxqZaj5Ln96Ka56yAgAAQfICAAUIEgIAAoMnQ Date: Wed, 30 Aug 2017 12:48:58 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BA19E3D@SHSMSX104.ccr.corp.intel.com> References: <20170829130926.17396-1-hao.a.wu@intel.com> In-Reply-To: Accept-Language: en-US, zh-CN 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.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] SourceLevelDebugPkg/DebugAgentCommon: Use global variable for revision X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 Aug 2017 12:46:21 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Yes. I agree with Mike's idea to use FixedAtBuild PCD for the debug agent t= ransfer protocol revision. -----Original Message----- From: Wu, Hao A=20 Sent: Wednesday, August 30, 2017 7:13 PM To: Kinney, Michael D ; Ard Biesheuvel Cc: Ni, Ruiyu ; edk2-devel@lists.01.org Subject: RE: [edk2] [PATCH] SourceLevelDebugPkg/DebugAgentCommon: Use globa= l variable for revision Thanks a lot for the feedbacks. I will switch to using PCD for configuring revision and send out V2 of the = patch. Best Regards, Hao Wu > -----Original Message----- > From: Kinney, Michael D > Sent: Wednesday, August 30, 2017 12:00 AM > To: Ard Biesheuvel; Wu, Hao A; Kinney, Michael D > Cc: Ni, Ruiyu; edk2-devel@lists.01.org > Subject: RE: [edk2] [PATCH] SourceLevelDebugPkg/DebugAgentCommon: Use=20 > global variable for revision >=20 > Ard, >=20 > I agree. Adding const would be an improvement too. >=20 > But I would prefer to not add a global variable at all. >=20 > When could this check ever evaluate to false? Compression has been=20 > supported for a long time now. >=20 > It appears that a developer would have to modify=20 > SourceLevelDebugPkg\Include\TransferProtocol.h to select an older=20 > protocol revision. >=20 > If we really want build configuration of the protocol revision then we=20 > should use a PCD feature flag or a FixedAtBuild PCD to allow the=20 > developer to configure it from the DSC file. >=20 > Mike >=20 > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf=20 > > Of Ard Biesheuvel > > Sent: Tuesday, August 29, 2017 8:01 AM > > To: Wu, Hao A > > Cc: Ni, Ruiyu ; edk2-devel@lists.01.org > > Subject: Re: [edk2] [PATCH] > > SourceLevelDebugPkg/DebugAgentCommon: Use global variable for=20 > > revision > > > > On 29 August 2017 at 14:09, Hao Wu wrote: > > > Since some static code checkers complain that comparing two > > macros will > > > generate a constant result, this commit uses a global > > variable to > > > reflect the revision information of the debug agent, rather > > than using a > > > macro directly. > > > > > > Cc: Ruiyu Ni > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Hao Wu > > > --- > > > .../Library/DebugAgent/DebugAgentCommon/DebugAgent.c > > | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git > > a/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/Debug > > Agent.c > > b/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/Debug > > Agent.c > > > index f156fe24db..93af009f96 100644 > > > --- > > a/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/Debug > > Agent.c > > > +++ > > b/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/Debug > > Agent.c > > > @@ -130,7 +130,8 @@ GLOBAL_REMOVE_IF_UNREFERENCED > > EFI_VECTOR_HANDOFF_INFO mVectorHandoffInfoDebugAge > > > } > > > }; > > > > > > -GLOBAL_REMOVE_IF_UNREFERENCED UINTN mVectorHandoffInfoCount > > =3D sizeof (mVectorHandoffInfoDebugAgent) / sizeof=20 > > (EFI_VECTOR_HANDOFF_INFO); > > > +GLOBAL_REMOVE_IF_UNREFERENCED UINTN mVectorHandoffInfoCount > > =3D sizeof (mVectorHandoffInfoDebugAgent) / sizeof=20 > > (EFI_VECTOR_HANDOFF_INFO); > > > +GLOBAL_REMOVE_IF_UNREFERENCED UINT32 mDebugAgentRevision > > =3D DEBUG_AGENT_REVISION; > > > > > > /** > > > Calculate CRC16 for target data. > > > @@ -1564,7 +1565,7 @@ ReadMemoryAndSendResponsePacket ( > > > // Compression/decompression support was added since > > revision 0.4. > > > // Revision 0.3 shouldn't compress the packet. > > > // > > > - if (DEBUG_AGENT_REVISION >=3D DEBUG_AGENT_REVISION_04) { > > > + if (mDebugAgentRevision >=3D DEBUG_AGENT_REVISION_04) { > > > // > > > // Get the compressed data size without modifying the > > packet. > > > // > > > @@ -2192,7 +2193,7 @@ CommandCommunication ( > > > break; > > > > > > case DEBUG_COMMAND_GET_REVISION: > > > - DebugAgentRevision.Revision =3D DEBUG_AGENT_REVISION; > > > + DebugAgentRevision.Revision =3D mDebugAgentRevision; > > > DebugAgentRevision.Capabilities =3D > > DEBUG_AGENT_CAPABILITIES; > > > Status =3D SendDataResponsePacket ((UINT8 *) > > &DebugAgentRevision, (UINT16) sizeof=20 > > (DEBUG_DATA_RESPONSE_GET_REVISION), DebugHeader); > > > break; > > > -- > > > 2.12.0.windows.1 > > > > > > > Can we *please* fix the tools instead? mDebugAgentRevision has=20 > > external linkage, and so the compiler is forced to perform this=20 > > check at runtime instead of at build time. Or at least use a STATIC=20 > > variable so the compiler can see that mDebugAgentRevision is never=20 > > modified from its default value. > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel