* [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
* 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
* [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
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