Re: [PATCH] SourceLevelDebugPkg/DebugAgentCommon: Use global variable for revision

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 29 August 2017 at 14:09, Hao Wu <hao.a.wu@intel.com> 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 <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>  .../Library/DebugAgent/DebugAgentCommon/DebugAgent.c               | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugAgent.c b/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugAgent.c
> index f156fe24db..93af009f96 100644
> --- a/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugAgent.c
> +++ b/SourceLevelDebugPkg/Library/DebugAgent/DebugAgentCommon/DebugAgent.c
> @@ -130,7 +130,8 @@ GLOBAL_REMOVE_IF_UNREFERENCED EFI_VECTOR_HANDOFF_INFO mVectorHandoffInfoDebugAge
>    }
>  };
>
> -GLOBAL_REMOVE_IF_UNREFERENCED UINTN mVectorHandoffInfoCount = sizeof (mVectorHandoffInfoDebugAgent) / sizeof (EFI_VECTOR_HANDOFF_INFO);
> +GLOBAL_REMOVE_IF_UNREFERENCED UINTN  mVectorHandoffInfoCount = sizeof (mVectorHandoffInfoDebugAgent) / sizeof (EFI_VECTOR_HANDOFF_INFO);
> +GLOBAL_REMOVE_IF_UNREFERENCED UINT32 mDebugAgentRevision     = 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 >= DEBUG_AGENT_REVISION_04) {
> +    if (mDebugAgentRevision >= 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 = DEBUG_AGENT_REVISION;
> +      DebugAgentRevision.Revision = mDebugAgentRevision;
>        DebugAgentRevision.Capabilities = DEBUG_AGENT_CAPABILITIES;
>        Status = 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.