[PATCH 0/2] MdeModulePkg: UDF fixes and cleanups, round 2

[PATCH 0/2] MdeModulePkg: UDF fixes and cleanups, round 2

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: udf_fixes_cleanups_round2

Once these patches are sufficiently reviewed, please don't wait for me
to commit them.

Further UdfDxe issues should be please reported in the TianoCore
Bugzilla.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>

Thanks
Laszlo

Laszlo Ersek (2):
  MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]
  MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 1/2] MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]

[Laszlo Ersek]

The ECMA-167 standard (3rd Edition, June 1997) reserves values 4 through 7
in the ICB.Flags[2:0] bit-field for future standardization; see "14.6 ICB
Tag" / "14.6.8 Flags (RBP 18)".

https://www.ecma-international.org/publications/standards/Ecma-167.htm

The

  switch (RecordingFlags)

statement in the ReadFile() function handles all the standard values,
using the constants of the UDF_FE_RECORDING_FLAGS enum type. However, the
reserved values are not caught with a "default" case label, which both
breaks the edk2 Coding Style Spec, and leaves the Status variable
un-initialized, before we return Status under the Done label.

Set Status to EFI_UNSUPPORTED if we encounter a reserved value.

This issue was reported by Ard's and Gerd's CI systems independently
(through build failures with GCC48/GCC49, DEBUG/RELEASE targets).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 72862653738e..096fbb4452cb 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1150,6 +1150,14 @@ ReadFile (
     ASSERT (FALSE);
     Status = EFI_UNSUPPORTED;
     break;
+
+  default:
+    //
+    // A flag value reserved by the ECMA-167 standard (3rd Edition - June
+    // 1997); 14.6 ICB Tag; 14.6.8 Flags (RBP 18); was found.
+    //
+    Status = EFI_UNSUPPORTED;
+    break;
   }
 
 Done:
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Laszlo Ersek]

When building the driver for DEBUG/RELEASE, GCC48/GCC49 warn about
ReadFile() possibly using "BytesLeft" without initializing it first.

This is not the case. The reads of "BytesLeft" are only reachable if
(ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ). But, in that case, we
also set "BytesLeft" to "ReadFileInfo->FileDataSize", near the top of the
function.

Assign "BytesLeft" zero at the top, and add a comment that conforms to the
pending Coding Style Spec feature request at
<https://bugzilla.tianocore.org/show_bug.cgi?id=607>.

This issue was reported by Ard's and Gerd's CI systems independently.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 096fbb4452cb..392494b2eb3f 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -893,6 +893,11 @@ ReadFile (
   LogicalBlockSize  = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
   DoFreeAed         = FALSE;
 
+  //
+  // set BytesLeft to suppress incorrect compiler/analyzer warnings
+  //
+  BytesLeft = 0;
+
   switch (ReadFileInfo->Flags) {
   case READ_FILE_GET_FILESIZE:
   case READ_FILE_ALLOCATE_AND_READ:
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 1/2] MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]

[Ni, Ruiyu]

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

Thanks/Ray

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, September 13, 2017 6:26 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric
> <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 1/2] MdeModulePkg/UdfDxe: reject reserved values in
> ICB.Flags[2:0]
> 
> The ECMA-167 standard (3rd Edition, June 1997) reserves values 4 through 7
> in the ICB.Flags[2:0] bit-field for future standardization; see "14.6 ICB Tag" /
> "14.6.8 Flags (RBP 18)".
> 
> https://www.ecma-international.org/publications/standards/Ecma-167.htm
> 
> The
> 
>   switch (RecordingFlags)
> 
> statement in the ReadFile() function handles all the standard values, using
> the constants of the UDF_FE_RECORDING_FLAGS enum type. However, the
> reserved values are not caught with a "default" case label, which both breaks
> the edk2 Coding Style Spec, and leaves the Status variable un-initialized,
> before we return Status under the Done label.
> 
> Set Status to EFI_UNSUPPORTED if we encounter a reserved value.
> 
> This issue was reported by Ard's and Gerd's CI systems independently
> (through build failures with GCC48/GCC49, DEBUG/RELEASE targets).
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reported-by: Gerd Hoffmann <kraxel@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 8
> ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> index 72862653738e..096fbb4452cb 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> @@ -1150,6 +1150,14 @@ ReadFile (
>      ASSERT (FALSE);
>      Status = EFI_UNSUPPORTED;
>      break;
> +
> +  default:
> +    //
> +    // A flag value reserved by the ECMA-167 standard (3rd Edition - June
> +    // 1997); 14.6 ICB Tag; 14.6.8 Flags (RBP 18); was found.
> +    //
> +    Status = EFI_UNSUPPORTED;
> +    break;
>    }
> 
>  Done:
> --
> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, September 13, 2017 6:26 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

When building the driver for DEBUG/RELEASE, GCC48/GCC49 warn about
ReadFile() possibly using "BytesLeft" without initializing it first.

This is not the case. The reads of "BytesLeft" are only reachable if (ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ). But, in that case, we also set "BytesLeft" to "ReadFileInfo->FileDataSize", near the top of the function.

Assign "BytesLeft" zero at the top, and add a comment that conforms to the pending Coding Style Spec feature request at <https://bugzilla.tianocore.org/show_bug.cgi?id=607>.

This issue was reported by Ard's and Gerd's CI systems independently.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 096fbb4452cb..392494b2eb3f 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -893,6 +893,11 @@ ReadFile (
   LogicalBlockSize  = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
   DoFreeAed         = FALSE;
 
+  //
+  // set BytesLeft to suppress incorrect compiler/analyzer warnings  //  
+ BytesLeft = 0;
+
   switch (ReadFileInfo->Flags) {
   case READ_FILE_GET_FILESIZE:
   case READ_FILE_ALLOCATE_AND_READ:
--
2.14.1.3.gb7cf6e02401b

Re: [PATCH 1/2] MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, September 13, 2017 6:26 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 1/2] MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]

The ECMA-167 standard (3rd Edition, June 1997) reserves values 4 through 7
in the ICB.Flags[2:0] bit-field for future standardization; see "14.6 ICB
Tag" / "14.6.8 Flags (RBP 18)".

https://www.ecma-international.org/publications/standards/Ecma-167.htm

The

  switch (RecordingFlags)

statement in the ReadFile() function handles all the standard values,
using the constants of the UDF_FE_RECORDING_FLAGS enum type. However, the
reserved values are not caught with a "default" case label, which both
breaks the edk2 Coding Style Spec, and leaves the Status variable
un-initialized, before we return Status under the Done label.

Set Status to EFI_UNSUPPORTED if we encounter a reserved value.

This issue was reported by Ard's and Gerd's CI systems independently
(through build failures with GCC48/GCC49, DEBUG/RELEASE targets).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 72862653738e..096fbb4452cb 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1150,6 +1150,14 @@ ReadFile (
     ASSERT (FALSE);
     Status = EFI_UNSUPPORTED;
     break;
+
+  default:
+    //
+    // A flag value reserved by the ECMA-167 standard (3rd Edition - June
+    // 1997); 14.6 ICB Tag; 14.6.8 Flags (RBP 18); was found.
+    //
+    Status = EFI_UNSUPPORTED;
+    break;
   }
 
 Done:
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Zeng, Star]

Beyond the Rb (I do not want to block this patch series), I am curious about one question.

There may be more this kind of workarounds to fix the build failure.
Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?


ProcessorBind.h:
#if defined(_MSC_EXTENSIONS)

...

#if _MSC_VER == 1800 || _MSC_VER == 1900

//
// Disable these warnings for VS2013.
//

//
// This warning is for potentially uninitialized local variable, and it may cause false 
// positive issues in VS2013 and VS2015 build
//
#pragma warning ( disable : 4701 )
  
//
// This warning is for potentially uninitialized local pointer variable, and it may cause 
// false positive issues in VS2013 and VS2015 build
//
#pragma warning ( disable : 4703 )
  
#endif

#endif


Thanks,
Star
-----Original Message-----
From: Zeng, Star 
Sent: Wednesday, September 13, 2017 2:34 PM
To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, September 13, 2017 6:26 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

When building the driver for DEBUG/RELEASE, GCC48/GCC49 warn about
ReadFile() possibly using "BytesLeft" without initializing it first.

This is not the case. The reads of "BytesLeft" are only reachable if (ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ). But, in that case, we also set "BytesLeft" to "ReadFileInfo->FileDataSize", near the top of the function.

Assign "BytesLeft" zero at the top, and add a comment that conforms to the pending Coding Style Spec feature request at <https://bugzilla.tianocore.org/show_bug.cgi?id=607>.

This issue was reported by Ard's and Gerd's CI systems independently.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 096fbb4452cb..392494b2eb3f 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -893,6 +893,11 @@ ReadFile (
   LogicalBlockSize  = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
   DoFreeAed         = FALSE;
 
+  //
+  // set BytesLeft to suppress incorrect compiler/analyzer warnings  // 
+ BytesLeft = 0;
+
   switch (ReadFileInfo->Flags) {
   case READ_FILE_GET_FILESIZE:
   case READ_FILE_ALLOCATE_AND_READ:
--
2.14.1.3.gb7cf6e02401b

Re: [PATCH 0/2] MdeModulePkg: UDF fixes and cleanups, round 2

[Paulo Alcantara]

On September 12, 2017 7:26:10 PM GMT-03:00, Laszlo Ersek <lersek@redhat.com> wrote:
>Repo:   https://github.com/lersek/edk2.git
>Branch: udf_fixes_cleanups_round2
>
>Once these patches are sufficiently reviewed, please don't wait for me
>to commit them.
>
>Further UdfDxe issues should be please reported in the TianoCore
>Bugzilla.
>
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Cc: Eric Dong <eric.dong@intel.com>
>Cc: Paulo Alcantara <pcacjr@zytor.com>
>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>Cc: Star Zeng <star.zeng@intel.com>
>
>Thanks
>Laszlo
>
>Laszlo Ersek (2):
>  MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]
> MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
>
>MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 13
>+++++++++++++
> 1 file changed, 13 insertions(+)

Reviewed-by: Paulo Alcantara <pcacjr@zytor.com>

Thanks,
Paulo

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Laszlo Ersek]

On 09/13/17 08:43, Zeng, Star wrote:
> Beyond the Rb (I do not want to block this patch series), I am curious about one question.
> 
> There may be more this kind of workarounds to fix the build failure.
> Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?
> 
> 
> ProcessorBind.h:
> #if defined(_MSC_EXTENSIONS)
> 
> ...
> 
> #if _MSC_VER == 1800 || _MSC_VER == 1900
> 
> //
> // Disable these warnings for VS2013.
> //
> 
> //
> // This warning is for potentially uninitialized local variable, and it may cause false 
> // positive issues in VS2013 and VS2015 build
> //
> #pragma warning ( disable : 4701 )
>   
> //
> // This warning is for potentially uninitialized local pointer variable, and it may cause 
> // false positive issues in VS2013 and VS2015 build
> //
> #pragma warning ( disable : 4703 )
>   
> #endif
> 
> #endif

I think starting with gcc-4.6, gcc supports the "diagnostics" pragma,
which can be used to suppress warnings.

Unfortunately, there's no pragma to suppress *only* the incorrect
warnings :) So if we set the pragma, we could lose even those warnings
that point out real bugs.

Thanks
Laszlo

> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Zeng, Star 
> Sent: Wednesday, September 13, 2017 2:34 PM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
> 
> Reviewed-by: Star Zeng <star.zeng@intel.com>
> 
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, September 13, 2017 6:26 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
> 
> When building the driver for DEBUG/RELEASE, GCC48/GCC49 warn about
> ReadFile() possibly using "BytesLeft" without initializing it first.
> 
> This is not the case. The reads of "BytesLeft" are only reachable if (ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ). But, in that case, we also set "BytesLeft" to "ReadFileInfo->FileDataSize", near the top of the function.
> 
> Assign "BytesLeft" zero at the top, and add a comment that conforms to the pending Coding Style Spec feature request at <https://bugzilla.tianocore.org/show_bug.cgi?id=607>.
> 
> This issue was reported by Ard's and Gerd's CI systems independently.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reported-by: Gerd Hoffmann <kraxel@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> index 096fbb4452cb..392494b2eb3f 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> @@ -893,6 +893,11 @@ ReadFile (
>    LogicalBlockSize  = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
>    DoFreeAed         = FALSE;
>  
> +  //
> +  // set BytesLeft to suppress incorrect compiler/analyzer warnings  // 
> + BytesLeft = 0;
> +
>    switch (ReadFileInfo->Flags) {
>    case READ_FILE_GET_FILESIZE:
>    case READ_FILE_ALLOCATE_AND_READ:
> --
> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Ard Biesheuvel]

On 13 September 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/13/17 08:43, Zeng, Star wrote:
>> Beyond the Rb (I do not want to block this patch series), I am curious about one question.
>>
>> There may be more this kind of workarounds to fix the build failure.
>> Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?
>>
>>
>> ProcessorBind.h:
>> #if defined(_MSC_EXTENSIONS)
>>
>> ...
>>
>> #if _MSC_VER == 1800 || _MSC_VER == 1900
>>
>> //
>> // Disable these warnings for VS2013.
>> //
>>
>> //
>> // This warning is for potentially uninitialized local variable, and it may cause false
>> // positive issues in VS2013 and VS2015 build
>> //
>> #pragma warning ( disable : 4701 )
>>
>> //
>> // This warning is for potentially uninitialized local pointer variable, and it may cause
>> // false positive issues in VS2013 and VS2015 build
>> //
>> #pragma warning ( disable : 4703 )
>>
>> #endif
>>
>> #endif
>
> I think starting with gcc-4.6, gcc supports the "diagnostics" pragma,
> which can be used to suppress warnings.
>
> Unfortunately, there's no pragma to suppress *only* the incorrect
> warnings :) So if we set the pragma, we could lose even those warnings
> that point out real bugs.
>

That applies to the VS case as well. But I think doing this for older
GCCs is fine, most EDK2 developers use a newer version anyway, so we
will not lose any coverage by doing so.

Re: [PATCH 0/2] MdeModulePkg: UDF fixes and cleanups, round 2

[Laszlo Ersek]

On 09/13/17 15:42, Paulo Alcantara wrote:
> 
> 
> On September 12, 2017 7:26:10 PM GMT-03:00, Laszlo Ersek <lersek@redhat.com> wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: udf_fixes_cleanups_round2
>>
>> Once these patches are sufficiently reviewed, please don't wait for me
>> to commit them.
>>
>> Further UdfDxe issues should be please reported in the TianoCore
>> Bugzilla.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Paulo Alcantara <pcacjr@zytor.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (2):
>>  MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]
>> MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
>>
>> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 13
>> +++++++++++++
>> 1 file changed, 13 insertions(+)
> 
> Reviewed-by: Paulo Alcantara <pcacjr@zytor.com>

Thanks Everyone, pushed as c3246da7bf0a..5afa5b815936.

Laszlo

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Zeng, Star]

Comparing adding workaround in code with suppressing it in *OLD* version GCCs, I prefer the latter personally.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Thursday, September 14, 2017 2:52 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

On 13 September 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/13/17 08:43, Zeng, Star wrote:
>> Beyond the Rb (I do not want to block this patch series), I am curious about one question.
>>
>> There may be more this kind of workarounds to fix the build failure.
>> Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?
>>
>>
>> ProcessorBind.h:
>> #if defined(_MSC_EXTENSIONS)
>>
>> ...
>>
>> #if _MSC_VER == 1800 || _MSC_VER == 1900
>>
>> //
>> // Disable these warnings for VS2013.
>> //
>>
>> //
>> // This warning is for potentially uninitialized local variable, and 
>> it may cause false // positive issues in VS2013 and VS2015 build // 
>> #pragma warning ( disable : 4701 )
>>
>> //
>> // This warning is for potentially uninitialized local pointer 
>> variable, and it may cause // false positive issues in VS2013 and 
>> VS2015 build // #pragma warning ( disable : 4703 )
>>
>> #endif
>>
>> #endif
>
> I think starting with gcc-4.6, gcc supports the "diagnostics" pragma, 
> which can be used to suppress warnings.
>
> Unfortunately, there's no pragma to suppress *only* the incorrect 
> warnings :) So if we set the pragma, we could lose even those warnings 
> that point out real bugs.
>

That applies to the VS case as well. But I think doing this for older GCCs is fine, most EDK2 developers use a newer version anyway, so we will not lose any coverage by doing so.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Zeng, Star]

Seemingly, VS has similar issue with GCC.

VS2010/VS2012 still have the building failures below after this patch. :(
edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1083) : error C2220: warning treated as error - no 'executable' file generated
edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1083) : warning C4701: potentially uninitialized local variable 'FilePosition' used
edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1078) : warning C4701: potentially uninitialized local variable 'FinishedSeeking' used
edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1167) : warning C4701: potentially uninitialized local variable 'Data' used
edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1167) : warning C4703: potentially uninitialized local pointer variable 'Data' used


Liming, Dandan and Hao,
Do you remember how we fix this kind of false alarm before?
Just initialize the variable at the beginning of the function?



Thanks,
Star
-----Original Message-----
From: Zeng, Star 
Sent: Thursday, September 14, 2017 8:43 AM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

Comparing adding workaround in code with suppressing it in *OLD* version GCCs, I prefer the latter personally.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Thursday, September 14, 2017 2:52 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

On 13 September 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/13/17 08:43, Zeng, Star wrote:
>> Beyond the Rb (I do not want to block this patch series), I am curious about one question.
>>
>> There may be more this kind of workarounds to fix the build failure.
>> Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?
>>
>>
>> ProcessorBind.h:
>> #if defined(_MSC_EXTENSIONS)
>>
>> ...
>>
>> #if _MSC_VER == 1800 || _MSC_VER == 1900
>>
>> //
>> // Disable these warnings for VS2013.
>> //
>>
>> //
>> // This warning is for potentially uninitialized local variable, and 
>> it may cause false // positive issues in VS2013 and VS2015 build // 
>> #pragma warning ( disable : 4701 )
>>
>> //
>> // This warning is for potentially uninitialized local pointer 
>> variable, and it may cause // false positive issues in VS2013 and
>> VS2015 build // #pragma warning ( disable : 4703 )
>>
>> #endif
>>
>> #endif
>
> I think starting with gcc-4.6, gcc supports the "diagnostics" pragma, 
> which can be used to suppress warnings.
>
> Unfortunately, there's no pragma to suppress *only* the incorrect 
> warnings :) So if we set the pragma, we could lose even those warnings 
> that point out real bugs.
>

That applies to the VS case as well. But I think doing this for older GCCs is fine, most EDK2 developers use a newer version anyway, so we will not lose any coverage by doing so.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Gao, Liming]

Right. Just initialize them. They are not reported in VS2013&VS2015, because VS2013 and VS2015 disables this warning in ProcessorBind.h. If they are all false warning messages, I think we can propose to disable this warning for all VS tool chain. 

For GCC, if this warning is also false, could we add compiler to disable it, like -Wno-unused-but-set-variable.

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 14, 2017 9:21 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
> 
> Seemingly, VS has similar issue with GCC.
> 
> VS2010/VS2012 still have the building failures below after this patch. :(
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1083) : error C2220: warning treated as error - no 'executable' file
> generated
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1083) : warning C4701: potentially uninitialized local variable
> 'FilePosition' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1078) : warning C4701: potentially uninitialized local variable
> 'FinishedSeeking' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1167) : warning C4701: potentially uninitialized local variable
> 'Data' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1167) : warning C4703: potentially uninitialized local pointer
> variable 'Data' used
> 
> 
> Liming, Dandan and Hao,
> Do you remember how we fix this kind of false alarm before?
> Just initialize the variable at the beginning of the function?
> 
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 14, 2017 8:43 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming
> <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
> 
> Comparing adding workaround in code with suppressing it in *OLD* version GCCs, I prefer the latter personally.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Thursday, September 14, 2017 2:52 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming
> <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
> 
> On 13 September 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
> > On 09/13/17 08:43, Zeng, Star wrote:
> >> Beyond the Rb (I do not want to block this patch series), I am curious about one question.
> >>
> >> There may be more this kind of workarounds to fix the build failure.
> >> Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?
> >>
> >>
> >> ProcessorBind.h:
> >> #if defined(_MSC_EXTENSIONS)
> >>
> >> ...
> >>
> >> #if _MSC_VER == 1800 || _MSC_VER == 1900
> >>
> >> //
> >> // Disable these warnings for VS2013.
> >> //
> >>
> >> //
> >> // This warning is for potentially uninitialized local variable, and
> >> it may cause false // positive issues in VS2013 and VS2015 build //
> >> #pragma warning ( disable : 4701 )
> >>
> >> //
> >> // This warning is for potentially uninitialized local pointer
> >> variable, and it may cause // false positive issues in VS2013 and
> >> VS2015 build // #pragma warning ( disable : 4703 )
> >>
> >> #endif
> >>
> >> #endif
> >
> > I think starting with gcc-4.6, gcc supports the "diagnostics" pragma,
> > which can be used to suppress warnings.
> >
> > Unfortunately, there's no pragma to suppress *only* the incorrect
> > warnings :) So if we set the pragma, we could lose even those warnings
> > that point out real bugs.
> >
> 
> That applies to the VS case as well. But I think doing this for older GCCs is fine, most EDK2 developers use a newer version anyway, so
> we will not lose any coverage by doing so.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Laszlo Ersek]

On 09/14/17 02:42, Zeng, Star wrote:
> Comparing adding workaround in code with suppressing it in *OLD* version GCCs, I prefer the latter personally.

But, how old is old?

The base compiler in RHEL-7 is gcc-4.8. That's what I use every day.

The base compiler in Debian old-old-stable (still supported), is gcc-4.7
(for IA32 and X64).

The base compiler in RHEL-6 (still supported) is gcc-4.4. Is that old?

Thanks
Laszlo

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Thursday, September 14, 2017 2:52 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
> 
> On 13 September 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/13/17 08:43, Zeng, Star wrote:
>>> Beyond the Rb (I do not want to block this patch series), I am curious about one question.
>>>
>>> There may be more this kind of workarounds to fix the build failure.
>>> Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?
>>>
>>>
>>> ProcessorBind.h:
>>> #if defined(_MSC_EXTENSIONS)
>>>
>>> ...
>>>
>>> #if _MSC_VER == 1800 || _MSC_VER == 1900
>>>
>>> //
>>> // Disable these warnings for VS2013.
>>> //
>>>
>>> //
>>> // This warning is for potentially uninitialized local variable, and 
>>> it may cause false // positive issues in VS2013 and VS2015 build // 
>>> #pragma warning ( disable : 4701 )
>>>
>>> //
>>> // This warning is for potentially uninitialized local pointer 
>>> variable, and it may cause // false positive issues in VS2013 and 
>>> VS2015 build // #pragma warning ( disable : 4703 )
>>>
>>> #endif
>>>
>>> #endif
>>
>> I think starting with gcc-4.6, gcc supports the "diagnostics" pragma, 
>> which can be used to suppress warnings.
>>
>> Unfortunately, there's no pragma to suppress *only* the incorrect 
>> warnings :) So if we set the pragma, we could lose even those warnings 
>> that point out real bugs.
>>
> 
> That applies to the VS case as well. But I think doing this for older GCCs is fine, most EDK2 developers use a newer version anyway, so we will not lose any coverage by doing so.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Laszlo Ersek]

On 09/14/17 03:20, Zeng, Star wrote:
> Seemingly, VS has similar issue with GCC.
> 
> VS2010/VS2012 still have the building failures below after this patch. :(
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1083) : error C2220: warning treated as error - no 'executable' file generated
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1083) : warning C4701: potentially uninitialized local variable 'FilePosition' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1078) : warning C4701: potentially uninitialized local variable 'FinishedSeeking' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1167) : warning C4701: potentially uninitialized local variable 'Data' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1167) : warning C4703: potentially uninitialized local pointer variable 'Data' used
> 
> 
> Liming, Dandan and Hao,
> Do you remember how we fix this kind of false alarm before?
> Just initialize the variable at the beginning of the function?

I think that all such warnings should be evaluated carefully.

Can you please file a TianoCore BZ about the above? Paulo might want to
take a look.

Thanks!
Laszlo


> -----Original Message-----
> From: Zeng, Star 
> Sent: Thursday, September 14, 2017 8:43 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
> 
> Comparing adding workaround in code with suppressing it in *OLD* version GCCs, I prefer the latter personally.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Thursday, September 14, 2017 2:52 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
> 
> On 13 September 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/13/17 08:43, Zeng, Star wrote:
>>> Beyond the Rb (I do not want to block this patch series), I am curious about one question.
>>>
>>> There may be more this kind of workarounds to fix the build failure.
>>> Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?
>>>
>>>
>>> ProcessorBind.h:
>>> #if defined(_MSC_EXTENSIONS)
>>>
>>> ...
>>>
>>> #if _MSC_VER == 1800 || _MSC_VER == 1900
>>>
>>> //
>>> // Disable these warnings for VS2013.
>>> //
>>>
>>> //
>>> // This warning is for potentially uninitialized local variable, and 
>>> it may cause false // positive issues in VS2013 and VS2015 build // 
>>> #pragma warning ( disable : 4701 )
>>>
>>> //
>>> // This warning is for potentially uninitialized local pointer 
>>> variable, and it may cause // false positive issues in VS2013 and
>>> VS2015 build // #pragma warning ( disable : 4703 )
>>>
>>> #endif
>>>
>>> #endif
>>
>> I think starting with gcc-4.6, gcc supports the "diagnostics" pragma, 
>> which can be used to suppress warnings.
>>
>> Unfortunately, there's no pragma to suppress *only* the incorrect 
>> warnings :) So if we set the pragma, we could lose even those warnings 
>> that point out real bugs.
>>
> 
> That applies to the VS case as well. But I think doing this for older GCCs is fine, most EDK2 developers use a newer version anyway, so we will not lose any coverage by doing so.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Zeng, Star]

Is there GCC compiler version fixed this kind of false alarm?
If yes, the lesser versions without fix are what I mean *OLD*.


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Thursday, September 14, 2017 4:17 PM
To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

On 09/14/17 02:42, Zeng, Star wrote:
> Comparing adding workaround in code with suppressing it in *OLD* version GCCs, I prefer the latter personally.

But, how old is old?

The base compiler in RHEL-7 is gcc-4.8. That's what I use every day.

The base compiler in Debian old-old-stable (still supported), is gcc-4.7 (for IA32 and X64).

The base compiler in RHEL-6 (still supported) is gcc-4.4. Is that old?

Thanks
Laszlo

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Ard Biesheuvel
> Sent: Thursday, September 14, 2017 2:52 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; 
> edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming 
> <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress 
> incorrect compiler warning in ReadFile()
> 
> On 13 September 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/13/17 08:43, Zeng, Star wrote:
>>> Beyond the Rb (I do not want to block this patch series), I am curious about one question.
>>>
>>> There may be more this kind of workarounds to fix the build failure.
>>> Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?
>>>
>>>
>>> ProcessorBind.h:
>>> #if defined(_MSC_EXTENSIONS)
>>>
>>> ...
>>>
>>> #if _MSC_VER == 1800 || _MSC_VER == 1900
>>>
>>> //
>>> // Disable these warnings for VS2013.
>>> //
>>>
>>> //
>>> // This warning is for potentially uninitialized local variable, and 
>>> it may cause false // positive issues in VS2013 and VS2015 build // 
>>> #pragma warning ( disable : 4701 )
>>>
>>> //
>>> // This warning is for potentially uninitialized local pointer 
>>> variable, and it may cause // false positive issues in VS2013 and
>>> VS2015 build // #pragma warning ( disable : 4703 )
>>>
>>> #endif
>>>
>>> #endif
>>
>> I think starting with gcc-4.6, gcc supports the "diagnostics" pragma, 
>> which can be used to suppress warnings.
>>
>> Unfortunately, there's no pragma to suppress *only* the incorrect 
>> warnings :) So if we set the pragma, we could lose even those 
>> warnings that point out real bugs.
>>
> 
> That applies to the VS case as well. But I think doing this for older GCCs is fine, most EDK2 developers use a newer version anyway, so we will not lose any coverage by doing so.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Zeng, Star]

It seems generic compiler issue to have this kind of false alarm.
And it seems hard decision to disable the warning for all the compilers.
We'd better to fix the build failure first, I will submit patch that will use similar method with this patch.


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Thursday, September 14, 2017 4:20 PM
To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Paulo Alcantara <pcacjr@zytor.com>
Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

On 09/14/17 03:20, Zeng, Star wrote:
> Seemingly, VS has similar issue with GCC.
> 
> VS2010/VS2012 still have the building failures below after this patch. 
> :(
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1083) : 
> error C2220: warning treated as error - no 'executable' file generated
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1083) : 
> warning C4701: potentially uninitialized local variable 'FilePosition' 
> used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1078) : 
> warning C4701: potentially uninitialized local variable 
> 'FinishedSeeking' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1167) : 
> warning C4701: potentially uninitialized local variable 'Data' used
> edk2\mdemodulepkg\universal\disk\udfdxe\filesystemoperations.c(1167) : 
> warning C4703: potentially uninitialized local pointer variable 'Data' 
> used
> 
> 
> Liming, Dandan and Hao,
> Do you remember how we fix this kind of false alarm before?
> Just initialize the variable at the beginning of the function?

I think that all such warnings should be evaluated carefully.

Can you please file a TianoCore BZ about the above? Paulo might want to take a look.

Thanks!
Laszlo


> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 14, 2017 8:43 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek 
> <lersek@redhat.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; 
> edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming 
> <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress 
> incorrect compiler warning in ReadFile()
> 
> Comparing adding workaround in code with suppressing it in *OLD* version GCCs, I prefer the latter personally.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Ard Biesheuvel
> Sent: Thursday, September 14, 2017 2:52 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; 
> edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming 
> <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress 
> incorrect compiler warning in ReadFile()
> 
> On 13 September 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/13/17 08:43, Zeng, Star wrote:
>>> Beyond the Rb (I do not want to block this patch series), I am curious about one question.
>>>
>>> There may be more this kind of workarounds to fix the build failure.
>>> Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?
>>>
>>>
>>> ProcessorBind.h:
>>> #if defined(_MSC_EXTENSIONS)
>>>
>>> ...
>>>
>>> #if _MSC_VER == 1800 || _MSC_VER == 1900
>>>
>>> //
>>> // Disable these warnings for VS2013.
>>> //
>>>
>>> //
>>> // This warning is for potentially uninitialized local variable, and 
>>> it may cause false // positive issues in VS2013 and VS2015 build // 
>>> #pragma warning ( disable : 4701 )
>>>
>>> //
>>> // This warning is for potentially uninitialized local pointer 
>>> variable, and it may cause // false positive issues in VS2013 and
>>> VS2015 build // #pragma warning ( disable : 4703 )
>>>
>>> #endif
>>>
>>> #endif
>>
>> I think starting with gcc-4.6, gcc supports the "diagnostics" pragma, 
>> which can be used to suppress warnings.
>>
>> Unfortunately, there's no pragma to suppress *only* the incorrect 
>> warnings :) So if we set the pragma, we could lose even those 
>> warnings that point out real bugs.
>>
> 
> That applies to the VS case as well. But I think doing this for older GCCs is fine, most EDK2 developers use a newer version anyway, so we will not lose any coverage by doing so.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Laszlo Ersek]

On 09/14/17 10:50, Zeng, Star wrote:
> Is there GCC compiler version fixed this kind of false alarm?
> If yes, the lesser versions without fix are what I mean *OLD*.

Unfortunately, I cannot answer your question, for two reasons:

- First, I'm unsure what you mean by "lesser version". For example, if
gcc-4.8.4 emits a warning incorrectly, but gcc-4.8.5 does not, then I
agree we can require users to use gcc-4.8.5. Linux distributions
consider this a "stable update" anyway, and will generally ship the
version with the higher "patch level". (The version number comes
together from <major>.<minor>.<patchlevel>.)

However, if the same difference is between gcc-4.7 and gcc-4.8, then the
same requirement cannot be made. Distros will not upgrade gcc from one
minor version to another in a stable release.

- Second, even if I knew how exactly to interpret your question, I can't
answer! The way the warnings change in gcc over time is totally
impenetrable to the end-user. The data flow analysis is tied to the
optimizer AFAICT, and unless you are a gcc developer yourself, the
warning changes appear simply as chaos, from release to release. In
practice, new releases will stop emitting invalid warnings that earlier
releases used to, but the new release may also emit new incorrect
warnings. (I.e., it might emit the *same kind* of warning, incorrectly,
for a new kind of *context*.)

Laszlo


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Thursday, September 14, 2017 4:17 PM
> To: Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()
> 
> On 09/14/17 02:42, Zeng, Star wrote:
>> Comparing adding workaround in code with suppressing it in *OLD* version GCCs, I prefer the latter personally.
> 
> But, how old is old?
> 
> The base compiler in RHEL-7 is gcc-4.8. That's what I use every day.
> 
> The base compiler in Debian old-old-stable (still supported), is gcc-4.7 (for IA32 and X64).
> 
> The base compiler in RHEL-6 (still supported) is gcc-4.4. Is that old?
> 
> Thanks
> Laszlo
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
>> Ard Biesheuvel
>> Sent: Thursday, September 14, 2017 2:52 AM
>> To: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; 
>> edk2-devel-01 <edk2-devel@lists.01.org>; Gao, Liming 
>> <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: Re: [edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress 
>> incorrect compiler warning in ReadFile()
>>
>> On 13 September 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 09/13/17 08:43, Zeng, Star wrote:
>>>> Beyond the Rb (I do not want to block this patch series), I am curious about one question.
>>>>
>>>> There may be more this kind of workarounds to fix the build failure.
>>>> Is it possible to disable the warning (like below example for VS) for specific version of GCC for this kind of false alarm?
>>>>
>>>>
>>>> ProcessorBind.h:
>>>> #if defined(_MSC_EXTENSIONS)
>>>>
>>>> ...
>>>>
>>>> #if _MSC_VER == 1800 || _MSC_VER == 1900
>>>>
>>>> //
>>>> // Disable these warnings for VS2013.
>>>> //
>>>>
>>>> //
>>>> // This warning is for potentially uninitialized local variable, and 
>>>> it may cause false // positive issues in VS2013 and VS2015 build // 
>>>> #pragma warning ( disable : 4701 )
>>>>
>>>> //
>>>> // This warning is for potentially uninitialized local pointer 
>>>> variable, and it may cause // false positive issues in VS2013 and
>>>> VS2015 build // #pragma warning ( disable : 4703 )
>>>>
>>>> #endif
>>>>
>>>> #endif
>>>
>>> I think starting with gcc-4.6, gcc supports the "diagnostics" pragma, 
>>> which can be used to suppress warnings.
>>>
>>> Unfortunately, there's no pragma to suppress *only* the incorrect 
>>> warnings :) So if we set the pragma, we could lose even those 
>>> warnings that point out real bugs.
>>>
>>
>> That applies to the VS case as well. But I think doing this for older GCCs is fine, most EDK2 developers use a newer version anyway, so we will not lose any coverage by doing so.
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>