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 9B30D21E95E14 for ; Wed, 30 Aug 2017 04:10:15 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Aug 2017 04:12:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,448,1498546800"; d="scan'208";a="146235196" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga006.fm.intel.com with ESMTP; 30 Aug 2017 04:12:56 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 30 Aug 2017 04:12:56 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.117]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.168]) with mapi id 14.03.0319.002; Wed, 30 Aug 2017 19:12:54 +0800 From: "Wu, Hao A" To: "Kinney, Michael D" , Ard Biesheuvel CC: "Ni, Ruiyu" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH] SourceLevelDebugPkg/DebugAgentCommon: Use global variable for revision Thread-Index: AQHTIMgMUdv7Jlqh+E2uTmQYLG2IvKKa56yAgAAQfICAAcebcA== Date: Wed, 30 Aug 2017 11:12:53 +0000 Message-ID: References: <20170829130926.17396-1-hao.a.wu@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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 11:10:15 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 > 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 supported for a long time now. >=20 > It appears that a developer would have to modify > SourceLevelDebugPkg\Include\TransferProtocol.h to select an > older protocol revision. >=20 > If we really want build configuration of the protocol revision > then we should use a PCD feature flag or a FixedAtBuild PCD to > allow the 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 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 > > 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 > > (EFI_VECTOR_HANDOFF_INFO); > > > +GLOBAL_REMOVE_IF_UNREFERENCED UINTN mVectorHandoffInfoCount > > =3D sizeof (mVectorHandoffInfoDebugAgent) / sizeof > > (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 > > (DEBUG_DATA_RESPONSE_GET_REVISION), DebugHeader); > > > break; > > > -- > > > 2.12.0.windows.1 > > > > > > > Can we *please* fix the tools instead? mDebugAgentRevision has > > external linkage, and so the compiler is forced to perform this > > check > > at runtime instead of at build time. Or at least use a STATIC > > variable > > so the compiler can see that mDebugAgentRevision is never > > modified > > from its default value. > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel