Re: [edk2-devel] [Patch v2] MdeModulePkg: Initialize local variable value before they are used

["Laszlo Ersek" <lersek@redhat.com>]

On 03/18/21 05:50, gaoliming wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3228
> This change is to fix the false compiler error on GCC49 release build.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn>
> Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  In V2, add the comments for the false compiler warning
> 
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c             | 5 +++++
>  .../VariablePolicyHelperLib/VariablePolicyHelperLib.c        | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> index e99a812a44..1053695b3b 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> @@ -1127,6 +1127,11 @@ AhciDmaTransfer (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> +  //
> +  // Set Status to suppress incorrect compiler/analyzer warnings
> +  //
> +  Status = EFI_SUCCESS;
> +
>    //
>    // DMA buffer allocation. Needs to be done only once for both sync and async
>    // DMA transfers irrespective of number of retries.

Yes, it's indeed a compiler problem. In the middle of the function, we
have control flow like this:

  if (Task == NULL) {
    for (Retry = 0; Retry < AHCI_COMMAND_RETRIES; Retry++) {
      Status = AhciStartCommand (...);
    }
  } else {
    if (!Task->IsStart) {
      Status = AhciStartCommand (...);
    }
    if (Task->IsStart) {
      Status = AhciCheckFisReceived (...);
    }
  }

There is no path through this without setting "Status", so the
subsequent warning is indeed unjustified.


> diff --git a/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c b/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
> index 0c9299c8b0..6bcb95247f 100644
> --- a/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
> +++ b/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
> @@ -115,6 +115,11 @@ CreateBasicVariablePolicy (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> +  //
> +  // Set NameSize to suppress incorrect compiler/analyzer warnings
> +  //
> +  NameSize  = 0;
> +
>    // Now we've gotta determine the total size of the buffer required for
>    // the VariablePolicy structure.
>    TotalSize = sizeof( VARIABLE_POLICY_ENTRY );
> 

Also correct: NameSize is only read if (Name != NULL), but in that case,
NameSize is also set. And Name does not change between the two checks.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo