public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v3 0/2] Fix warnings reported by VS2017 compiler
@ 2020-05-18 12:46 Sami Mujawar
  2020-05-18 12:46 ` [PATCH v1-resend 1/2] ArmPlatformPkg: Fix UART divisor warning Sami Mujawar
  2020-05-18 12:46 ` [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning Sami Mujawar
  0 siblings, 2 replies; 8+ messages in thread
From: Sami Mujawar @ 2020-05-18 12:46 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, Alexei.Fedorov, ard.biesheuvel, leif,
	Matteo.Carlini, Laura.Moretta, nd

This patch series fixes warnings reported by the VS2017 compiler
when the static code analysis option is enabled.

The issues fixed in this series were identified
by building DynamicTablesPkg/DynamicTablesPkg.dsc
using the VS2017 compiler with the static code
analysis option.

This v3 patch series is a subset of the v1 patch
series and contains the patches for ArmPlatformPkg.
The v1 patch series can be found at:
https://edk2.groups.io/g/devel/message/46261

The changes for the v3 patch series can be seen at:
https://github.com/samimujawar/edk2/tree/503_vs2017_compile_issue_v3

Sami Mujawar (2):
  ArmPlatformPkg: Fix UART divisor warning
  ArmPlatformPkg: Fix comparison of constants warning

 ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v1-resend 1/2] ArmPlatformPkg: Fix UART divisor warning
  2020-05-18 12:46 [PATCH v3 0/2] Fix warnings reported by VS2017 compiler Sami Mujawar
@ 2020-05-18 12:46 ` Sami Mujawar
  2020-07-02 11:50   ` [edk2-devel] " Sami Mujawar
  2020-05-18 12:46 ` [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning Sami Mujawar
  1 sibling, 1 reply; 8+ messages in thread
From: Sami Mujawar @ 2020-05-18 12:46 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, Alexei.Fedorov, ard.biesheuvel, leif,
	Matteo.Carlini, Laura.Moretta, philmd, nd

The VS2017 compiler reports 'warning C4244: '=': conversion
from 'UINT64' to 'UINT32', possible loss of data' for the
calculation of the UART Divisor value.

Fix this warning by adding appropriate typecast and a validation
that ensures that the UART divisor value generated does not exceed
MAX_UINT32.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
---

Notes:
    v1:
     - Fix UART divisor warning reported by VS2017                [SAMI]
     - Resending patch as part of separate series                 [SAMI]
       Ref: https://edk2.groups.io/g/devel/message/46279

 ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
index 801990d9551a638c17d560d4226137b8a3ee47bb..2d3c279cce49304959953ec4a34b50e09a7d0045 100644
--- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
+++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
@@ -2,7 +2,7 @@
   Serial I/O Port library functions with no library constructor/destructor
 
   Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
-  Copyright (c) 2011 - 2016, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -78,6 +78,7 @@ PL011UartInitializePort (
   UINT32      Integer;
   UINT32      Fractional;
   UINT32      HardwareFifoDepth;
+  UINT64      DivisorValue;
 
   HardwareFifoDepth = (PL011_UARTPID2_VER (MmioRead32 (UartBase + UARTPID2)) \
                        > PL011_VER_R1P4) \
@@ -188,7 +189,12 @@ PL011UartInitializePort (
       return RETURN_INVALID_PARAMETER;
     }
 
-    Divisor = (UartClkInHz * 4) / *BaudRate;
+    DivisorValue = (((UINT64)UartClkInHz * 4) / *BaudRate);
+    if (DivisorValue > MAX_UINT32) {
+      return RETURN_INVALID_PARAMETER;
+    }
+
+    Divisor = (UINT32)DivisorValue;
     Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS;
     Fractional = Divisor & FRACTION_PART_MASK;
   }
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning
  2020-05-18 12:46 [PATCH v3 0/2] Fix warnings reported by VS2017 compiler Sami Mujawar
  2020-05-18 12:46 ` [PATCH v1-resend 1/2] ArmPlatformPkg: Fix UART divisor warning Sami Mujawar
@ 2020-05-18 12:46 ` Sami Mujawar
  2020-05-18 17:51   ` Leif Lindholm
  2020-05-20 10:47   ` Philippe Mathieu-Daudé
  1 sibling, 2 replies; 8+ messages in thread
From: Sami Mujawar @ 2020-05-18 12:46 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, Alexei.Fedorov, ard.biesheuvel, leif,
	Matteo.Carlini, Laura.Moretta, philmd, nd

The VS2017 compiler reports 'warning C6326: potential
comparison of a constant with another constant' when
a fixed PCD value is compared with a constant value.

The faulting code is as marked by '-->' below:

--> if (FixedPcdGet32 (PL011UartInteger) != 0) {
      Integer = FixedPcdGet32 (PL011UartInteger);
      Fractional = FixedPcdGet32 (PL011UartFractional);
    } else {
    ...

The macro FixedPcdGet32 (PL011UartInteger) evaluates
to a macro _PCD_VALUE_PL011UartInteger that is defined
by the build system to represent the UART Integer
value. Therefore, the VS2017 compiler reports the above
warning.

In this case the warning reported by the Visual Studio
compiler does not evaluate to an issue. However, it can
be useful to detect potential issues in other scenarios.
Other compilers may either be incapable of detecting and
reporting comparison with constant warnings or may be
good at reducing false positives. So, it is definitely
useful to keep this warning enabled, and disabling it
case by case is a suitable option.

Therefore, disable this warning for Visual studio
compilers using the pragma suppress directive that:
'Pushes the current state of the pragma on the stack,
disables the specified warning for the next line, and
then pops the warning stack so that the pragma state
is reset.'

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---

Notes:
    v2:
     - Update patch to selectively suppress comparison of         [SAMI]
       constant warning and submit as a separate series.
    
    v1:
     - Fix comparison of constant warning reported by VS2017      [SAMI]
     - Various feedbacks can be seen at:
       https://edk2.groups.io/g/devel/topic/32999801#46278

 ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
index 2d3c279cce49304959953ec4a34b50e09a7d0045..3c915e1e8de22a0b0b4cc46d495a5a6cbc784013 100644
--- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
+++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
@@ -2,7 +2,7 @@
   Serial I/O Port library functions with no library constructor/destructor
 
   Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
-  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -174,6 +174,14 @@ PL011UartInitializePort (
   //
 
   // If PL011 Integer value has been defined then always ignore the BAUD rate
+#if defined(_MSC_EXTENSIONS)
+  // Suppress 'warning C6326' reported by Visual Studio compiler using
+  // the suppress pragma directive that: 'Pushes the current state of
+  // the pragma on the stack, disables the specified warning for the
+  // next line, and then pops the warning stack so that the pragma state
+  // is reset.'
+#pragma warning(suppress:6326)
+#endif
   if (FixedPcdGet32 (PL011UartInteger) != 0) {
     Integer = FixedPcdGet32 (PL011UartInteger);
     Fractional = FixedPcdGet32 (PL011UartFractional);
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning
  2020-05-18 12:46 ` [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning Sami Mujawar
@ 2020-05-18 17:51   ` Leif Lindholm
  2020-05-20 10:47   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 8+ messages in thread
From: Leif Lindholm @ 2020-05-18 17:51 UTC (permalink / raw)
  To: Sami Mujawar
  Cc: devel, Alexei.Fedorov, ard.biesheuvel, Matteo.Carlini,
	Laura.Moretta, philmd, nd

On Mon, May 18, 2020 at 13:46:46 +0100, Sami Mujawar wrote:
> The VS2017 compiler reports 'warning C6326: potential
> comparison of a constant with another constant' when
> a fixed PCD value is compared with a constant value.
> 
> The faulting code is as marked by '-->' below:
> 
> --> if (FixedPcdGet32 (PL011UartInteger) != 0) {
>       Integer = FixedPcdGet32 (PL011UartInteger);
>       Fractional = FixedPcdGet32 (PL011UartFractional);
>     } else {
>     ...
> 
> The macro FixedPcdGet32 (PL011UartInteger) evaluates
> to a macro _PCD_VALUE_PL011UartInteger that is defined
> by the build system to represent the UART Integer
> value. Therefore, the VS2017 compiler reports the above
> warning.
> 
> In this case the warning reported by the Visual Studio
> compiler does not evaluate to an issue. However, it can
> be useful to detect potential issues in other scenarios.
> Other compilers may either be incapable of detecting and
> reporting comparison with constant warnings or may be
> good at reducing false positives. So, it is definitely
> useful to keep this warning enabled, and disabling it
> case by case is a suitable option.
> 
> Therefore, disable this warning for Visual studio
> compilers using the pragma suppress directive that:
> 'Pushes the current state of the pragma on the stack,
> disables the specified warning for the next line, and
> then pops the warning stack so that the pragma state
> is reset.'
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

However, this patch set is garbled. The cover-letter claims to be v3,
1/2 claims to be v1-resend and 2/2 claims to be v2.
Please use git format-patch for generating your patches before sending
out - that will do the right thing.

/
    Leif

> ---
> 
> Notes:
>     v2:
>      - Update patch to selectively suppress comparison of         [SAMI]
>        constant warning and submit as a separate series.
>     
>     v1:
>      - Fix comparison of constant warning reported by VS2017      [SAMI]
>      - Various feedbacks can be seen at:
>        https://edk2.groups.io/g/devel/topic/32999801#46278
> 
>  ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> index 2d3c279cce49304959953ec4a34b50e09a7d0045..3c915e1e8de22a0b0b4cc46d495a5a6cbc784013 100644
> --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> @@ -2,7 +2,7 @@
>    Serial I/O Port library functions with no library constructor/destructor
>  
>    Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
> -  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -174,6 +174,14 @@ PL011UartInitializePort (
>    //
>  
>    // If PL011 Integer value has been defined then always ignore the BAUD rate
> +#if defined(_MSC_EXTENSIONS)
> +  // Suppress 'warning C6326' reported by Visual Studio compiler using
> +  // the suppress pragma directive that: 'Pushes the current state of
> +  // the pragma on the stack, disables the specified warning for the
> +  // next line, and then pops the warning stack so that the pragma state
> +  // is reset.'
> +#pragma warning(suppress:6326)
> +#endif
>    if (FixedPcdGet32 (PL011UartInteger) != 0) {
>      Integer = FixedPcdGet32 (PL011UartInteger);
>      Fractional = FixedPcdGet32 (PL011UartFractional);



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning
  2020-05-18 12:46 ` [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning Sami Mujawar
  2020-05-18 17:51   ` Leif Lindholm
@ 2020-05-20 10:47   ` Philippe Mathieu-Daudé
  2020-05-20 11:25     ` Sami Mujawar
  1 sibling, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-20 10:47 UTC (permalink / raw)
  To: Sami Mujawar, devel
  Cc: Alexei.Fedorov, ard.biesheuvel, leif, Matteo.Carlini,
	Laura.Moretta, nd

Hi Sami,

On 5/18/20 2:46 PM, Sami Mujawar wrote:
> The VS2017 compiler reports 'warning C6326: potential
> comparison of a constant with another constant' when
> a fixed PCD value is compared with a constant value.
> 
> The faulting code is as marked by '-->' below:
> 
> --> if (FixedPcdGet32 (PL011UartInteger) != 0) {
>        Integer = FixedPcdGet32 (PL011UartInteger);
>        Fractional = FixedPcdGet32 (PL011UartFractional);
>      } else {
>      ...
> 
> The macro FixedPcdGet32 (PL011UartInteger) evaluates
> to a macro _PCD_VALUE_PL011UartInteger that is defined
> by the build system to represent the UART Integer
> value. Therefore, the VS2017 compiler reports the above
> warning.
> 
> In this case the warning reported by the Visual Studio
> compiler does not evaluate to an issue. However, it can
> be useful to detect potential issues in other scenarios.
> Other compilers may either be incapable of detecting and
> reporting comparison with constant warnings or may be
> good at reducing false positives. So, it is definitely
> useful to keep this warning enabled, and disabling it
> case by case is a suitable option.
> 
> Therefore, disable this warning for Visual studio
> compilers using the pragma suppress directive that:
> 'Pushes the current state of the pragma on the stack,
> disables the specified warning for the next line, and
> then pops the warning stack so that the pragma state
> is reset.'
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> 
> Notes:
>      v2:
>       - Update patch to selectively suppress comparison of         [SAMI]
>         constant warning and submit as a separate series.
>      
>      v1:
>       - Fix comparison of constant warning reported by VS2017      [SAMI]
>       - Various feedbacks can be seen at:
>         https://edk2.groups.io/g/devel/topic/32999801#46278
> 
>   ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> index 2d3c279cce49304959953ec4a34b50e09a7d0045..3c915e1e8de22a0b0b4cc46d495a5a6cbc784013 100644
> --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> @@ -2,7 +2,7 @@
>     Serial I/O Port library functions with no library constructor/destructor
>   
>     Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
> -  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
>   
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   
> @@ -174,6 +174,14 @@ PL011UartInitializePort (
>     //
>   
>     // If PL011 Integer value has been defined then always ignore the BAUD rate
> +#if defined(_MSC_EXTENSIONS)
> +  // Suppress 'warning C6326' reported by Visual Studio compiler using
> +  // the suppress pragma directive that: 'Pushes the current state of
> +  // the pragma on the stack, disables the specified warning for the
> +  // next line, and then pops the warning stack so that the pragma state
> +  // is reset.'

We don't need to document how #pragma works each time we use it in the 
source code...

What about a simpler comment, referring the particular Visual Studio 
version:

//

// Disable 'potential comparison of a constant with another constant'
// warning with VS2017 compiler static code analysis option is enabled
//

> +#pragma warning(suppress:6326)
> +#endif
>     if (FixedPcdGet32 (PL011UartInteger) != 0) {
>       Integer = FixedPcdGet32 (PL011UartInteger);
>       Fractional = FixedPcdGet32 (PL011UartFractional);
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning
  2020-05-20 10:47   ` Philippe Mathieu-Daudé
@ 2020-05-20 11:25     ` Sami Mujawar
  2020-05-20 11:37       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 8+ messages in thread
From: Sami Mujawar @ 2020-05-20 11:25 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, devel@edk2.groups.io
  Cc: Alexei Fedorov, Ard Biesheuvel, leif@nuviainc.com, Matteo Carlini,
	Laura Moretta, nd

Hi Philippe,

I had put a descriptive message to make people aware that the pragma effects the next line of code only. But I agree we can abbreviate it as per your suggestion.
However, there is an ongoing discussion about an alternative (by which we could avoid the pragma) on another thread at https://edk2.groups.io/g/devel/message/59948.

Regards,

Sami Mujawar

-----Original Message-----
From: Philippe Mathieu-Daudé <philmd@redhat.com> 
Sent: 20 May 2020 11:47 AM
To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com; Matteo Carlini <Matteo.Carlini@arm.com>; Laura Moretta <Laura.Moretta@arm.com>; nd <nd@arm.com>
Subject: Re: [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning

Hi Sami,

On 5/18/20 2:46 PM, Sami Mujawar wrote:
> The VS2017 compiler reports 'warning C6326: potential comparison of a 
> constant with another constant' when a fixed PCD value is compared 
> with a constant value.
> 
> The faulting code is as marked by '-->' below:
> 
> --> if (FixedPcdGet32 (PL011UartInteger) != 0) {
>        Integer = FixedPcdGet32 (PL011UartInteger);
>        Fractional = FixedPcdGet32 (PL011UartFractional);
>      } else {
>      ...
> 
> The macro FixedPcdGet32 (PL011UartInteger) evaluates to a macro 
> _PCD_VALUE_PL011UartInteger that is defined by the build system to 
> represent the UART Integer value. Therefore, the VS2017 compiler 
> reports the above warning.
> 
> In this case the warning reported by the Visual Studio compiler does 
> not evaluate to an issue. However, it can be useful to detect 
> potential issues in other scenarios.
> Other compilers may either be incapable of detecting and reporting 
> comparison with constant warnings or may be good at reducing false 
> positives. So, it is definitely useful to keep this warning enabled, 
> and disabling it case by case is a suitable option.
> 
> Therefore, disable this warning for Visual studio compilers using the 
> pragma suppress directive that:
> 'Pushes the current state of the pragma on the stack, disables the 
> specified warning for the next line, and then pops the warning stack 
> so that the pragma state is reset.'
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> 
> Notes:
>      v2:
>       - Update patch to selectively suppress comparison of         [SAMI]
>         constant warning and submit as a separate series.
>      
>      v1:
>       - Fix comparison of constant warning reported by VS2017      [SAMI]
>       - Various feedbacks can be seen at:
>         https://edk2.groups.io/g/devel/topic/32999801#46278
> 
>   ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c 
> b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> index 
> 2d3c279cce49304959953ec4a34b50e09a7d0045..3c915e1e8de22a0b0b4cc46d495a
> 5a6cbc784013 100644
> --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
> @@ -2,7 +2,7 @@
>     Serial I/O Port library functions with no library 
> constructor/destructor
>   
>     Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
> -  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
>   
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   
> @@ -174,6 +174,14 @@ PL011UartInitializePort (
>     //
>   
>     // If PL011 Integer value has been defined then always ignore the 
> BAUD rate
> +#if defined(_MSC_EXTENSIONS)
> +  // Suppress 'warning C6326' reported by Visual Studio compiler 
> +using
> +  // the suppress pragma directive that: 'Pushes the current state of
> +  // the pragma on the stack, disables the specified warning for the
> +  // next line, and then pops the warning stack so that the pragma 
> +state
> +  // is reset.'

We don't need to document how #pragma works each time we use it in the source code...

What about a simpler comment, referring the particular Visual Studio
version:

//

// Disable 'potential comparison of a constant with another constant'
// warning with VS2017 compiler static code analysis option is enabled //

> +#pragma warning(suppress:6326)
> +#endif
>     if (FixedPcdGet32 (PL011UartInteger) != 0) {
>       Integer = FixedPcdGet32 (PL011UartInteger);
>       Fractional = FixedPcdGet32 (PL011UartFractional);
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning
  2020-05-20 11:25     ` Sami Mujawar
@ 2020-05-20 11:37       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-20 11:37 UTC (permalink / raw)
  To: Sami Mujawar, devel@edk2.groups.io
  Cc: Alexei Fedorov, Ard Biesheuvel, leif@nuviainc.com, Matteo Carlini,
	Laura Moretta, nd

[Top-posting is difficult to read on technical lists; it's better to 
reply inline; maybe GitHub will fix this problem after all]

On 5/20/20 1:25 PM, Sami Mujawar wrote:
> Hi Philippe,
> 
> I had put a descriptive message to make people aware that the pragma effects the next line of code only. But I agree we can abbreviate it as per your suggestion.

Usually #pragma should be avoided at all cost, but is acceptable as last 
resort aid. Since you use it in a C source file (and not an header) it 
is a bit implicit that the scope is local. Sometime too verbose 
documentation is harmful. Anyway I'm diverging.

> However, there is an ongoing discussion about an alternative (by which we could avoid the pragma) on another thread at https://edk2.groups.io/g/devel/message/59948.

Good, this approach is preferable.

Regards,

Phil.

> 
> Regards,
> 
> Sami Mujawar
> 
> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> Sent: 20 May 2020 11:47 AM
> To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io
> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com; Matteo Carlini <Matteo.Carlini@arm.com>; Laura Moretta <Laura.Moretta@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning
> 
> Hi Sami,
> 
> On 5/18/20 2:46 PM, Sami Mujawar wrote:
>> The VS2017 compiler reports 'warning C6326: potential comparison of a
>> constant with another constant' when a fixed PCD value is compared
>> with a constant value.
>>
>> The faulting code is as marked by '-->' below:
>>
>> --> if (FixedPcdGet32 (PL011UartInteger) != 0) {
>>         Integer = FixedPcdGet32 (PL011UartInteger);
>>         Fractional = FixedPcdGet32 (PL011UartFractional);
>>       } else {
>>       ...
>>
>> The macro FixedPcdGet32 (PL011UartInteger) evaluates to a macro
>> _PCD_VALUE_PL011UartInteger that is defined by the build system to
>> represent the UART Integer value. Therefore, the VS2017 compiler
>> reports the above warning.
>>
>> In this case the warning reported by the Visual Studio compiler does
>> not evaluate to an issue. However, it can be useful to detect
>> potential issues in other scenarios.
>> Other compilers may either be incapable of detecting and reporting
>> comparison with constant warnings or may be good at reducing false
>> positives. So, it is definitely useful to keep this warning enabled,
>> and disabling it case by case is a suitable option.
>>
>> Therefore, disable this warning for Visual studio compilers using the
>> pragma suppress directive that:
>> 'Pushes the current state of the pragma on the stack, disables the
>> specified warning for the next line, and then pops the warning stack
>> so that the pragma state is reset.'
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>
>> Notes:
>>       v2:
>>        - Update patch to selectively suppress comparison of         [SAMI]
>>          constant warning and submit as a separate series.
>>       
>>       v1:
>>        - Fix comparison of constant warning reported by VS2017      [SAMI]
>>        - Various feedbacks can be seen at:
>>          https://edk2.groups.io/g/devel/topic/32999801#46278
>>
>>    ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 +++++++++-
>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
>> b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
>> index
>> 2d3c279cce49304959953ec4a34b50e09a7d0045..3c915e1e8de22a0b0b4cc46d495a
>> 5a6cbc784013 100644
>> --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
>> +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c
>> @@ -2,7 +2,7 @@
>>      Serial I/O Port library functions with no library
>> constructor/destructor
>>    
>>      Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
>> -  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>
>> +  Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
>>    
>>      SPDX-License-Identifier: BSD-2-Clause-Patent
>>    
>> @@ -174,6 +174,14 @@ PL011UartInitializePort (
>>      //
>>    
>>      // If PL011 Integer value has been defined then always ignore the
>> BAUD rate
>> +#if defined(_MSC_EXTENSIONS)
>> +  // Suppress 'warning C6326' reported by Visual Studio compiler
>> +using
>> +  // the suppress pragma directive that: 'Pushes the current state of
>> +  // the pragma on the stack, disables the specified warning for the
>> +  // next line, and then pops the warning stack so that the pragma
>> +state
>> +  // is reset.'
> 
> We don't need to document how #pragma works each time we use it in the source code...
> 
> What about a simpler comment, referring the particular Visual Studio
> version:
> 
> //
> 
> // Disable 'potential comparison of a constant with another constant'
> // warning with VS2017 compiler static code analysis option is enabled //
> 
>> +#pragma warning(suppress:6326)
>> +#endif
>>      if (FixedPcdGet32 (PL011UartInteger) != 0) {
>>        Integer = FixedPcdGet32 (PL011UartInteger);
>>        Fractional = FixedPcdGet32 (PL011UartFractional);
>>
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH v1-resend 1/2] ArmPlatformPkg: Fix UART divisor warning
  2020-05-18 12:46 ` [PATCH v1-resend 1/2] ArmPlatformPkg: Fix UART divisor warning Sami Mujawar
@ 2020-07-02 11:50   ` Sami Mujawar
  0 siblings, 0 replies; 8+ messages in thread
From: Sami Mujawar @ 2020-07-02 11:50 UTC (permalink / raw)
  To: Sami Mujawar, devel

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

Hi Ard, Leif,

I probably have got the 'v1-resend' in subject line wrong. Basically this patch has not changed since the v1 posting.
This was last submitted as part of the v3 series which can be seen at https://edk2.groups.io/g/devel/message/59738?p=,,,20,0,0,0::Created,,ArmPlatformPkg%3A+Fix+UART+divisor+warning,20,2,0,74289923

The other patch 2/2 in this series is at https://edk2.groups.io/g/devel/topic/74377676

Kindly let me know if you want me to repost the series or if any other change is needed.

Regards,

Sami Mujawar

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-07-02 11:50 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-18 12:46 [PATCH v3 0/2] Fix warnings reported by VS2017 compiler Sami Mujawar
2020-05-18 12:46 ` [PATCH v1-resend 1/2] ArmPlatformPkg: Fix UART divisor warning Sami Mujawar
2020-07-02 11:50   ` [edk2-devel] " Sami Mujawar
2020-05-18 12:46 ` [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning Sami Mujawar
2020-05-18 17:51   ` Leif Lindholm
2020-05-20 10:47   ` Philippe Mathieu-Daudé
2020-05-20 11:25     ` Sami Mujawar
2020-05-20 11:37       ` Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox