From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 29B1A21DF806E for ; Tue, 29 Aug 2017 08:59:20 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Aug 2017 09:01:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,445,1498546800"; d="scan'208";a="145221978" Received: from orsmsx104.amr.corp.intel.com ([10.22.225.131]) by fmsmga005.fm.intel.com with ESMTP; 29 Aug 2017 09:01:25 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.25]) by ORSMSX104.amr.corp.intel.com ([169.254.4.142]) with mapi id 14.03.0319.002; Tue, 29 Aug 2017 09:00:21 -0700 From: "Kinney, Michael D" To: Ard Biesheuvel , "Wu, Hao A" , "Kinney, Michael D" CC: "Ni, Ruiyu" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH] SourceLevelDebugPkg/DebugAgentCommon: Use global variable for revision Thread-Index: AQHTIMgWaPCL1dlCH0m02n9ZgIOnv6Kb4yGA//+ZV2A= Date: Tue, 29 Aug 2017 16:00:21 +0000 Message-ID: References: <20170829130926.17396-1-hao.a.wu@intel.com> In-Reply-To: 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.140] 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: Tue, 29 Aug 2017 15:59:20 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Ard, I agree. Adding const would be an improvement too. But I would prefer to not add a global variable at all. When could this check ever evaluate to false? Compression has been supported for a long time now. It appears that a developer would have to modify SourceLevelDebugPkg\Include\TransferProtocol.h to select an older protocol revision. 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. Mike > -----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 >=20 > 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 > > >=20 > 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