[PATCH v1 0/2] Fix SRAT generator warnings repoted by VS2017 compiler

[PATCH v1 0/2] Fix SRAT generator warnings repoted by VS2017 compiler

[Sami Mujawar]

This patch series fixes issues reported in the SRAT generator by the
VS2017 compiler with the static analysis option enabled.

The changes can be seen at:
https://github.com/samimujawar/edk2/tree/702_srat_vs2017_compile_warning_v1

Sami Mujawar (2):
  DynamicTablesPkg: SRAT: Fix entry points
  DynamicTablesPkg: SRAT: Fix uninitialized memory usage

 DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 1/2] DynamicTablesPkg: SRAT: Fix entry points

[Sami Mujawar]

VS2017 reports 'warning C4028: formal parameter 2 different
from declaration' for the library constructor and destructor
interfaces for the SRAT Generator modules.

Remove the CONST qualifier for the SystemTable pointer (the
second parameter to the constructor/destructor/DXE Entry
point) to make it compatible with the formal declaration.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
index 5d56af66608d862e6eca81da812d719f110867d2..74cb7d92a5d8cddd3df8334f3ab55e6fa3e7267a 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
@@ -800,8 +800,8 @@ ACPI_TABLE_GENERATOR SratGenerator = {
 EFI_STATUS
 EFIAPI
 AcpiSratLibConstructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
@@ -823,8 +823,8 @@ AcpiSratLibConstructor (
 EFI_STATUS
 EFIAPI
 AcpiSratLibDestructor (
-  IN CONST EFI_HANDLE                ImageHandle,
-  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
+  IN  EFI_HANDLE           ImageHandle,
+  IN  EFI_SYSTEM_TABLE  *  SystemTable
   )
 {
   EFI_STATUS  Status;
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 2/2] DynamicTablesPkg: SRAT: Fix uninitialized memory usage

[Sami Mujawar]

On enabling the /analyse option the VS2017 compiler
reports: warning C6001: Using uninitialized memory.

This warning is reported for the Status variable in
AddGenericInitiatorAffinity() as it is not initialised
to a default value. This condition is only valid if
GenInitAffCount is equal to 0. Since GenInitAffCount
is already checked in BuildSratTable() this condition
can never happen.

The value of the Status variable is returned in
failure cases from appropriate locations in
AddGenericInitiatorAffinity(). The only case
where Status value is being used un-initialised
is the return statement at the end of
AddGenericInitiatorAffinity().

Therefore, to fix this issue EFI_SUCCESS can be
safely returned instead of returning the Status
variable at the end of the function.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
index 74cb7d92a5d8cddd3df8334f3ab55e6fa3e7267a..620e2929ef2460b6bf318fa85f8bca984608b955 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
@@ -413,7 +413,7 @@ AddGenericInitiatorAffinity (
     GenInitAff++;
     GenInitAffInfo++;
   }// while
-  return Status;
+  return EFI_SUCCESS;
 }
 
 /** Construct the SRAT ACPI table.
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [edk2-devel] [PATCH v1 1/2] DynamicTablesPkg: SRAT: Fix entry points

[Ard Biesheuvel]

On Sun, 29 Mar 2020 at 19:38, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> VS2017 reports 'warning C4028: formal parameter 2 different
> from declaration' for the library constructor and destructor
> interfaces for the SRAT Generator modules.
>
> Remove the CONST qualifier for the SystemTable pointer (the
> second parameter to the constructor/destructor/DXE Entry
> point) to make it compatible with the formal declaration.
>

You are removing the CONST qualifier from two places, no?

> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
> index 5d56af66608d862e6eca81da812d719f110867d2..74cb7d92a5d8cddd3df8334f3ab55e6fa3e7267a 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
> @@ -800,8 +800,8 @@ ACPI_TABLE_GENERATOR SratGenerator = {
>  EFI_STATUS
>  EFIAPI
>  AcpiSratLibConstructor (
> -  IN CONST EFI_HANDLE                ImageHandle,
> -  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
> +  IN  EFI_HANDLE           ImageHandle,
> +  IN  EFI_SYSTEM_TABLE  *  SystemTable
>    )
>  {
>    EFI_STATUS  Status;
> @@ -823,8 +823,8 @@ AcpiSratLibConstructor (
>  EFI_STATUS
>  EFIAPI
>  AcpiSratLibDestructor (
> -  IN CONST EFI_HANDLE                ImageHandle,
> -  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
> +  IN  EFI_HANDLE           ImageHandle,
> +  IN  EFI_SYSTEM_TABLE  *  SystemTable

Can we keep the * with the argument name, please?

>    )
>  {
>    EFI_STATUS  Status;
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
> 
>

Re: [edk2-devel] [PATCH v1 0/2] Fix SRAT generator warnings repoted by VS2017 compiler

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

Re: [edk2-devel] [PATCH v1 1/2] DynamicTablesPkg: SRAT: Fix entry points

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 52 bytes --]

Reviewed by: Alexei Fedorov Alexei.Fedorov@arm.com

[-- Attachment #2: Type: text/html, Size: 96 bytes --]

Re: [edk2-devel] [PATCH v1 2/2] DynamicTablesPkg: SRAT: Fix uninitialized memory usage

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

Re: [edk2-devel] [PATCH v1 1/2] DynamicTablesPkg: SRAT: Fix entry points

[Sami Mujawar]

Hi Ard,

Please see my reply inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@linaro.org> 
Sent: 30 March 2020 09:08 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Laura Moretta <Laura.Moretta@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 1/2] DynamicTablesPkg: SRAT: Fix entry points

On Sun, 29 Mar 2020 at 19:38, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> VS2017 reports 'warning C4028: formal parameter 2 different from 
> declaration' for the library constructor and destructor interfaces for 
> the SRAT Generator modules.
>
> Remove the CONST qualifier for the SystemTable pointer (the second 
> parameter to the constructor/destructor/DXE Entry
> point) to make it compatible with the formal declaration.
>

You are removing the CONST qualifier from two places, no?
[SAMI] Yes, CONST is removed from 2 places. I will update the commit message. If you think this must be changed, then we need to do that as part of a separate patch series.

> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c | 8 
> ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git 
> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c 
> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
> index 
> 5d56af66608d862e6eca81da812d719f110867d2..74cb7d92a5d8cddd3df8334f3ab5
> 5e6fa3e7267a 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
> @@ -800,8 +800,8 @@ ACPI_TABLE_GENERATOR SratGenerator = {  EFI_STATUS  
> EFIAPI  AcpiSratLibConstructor (
> -  IN CONST EFI_HANDLE                ImageHandle,
> -  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
> +  IN  EFI_HANDLE           ImageHandle,
> +  IN  EFI_SYSTEM_TABLE  *  SystemTable
>    )
>  {
>    EFI_STATUS  Status;
> @@ -823,8 +823,8 @@ AcpiSratLibConstructor (  EFI_STATUS  EFIAPI  
> AcpiSratLibDestructor (
> -  IN CONST EFI_HANDLE                ImageHandle,
> -  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
> +  IN  EFI_HANDLE           ImageHandle,
> +  IN  EFI_SYSTEM_TABLE  *  SystemTable

Can we keep the * with the argument name, please?
[SAMI] We have used the above convention throughout the package.

>    )
>  {
>    EFI_STATUS  Status;
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
> 
>