[edk2-devel] Can RELEASE target disable -Werror CC

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
@ 2023-08-10 10:44 汪流
  2023-08-10 13:50 ` Andrew Fish via groups.io
  0 siblings, 1 reply; 13+ messages in thread
From: 汪流 @ 2023-08-10 10:44 UTC (permalink / raw)
  To: devel

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

I want to build a rpm package for edk2-stable202305 on riscv64, however I get some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE


This is my packet log:   

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64

https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107691): https://edk2.groups.io/g/devel/message/107691
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-10 10:44 [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？ 汪流
@ 2023-08-10 13:50 ` Andrew Fish via groups.io
  2023-08-16  7:46   ` 汪流
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Fish via groups.io @ 2023-08-10 13:50 UTC (permalink / raw)
  To: edk2-devel-groups-io, wangliu

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

I think that you are advocating that since we have code that error checks on DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?

/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24: error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]
      ASSERT (Prot != NULL);
      //
      // EFI_ALREADY_STARTED is not an error for bus driver.
      // Return the corresponding protocol interface.
      //
      *Interface = Prot->Interface;

A given platform can add ASSERT into release builds if it wants. I’ve actually done that for power on before. If your compiler supports LTO you are not required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to configure your debug level, per build type. 

Maybe we should just have the error checks in all paths?

Thanks,

Andrew Fish

> On Aug 10, 2023, at 6:44 AM, 汪流 <wangliu@iscas.ac.cn> wrote:
> 
> I want to build a rpm package for edk2-stable202305 on riscv64, however I get some uninitialized variable error, I have found that the reason is -Werror flag.
> My build target was release. I think the flag should used in the debug ,not in release.
> My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE
> This is my packet log:   
> 
> https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64
> 
> https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107692): https://edk2.groups.io/g/devel/message/107692
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-10 13:50 ` Andrew Fish via groups.io
@ 2023-08-16  7:46   ` 汪流
  2023-08-16 17:02     ` Michael D Kinney
  0 siblings, 1 reply; 13+ messages in thread
From: 汪流 @ 2023-08-16  7:46 UTC (permalink / raw)
  To: Andrew (EFI) Fish; +Cc: edk2-devel-groups-io

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

Yes, the RELEASE build compiler flags should be relaxed, all error checks that the DEBUG target should do, it doesn't belong to the RELEASE's job.

---
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
index 28d9af4..ac3b5ec 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
@@ -54,7 +54,7 @@
!include MdePkg/MdeLibs.dsc.inc

[BuildOptions]
-  GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG
+  GCC:RELEASE_*_*_CC_FLAGS       = -flto
!ifdef $(SOURCE_DEBUG_ENABLE)
  GCC:*_*_RISCV64_GENFW_FLAGS    = --keepexceptiontable
!endif
--
2.39.1

I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it compiled successfully, the build log: https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

Do you have other way to add LTO flag to compile that don't change the source code, i tried to use environment variable, like exported CFLAGS=-flto to compile, but the compiler doesn't work with it.

-----原始郵件-----
發件人:"Andrew (EFI) Fish" <afish@apple.com>
發送時間:2023-08-10 21:50:55 (星期四)
收件人: edk2-devel-groups-io <devel@edk2.groups.io>, wangliu@iscas.ac.cn
抄送:
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

I think that you are advocating that since we have code that error checks on DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?

/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24: error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]
      ASSERT (Prot != NULL);
      //
      // EFI_ALREADY_STARTED is not an error for bus driver.
      // Return the corresponding protocol interface.
      //
      *Interface = Prot->Interface;

A given platform can add ASSERT into release builds if it wants. I’ve actually done that for power on before. If your compiler supports LTO you are not required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to configure your debug level, per build type. 

Maybe we should just have the error checks in all paths?

Thanks,

Andrew Fish

On Aug 10, 2023, at 6:44 AM, 汪流 <wangliu@iscas.ac.cn> wrote:

I want to build a rpm package for edk2-stable202305 on riscv64, however I get some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE

This is my packet log:   

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64

https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107792): https://edk2.groups.io/g/devel/message/107792
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-16  7:46   ` 汪流
@ 2023-08-16 17:02     ` Michael D Kinney
  2023-08-16 17:41       ` Andrew Fish via groups.io
  0 siblings, 1 reply; 13+ messages in thread
From: Michael D Kinney @ 2023-08-16 17:02 UTC (permalink / raw)
  To: devel@edk2.groups.io, wangliu@iscas.ac.cn, Andrew (EFI) Fish
  Cc: Kinney, Michael D

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

Can you provide the specific build error?

So far, we have not had to relax that flag for any RELEASE builds.

Thanks,

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of ??
Sent: Wednesday, August 16, 2023 12:46 AM
To: Andrew (EFI) Fish <afish@apple.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Yes, the RELEASE build compiler flags should be relaxed, all error checks that the DEBUG target should do, it doesn't belong to the RELEASE's job.

---
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
index 28d9af4..ac3b5ec 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
@@ -54,7 +54,7 @@
!include MdePkg/MdeLibs.dsc.inc

[BuildOptions]
-  GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG
+  GCC:RELEASE_*_*_CC_FLAGS       = -flto
!ifdef $(SOURCE_DEBUG_ENABLE)
  GCC:*_*_RISCV64_GENFW_FLAGS    = --keepexceptiontable
!endif
--
2.39.1

I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it compiled successfully, the build log: https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

Do you have other way to add LTO flag to compile that don't change the source code, i tried to use environment variable, like exported CFLAGS=-flto to compile, but the compiler doesn't work with it.

-----原始郵件-----
發件人:"Andrew (EFI) Fish" <afish@apple.com<mailto:afish@apple.com>>
發送時間:2023-08-10 21:50:55 (星期四)
收件人: edk2-devel-groups-io <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>, wangliu@iscas.ac.cn<mailto:wangliu@iscas.ac.cn>
抄送:
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
I think that you are advocating that since we have code that error checks on DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?

/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24: error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]
      ASSERT (Prot != NULL);
      //
      // EFI_ALREADY_STARTED is not an error for bus driver.
      // Return the corresponding protocol interface.
      //
      *Interface = Prot->Interface;

A given platform can add ASSERT into release builds if it wants. I’ve actually done that for power on before. If your compiler supports LTO you are not required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to configure your debug level, per build type.

Maybe we should just have the error checks in all paths?

Thanks,

Andrew Fish

On Aug 10, 2023, at 6:44 AM, 汪流 <wangliu@iscas.ac.cn<mailto:wangliu@iscas.ac.cn>> wrote:

I want to build a rpm package for edk2-stable202305 on riscv64, however I get some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE

This is my packet log:

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64
https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107799): https://edk2.groups.io/g/devel/message/107799
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-16 17:02     ` Michael D Kinney
@ 2023-08-16 17:41       ` Andrew Fish via groups.io
  2023-08-16 17:50         ` Michael D Kinney
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Fish via groups.io @ 2023-08-16 17:41 UTC (permalink / raw)
  To: edk2-devel-groups-io, Mike Kinney; +Cc: wangliu@iscas.ac.cn

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

Mike,

I seem to remember it was a maybe-uninitialized compiler error. The DEBUG build did not hit it due to the NULL check in the ASSERT. The NULL check only exists if ASSERT() is enabled. So if ASSERT is disabled this code in the DXE Core will dereference a NULL pointer. 

>       ASSERT (Prot != NULL);
>       // 
>       // EFI_ALREADY_STARTED is not an error for bus driver.
>       // Return the corresponding protocol interface.
>       // 
>       *Interface = Prot->Interface;


Thanks,

Andrew Fish

> On Aug 16, 2023, at 10:02 AM, Michael D Kinney <michael.d.kinney@intel.com> wrote:
> 
> Can you provide the specific build error?
>  
> So far, we have not had to relax that flag for any RELEASE builds.
>  
> Thanks,
>  
> Mike
>  
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of ??
> Sent: Wednesday, August 16, 2023 12:46 AM
> To: Andrew (EFI) Fish <afish@apple.com <mailto:afish@apple.com>>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>
> Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
>  
> Yes, the RELEASE build compiler flags should be relaxed, all error checks that the DEBUG target should do, it doesn't belong to the RELEASE's job.
> 
>  
> 
> --- 
> OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +- 
> 1 file changed, 1 insertion(+), 1 deletion(-) 
> 
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
> index 28d9af4..ac3b5ec 100644 
> --- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
> +++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
> @@ -54,7 +54,7 @@ 
> !include MdePkg/MdeLibs.dsc.inc 
> 
> [BuildOptions] 
> -  GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG 
> +  GCC:RELEASE_*_*_CC_FLAGS       = -flto 
> !ifdef $(SOURCE_DEBUG_ENABLE) 
>   GCC:*_*_RISCV64_GENFW_FLAGS    = --keepexceptiontable 
> !endif 
> -- 
> 2.39.1
> 
> 
> I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it compiled successfully, the build log:https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64
> 
> Do you have other way to add LTO flag to compile that don't change the source code, i tried to use environment variable, like exported CFLAGS=-flto to compile, but the compiler doesn't work with it.
> 
>  
>  
> 
>  
> 
>  
> 
> -----原始郵件-----
> 發件人:"Andrew (EFI) Fish" <afish@apple.com <mailto:afish@apple.com>>
> 發送時間:2023-08-10 21:50:55 (星期四)
> 收件人: edk2-devel-groups-io <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>, wangliu@iscas.ac.cn <mailto:wangliu@iscas.ac.cn>
> 抄送: 
> 主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
> 
> I think that you are advocating that since we have code that error checks on DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?
>  
> /home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24: error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]
>       ASSERT (Prot != NULL);
>       // 
>       // EFI_ALREADY_STARTED is not an error for bus driver.
>       // Return the corresponding protocol interface.
>       // 
>       *Interface = Prot->Interface;
>  
> A given platform can add ASSERT into release builds if it wants. I’ve actually done that for power on before. If your compiler supports LTO you are not required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to configure your debug level, per build type.  
>  
> Maybe we should just have the error checks in all paths?
>  
> Thanks, 
>  
> Andrew Fish 
> 
> 
> On Aug 10, 2023, at 6:44 AM, 汪流 <wangliu@iscas.ac.cn <mailto:wangliu@iscas.ac.cn>> wrote:
>  
> I want to build a rpm package for edk2-stable202305 on riscv64, however I get some uninitialized variable error, I have found that the reason is -Werror flag.
> My build target was release. I think the flag should used in the debug ,not in release.
> My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE
> This is my packet log:    
> 
> https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64
> 
> https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log
>  
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107801): https://edk2.groups.io/g/devel/message/107801
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-16 17:41       ` Andrew Fish via groups.io
@ 2023-08-16 17:50         ` Michael D Kinney
  2023-08-17  6:30           ` 汪流
  0 siblings, 1 reply; 13+ messages in thread
From: Michael D Kinney @ 2023-08-16 17:50 UTC (permalink / raw)
  To: Andrew (EFI) Fish, edk2-devel-groups-io
  Cc: wangliu@iscas.ac.cn, Kinney, Michael D

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

Hi Andrew,

There are compiler flags we set to suppress some of these types of specific warnings to avoid disable warnings as errors.

For example, this commit to tools_def.template:

https://github.com/tianocore/edk2/commit/8e985ac3fdb2b117968ac1fa1f54666e166af8ac

I would like to see the compiler, version, arch, log info with the specific warning being flagged to see if we can apply the techniques we have been able to apply in the past.

Mike

From: Andrew (EFI) Fish <afish@apple.com>
Sent: Wednesday, August 16, 2023 10:41 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: wangliu@iscas.ac.cn
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Mike,

I seem to remember it was a maybe-uninitialized compiler error. The DEBUG build did not hit it due to the NULL check in the ASSERT. The NULL check only exists if ASSERT() is enabled. So if ASSERT is disabled this code in the DXE Core will dereference a NULL pointer.

      ASSERT (Prot != NULL);
      //
      // EFI_ALREADY_STARTED is not an error for bus driver.
      // Return the corresponding protocol interface.
      //
      *Interface = Prot->Interface;

Thanks,

Andrew Fish

On Aug 16, 2023, at 10:02 AM, Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:

Can you provide the specific build error?

So far, we have not had to relax that flag for any RELEASE builds.

Thanks,

Mike

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of ??
Sent: Wednesday, August 16, 2023 12:46 AM
To: Andrew (EFI) Fish <afish@apple.com<mailto:afish@apple.com>>
Cc: edk2-devel-groups-io <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Yes, the RELEASE build compiler flags should be relaxed, all error checks that the DEBUG target should do, it doesn't belong to the RELEASE's job.

---
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
index 28d9af4..ac3b5ec 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
@@ -54,7 +54,7 @@
!include MdePkg/MdeLibs.dsc.inc

[BuildOptions]
-  GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG
+  GCC:RELEASE_*_*_CC_FLAGS       = -flto
!ifdef $(SOURCE_DEBUG_ENABLE)
  GCC:*_*_RISCV64_GENFW_FLAGS    = --keepexceptiontable
!endif
--
2.39.1

I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it compiled successfully, the build log:https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

Do you have other way to add LTO flag to compile that don't change the source code, i tried to use environment variable, like exported CFLAGS=-flto to compile, but the compiler doesn't work with it.

-----原始郵件-----
發件人:"Andrew (EFI) Fish" <afish@apple.com<mailto:afish@apple.com>>
發送時間:2023-08-10 21:50:55 (星期四)
收件人: edk2-devel-groups-io <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>, wangliu@iscas.ac.cn<mailto:wangliu@iscas.ac.cn>
抄送:
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
I think that you are advocating that since we have code that error checks on DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?

/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24: error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]
      ASSERT (Prot != NULL);
      //
      // EFI_ALREADY_STARTED is not an error for bus driver.
      // Return the corresponding protocol interface.
      //
      *Interface = Prot->Interface;

A given platform can add ASSERT into release builds if it wants. I’ve actually done that for power on before. If your compiler supports LTO you are not required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to configure your debug level, per build type.

Maybe we should just have the error checks in all paths?

Thanks,

Andrew Fish

On Aug 10, 2023, at 6:44 AM, 汪流 <wangliu@iscas.ac.cn<mailto:wangliu@iscas.ac.cn>> wrote:

I want to build a rpm package for edk2-stable202305 on riscv64, however I get some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE

This is my packet log:

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64
https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107802): https://edk2.groups.io/g/devel/message/107802
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-16 17:50         ` Michael D Kinney
@ 2023-08-17  6:30           ` 汪流
  2023-08-17 11:45             ` Andrew Fish via groups.io
  0 siblings, 1 reply; 13+ messages in thread
From: 汪流 @ 2023-08-17  6:30 UTC (permalink / raw)
  To: Kinney, Michael D; +Cc: Andrew (EFI) Fish, edk2-devel-groups-io

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

This is the failed build log: https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

    arch:        riscv64

    gcc version： 10.3.1

-----原始郵件-----
發件人:"Kinney, Michael D" <michael.d.kinney@intel.com>
發送時間:2023-08-17 01:50:37 (星期四)
收件人: "Andrew (EFI) Fish" <afish@apple.com>, edk2-devel-groups-io <devel@edk2.groups.io>
抄送: "wangliu@iscas.ac.cn" <wangliu@iscas.ac.cn>, "Kinney, Michael D" <michael.d.kinney@intel.com>
主題: RE: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Hi Andrew,

There are compiler flags we set to suppress some of these types of specific warnings to avoid disable warnings as errors.

For example, this commit to tools_def.template:

https://github.com/tianocore/edk2/commit/8e985ac3fdb2b117968ac1fa1f54666e166af8ac

I would like to see the compiler, version, arch, log info with the specific warning being flagged to see if we can apply the techniques we have been able to apply in the past.

Mike

From: Andrew (EFI) Fish <afish@apple.com>
Sent: Wednesday, August 16, 2023 10:41 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: wangliu@iscas.ac.cn
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Mike,

I seem to remember it was a maybe-uninitialized compiler error. The DEBUG build did not hit it due to the NULL check in the ASSERT. The NULL check only exists if ASSERT() is enabled. So if ASSERT is disabled this code in the DXE Core will dereference a NULL pointer. 

      ASSERT (Prot != NULL);

      // 

      // EFI_ALREADY_STARTED is not an error for bus driver.

      // Return the corresponding protocol interface.

      // 

      *Interface = Prot->Interface;

Thanks,

Andrew Fish

On Aug 16, 2023, at 10:02 AM, Michael D Kinney <michael.d.kinney@intel.com> wrote:

Can you provide the specific build error?

So far, we have not had to relax that flag for any RELEASE builds.

Thanks,

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of ??
Sent: Wednesday, August 16, 2023 12:46 AM
To: Andrew (EFI) Fish <afish@apple.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Yes, the RELEASE build compiler flags should be relaxed, all error checks that the DEBUG target should do, it doesn't belong to the RELEASE's job.

--- 
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +- 
1 file changed, 1 insertion(+), 1 deletion(-) 

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
index 28d9af4..ac3b5ec 100644 
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
@@ -54,7 +54,7 @@ 
!include MdePkg/MdeLibs.dsc.inc 

[BuildOptions] 
-  GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG 
+  GCC:RELEASE_*_*_CC_FLAGS       = -flto 
!ifdef $(SOURCE_DEBUG_ENABLE) 
  GCC:*_*_RISCV64_GENFW_FLAGS    = --keepexceptiontable 
!endif 
-- 
2.39.1

I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it compiled successfully, the build log:https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

Do you have other way to add LTO flag to compile that don't change the source code, i tried to use environment variable, like exported CFLAGS=-flto to compile, but the compiler doesn't work with it.

-----原始郵件-----
發件人:"Andrew (EFI) Fish" <afish@apple.com>
發送時間:2023-08-10 21:50:55 (星期四)
收件人: edk2-devel-groups-io <devel@edk2.groups.io>, wangliu@iscas.ac.cn
抄送: 
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

I think that you are advocating that since we have code that error checks on DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?

/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24: error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]

      ASSERT (Prot != NULL);

      // 

      // EFI_ALREADY_STARTED is not an error for bus driver.

      // Return the corresponding protocol interface.

      // 

      *Interface = Prot->Interface;

A given platform can add ASSERT into release builds if it wants. I’ve actually done that for power on before. If your compiler supports LTO you are not required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to configure your debug level, per build type.  

Maybe we should just have the error checks in all paths?

Thanks, 

Andrew Fish 

On Aug 10, 2023, at 6:44 AM, 汪流 <wangliu@iscas.ac.cn> wrote:

I want to build a rpm package for edk2-stable202305 on riscv64, however I get some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE

This is my packet log:    

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64

https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107824): https://edk2.groups.io/g/devel/message/107824
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-17  6:30           ` 汪流
@ 2023-08-17 11:45             ` Andrew Fish via groups.io
  2023-08-17 20:00               ` Michael D Kinney
                                 ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Andrew Fish via groups.io @ 2023-08-17 11:45 UTC (permalink / raw)
  To: devel, wangliu; +Cc: Kinney, Michael D

[-- Attachment #1: Type: text/html, Size: 24367 bytes --]

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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-17 11:45             ` Andrew Fish via groups.io
@ 2023-08-17 20:00               ` Michael D Kinney
  2023-08-17 20:17                 ` Pedro Falcato
  2023-08-21  7:04               ` 汪流
       [not found]               ` <177D54194984ABD0.15959@groups.io>
  2 siblings, 1 reply; 13+ messages in thread
From: Michael D Kinney @ 2023-08-17 20:00 UTC (permalink / raw)
  To: Andrew (EFI) Fish, devel@edk2.groups.io, wangliu@iscas.ac.cn
  Cc: Kinney, Michael D

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

I am surprised this same GCC error is not being seen for other CPU archs.

The simple fix is to this specific issue is to initialize CommandLine to NULL at the beginning of the function.

We have observed that compilers continue to add more and more static analysis like features over time.  Code that compiled without warnings/errors with an earlier compiler may generate warnings/errors on a newer compiler.  This looks like an example of this case.

Mike

From: Andrew (EFI) Fish <afish@apple.com>
Sent: Thursday, August 17, 2023 4:45 AM
To: devel@edk2.groups.io; wangliu@iscas.ac.cn
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Can you add -Wno-maybe-uninitialized to compiler flags to work around the error?
On Aug 16, 2023, at 11:31 PM, 汪流 <wangliu@iscas.ac.cn<mailto:wangliu@iscas.ac.cn>> wrote:

This is the failed build log: https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

    arch:        riscv64

    gcc version： 10.3.1

-----原始郵件-----
發件人:"Kinney, Michael D" <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
發送時間:2023-08-17 01:50:37 (星期四)
收件人: "Andrew (EFI) Fish" <afish@apple.com<mailto:afish@apple.com>>, edk2-devel-groups-io <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
抄送: "wangliu@iscas.ac.cn<mailto:wangliu@iscas.ac.cn>" <wangliu@iscas.ac.cn<mailto:wangliu@iscas.ac.cn>>, "Kinney, Michael D" <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
主題: RE: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
Hi Andrew,

There are compiler flags we set to suppress some of these types of specific warnings to avoid disable warnings as errors.

For example, this commit to tools_def.template:

https://github.com/tianocore/edk2/commit/8e985ac3fdb2b117968ac1fa1f54666e166af8ac

I would like to see the compiler, version, arch, log info with the specific warning being flagged to see if we can apply the techniques we have been able to apply in the past.

Mike

From: Andrew (EFI) Fish <afish@apple.com<mailto:afish@apple.com>>
Sent: Wednesday, August 16, 2023 10:41 AM
To: edk2-devel-groups-io <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: wangliu@iscas.ac.cn<mailto:wangliu@iscas.ac.cn>
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Mike,

I seem to remember it was a maybe-uninitialized compiler error. The DEBUG build did not hit it due to the NULL check in the ASSERT. The NULL check only exists if ASSERT() is enabled. So if ASSERT is disabled this code in the DXE Core will dereference a NULL pointer.

      ASSERT (Prot != NULL);
      //
      // EFI_ALREADY_STARTED is not an error for bus driver.
      // Return the corresponding protocol interface.
      //
      *Interface = Prot->Interface;

Thanks,

Andrew Fish

On Aug 16, 2023, at 10:02 AM, Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:

Can you provide the specific build error?

So far, we have not had to relax that flag for any RELEASE builds.

Thanks,

Mike

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of ??
Sent: Wednesday, August 16, 2023 12:46 AM
To: Andrew (EFI) Fish <afish@apple.com<mailto:afish@apple.com>>
Cc: edk2-devel-groups-io <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Yes, the RELEASE build compiler flags should be relaxed, all error checks that the DEBUG target should do, it doesn't belong to the RELEASE's job.

---
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
index 28d9af4..ac3b5ec 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
@@ -54,7 +54,7 @@
!include MdePkg/MdeLibs.dsc.inc

[BuildOptions]
-  GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG
+  GCC:RELEASE_*_*_CC_FLAGS       = -flto
!ifdef $(SOURCE_DEBUG_ENABLE)
  GCC:*_*_RISCV64_GENFW_FLAGS    = --keepexceptiontable
!endif
--
2.39.1

I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it compiled successfully, the build log:https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

Do you have other way to add LTO flag to compile that don't change the source code, i tried to use environment variable, like exported CFLAGS=-flto to compile, but the compiler doesn't work with it.

-----原始郵件-----
發件人:"Andrew (EFI) Fish" <afish@apple.com<mailto:afish@apple.com>>
發送時間:2023-08-10 21:50:55 (星期四)
收件人: edk2-devel-groups-io <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>, wangliu@iscas.ac.cn<mailto:wangliu@iscas.ac.cn>
抄送:
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
I think that you are advocating that since we have code that error checks on DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?

/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24: error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]
      ASSERT (Prot != NULL);
      //
      // EFI_ALREADY_STARTED is not an error for bus driver.
      // Return the corresponding protocol interface.
      //
      *Interface = Prot->Interface;

A given platform can add ASSERT into release builds if it wants. I’ve actually done that for power on before. If your compiler supports LTO you are not required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to configure your debug level, per build type.

Maybe we should just have the error checks in all paths?

Thanks,

Andrew Fish

On Aug 10, 2023, at 6:44 AM, 汪流 <wangliu@iscas.ac.cn<mailto:wangliu@iscas.ac.cn>> wrote:

I want to build a rpm package for edk2-stable202305 on riscv64, however I get some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE

This is my packet log:

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64
https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107838): https://edk2.groups.io/g/devel/message/107838
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-17 20:00               ` Michael D Kinney
@ 2023-08-17 20:17                 ` Pedro Falcato
  2023-08-17 20:50                   ` Michael D Kinney
  0 siblings, 1 reply; 13+ messages in thread
From: Pedro Falcato @ 2023-08-17 20:17 UTC (permalink / raw)
  To: devel, michael.d.kinney; +Cc: Andrew (EFI) Fish, wangliu@iscas.ac.cn

On Thu, Aug 17, 2023 at 9:00 PM Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> I am surprised this same GCC error is not being seen for other CPU archs.
>
>
>
> The simple fix is to this specific issue is to initialize CommandLine to NULL at the beginning of the function.

The correct fix here is to -Wno-error=maybe-uninitialized. This
warning is error prone and IIRC already disabled in the Linux kernel.
Heck, Wno-error=uninitialized wouldn't be a bad idea either, we
recently uncovered a GCC bug on it
(https://lore.kernel.org/all/20230719190045.4007391-1-arnd@kernel.org/).
>
> We have observed that compilers continue to add more and more static analysis like features over time.  Code that compiled without warnings/errors with an earlier compiler may generate warnings/errors on a newer compiler.  This looks like an example of this case.
Well, in this case the static analysis is wrong :)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107839): https://edk2.groups.io/g/devel/message/107839
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-17 20:17                 ` Pedro Falcato
@ 2023-08-17 20:50                   ` Michael D Kinney
  0 siblings, 0 replies; 13+ messages in thread
From: Michael D Kinney @ 2023-08-17 20:50 UTC (permalink / raw)
  To: Pedro Falcato, devel@edk2.groups.io
  Cc: Andrew (EFI) Fish, wangliu@iscas.ac.cn, Kinney, Michael D

Hi Pedro,

I agree that compiler and static analysis absolutely do get false positive
results.

When we see these, we have a choice to make
* Document for all consumers that a specific tool has a false positive and
  we can all safely ignore the report. This is challenging to communicate
  these to all downstream consumers.
* Disable warnings/errors if the report is from a compiler.
* Do a small source code change to address the false positive.

We have a history of doing the last 2.

I would also point out that when these false positives are generated,
they tend to be in larger, more complex functions.  Another option 
to consider is to refactor the code into smaller parts so the code
is both easier to maintain/support and improve the static analysis
results with fewer false positives.

Each report needs to be handled on a case by case basis.

Mike



> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Thursday, August 17, 2023 1:18 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Andrew (EFI) Fish <afish@apple.com>; wangliu@iscas.ac.cn
> Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
> 
> On Thu, Aug 17, 2023 at 9:00 PM Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
> >
> > I am surprised this same GCC error is not being seen for other CPU
> archs.
> >
> >
> >
> > The simple fix is to this specific issue is to initialize CommandLine
> to NULL at the beginning of the function.
> 
> The correct fix here is to -Wno-error=maybe-uninitialized. This
> warning is error prone and IIRC already disabled in the Linux kernel.
> Heck, Wno-error=uninitialized wouldn't be a bad idea either, we
> recently uncovered a GCC bug on it
> (https://lore.kernel.org/all/20230719190045.4007391-1-arnd@kernel.org/).
> >
> > We have observed that compilers continue to add more and more static
> analysis like features over time.  Code that compiled without
> warnings/errors with an earlier compiler may generate warnings/errors on
> a newer compiler.  This looks like an example of this case.
> Well, in this case the static analysis is wrong :)
> 
> --
> Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107840): https://edk2.groups.io/g/devel/message/107840
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
  2023-08-17 11:45             ` Andrew Fish via groups.io
  2023-08-17 20:00               ` Michael D Kinney
@ 2023-08-21  7:04               ` 汪流
       [not found]               ` <177D54194984ABD0.15959@groups.io>
  2 siblings, 0 replies; 13+ messages in thread
From: 汪流 @ 2023-08-21  7:04 UTC (permalink / raw)
  To: Andrew (EFI) Fish; +Cc: devel, Kinney, Michael D

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

I successfully resolved the build issue by adding the flag -Wno-maybe-uninitialized in the BaseTools/Conf/tools_def.template file. I have also submitted a patch email to the mailing list with the subject: '[edk2-devel] [PATCH] BaseTools/Conf/tools_def.template: Fix uninitialized variable error. Here's the link to the successful build log: https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64.

-----原始郵件-----
發件人:"Andrew (EFI) Fish" <afish@apple.com>
發送時間:2023-08-17 19:45:01 (星期四)
收件人: devel@edk2.groups.io, wangliu@iscas.ac.cn
抄送: "Kinney, Michael D" <michael.d.kinney@intel.com>
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Can you add -Wno-maybe-uninitialized to compiler flags to work around the error?
On Aug 16, 2023, at 11:31 PM, 汪流 <wangliu@iscas.ac.cn> wrote:

This is the failed build log: https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

    arch:        riscv64

    gcc version： 10.3.1

-----原始郵件-----
發件人:"Kinney, Michael D" <michael.d.kinney@intel.com>
發送時間:2023-08-17 01:50:37 (星期四)
收件人: "Andrew (EFI) Fish" <afish@apple.com>, edk2-devel-groups-io <devel@edk2.groups.io>
抄送: "wangliu@iscas.ac.cn" <wangliu@iscas.ac.cn>, "Kinney, Michael D" <michael.d.kinney@intel.com>
主題: RE: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Hi Andrew,

There are compiler flags we set to suppress some of these types of specific warnings to avoid disable warnings as errors.

For example, this commit to tools_def.template:

https://github.com/tianocore/edk2/commit/8e985ac3fdb2b117968ac1fa1f54666e166af8ac

I would like to see the compiler, version, arch, log info with the specific warning being flagged to see if we can apply the techniques we have been able to apply in the past.

Mike

From: Andrew (EFI) Fish <afish@apple.com>
Sent: Wednesday, August 16, 2023 10:41 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: wangliu@iscas.ac.cn
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Mike,

I seem to remember it was a maybe-uninitialized compiler error. The DEBUG build did not hit it due to the NULL check in the ASSERT. The NULL check only exists if ASSERT() is enabled. So if ASSERT is disabled this code in the DXE Core will dereference a NULL pointer. 

      ASSERT (Prot != NULL);

      // 

      // EFI_ALREADY_STARTED is not an error for bus driver.

      // Return the corresponding protocol interface.

      // 

      *Interface = Prot->Interface;

Thanks,

Andrew Fish

On Aug 16, 2023, at 10:02 AM, Michael D Kinney <michael.d.kinney@intel.com> wrote:

Can you provide the specific build error?

So far, we have not had to relax that flag for any RELEASE builds.

Thanks,

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of ??
Sent: Wednesday, August 16, 2023 12:46 AM
To: Andrew (EFI) Fish <afish@apple.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Yes, the RELEASE build compiler flags should be relaxed, all error checks that the DEBUG target should do, it doesn't belong to the RELEASE's job.

--- 
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +- 
1 file changed, 1 insertion(+), 1 deletion(-) 

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
index 28d9af4..ac3b5ec 100644 
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
@@ -54,7 +54,7 @@ 
!include MdePkg/MdeLibs.dsc.inc 

[BuildOptions] 
-  GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG 
+  GCC:RELEASE_*_*_CC_FLAGS       = -flto 
!ifdef $(SOURCE_DEBUG_ENABLE) 
  GCC:*_*_RISCV64_GENFW_FLAGS    = --keepexceptiontable 
!endif 
-- 
2.39.1

I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it compiled successfully, the build log:https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

Do you have other way to add LTO flag to compile that don't change the source code, i tried to use environment variable, like exported CFLAGS=-flto to compile, but the compiler doesn't work with it.

-----原始郵件-----
發件人:"Andrew (EFI) Fish" <afish@apple.com>
發送時間:2023-08-10 21:50:55 (星期四)
收件人: edk2-devel-groups-io <devel@edk2.groups.io>, wangliu@iscas.ac.cn
抄送: 
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

I think that you are advocating that since we have code that error checks on DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?

/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24: error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]

      ASSERT (Prot != NULL);

      // 

      // EFI_ALREADY_STARTED is not an error for bus driver.

      // Return the corresponding protocol interface.

      // 

      *Interface = Prot->Interface;

A given platform can add ASSERT into release builds if it wants. I’ve actually done that for power on before. If your compiler supports LTO you are not required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to configure your debug level, per build type.  

Maybe we should just have the error checks in all paths?

Thanks, 

Andrew Fish 

On Aug 10, 2023, at 6:44 AM, 汪流 <wangliu@iscas.ac.cn> wrote:

I want to build a rpm package for edk2-stable202305 on riscv64, however I get some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE

This is my packet log:    

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64

https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107912): https://edk2.groups.io/g/devel/message/107912
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

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

[parent not found: <177D54194984ABD0.15959@groups.io>]

* Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？
       [not found]               ` <177D54194984ABD0.15959@groups.io>
@ 2023-08-25 10:25                 ` 汪流
  0 siblings, 0 replies; 13+ messages in thread
From: 汪流 @ 2023-08-25 10:25 UTC (permalink / raw)
  To: devel; +Cc: Andrew (EFI) Fish, Kinney, Michael D

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

Is there an update on the status of the PR? Is there any additional information or feedback needed before it can be merged?

This is the PR： https://github.com/tianocore/edk2/pull/4734

-----原始郵件-----
發件人:"汪流" <wangliu@iscas.ac.cn>
發送時間:2023-08-21 15:04:54 (星期一)
收件人: "Andrew (EFI) Fish" <afish@apple.com>
抄送: devel@edk2.groups.io, "Kinney, Michael D" <michael.d.kinney@intel.com>
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

I successfully resolved the build issue by adding the flag -Wno-maybe-uninitialized in the BaseTools/Conf/tools_def.template file. I have also submitted a patch email to the mailing list with the subject: '[edk2-devel] [PATCH] BaseTools/Conf/tools_def.template: Fix uninitialized variable error. Here's the link to the successful build log: https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64.

-----原始郵件-----
發件人:"Andrew (EFI) Fish" <afish@apple.com>
發送時間:2023-08-17 19:45:01 (星期四)
收件人: devel@edk2.groups.io, wangliu@iscas.ac.cn
抄送: "Kinney, Michael D" <michael.d.kinney@intel.com>
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Can you add -Wno-maybe-uninitialized to compiler flags to work around the error?

On Aug 16, 2023, at 11:31 PM, 汪流 <wangliu@iscas.ac.cn> wrote:

This is the failed build log: https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

    arch:        riscv64

    gcc version： 10.3.1

-----原始郵件-----
發件人:"Kinney, Michael D" <michael.d.kinney@intel.com>
發送時間:2023-08-17 01:50:37 (星期四)
收件人: "Andrew (EFI) Fish" <afish@apple.com>, edk2-devel-groups-io <devel@edk2.groups.io>
抄送: "wangliu@iscas.ac.cn" <wangliu@iscas.ac.cn>, "Kinney, Michael D" <michael.d.kinney@intel.com>
主題: RE: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Hi Andrew,

There are compiler flags we set to suppress some of these types of specific warnings to avoid disable warnings as errors.

For example, this commit to tools_def.template:

https://github.com/tianocore/edk2/commit/8e985ac3fdb2b117968ac1fa1f54666e166af8ac

I would like to see the compiler, version, arch, log info with the specific warning being flagged to see if we can apply the techniques we have been able to apply in the past.

Mike

From: Andrew (EFI) Fish <afish@apple.com>
Sent: Wednesday, August 16, 2023 10:41 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: wangliu@iscas.ac.cn
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Mike,

I seem to remember it was a maybe-uninitialized compiler error. The DEBUG build did not hit it due to the NULL check in the ASSERT. The NULL check only exists if ASSERT() is enabled. So if ASSERT is disabled this code in the DXE Core will dereference a NULL pointer. 

      ASSERT (Prot != NULL);

      // 

      // EFI_ALREADY_STARTED is not an error for bus driver.

      // Return the corresponding protocol interface.

      // 

      *Interface = Prot->Interface;

Thanks,

Andrew Fish

On Aug 16, 2023, at 10:02 AM, Michael D Kinney <michael.d.kinney@intel.com> wrote:

Can you provide the specific build error?

So far, we have not had to relax that flag for any RELEASE builds.

Thanks,

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of ??
Sent: Wednesday, August 16, 2023 12:46 AM
To: Andrew (EFI) Fish <afish@apple.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>
Subject: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

Yes, the RELEASE build compiler flags should be relaxed, all error checks that the DEBUG target should do, it doesn't belong to the RELEASE's job.

--- 
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 2 +- 
1 file changed, 1 insertion(+), 1 deletion(-) 

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
index 28d9af4..ac3b5ec 100644 
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc 
@@ -54,7 +54,7 @@ 
!include MdePkg/MdeLibs.dsc.inc 

[BuildOptions] 
-  GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG 
+  GCC:RELEASE_*_*_CC_FLAGS       = -flto 
!ifdef $(SOURCE_DEBUG_ENABLE) 
  GCC:*_*_RISCV64_GENFW_FLAGS    = --keepexceptiontable 
!endif 
-- 
2.39.1

I have added the LTO flag and dropped MDEPKG_NDEBUG in the .dsc file, it compiled successfully, the build log:https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:openEuler:Mainline/edk2/openEuler_Mainline_standard_riscv64_gcc/riscv64

Do you have other way to add LTO flag to compile that don't change the source code, i tried to use environment variable, like exported CFLAGS=-flto to compile, but the compiler doesn't work with it.

-----原始郵件-----
發件人:"Andrew (EFI) Fish" <afish@apple.com>
發送時間:2023-08-10 21:50:55 (星期四)
收件人: edk2-devel-groups-io <devel@edk2.groups.io>, wangliu@iscas.ac.cn
抄送: 
主題: Re: [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？

I think that you are advocating that since we have code that error checks on DEBUG and not RELEASE builds we should relax the RELEASE build compiler flags?

/home/abuild/rpmbuild/BUILD/edk2-edk2-stable202305/MdeModulePkg/Core/Dxe/Hand/Handle.c:1183:24: error: ‘Prot’ may be used uninitialized [-Werror=maybe-uninitialized]

      ASSERT (Prot != NULL);

      // 

      // EFI_ALREADY_STARTED is not an error for bus driver.

      // Return the corresponding protocol interface.

      // 

      *Interface = Prot->Interface;

A given platform can add ASSERT into release builds if it wants. I’ve actually done that for power on before. If your compiler supports LTO you are not required to set MDEPKG_NDEBUG on RELEASE builds, and you can used a PCD to configure your debug level, per build type.  

Maybe we should just have the error checks in all paths?

Thanks, 

Andrew Fish 

On Aug 10, 2023, at 6:44 AM, 汪流 <wangliu@iscas.ac.cn> wrote:

I want to build a rpm package for edk2-stable202305 on riscv64, however I get some uninitialized variable error, I have found that the reason is -Werror flag.
My build target was release. I think the flag should used in the debug ,not in release.
My build command:  build -t GCC5 -n $NCPUS -b RELEASE -a RISCV64 -p OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc -D SECURE_BOOT_ENABLE=TRUE -D TPM_ENABLE=TRUE -D TPM_CONFIG_ENABLE=TRUE

This is my packet log:    

https://build.tarsier-infra.com/package/live_build_log/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/edk2-test/openEuler_Mainline_standard_riscv64_gcc/riscv64

https://build.tarsier-infra.com/build/home:ouuleilei:branches:home:ouuleilei:branches:openEuler:Mainline/openEuler_Mainline_standard_riscv64_gcc/riscv64/edk2-test/_log

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108036): https://edk2.groups.io/g/devel/message/108036
Mute This Topic: https://groups.io/mt/100663199/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

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

end of thread, other threads:[~2023-08-25 10:25 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-10 10:44 [edk2-devel] Can RELEASE target disable -Werror CC_FLAG？ 汪流
2023-08-10 13:50 ` Andrew Fish via groups.io
2023-08-16  7:46   ` 汪流
2023-08-16 17:02     ` Michael D Kinney
2023-08-16 17:41       ` Andrew Fish via groups.io
2023-08-16 17:50         ` Michael D Kinney
2023-08-17  6:30           ` 汪流
2023-08-17 11:45             ` Andrew Fish via groups.io
2023-08-17 20:00               ` Michael D Kinney
2023-08-17 20:17                 ` Pedro Falcato
2023-08-17 20:50                   ` Michael D Kinney
2023-08-21  7:04               ` 汪流
     [not found]               ` <177D54194984ABD0.15959@groups.io>
2023-08-25 10:25                 ` 汪流

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