[edk2] [PATCH] BaseTools/GCC: allow unused but set variables

Discussion:

[edk2] [PATCH] BaseTools/GCC: allow unused but set variables

Ard Biesheuvel

2015-07-08 13:24:18 UTC

This fixes a recurring problem where patches that have only been
tested on MS toolchains break the build on GCC because they use
variables that are only written but never read.

However, there is a valid pattern where this may happen as well.
For instance,

Status = SomeFunc (&OutVar);
ASSERT_EFI_ERROR (Status);
if (Outvar == ... ) { ... }
// Status never referenced again

may never access Status again at all in RELEASE builds, since the
ASSERT_EFI_ERROR () macro evaluates to nothing in that case.

So let's allow this pattern, by passing the appropriate GCC command
line option.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
BaseTools/Conf/tools_def.template | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 7edd7590956b..15d8db04232f 100644
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3813,7 +3813,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /LTCG /DLL /OPT:REF
DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =

-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h
+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h -Wno-error=unused-but-set-variable
DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency

--
1.9.1

Olivier Martin

2015-07-08 14:59:10 UTC

For ARM architectures, I only disable this flag for RELEASE build:

$ grep -r unused-but-set-variable Conf/tools_def.txt
DEFINE GCC46_IA32_CC_FLAGS = DEF(GCC45_IA32_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
DEFINE GCC46_X64_CC_FLAGS = DEF(GCC45_X64_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
RELEASE_GCC46_ARM_CC_FLAGS = DEF(GCC46_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_ARM_CC_FLAGS = DEF(GCC47_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_AARCH64_CC_FLAGS = DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: 08 July 2015 14:24
To: edk2-***@lists.sourceforge.net; ***@redhat.com; Olivier Martin; ***@linaro.org; ***@intel.com
Cc: ***@linaro.org; Ard Biesheuvel
Subject: [PATCH] BaseTools/GCC: allow unused but set variables

This fixes a recurring problem where patches that have only been tested on MS toolchains break the build on GCC because they use variables that are only written but never read.

However, there is a valid pattern where this may happen as well.
For instance,

Status = SomeFunc (&OutVar);
ASSERT_EFI_ERROR (Status);
if (Outvar == ... ) { ... }
// Status never referenced again

may never access Status again at all in RELEASE builds, since the ASSERT_EFI_ERROR () macro evaluates to nothing in that case.

So let's allow this pattern, by passing the appropriate GCC command line option.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
BaseTools/Conf/tools_def.template | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 7edd7590956b..15d8db04232f 100644
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3813,7 +3813,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /LTCG /DLL /OPT:REF
DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =

-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h
+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h -Wno-error=unused-but-set-variable
DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency
--
1.9.1

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Ard Biesheuvel

2015-07-08 15:02:30 UTC

OK, I hadn't noticed that. This means my patch would be only for the
convenience of the Intel engineers (and my peace of mind as operator
of a Jenkins job that gets broken all the time :-)) since the other
use case I described is already covered by these.

So please disregard my patch then.

Post by Olivier Martin
$ grep -r unused-but-set-variable Conf/tools_def.txt
DEFINE GCC46_IA32_CC_FLAGS = DEF(GCC45_IA32_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
DEFINE GCC46_X64_CC_FLAGS = DEF(GCC45_X64_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
RELEASE_GCC46_ARM_CC_FLAGS = DEF(GCC46_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_ARM_CC_FLAGS = DEF(GCC47_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_AARCH64_CC_FLAGS = DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
-----Original Message-----
Sent: 08 July 2015 14:24
Subject: [PATCH] BaseTools/GCC: allow unused but set variables
This fixes a recurring problem where patches that have only been tested on MS toolchains break the build on GCC because they use variables that are only written but never read.
However, there is a valid pattern where this may happen as well.
For instance,
Status = SomeFunc (&OutVar);
ASSERT_EFI_ERROR (Status);
if (Outvar == ... ) { ... }
// Status never referenced again
may never access Status again at all in RELEASE builds, since the ASSERT_EFI_ERROR () macro evaluates to nothing in that case.
So let's allow this pattern, by passing the appropriate GCC command line option.
Contributed-under: TianoCore Contribution Agreement 1.0
---
BaseTools/Conf/tools_def.template | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 7edd7590956b..15d8db04232f 100644
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3813,7 +3813,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /LTCG /DLL /OPT:REF
DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h
+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h -Wno-error=unused-but-set-variable
DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency
--
1.9.1
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Laszlo Ersek

2015-07-08 15:58:04 UTC

Post by Ard Biesheuvel
OK, I hadn't noticed that. This means my patch would be only for the
convenience of the Intel engineers (and my peace of mind as operator
of a Jenkins job that gets broken all the time :-)) since the other
use case I described is already covered by these.
So please disregard my patch then.

I think your idea is good. For RELEASE builds, the valid pattern you
described should be kept working, but for DEBUG and NOOPT builds, the
warning should be emitted and it should also break the build. It's
regrettable that it's always us having to report / clean up after the
Intel developers working with MSVC, but ultimately it leads to a tidier
code base.

(Perhaps NOOPT should be handled similarly to RELEASE, and not DEBUG. In
that case I should update the patch I attached to my other email. We
don't really use NOOPT with gcc at all, so I included it in my patch
only for completeness.)

Thanks
Laszlo

Post by Ard Biesheuvel

Post by Olivier Martin
$ grep -r unused-but-set-variable Conf/tools_def.txt
DEFINE GCC46_IA32_CC_FLAGS = DEF(GCC45_IA32_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
DEFINE GCC46_X64_CC_FLAGS = DEF(GCC45_X64_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
RELEASE_GCC46_ARM_CC_FLAGS = DEF(GCC46_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_ARM_CC_FLAGS = DEF(GCC47_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_AARCH64_CC_FLAGS = DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
-----Original Message-----
Sent: 08 July 2015 14:24
Subject: [PATCH] BaseTools/GCC: allow unused but set variables
This fixes a recurring problem where patches that have only been tested on MS toolchains break the build on GCC because they use variables that are only written but never read.
However, there is a valid pattern where this may happen as well.
For instance,
Status = SomeFunc (&OutVar);
ASSERT_EFI_ERROR (Status);
if (Outvar == ... ) { ... }
// Status never referenced again
may never access Status again at all in RELEASE builds, since the ASSERT_EFI_ERROR () macro evaluates to nothing in that case.
So let's allow this pattern, by passing the appropriate GCC command line option.
Contributed-under: TianoCore Contribution Agreement 1.0
---
BaseTools/Conf/tools_def.template | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 7edd7590956b..15d8db04232f 100644
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3813,7 +3813,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /LTCG /DLL /OPT:REF
DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h
+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h -Wno-error=unused-but-set-variable
DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency
--
1.9.1
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Jordan Justen

2015-07-08 23:42:25 UTC

Post by Laszlo Ersek

Post by Ard Biesheuvel
OK, I hadn't noticed that. This means my patch would be only for the
convenience of the Intel engineers (and my peace of mind as operator
of a Jenkins job that gets broken all the time :-)) since the other
use case I described is already covered by these.
So please disregard my patch then.

I think your idea is good. For RELEASE builds, the valid pattern you
described should be kept working, but for DEBUG and NOOPT builds, the
warning should be emitted and it should also break the build. It's
regrettable that it's always us having to report / clean up after the
Intel developers working with MSVC, but ultimately it leads to a tidier
code base.

What is the use case? Someone has a regular build pool that *only*
does RELEASE builds?

I think nearly all automated build systems will also do DEBUG builds,
and thus will still see the build error.

What about a lower bar for committing build break fixes? What if we
said that compiler warning fixes could be committed by any package
maintainer for any package as long as it is an obvious trivial fix and
it has at least one r-b?

I think qemu has a 'trivial' patch process. I can't remember the
details, but it may involve just Cc'ing the list with a different
name. Like:

Cc: edk2-trivial <edk2-***@lists.sourceforge.net>

Anyway, I don't really support this build flag change, but I suppose
it could be acceptable for RELEASE builds.

-Jordan

Post by Laszlo Ersek
(Perhaps NOOPT should be handled similarly to RELEASE, and not DEBUG. In
that case I should update the patch I attached to my other email. We
don't really use NOOPT with gcc at all, so I included it in my patch
only for completeness.)
Thanks
Laszlo

Post by Ard Biesheuvel

Post by Olivier Martin
$ grep -r unused-but-set-variable Conf/tools_def.txt
DEFINE GCC46_IA32_CC_FLAGS = DEF(GCC45_IA32_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
DEFINE GCC46_X64_CC_FLAGS = DEF(GCC45_X64_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
RELEASE_GCC46_ARM_CC_FLAGS = DEF(GCC46_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_ARM_CC_FLAGS = DEF(GCC47_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_AARCH64_CC_FLAGS = DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
-----Original Message-----
Sent: 08 July 2015 14:24
Subject: [PATCH] BaseTools/GCC: allow unused but set variables
This fixes a recurring problem where patches that have only been tested on MS toolchains break the build on GCC because they use variables that are only written but never read.
However, there is a valid pattern where this may happen as well.
For instance,
Status = SomeFunc (&OutVar);
ASSERT_EFI_ERROR (Status);
if (Outvar == ... ) { ... }
// Status never referenced again
may never access Status again at all in RELEASE builds, since the ASSERT_EFI_ERROR () macro evaluates to nothing in that case.
So let's allow this pattern, by passing the appropriate GCC command line option.
Contributed-under: TianoCore Contribution Agreement 1.0
---
BaseTools/Conf/tools_def.template | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 7edd7590956b..15d8db04232f 100644
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3813,7 +3813,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /LTCG /DLL /OPT:REF
DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h
+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h -Wno-error=unused-but-set-variable
DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency
--
1.9.1
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Laszlo Ersek

2015-07-09 04:56:36 UTC

Post by Jordan Justen

Post by Laszlo Ersek

Post by Ard Biesheuvel
OK, I hadn't noticed that. This means my patch would be only for the
convenience of the Intel engineers (and my peace of mind as operator
of a Jenkins job that gets broken all the time :-)) since the other
use case I described is already covered by these.
So please disregard my patch then.

I think your idea is good. For RELEASE builds, the valid pattern you
described should be kept working, but for DEBUG and NOOPT builds, the
warning should be emitted and it should also break the build. It's
regrettable that it's always us having to report / clean up after the
Intel developers working with MSVC, but ultimately it leads to a tidier
code base.

What is the use case? Someone has a regular build pool that *only*
does RELEASE builds?
I think nearly all automated build systems will also do DEBUG builds,
and thus will still see the build error.
What about a lower bar for committing build break fixes? What if we
said that compiler warning fixes could be committed by any package
maintainer for any package as long as it is an obvious trivial fix and
it has at least one r-b?

That sounds pretty good to me!

Post by Jordan Justen
I think qemu has a 'trivial' patch process. I can't remember the
details, but it may involve just Cc'ing the list with a different

http://wiki.qemu.org/Contribute/TrivialPatches

Post by Jordan Justen
Anyway, I don't really support this build flag change, but I suppose
it could be acceptable for RELEASE builds.

I think Ard abandoned the idea on seeing Olivier's followup, and I did
the same when I saw Bill's answer.

Your idea about streamlining the current fixup process is a good one;
let's adopt it. Does it need to be codified somewhere (Maintainers.txt,
Contributions.txt, ...)?

Cheers!
Laszlo

Post by Jordan Justen
-Jordan

Post by Laszlo Ersek
(Perhaps NOOPT should be handled similarly to RELEASE, and not DEBUG. In
that case I should update the patch I attached to my other email. We
don't really use NOOPT with gcc at all, so I included it in my patch
only for completeness.)
Thanks
Laszlo

Post by Ard Biesheuvel

Post by Olivier Martin
$ grep -r unused-but-set-variable Conf/tools_def.txt
DEFINE GCC46_IA32_CC_FLAGS = DEF(GCC45_IA32_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
DEFINE GCC46_X64_CC_FLAGS = DEF(GCC45_X64_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
RELEASE_GCC46_ARM_CC_FLAGS = DEF(GCC46_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_ARM_CC_FLAGS = DEF(GCC47_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_AARCH64_CC_FLAGS = DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
-----Original Message-----
Sent: 08 July 2015 14:24
Subject: [PATCH] BaseTools/GCC: allow unused but set variables
This fixes a recurring problem where patches that have only been tested on MS toolchains break the build on GCC because they use variables that are only written but never read.
However, there is a valid pattern where this may happen as well.
For instance,
Status = SomeFunc (&OutVar);
ASSERT_EFI_ERROR (Status);
if (Outvar == ... ) { ... }
// Status never referenced again
may never access Status again at all in RELEASE builds, since the ASSERT_EFI_ERROR () macro evaluates to nothing in that case.
So let's allow this pattern, by passing the appropriate GCC command line option.
Contributed-under: TianoCore Contribution Agreement 1.0
---
BaseTools/Conf/tools_def.template | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 7edd7590956b..15d8db04232f 100644
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3813,7 +3813,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /LTCG /DLL /OPT:REF
DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h
+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h -Wno-error=unused-but-set-variable
DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency
--
1.9.1
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Ard Biesheuvel

2015-07-09 07:16:16 UTC

Post by Jordan Justen

Post by Laszlo Ersek

Post by Ard Biesheuvel
OK, I hadn't noticed that. This means my patch would be only for the
convenience of the Intel engineers (and my peace of mind as operator
of a Jenkins job that gets broken all the time :-)) since the other
use case I described is already covered by these.
So please disregard my patch then.

I think your idea is good. For RELEASE builds, the valid pattern you
described should be kept working, but for DEBUG and NOOPT builds, the
warning should be emitted and it should also break the build. It's
regrettable that it's always us having to report / clean up after the
Intel developers working with MSVC, but ultimately it leads to a tidier
code base.

What is the use case? Someone has a regular build pool that *only*
does RELEASE builds?
I think nearly all automated build systems will also do DEBUG builds,
and thus will still see the build error.
What about a lower bar for committing build break fixes? What if we
said that compiler warning fixes could be committed by any package
maintainer for any package as long as it is an obvious trivial fix and
it has at least one r-b?

Yes, I think that sounds reasonable, and I would prefer it over
relaxing the GCC error policies.

Thanks,
Ard.

Post by Jordan Justen
I think qemu has a 'trivial' patch process. I can't remember the
details, but it may involve just Cc'ing the list with a different
Anyway, I don't really support this build flag change, but I suppose
it could be acceptable for RELEASE builds.
-Jordan

Post by Laszlo Ersek
(Perhaps NOOPT should be handled similarly to RELEASE, and not DEBUG. In
that case I should update the patch I attached to my other email. We
don't really use NOOPT with gcc at all, so I included it in my patch
only for completeness.)
Thanks
Laszlo

Post by Ard Biesheuvel

Post by Olivier Martin
$ grep -r unused-but-set-variable Conf/tools_def.txt
DEFINE GCC46_IA32_CC_FLAGS = DEF(GCC45_IA32_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
DEFINE GCC46_X64_CC_FLAGS = DEF(GCC45_X64_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
RELEASE_GCC46_ARM_CC_FLAGS = DEF(GCC46_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_ARM_CC_FLAGS = DEF(GCC47_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC47_AARCH64_CC_FLAGS = DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-variable
RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
-----Original Message-----
Sent: 08 July 2015 14:24
Subject: [PATCH] BaseTools/GCC: allow unused but set variables
This fixes a recurring problem where patches that have only been tested on MS toolchains break the build on GCC because they use variables that are only written but never read.
However, there is a valid pattern where this may happen as well.
For instance,
Status = SomeFunc (&OutVar);
ASSERT_EFI_ERROR (Status);
if (Outvar == ... ) { ... }
// Status never referenced again
may never access Status again at all in RELEASE builds, since the ASSERT_EFI_ERROR () macro evaluates to nothing in that case.
So let's allow this pattern, by passing the appropriate GCC command line option.
Contributed-under: TianoCore Contribution Agreement 1.0
---
BaseTools/Conf/tools_def.template | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 7edd7590956b..15d8db04232f 100644
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3813,7 +3813,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /LTCG /DLL /OPT:REF
DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h
+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h -Wno-error=unused-but-set-variable
DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency
--
1.9.1
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Laszlo Ersek

2015-07-08 15:54:46 UTC

Post by Ard Biesheuvel
This fixes a recurring problem where patches that have only been
tested on MS toolchains break the build on GCC because they use
variables that are only written but never read.
However, there is a valid pattern where this may happen as well.
For instance,
Status = SomeFunc (&OutVar);
ASSERT_EFI_ERROR (Status);
if (Outvar == ... ) { ... }
// Status never referenced again
may never access Status again at all in RELEASE builds, since the
ASSERT_EFI_ERROR () macro evaluates to nothing in that case.
So let's allow this pattern, by passing the appropriate GCC command
line option.
Contributed-under: TianoCore Contribution Agreement 1.0
---
BaseTools/Conf/tools_def.template | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 7edd7590956b..15d8db04232f 100644
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3813,7 +3813,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /LTCG /DLL /OPT:REF
DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h
+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h -Wno-error=unused-but-set-variable
DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency

The least recent gcc version supported by the build tools is gcc-4.4.
That version supports the -Wno-error option, according to the documentation.

However, the "unused-but-set-variable" option argument is only supported
starting with gcc-4.6; again according to the documentation:

https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/Warning-Options.html
https://gcc.gnu.org/onlinedocs/gcc-4.5.4/gcc/Warning-Options.html
https://gcc.gnu.org/onlinedocs/gcc-4.6.4/gcc/Warning-Options.html
https://gcc.gnu.org/onlinedocs/gcc-4.7.4/gcc/Warning-Options.html
https://gcc.gnu.org/onlinedocs/gcc-4.8.5/gcc/Warning-Options.html
https://gcc.gnu.org/onlinedocs/gcc-4.9.3/gcc/Warning-Options.html

Also, from <https://gcc.gnu.org/gcc-4.6/changes.html>:

* New -Wunused-but-set-variable and -Wunused-but-set-parameter
warnings were added for C, C++, Objective-C and Objective-C++.
These warnings diagnose variables respective parameters which are
only set in the code and never otherwise used. Usually such
variables are useless and often even the value assigned to them is
computed needlessly, sometimes expensively. The
-Wunused-but-set-variable warning is enabled by default by -Wall
flag and -Wunused-but-set-parameter by -Wall -Wextra flags

Regarding ASSERT_EFI_ERROR() being compiled out for RELEASE: that
implies we should add this option only for RELEASE builds. The GCC flags
can be filtered / specified for build type.

Combining these two restrictions, I think we should append this new gcc
option to

RELEASE_GCC46_*_CC_FLAGS
RELEASE_GCC47_*_CC_FLAGS
RELEASE_GCC48_*_CC_FLAGS
RELEASE_GCC49_*_CC_FLAGS

at most.

... Actually, we already have "-Wno-unused-but-set-variable" in a bunch
of places in "BaseTools/Conf/tools_def.template", so I'd rather propose
the attached patch.

Thanks
Laszlo

Scott Duplichan

2015-07-08 15:28:41 UTC

Ard Biesheuvel [mailto:***@linaro.org] wrote:

]Sent: Wednesday, July 08, 2015 08:24 AM
]To: edk2-***@lists.sourceforge.net; ***@redhat.com; ***@arm.com; ***@linaro.org; ]***@intel.com
]Subject: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables
]
]This fixes a recurring problem where patches that have only been
]tested on MS toolchains break the build on GCC because they use
]variables that are only written but never read.

An alternative point of view is that anyone who tests with MS tool chains
should also test with gcc tool chains. Windows hosted gcc tool chains of
all supported versions are free, small and easy to download and use:
http://sourceforge.net/projects/edk2developertoolsforwindows/

]However, there is a valid pattern where this may happen as well.
]For instance,
]
] Status = SomeFunc (&OutVar);
] ASSERT_EFI_ERROR (Status);
] if (Outvar == ... ) { ... }
] // Status never referenced again
]
]may never access Status again at all in RELEASE builds, since the
]ASSERT_EFI_ERROR () macro evaluates to nothing in that case.

To me this gcc warning is a good thing because it is pointing out
a coding problem. The coding problem is that error handling is
completely missing from the release build. Once the coding problem
is fixed, the warning goes away. After years of debate on the role
of ASSERT in EDK2 code, there is now a sort of official opinion in
the EDK2 Coding document:

ASSERT macros are development and debugging aids and
shall never be used for error handling. Assertions are
used to catch conditions caused by programming errors
that are resolved prior to product release.

The EDK II PCD, PcdDebugPropertyMask, can be used to
enable or disable the generation of code associated with
ASSERT usage. Thus, all code must be able to operate, and
recover in a reasonable maner [sic] with ASSERTs disabled.

If you never plan on using a release build, which I think might
be the case with OVMF, the RELEASE option could be removed from
the DSC file and -Wno-error=unused-but-set-variable could be
added through the DSC file for that project only.

Just my opinion anyway. We all know from the old Microsoft tool chains
how annoying unwanted warnings are.

Thanks,
Scott

]So let's allow this pattern, by passing the appropriate GCC command
]line option.
]
]Contributed-under: TianoCore Contribution Agreement 1.0
]Signed-off-by: Ard Biesheuvel <***@linaro.org>
]---
] BaseTools/Conf/tools_def.template | 2 +-
] 1 file changed, 1 insertion(+), 1 deletion(-)
]
]diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
]index 7edd7590956b..15d8db04232f 100644
]--- a/BaseTools/Conf/tools_def.template
]+++ b/BaseTools/Conf/tools_def.template
]@@ -3813,7 +3813,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO /NODEFAULTLIB /LTCG /DLL /OPT:REF
] DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
] RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
]
]-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include AutoGen.h
]+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include AutoGen.h -Wno-error=unused-but-set-variable
] DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-]blocks-and-partition -O2 -mno-stack-arg-probe
] DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
] DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency
]--
]1.9.1

Laszlo Ersek

2015-07-08 16:17:08 UTC

Post by Scott Duplichan
]Sent: Wednesday, July 08, 2015 08:24 AM
]Subject: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables
]
]This fixes a recurring problem where patches that have only been
]tested on MS toolchains break the build on GCC because they use
]variables that are only written but never read.
An alternative point of view is that anyone who tests with MS tool chains
should also test with gcc tool chains. Windows hosted gcc tool chains of
http://sourceforge.net/projects/edk2developertoolsforwindows/

I agree 100%, but experience shows that MSVC users cannot be bothered.

Post by Scott Duplichan
]However, there is a valid pattern where this may happen as well.
]For instance,
]
] Status = SomeFunc (&OutVar);
] ASSERT_EFI_ERROR (Status);
] if (Outvar == ... ) { ... }
] // Status never referenced again
]
]may never access Status again at all in RELEASE builds, since the
]ASSERT_EFI_ERROR () macro evaluates to nothing in that case.
To me this gcc warning is a good thing because it is pointing out
a coding problem. The coding problem is that error handling is
completely missing from the release build. Once the coding problem
is fixed, the warning goes away. After years of debate on the role
of ASSERT in EDK2 code, there is now a sort of official opinion in
ASSERT macros are development and debugging aids and
shall never be used for error handling. Assertions are
used to catch conditions caused by programming errors
that are resolved prior to product release.
The EDK II PCD, PcdDebugPropertyMask, can be used to
enable or disable the generation of code associated with
ASSERT usage. Thus, all code must be able to operate, and
recover in a reasonable maner [sic] with ASSERTs disabled.

In the above pattern, ASSERT_EFI_ERROR() is not necessarily used for
error handling. It is perfectly possible that the programmer knows that,
at that point, SomeFunc() *must* succeed, given the circumstances. One
way to express that is to write a comment before the function call, and
then ignore the return value completely. Another way is to add
ASSERT_EFI_ERROR().

If an invariant turns out to be false (fully unexpectedly), there's no
way to "recover"; the system state has been corrupted, and further
behavior is undefined.

So yes, ASSERT_EFI_ERROR() can be mis-used for error handling, but it is
not necessarily the only use case. For a given SomeFunc() -- and
situation --, the only reasonable "error handling" could be:

//
// This shall succeed here.
//
Status = SomeFunc (&OutVar);
ASSERT_EFI_ERROR (Status);

if (EFI_ERROR (Status)) {
//
// this should have never happened; system state is corrupt and we
// must not continue
//
CpuDeadLoop();
}

But I guess this just files under "debate on the role of ASSERT in EDK2
code" :)

Post by Scott Duplichan
If you never plan on using a release build, which I think might
be the case with OVMF, the RELEASE option could be removed from
the DSC file and -Wno-error=unused-but-set-variable could be
added through the DSC file for that project only.

Well, *I* don't plan to use anything but DEBUG builds of OVMF (because
ASSERT()s should never be compiled out, due to the above argument), but
others might want to.

Anyway, if, according to the refreshed coding style, ASSERT() must not
be used any longer for stating (and enforcing) invariants, then I guess
I'll just stop using ASSERT() altogether, and begin using _ASSERT()
instead, with an explicit check around it.

Namely, _ASSERT() is never compiled out. It is also never disabled by
PcdDebugPropertyMask. Yet, it does adhere to PcdDebugPropertyMask with
the *action* taken. So, henceforth I should be writing

//
// This shall succeed here.
//
Status = SomeFunc (&OutVar);

if (EFI_ERROR (Status)) {
//
// this should have never happened; system state is corrupt and we
// must not continue
//
_ASSERT (FALSE);
}

Thanks for helping me make up my mind :)

Post by Scott Duplichan
Just my opinion anyway. We all know from the old Microsoft tool chains
how annoying unwanted warnings are.

Right. I do agree with you that the optimal solution would be a build
farm where all developers could test-build against all supported
compilers. I doubt it's ever going to happen.

Thanks
Laszlo

Post by Scott Duplichan
Thanks,
Scott
]So let's allow this pattern, by passing the appropriate GCC command
]line option.
]
]Contributed-under: TianoCore Contribution Agreement 1.0
]---
] BaseTools/Conf/tools_def.template | 2 +-
] 1 file changed, 1 insertion(+), 1 deletion(-)
]
]diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
]index 7edd7590956b..15d8db04232f 100644
]--- a/BaseTools/Conf/tools_def.template
]+++ b/BaseTools/Conf/tools_def.template
] DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
] RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
]
]-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include AutoGen.h
]+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include AutoGen.h -Wno-error=unused-but-set-variable
] DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-]blocks-and-partition -O2 -mno-stack-arg-probe
] DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
] DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency
]--
]1.9.1

Bill Paul

2015-07-08 18:15:44 UTC

Of all the gin joints in all the towns in all the world, Laszlo Ersek had to

Post by Laszlo Ersek

Post by Scott Duplichan
]Sent: Wednesday, July 08, 2015 08:24 AM
unused but set variables ]
]This fixes a recurring problem where patches that have only been
]tested on MS toolchains break the build on GCC because they use
]variables that are only written but never read.
An alternative point of view is that anyone who tests with MS tool chains
should also test with gcc tool chains. Windows hosted gcc tool chains of
http://sourceforge.net/projects/edk2developertoolsforwindows/

I agree 100%, but experience shows that MSVC users cannot be bothered.

Microsoft uses the Windows Logo program to force developers to run their
driver code through the driver verifier (and pass) before they're allowed to
slap the Windows Logo on their product. This was partly because MSVC users
"couldn't be bothered" to rigorously validate their code, and Microsoft was
the one taking the heat when a bug in a vendor-supplied driver would lead to
Windows blue-screening. Maybe something similar culd be done here. :)

Post by Laszlo Ersek

Post by Scott Duplichan
]However, there is a valid pattern where this may happen as well.
]For instance,
]
] Status = SomeFunc (&OutVar);
] ASSERT_EFI_ERROR (Status);
] if (Outvar == ... ) { ... }
] // Status never referenced again
]
]may never access Status again at all in RELEASE builds, since the
]ASSERT_EFI_ERROR () macro evaluates to nothing in that case.
To me this gcc warning is a good thing because it is pointing out
a coding problem. The coding problem is that error handling is
completely missing from the release build. Once the coding problem
is fixed, the warning goes away. After years of debate on the role
of ASSERT in EDK2 code, there is now a sort of official opinion in
ASSERT macros are development and debugging aids and
shall never be used for error handling. Assertions are
used to catch conditions caused by programming errors
that are resolved prior to product release.
The EDK II PCD, PcdDebugPropertyMask, can be used to
enable or disable the generation of code associated with
ASSERT usage. Thus, all code must be able to operate, and
recover in a reasonable maner [sic] with ASSERTs disabled.

In the above pattern, ASSERT_EFI_ERROR() is not necessarily used for
error handling.

You are absolutely right: it's not. It's being used as a panic(). The problem
is that panic() is usually a function, not a macro, and it isn't compiled out
of the system. But ASSERT_EFI_ERROR() can be. If you build with the cross-
compiler toolchain (UNIXGCC) or you do a RELEASE build, then OVMF does this:

GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG

When you define MDEPKG_NDEBUG, ASSERT_EFI_ERROR(), ASSERT() and DEBUG() are
all no-ops.

Post by Laszlo Ersek
It is perfectly possible that the programmer knows that,
at that point, SomeFunc() *must* succeed, given the circumstances. One
way to express that is to write a comment before the function call, and
then ignore the return value completely. Another way is to add
ASSERT_EFI_ERROR().

I'm sorry, but no. That is not "another way." If ASSERT_EFI_ERROR() were
always present (i.e. even if you do -DMDEPKG_NDEBUG, it's still there), _THEN_
you could make this argument. But it's not.

Post by Laszlo Ersek
If an invariant turns out to be false (fully unexpectedly), there's no
way to "recover"; the system state has been corrupted, and further
behavior is undefined.

Yes. This is why you panic().

Post by Laszlo Ersek
So yes, ASSERT_EFI_ERROR() can be mis-used for error handling, but it is
not necessarily the only use case. For a given SomeFunc() -- and
//
// This shall succeed here.
//
Status = SomeFunc (&OutVar);
ASSERT_EFI_ERROR (Status);
if (EFI_ERROR (Status)) {
//
// this should have never happened; system state is corrupt and we
// must not continue
//
CpuDeadLoop();
}
But I guess this just files under "debate on the role of ASSERT in EDK2
code" :)

No. You can file it under "anti-pattern." Because that's what it is.

I also suspect that it would violate MISRA C rules, which in general preach
that code should be unambiguous to the reader and never provoke any undefined
behavior or side-effects. In the case where ASSERT_EFI_ERROR() is the only
thing referencing a local variable, there is a side-effect: in the absence of
GCC's warning, you're depending on the compiler to optimize away the variable
and the code that references it in the case where you do a RELEASE build.

Post by Laszlo Ersek

Post by Scott Duplichan
If you never plan on using a release build, which I think might
be the case with OVMF, the RELEASE option could be removed from
the DSC file and -Wno-error=unused-but-set-variable could be
added through the DSC file for that project only.

Well, *I* don't plan to use anything but DEBUG builds of OVMF (because
ASSERT()s should never be compiled out, due to the above argument), but
others might want to.

If it was never intended that ASSERT() et. al. to be compiled out, what was
the point of making them conditional (and having a RELEASE build target where
they're turned off) in the first place?

Post by Laszlo Ersek
Anyway, if, according to the refreshed coding style, ASSERT() must not
be used any longer for stating (and enforcing) invariants, then I guess
I'll just stop using ASSERT() altogether, and begin using _ASSERT()
instead, with an explicit check around it.

Even if the coding standard originally required you to use ASSERT() to test
invariants, I don't think it mandated that it be done in such a way that you'd
end up with the code pattern we're talking about here.

Post by Laszlo Ersek
Namely, _ASSERT() is never compiled out. It is also never disabled by
PcdDebugPropertyMask. Yet, it does adhere to PcdDebugPropertyMask with
the *action* taken. So, henceforth I should be writing
//
// This shall succeed here.
//
Status = SomeFunc (&OutVar);
if (EFI_ERROR (Status)) {
//
// this should have never happened; system state is corrupt and we
// must not continue
//
_ASSERT (FALSE);
}
Thanks for helping me make up my mind :)

That's great, but take heed: ASSERT() and ASSERT_EFI_ERROR() are not the only
macros being abused here. There is also DEBUG(). You will find several
instances of the following construction:

int SomeInterestingValue;

/* Populate SomeInterestingValue */

SomeInterestingValue = GetSomeInterestingStatus (handle);

/* SomeInterestingValue is never referenced again */

DEBUG(EFI_D_INFO, "INTERESTING VALUE IS: %d\n", SomeInterestingValue);

DEBUG() is also disabled when MDEPKG_NDEBUG is enabled, which creates the same
problem. What would be the correct way to handle this case?

It should be noted that once in a while I run into this sort of thing in
VxWorks as well, only it's often the other way around. We typically have
DEBUG()-style messages turned off by default, and sometimes there are debug
messages that refer to variables which no longer exist in the code. (The
variables were removed but the debug messages were never updated to match.)

Post by Laszlo Ersek

Post by Scott Duplichan
Just my opinion anyway. We all know from the old Microsoft tool chains
how annoying unwanted warnings are.

Right. I do agree with you that the optimal solution would be a build
farm where all developers could test-build against all supported
compilers. I doubt it's ever going to happen.

Given that UEFI firmware is supposed to be the standard for a large number of
commercially available computer platforms, I find your lack of faith...
disturbing.

Lastly, yesterday I actually spent some time updating the mingw-gcc-build.py
to support binutils 2.25 and GCC 4.9.3, and I ran into this exact issue. I had
to add -Wno-unused-but-set-variable to the UNIXGCC instances of CC_FLAGS in
tools_def.template in order to get the OVMF build to work. But along the way,
I noticed about a dozen instances of this anti-pattern. There are at least as
many cases that stem from DEBUG() rather than ASSERT() or ASSERT_EFI_ERROR().

However I also found at least one legitimate error case of a useless "set but
never used" variable too (i.e. a case that didn't involve conditional
compilation effects). This means that now that GCC has been muzzled to
disguise intentional abuses of this pattern, some unintentional abuses are now
being hidden too.

-Bill

Post by Laszlo Ersek
Thanks
Laszlo

Post by Scott Duplichan
Thanks,
Scott
]So let's allow this pattern, by passing the appropriate GCC command
]line option.
]
]Contributed-under: TianoCore Contribution Agreement 1.0
]---
] BaseTools/Conf/tools_def.template | 2 +-
] 1 file changed, 1 insertion(+), 1 deletion(-)
]
]diff --git a/BaseTools/Conf/tools_def.template
b/BaseTools/Conf/tools_def.template ]index 7edd7590956b..15d8db04232f
100644
]--- a/BaseTools/Conf/tools_def.template
]+++ b/BaseTools/Conf/tools_def.template
/NODEFAULTLIB /LTCG /DLL /OPT:REF ] DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG =
--add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug ]
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
]
]-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar
-fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include
AutoGen.h ]+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar
-fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include
AutoGen.h -Wno-error=unused-but-set-variable ] DEFINE GCC_IA32_CC_FLAGS
= DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks
-freorder-]blocks-and-partition -O2 -mno-stack-arg-probe ] DEFINE
GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone
-Wno-address -mno-stack-arg-probe ] DEFINE GCC_IPF_CC_FLAGS =
DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency ]--
]1.9.1

---------------------------------------------------------------------------
--- Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

--
=============================================================================
-Bill Paul (510) 749-2329 | Senior Member of Technical Staff,
***@windriver.com | Master of Unix-Fu - Wind River Systems
=============================================================================
"I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

Laszlo Ersek

2015-07-08 19:02:10 UTC

Post by Bill Paul
Of all the gin joints in all the towns in all the world, Laszlo Ersek had to

Post by Laszlo Ersek
In the above pattern, ASSERT_EFI_ERROR() is not necessarily used for
error handling.

You are absolutely right: it's not. It's being used as a panic().

Good point! I guess I've always used ASSERT() in the sense of
PANIC_UNLESS().

Post by Bill Paul
[...] There is also DEBUG(). You will find several
int SomeInterestingValue;
/* Populate SomeInterestingValue */
SomeInterestingValue = GetSomeInterestingStatus (handle);
/* SomeInterestingValue is never referenced again */
DEBUG(EFI_D_INFO, "INTERESTING VALUE IS: %d\n", SomeInterestingValue);
DEBUG() is also disabled when MDEPKG_NDEBUG is enabled, which creates the same
problem. What would be the correct way to handle this case?

Honestly, I don't know. I only concern myself with the case when even
DEBUG_VERBOSE is enabled (PcdDebugPrintErrorLevel == 0x8040004F). The
case when I have to look at debug messages and analyze behavior is the
*norm*, not the exception (even if OVMF is not at fault -- the user may
have passed wrong QEMU options, for example).

It's an incredible chore when a user reports behavior he doesn't
understand or expect, and I first have to ask him to reproduce on a
DEBUG_VERBOSE build, and that he send me *that* log. Usually he has no
access to such a build, cannot build one himself, so I get to build it
for him. Blech. (I think Gerd's packages do enable DEBUG_VERBOSE, but
I'm not sure.)

Post by Bill Paul
It should be noted that once in a while I run into this sort of thing in
VxWorks as well, only it's often the other way around. We typically have
DEBUG()-style messages turned off by default, and sometimes there are debug
messages that refer to variables which no longer exist in the code. (The
variables were removed but the debug messages were never updated to match.)

SeaBIOS employs the following trick to avoid this: all CONFIG_XXXX flags
are checked in regular if() statements, not in #if macros. That is, any
elmination happens during compilation / optimization / linking, not in
preprocessing. This allows the compiler to catch such errors even when a
CONFIG_XXXX option is predominantly false (or predominantly true).

Post by Bill Paul

Post by Laszlo Ersek
Right. I do agree with you that the optimal solution would be a build
farm where all developers could test-build against all supported
compilers. I doubt it's ever going to happen.

Given that UEFI firmware is supposed to be the standard for a large number of
commercially available computer platforms, I find your lack of faith...
disturbing.

My lack of faith is based on "experience" :) We've been complaining
about this for ages on the list. I think it is safe to assume that the
primary "participant" that has legal access to all supported Microsoft
toolchains is Intel. As described (and provided!) by Scott,
gcc-for-Windows is almost trivially available (all supported gcc
versions). It seems to follow that Intel should operate such a build
farm. Based on how long we've been whining about this, I don't think
it's going to happen any time soon.

Post by Bill Paul
Lastly, yesterday I actually spent some time updating the mingw-gcc-build.py
to support binutils 2.25 and GCC 4.9.3, and I ran into this exact issue. I had
to add -Wno-unused-but-set-variable to the UNIXGCC instances of CC_FLAGS in
tools_def.template in order to get the OVMF build to work. But along the way,
I noticed about a dozen instances of this anti-pattern. There are at least as
many cases that stem from DEBUG() rather than ASSERT() or ASSERT_EFI_ERROR().
However I also found at least one legitimate error case of a useless "set but
never used" variable too (i.e. a case that didn't involve conditional
compilation effects). This means that now that GCC has been muzzled to
disguise intentional abuses of this pattern, some unintentional abuses are now
being hidden too.

Optimally, we shouldn't conflate the different (deeper) reasons this
warning gets emitted for. Unfortunately, it's not practical even for me
to build OVMF in all possible configurations:
- DEBUG and RELEASE (2)
- gcc 4.4 to 4.9 (6)
- Ia32, X64, Ia32X64 for OVMF (3)

That's 36 configs (and yes, the data flow analysis is specific to both
target architecture and gcc version, so some warnings would only be
found in configs that I can't test).

Add ARM and AARCH64 for ArmVirtPkg (QEMU and Xen separately)... Secure
Boot enabled vs. disabled... ARM BDS vs. Intel BDS... For OVMF, CSM
enabled vs. disabled; IPv6 enabled vs. disabled... It's really
unmanageable without a build farm.

Yes, someone could say "hey Laszlo why don't you create a number of
virtual machines, with different gcc versions installed, and implement
at least the 'free software' build farm?" -- Because I don't have time
for it. (And that's the reason I can't really blame Intel people either,
for not setting up the MSVC+gcc build farm. Someone would have to take a
*personal* hit.)

For now we can say that we'll fix up such errors when we find out about
them (and thus we don't patch the warning flags now). Annoying, but it
does lead to a better code base.

Thanks
Laszlo

Post by Bill Paul
-Bill

Post by Laszlo Ersek
Thanks
Laszlo

Post by Scott Duplichan
Thanks,
Scott
]So let's allow this pattern, by passing the appropriate GCC command
]line option.
]
]Contributed-under: TianoCore Contribution Agreement 1.0
]---
] BaseTools/Conf/tools_def.template | 2 +-
] 1 file changed, 1 insertion(+), 1 deletion(-)
]
]diff --git a/BaseTools/Conf/tools_def.template
b/BaseTools/Conf/tools_def.template ]index 7edd7590956b..15d8db04232f
100644
]--- a/BaseTools/Conf/tools_def.template
]+++ b/BaseTools/Conf/tools_def.template
/NODEFAULTLIB /LTCG /DLL /OPT:REF ] DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG =
--add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug ]
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
]
]-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar
-fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include
AutoGen.h ]+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar
-fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include
AutoGen.h -Wno-error=unused-but-set-variable ] DEFINE GCC_IA32_CC_FLAGS
= DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks
-freorder-]blocks-and-partition -O2 -mno-stack-arg-probe ] DEFINE
GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone
-Wno-address -mno-stack-arg-probe ] DEFINE GCC_IPF_CC_FLAGS =
DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency ]--
]1.9.1

---------------------------------------------------------------------------
--- Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Andrew Fish

2015-07-08 19:26:32 UTC

Post by Laszlo Ersek

Post by Bill Paul
It should be noted that once in a while I run into this sort of thing in
VxWorks as well, only it's often the other way around. We typically have
DEBUG()-style messages turned off by default, and sometimes there are debug
messages that refer to variables which no longer exist in the code. (The
variables were removed but the debug messages were never updated to match.)

SeaBIOS employs the following trick to avoid this: all CONFIG_XXXX flags
are checked in regular if() statements, not in #if macros. That is, any
elmination happens during compilation / optimization / linking, not in
preprocessing. This allows the compiler to catch such errors even when a
CONFIG_XXXX option is predominantly false (or predominantly true).

This is how the edk2 works. Your platform has to âopt-inâ to the #if. The default is to use PCDs, and that is what happens on VC++. The MDEPKG_NDEBUG
#define was added for compilers, like gcc, that could not dead strip the code.

https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Include/Library/DebugLib.h <https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Include/Library/DebugLib.h>
#if !defined(MDEPKG_NDEBUG)
#define ASSERT(Expression) \
do { \
if (DebugAssertEnabled ()) { \
if (!(Expression)) { \
_ASSERT (Expression); \
} \
} \
} while (FALSE)
#else
#define ASSERT(Expression)
#endif
If you are not using the optional MDEPKG_NDEBUG flag then DEBUG and ASSERT macros just become policy for a release build based on PCD.

Also in most of the DebugLib instances what do on the ASSERT is a policy choice (breakpoint, dead loop, no-op).
https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c <https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c>
VOID
EFIAPI
DebugAssert (
IN CONST CHAR8 *FileName,
IN UINTN LineNumber,
IN CONST CHAR8 *Description
)
{
CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH];

//
// Generate the ASSERT() message in Ascii format
//
AsciiSPrint (Buffer, sizeof (Buffer), "ASSERT %a(%d): %a\n", FileName, LineNumber, Description);

//
// Send the print string to the Console Output device
//
SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));

//
// Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
//
if ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
CpuBreakpoint ();
} else if ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
CpuDeadLoop ();
}
}
Thanks,

Andrew Fish

Laszlo Ersek

2015-07-08 20:43:38 UTC

Post by Laszlo Ersek

Post by Bill Paul
It should be noted that once in a while I run into this sort of thing in
VxWorks as well, only it's often the other way around. We typically have
DEBUG()-style messages turned off by default, and sometimes there are debug
messages that refer to variables which no longer exist in the code. (The
variables were removed but the debug messages were never updated to match.)

SeaBIOS employs the following trick to avoid this: all CONFIG_XXXX flags
are checked in regular if() statements, not in #if macros. That is, any
elmination happens during compilation / optimization / linking, not in
preprocessing. This allows the compiler to catch such errors even when a
CONFIG_XXXX option is predominantly false (or predominantly true).

This is how the edk2 works. Your platform has to “opt-in” to the #if.
The default is to use PCDs, and that is what happens on VC++.
The MDEPKG_NDEBUG
#define was added for compilers, like gcc, that could not dead strip the code.
https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Include/Library/DebugLib.h
#if !defined(MDEPKG_NDEBUG)
#define ASSERT(Expression) \
do { \
if (DebugAssertEnabled ()) { \
if (!(Expression)) { \
_ASSERT (Expression); \
} \
} \
} while (FALSE)
#else
#define ASSERT(Expression)
#endif
If you are not using the optional MDEPKG_NDEBUG flag then DEBUG and
ASSERT macros just become policy for a release build based on PCD.

Ah! That's very useful background info.

Perhaps it's time for dropping MDEPKG_NDEBUG? We have:

[BuildOptions]
GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG

I wonder why MDEPKG_NDEBUG was then added for MSFT in the first place
(assuming MSVC had always been able to remove dead code), but in any
case, perhaps we should restrict MDEPKG_NDEBUG to RELEASE builds with
older (4.4, 4.5, ... ?) gcc.

... I experimented a bit now. I removed MDEPKG_NDEBUG, and watched how
the size of DXEFV changed when I flipped the DEBUG_VERBOSE bit on and
off. Summary: no matter what I tried in place of MDEPKG_NDEBUG (-O0,
-O1, -O1 -flto), given one specific CC_FLAGS (and DLINK_FLAGS) setting,
inverting the DEBUG_VERBOSE bit in the PCD mask had no effect.

I think it would make a difference if -flto *actually* worked, but from
a quick google search, I think it either doesn't work with gcc-4.8 at
all, *or* the edk2 build system would have to use FLTO-aware binutils
and linker wrappers (or parameters). I have no clue how to set that up.
So for now we'll have to stick with MDEPKG_NDEBUG I guess.

Thanks!
Laszlo

Also in most of the DebugLib instances what do on the ASSERT is a policy
choice (breakpoint, dead loop, no-op).
https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
VOID
EFIAPI
DebugAssert (
IN CONST CHAR8 *FileName,
IN UINTN LineNumber,
IN CONST CHAR8 *Description
)
{
CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH];
//
// Generate the ASSERT() message in Ascii format
//
AsciiSPrint (Buffer, sizeof (Buffer), "ASSERT %a(%d): %a\n", FileName, LineNumber, Description);
//
// Send the print string to the Console Output device
//
SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
//
// Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
//
if ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
CpuBreakpoint ();
} else if ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
CpuDeadLoop ();
}
}
Thanks,
Andrew Fish
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Andrew Fish

2015-07-08 20:53:50 UTC

Post by Laszlo Ersek

Post by Andrew Fish
https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Include/Library/DebugLib.h <https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Include/Library/DebugLib.h>
#if !defined(MDEPKG_NDEBUG)
#define ASSERT(Expression) \
do { \
if (DebugAssertEnabled ()) { \
if (!(Expression)) { \
_ASSERT (Expression); \
} \
} \
} while (FALSE)
#else
#define ASSERT(Expression)
#endif
If you are not using the optional MDEPKG_NDEBUG flag then DEBUG and
ASSERT macros just become policy for a release build based on PCD.

Ah! That's very useful background info.
[BuildOptions]
GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
I wonder why MDEPKG_NDEBUG was then added for MSFT in the first place
(assuming MSVC had always been able to remove dead code),

It might make MSVC catch the same errors as GCC? Not sure about that?

I wonder is some hackery could make MDEPKG_NDEBUG path behave the same way?

#else
#define ASSERT(Expression) do {} while (FALSE && (Expression))
#endif

Maybe we could find something that all the compilers dead strip, but behaves more like the non #if path?

Thanks,

Andrew Fish

Post by Laszlo Ersek
but in any
case, perhaps we should restrict MDEPKG_NDEBUG to RELEASE builds with
older (4.4, 4.5, ... ?) gcc.
... I experimented a bit now. I removed MDEPKG_NDEBUG, and watched how
the size of DXEFV changed when I flipped the DEBUG_VERBOSE bit on and
off. Summary: no matter what I tried in place of MDEPKG_NDEBUG (-O0,
-O1, -O1 -flto), given one specific CC_FLAGS (and DLINK_FLAGS) setting,
inverting the DEBUG_VERBOSE bit in the PCD mask had no effect.
I think it would make a difference if -flto *actually* worked, but from
a quick google search, I think it either doesn't work with gcc-4.8 at
all, *or* the edk2 build system would have to use FLTO-aware binutils
and linker wrappers (or parameters). I have no clue how to set that up.
So for now we'll have to stick with MDEPKG_NDEBUG I guess.
Thanks!
Laszlo

Laszlo Ersek

2015-07-08 21:21:24 UTC

Post by Andrew Fish

Post by Laszlo Ersek

Post by Andrew Fish
https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Include/Library/DebugLib.h
#if !defined(MDEPKG_NDEBUG)
#define ASSERT(Expression) \
do { \
if (DebugAssertEnabled ()) { \
if (!(Expression)) { \
_ASSERT (Expression); \
} \
} \
} while (FALSE)
#else
#define ASSERT(Expression)
#endif
If you are not using the optional MDEPKG_NDEBUG flag then DEBUG and
ASSERT macros just become policy for a release build based on PCD.

Ah! That's very useful background info.
[BuildOptions]
GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
I wonder why MDEPKG_NDEBUG was then added for MSFT in the first place
(assuming MSVC had always been able to remove dead code),

It might make MSVC catch the same errors as GCC? Not sure about that?
I wonder is some hackery could make MDEPKG_NDEBUG path behave the same way?
#else
#define ASSERT(Expression) do {} while (FALSE && (Expression))
#endif

I guess this could work, yes.

Post by Andrew Fish
Maybe we could find something that all the compilers dead strip, but
behaves more like the non #if path?

One idea (not sure if it makes any sense): the DebugAssertEnabled()
function call could be replaced with an open coded *fixed* PCD check.
(Yes, this would actually remove the DebugAssertEnabled() function from
the DebugLib interface, because the implementation would become
centralized. It would also require all platforms to provide the PCD as a
fixed one.)

Then no cross object file analysis would be necessary, just "basic"
constant folding and local dead code elimination.

Something like:

#define ASSERT(Expression) \
do { \
if ((FixedPcdGet8 (PcdDebugPropertyMask) & \
DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0 && \
!(Expression)) { \
_ASSERT (Expression); \
} \
} while (FALSE)

Similarly for DebugPrintEnabled().

Thanks
Laszlo

Post by Andrew Fish
Thanks,
Andrew Fish

Post by Laszlo Ersek
but in any
case, perhaps we should restrict MDEPKG_NDEBUG to RELEASE builds with
older (4.4, 4.5, ... ?) gcc.
... I experimented a bit now. I removed MDEPKG_NDEBUG, and watched how
the size of DXEFV changed when I flipped the DEBUG_VERBOSE bit on and
off. Summary: no matter what I tried in place of MDEPKG_NDEBUG (-O0,
-O1, -O1 -flto), given one specific CC_FLAGS (and DLINK_FLAGS) setting,
inverting the DEBUG_VERBOSE bit in the PCD mask had no effect.
I think it would make a difference if -flto *actually* worked, but from
a quick google search, I think it either doesn't work with gcc-4.8 at
all, *or* the edk2 build system would have to use FLTO-aware binutils
and linker wrappers (or parameters). I have no clue how to set that up.
So for now we'll have to stick with MDEPKG_NDEBUG I guess.
Thanks!
Laszlo

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Andrew Fish

2015-07-08 21:34:16 UTC

Post by Laszlo Ersek

Post by Andrew Fish

Post by Laszlo Ersek
I wonder why MDEPKG_NDEBUG was then added for MSFT in the first place
(assuming MSVC had always been able to remove dead code),

It might make MSVC catch the same errors as GCC? Not sure about that?
I wonder is some hackery could make MDEPKG_NDEBUG path behave the same way?
#else
#define ASSERT(Expression) do {} while (FALSE && (Expression))
#endif

I guess this could work, yes.

Post by Andrew Fish
Maybe we could find something that all the compilers dead strip, but
behaves more like the non #if path?

One idea (not sure if it makes any sense): the DebugAssertEnabled()
function call could be replaced with an open coded *fixed* PCD check.
(Yes, this would actually remove the DebugAssertEnabled() function from
the DebugLib interface, because the implementation would become
centralized. It would also require all platforms to provide the PCD as a
fixed one.)
Then no cross object file analysis would be necessary, just "basic"
constant folding and local dead code elimination.
#define ASSERT(Expression) \
do { \
if ((FixedPcdGet8 (PcdDebugPropertyMask) & \
DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0 && \
!(Expression)) { \
_ASSERT (Expression); \
} \
} while (FALSE)

Unfortunately this does not work for libraries. The call is in place to fix the library issue. To make the libraries portable the PCD reference needs to be an external reference from the library. If you use the PCD form that accesses the global, that does not dead strip, as that global lives in AutoGen.c and can only be optimized via LTO.

Thanks,

Andrew Fish

Post by Laszlo Ersek
Similarly for DebugPrintEnabled().
Thanks
Laszlo

Bruce Cran

2015-07-08 22:12:06 UTC

Post by Laszlo Ersek
I think it would make a difference if -flto *actually* worked, but from
a quick google search, I think it either doesn't work with gcc-4.8 at
all, *or* the edk2 build system would have to use FLTO-aware binutils
and linker wrappers (or parameters). I have no clue how to set that up.
So for now we'll have to stick with MDEPKG_NDEBUG I guess.

I've been trying to get -flto working this week, but without success (yet!).
I'm currently running into a problem of ld (built from source) apparently not
knowing about the lto plugin (and passing the liblto.so file via -plugin causes
an assert failure), but I'll keep trying.

Also, in terms of edk2-related resources, if anyone's interested I've been
running doxygen on the sources and putting the results in
http://bluestop.org/edk2/docs - the log files contain the tons of warnings the
tool generates, while the directories (http://bluestop.org/edk2/docs/trunk, http://bluestop.org/edk2/docs/UDK2014.SP1 etc.)
contain the API docs themselves. I started generating them since I noticed
feishare.com hadn't been updated for a couple of years and wasn't aware of any
other sites hosting a copy.

--
Bruce

Scott Duplichan

2015-07-09 02:31:09 UTC

Bruce Cran [mailto:***@cran.org.uk] wrote:

]Sent: Wednesday, July 08, 2015 05:12 PM
]To: edk2-***@lists.sourceforge.net
]Subject: Re: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables
]
]On Wed, Jul 08, 2015 at 10:43:38PM +0200, Laszlo Ersek wrote:
]
]> I think it would make a difference if -flto *actually* worked, but from
]> a quick google search, I think it either doesn't work with gcc-4.8 at
]> all, *or* the edk2 build system would have to use FLTO-aware binutils
]> and linker wrappers (or parameters). I have no clue how to set that up.
]> So for now we'll have to stick with MDEPKG_NDEBUG I guess.
]
]I've been trying to get -flto working this week, but without success (yet!).
]I'm currently running into a problem of ld (built from source) apparently not
]knowing about the lto plugin (and passing the liblto.so file via -plugin causes
]an assert failure), but I'll keep trying.

Last year I spent some time to get gcc -flto working properly with EDK2.
At that time, the people here were busy with other things and there didn't
seem to be a lot of interest in gcc link time optimization. So I never
submitted a patch. Maybe it is time to revive this effort. Here is what I
found:

http://notabs.org/uefi/gcc-lto.htm

So 6 conditions need to be met for ghcc link time optimization to work
properly with EDK2:

1) Add -flto to the compile flags
2) Use gcc to launch ld instead of invoking ld directly
3) Include the compile flags on the link command line
4) Use gcc-ar in place of ar when building static libraries
5) Library code that resolves helper function calls generated by the compiler must be compiled without the -flto flag
6) These libraries must be prefixed with -Wl,-plugin-opt=-pass-through= on the link command line.

A patch from late 2014 is here:
http://sourceforge.net/projects/edk2developertoolsforwindows/files/Patches/Link%20Time%20Optimization/

Thanks,
Scott

]Also, in terms of edk2-related resources, if anyone's interested I've been
]running doxygen on the sources and putting the results in
]http://bluestop.org/edk2/docs - the log files contain the tons of warnings the
]tool generates, while the directories (http://bluestop.org/edk2/docs/trunk, ]http://bluestop.org/edk2/docs/UDK2014.SP1 etc.)
]contain the API docs themselves. I started generating them since I noticed
]feishare.com hadn't been updated for a couple of years and wasn't aware of any
]other sites hosting a copy.
]
]--
]Bruce

Laszlo Ersek

2015-07-09 05:35:37 UTC

Post by Scott Duplichan
]Sent: Wednesday, July 08, 2015 05:12 PM
]Subject: Re: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables
]
]
]> I think it would make a difference if -flto *actually* worked, but from
]> a quick google search, I think it either doesn't work with gcc-4.8 at
]> all, *or* the edk2 build system would have to use FLTO-aware binutils
]> and linker wrappers (or parameters). I have no clue how to set that up.
]> So for now we'll have to stick with MDEPKG_NDEBUG I guess.
]
]I've been trying to get -flto working this week, but without success (yet!).
]I'm currently running into a problem of ld (built from source) apparently not
]knowing about the lto plugin (and passing the liblto.so file via -plugin causes
]an assert failure), but I'll keep trying.
Last year I spent some time to get gcc -flto working properly with EDK2.
At that time, the people here were busy with other things and there didn't
seem to be a lot of interest in gcc link time optimization. So I never
submitted a patch. Maybe it is time to revive this effort. Here is what I
http://notabs.org/uefi/gcc-lto.htm

I apologize if I should remember this from last year, but I don't. Sorry
about that. Do you think you could revive it? The space savings would be
nice, but more importantly (to me), that would allow us to "opt out" of
MDEPKG_NDEBUG in at least OvmfPkg. (Based on Andrew's description, I now
believe that opting out of MDEPKG_NDEBUG is a good thing -- it would
allow the compiler to check more things, rather than "hiding" those
things with the preprocessor.)

In the article you wrote,

Post by Scott Duplichan
gcc link time optimization exposes a few warnings that do not occur
with the standard builds

and I guess Bill hinted that he saw similar warnings, and at least one
of those indicated a real problem?... So maybe we should find and fix
these for good.

Post by Scott Duplichan
So 6 conditions need to be met for ghcc link time optimization to work
1) Add -flto to the compile flags

Right.

Post by Scott Duplichan
2) Use gcc to launch ld instead of invoking ld directly

This was consistent with my google results, and where I gave up.

Post by Scott Duplichan
3) Include the compile flags on the link command line

I did that too (DLINK_FLAGS?)

Post by Scott Duplichan
4) Use gcc-ar in place of ar when building static libraries

Also why I gave up... But now I checked (based on your article), and my
RHEL-7 laptop actually satisfies these requirements. It has gcc-ar, has
plugin support, etc. I ran your (very helpful!) build test from your
article, and it seemed to work:

main.exe: file format elf64-x86-64
Disassembly of section .text:
0000000000400150 <main>:
400150: 31 c0 xor eax,eax
400152: c3 ret
400153: 90 nop

Thanks!
Laszlo

Post by Scott Duplichan
5) Library code that resolves helper function calls generated by the compiler must be compiled without the -flto flag
6) These libraries must be prefixed with -Wl,-plugin-opt=-pass-through= on the link command line.
http://sourceforge.net/projects/edk2developertoolsforwindows/files/Patches/Link%20Time%20Optimization/
Thanks,
Scott
]Also, in terms of edk2-related resources, if anyone's interested I've been
]running doxygen on the sources and putting the results in
]http://bluestop.org/edk2/docs - the log files contain the tons of warnings the
]tool generates, while the directories (http://bluestop.org/edk2/docs/trunk, ]http://bluestop.org/edk2/docs/UDK2014.SP1 etc.)
]contain the API docs themselves. I started generating them since I noticed
]feishare.com hadn't been updated for a couple of years and wasn't aware of any
]other sites hosting a copy.
]
]--
]Bruce
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Bill Paul

2015-07-09 17:35:11 UTC

Of all the gin joints in all the towns in all the world, Laszlo Ersek had to
walk into mine at 22:35:37 on Wednesday 08 July 2015 and say:

[...]

Post by Laszlo Ersek

Post by Scott Duplichan
Last year I spent some time to get gcc -flto working properly with EDK2.
At that time, the people here were busy with other things and there
didn't seem to be a lot of interest in gcc link time optimization. So I
never submitted a patch. Maybe it is time to revive this effort. Here is
what I
http://notabs.org/uefi/gcc-lto.htm

I apologize if I should remember this from last year, but I don't. Sorry
about that. Do you think you could revive it? The space savings would be
nice, but more importantly (to me), that would allow us to "opt out" of
MDEPKG_NDEBUG in at least OvmfPkg. (Based on Andrew's description, I now
believe that opting out of MDEPKG_NDEBUG is a good thing -- it would
allow the compiler to check more things, rather than "hiding" those
things with the preprocessor.)
In the article you wrote,

Post by Scott Duplichan
gcc link time optimization exposes a few warnings that do not occur
with the standard builds

and I guess Bill hinted that he saw similar warnings, and at least one
of those indicated a real problem?... So maybe we should find and fix
these for good.

I went back to check on this again -- I changed the OVMF build rules to leave
MDEPKG_NDEBUG turned off to avoid any of the warnings related to ASSERT() et.
al. -- and found exactly one case:

/home/wpaul/edk/edk2/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c: In function
'QemuFwCfgFindFile':
/home/wpaul/edk/edk2/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c:280:12:
error: variable 'FileReserved' set but not used [-Werror=unused-but-set-
variable]
UINT16 FileReserved;

The code in question is:

[...]
for (Idx = 0; Idx < Count; ++Idx) {
UINT32 FileSize;
UINT16 FileSelect;
UINT16 FileReserved;
CHAR8 FName[QEMU_FW_CFG_FNAME_SIZE];

FileSize = QemuFwCfgRead32 ();
FileSelect = QemuFwCfgRead16 ();
FileReserved = QemuFwCfgRead16 ();
InternalQemuFwCfgReadBytes (sizeof (FName), FName);

if (AsciiStrCmp (Name, FName) == 0) {
*Item = SwapBytes16 (FileSelect);
*Size = SwapBytes32 (FileSize);
return RETURN_SUCCESS;
}
}
[...]

I guess this isn't really a bug. When iterating over these file structures,
it's necessary to read in this reserved field in order to consume the header
fields and advance to the start of the file name field, but since the reserved
field is not actually used the compiler complains.

I got around the warning by doing this:

[...]
FileSize = QemuFwCfgRead32 ();
FileSelect = QemuFwCfgRead16 ();
FileReserved = QemuFwCfgRead16 ();
(void)FileReserved; /* Force a do-nothing reference. */
InternalQemuFwCfgReadBytes (sizeof (FName), FName);
[...]

-Bill

--
=============================================================================
-Bill Paul (510) 749-2329 | Senior Member of Technical Staff,
***@windriver.com | Master of Unix-Fu - Wind River Systems
=============================================================================
"I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

Laszlo Ersek

2015-07-09 18:19:04 UTC

Post by Bill Paul
Of all the gin joints in all the towns in all the world, Laszlo Ersek had to
[...]

Post by Laszlo Ersek

Post by Scott Duplichan
Last year I spent some time to get gcc -flto working properly with EDK2.
At that time, the people here were busy with other things and there
didn't seem to be a lot of interest in gcc link time optimization. So I
never submitted a patch. Maybe it is time to revive this effort. Here is
what I
http://notabs.org/uefi/gcc-lto.htm

I apologize if I should remember this from last year, but I don't. Sorry
about that. Do you think you could revive it? The space savings would be
nice, but more importantly (to me), that would allow us to "opt out" of
MDEPKG_NDEBUG in at least OvmfPkg. (Based on Andrew's description, I now
believe that opting out of MDEPKG_NDEBUG is a good thing -- it would
allow the compiler to check more things, rather than "hiding" those
things with the preprocessor.)
In the article you wrote,

Post by Scott Duplichan
gcc link time optimization exposes a few warnings that do not occur
with the standard builds

and I guess Bill hinted that he saw similar warnings, and at least one
of those indicated a real problem?... So maybe we should find and fix
these for good.

I went back to check on this again -- I changed the OVMF build rules to leave
MDEPKG_NDEBUG turned off to avoid any of the warnings related to ASSERT() et.
/home/wpaul/edk/edk2/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c: In function
error: variable 'FileReserved' set but not used [-Werror=unused-but-set-
variable]
UINT16 FileReserved;
[...]
for (Idx = 0; Idx < Count; ++Idx) {
UINT32 FileSize;
UINT16 FileSelect;
UINT16 FileReserved;
CHAR8 FName[QEMU_FW_CFG_FNAME_SIZE];
FileSize = QemuFwCfgRead32 ();
FileSelect = QemuFwCfgRead16 ();
FileReserved = QemuFwCfgRead16 ();
InternalQemuFwCfgReadBytes (sizeof (FName), FName);
if (AsciiStrCmp (Name, FName) == 0) {
*Item = SwapBytes16 (FileSelect);
*Size = SwapBytes32 (FileSize);
return RETURN_SUCCESS;
}
}
[...]
I guess this isn't really a bug. When iterating over these file structures,
it's necessary to read in this reserved field in order to consume the header
fields and advance to the start of the file name field, but since the reserved
field is not actually used the compiler complains.
[...]
FileSize = QemuFwCfgRead32 ();
FileSelect = QemuFwCfgRead16 ();
FileReserved = QemuFwCfgRead16 ();
(void)FileReserved; /* Force a do-nothing reference. */
InternalQemuFwCfgReadBytes (sizeof (FName), FName);
[...]

Right. One could even say,

(VOID)QemuFwCfgRead16 ();

but the way you fixed it is much easier to read. (Just VOID should be
spelled upper case, and, theoretically, there should be a space after
"(VOID)", but that latter rule I keep actively subverting, because it is
wrong.)

How do you feel about submitting a patch for this? :)

Thanks
Laszlo

Post by Bill Paul
-Bill

Jordan Justen

2015-07-10 07:23:55 UTC

Post by Scott Duplichan
]Sent: Wednesday, July 08, 2015 05:12 PM
]Subject: Re: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables
]
]
]> I think it would make a difference if -flto *actually* worked, but from
]> a quick google search, I think it either doesn't work with gcc-4.8 at
]> all, *or* the edk2 build system would have to use FLTO-aware binutils
]> and linker wrappers (or parameters). I have no clue how to set that up.
]> So for now we'll have to stick with MDEPKG_NDEBUG I guess.
]
]I've been trying to get -flto working this week, but without success (yet!).
]I'm currently running into a problem of ld (built from source) apparently not
]knowing about the lto plugin (and passing the liblto.so file via -plugin causes
]an assert failure), but I'll keep trying.
Last year I spent some time to get gcc -flto working properly with EDK2.
At that time, the people here were busy with other things and there didn't
seem to be a lot of interest in gcc link time optimization. So I never
submitted a patch. Maybe it is time to revive this effort. Here is what I
http://notabs.org/uefi/gcc-lto.htm
So 6 conditions need to be met for ghcc link time optimization to work
1) Add -flto to the compile flags
2) Use gcc to launch ld instead of invoking ld directly
3) Include the compile flags on the link command line
4) Use gcc-ar in place of ar when building static libraries
5) Library code that resolves helper function calls generated by the compiler must be compiled without the -flto flag
6) These libraries must be prefixed with -Wl,-plugin-opt=-pass-through= on the link command line.
http://sourceforge.net/projects/edk2developertoolsforwindows/files/Patches/Link%20Time%20Optimization/

BaseTools/Contributions.txt (Contributed-under...)

If you don't submit it edk2-devel, and continue to follow up on it,
then work can easily slip through the cracks.

That said, I've spent a little time looking at your work, so it's not
true that there was no interest. But, if you have something that is
*actually working* for DuetPkg, EmulatorPkg and OvmfPkg, can you put
together a patch series?

I don't see how we can manage 5 & 6 with our current BaseTools. How
did you manage it? Searching for 'pass-through' in your patch doesn't
yield any results.

Thanks,

-Jordan

Scott Duplichan

2015-07-11 07:43:09 UTC

Jordan Justen [mailto:***@intel.com] wrote:

]Sent: Friday, July 10, 2015 02:24 AM
]To: edk2-***@lists.sourceforge.net; Scott Duplichan
]Subject: Re: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables
]
]On 2015-07-08 19:31:09, Scott Duplichan wrote:
]> Bruce Cran [mailto:***@cran.org.uk] wrote:
]>
]> ]Sent: Wednesday, July 08, 2015 05:12 PM
]> ]To: edk2-***@lists.sourceforge.net
]> ]Subject: Re: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables
]> ]
]> ]On Wed, Jul 08, 2015 at 10:43:38PM +0200, Laszlo Ersek wrote:
]> ]
]> ]> I think it would make a difference if -flto *actually* worked, but from
]> ]> a quick google search, I think it either doesn't work with gcc-4.8 at
]> ]> all, *or* the edk2 build system would have to use FLTO-aware binutils
]> ]> and linker wrappers (or parameters). I have no clue how to set that up.
]> ]> So for now we'll have to stick with MDEPKG_NDEBUG I guess.
]> ]
]> ]I've been trying to get -flto working this week, but without success (yet!).
]> ]I'm currently running into a problem of ld (built from source) apparently not
]> ]knowing about the lto plugin (and passing the liblto.so file via -plugin causes
]> ]an assert failure), but I'll keep trying.
]>
]> Last year I spent some time to get gcc -flto working properly with EDK2.
]> At that time, the people here were busy with other things and there didn't
]> seem to be a lot of interest in gcc link time optimization. So I never
]> submitted a patch. Maybe it is time to revive this effort. Here is what I
]> found:
]>
]> http://notabs.org/uefi/gcc-lto.htm
]>
]> So 6 conditions need to be met for ghcc link time optimization to work
]> properly with EDK2:
]>
]> 1) Add -flto to the compile flags
]> 2) Use gcc to launch ld instead of invoking ld directly
]> 3) Include the compile flags on the link command line
]> 4) Use gcc-ar in place of ar when building static libraries
]> 5) Library code that resolves helper function calls generated by the compiler must be compiled ]without the -flto flag
]> 6) These libraries must be prefixed with -Wl,-plugin-opt=-pass-through= on the link command line.
]>
]> A patch from late 2014 is here:
]> http://sourceforge.net/projects/edk2developertoolsforwindows/files/Patches/Link%20Time%20Optimization/
]
]BaseTools/Contributions.txt (Contributed-under...)
]
]If you don't submit it edk2-devel, and continue to follow up on it,
]then work can easily slip through the cracks.
]
]That said, I've spent a little time looking at your work, so it's not
]true that there was no interest. But, if you have something that is
]*actually working* for DuetPkg, EmulatorPkg and OvmfPkg, can you put
]together a patch series?

The patch isn't ready for review. One reason is that it exposes
a problem that has no simple solution that I can find. A BaseTools
change may be needed. Link time code generation allows gcc to warn
about things that it otherwise can't. With -flto, gcc warns about:

extern const VOID* _gPcd_FixedAtBuild_PcdShellSupplier[];
AutoGen.c:207:15: note: previously declared here
GLOBAL_REMOVE_IF_UNREFERENCED const UINT16
_gPcd_FixedAtBuild_PcdShellSupplier[7] = {69, 68, 75, 32, 73, 73, 0 };

Both definitions are generated so they can't easily be changed.
This slight inconsistency in the array definition stops the build
because of warnings as errors. For this -flto proof of concept
experiment, I disable warning as errors. But the problem needs
to be resolved before a patch can be submitted. It was discussed
in a thread titled "Status of gcc link time optimization for EDK2 use".
Sergey made a python patch, but as I remember it didn't quite fix
every occurrence.

]I don't see how we can manage 5 & 6 with our current BaseTools. How
]did you manage it? Searching for 'pass-through' in your patch doesn't
]yield any results.

I punted and omitted -flto for that case. It is only a serious problem
for ARM:

"While making the EDK2 build meet requirement 5 is easy, the same
is not true for requirement 6 in some cases. For example, the ARM
build resolves compiler generated helper function calls with
CompilerIntrinsicsLib.lib and BaseStackCheckLib.lib. The path to
these libraries varies with the package name and build options.
Without a significant modification to the EDK2 build tools, there
is no way to generate the requires prefixed path for use on the
linker command line. This limitation affects ARM builds the most
because gcc ARM code uses more compiler generated helper function
calls than other target architectures. For this reason, link time
optimization is not enabled for ARM in the gcc LTO EDK2 patch. The
use of compiler generated helper function calls is kept to a minimum
in x86 code. For the IA32 builds the compiler generates a floating
point helper call when building stdlib. The patch disables link time
optimization for IA32 stdlib to avoid the
-Wl,-plugin-opt=-pass-through= requirement."

I will update the patch to work with the current EDK2 code. Here are
another couple of things to decide.

1) Which gcc version(s) get -flto? Probably 4.9 and later make sense.
2) Use of gcc -flto probably needs to be optional. We could make
two tool chain definitions, such as GCC49 and GCC49LTO. But that
creates a lot of duplication in an already big template file.
For the proof of concept experiment, -flto can be disabled by
setting an environment variable.

]Thanks,
]
]-Jordan

Laszlo Ersek

2015-07-13 11:52:10 UTC

Post by Scott Duplichan
]Sent: Friday, July 10, 2015 02:24 AM
]Subject: Re: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables
]
]>
]> ]Sent: Wednesday, July 08, 2015 05:12 PM
]> ]Subject: Re: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables
]> ]
]> ]
]> ]> I think it would make a difference if -flto *actually* worked, but from
]> ]> a quick google search, I think it either doesn't work with gcc-4.8 at
]> ]> all, *or* the edk2 build system would have to use FLTO-aware binutils
]> ]> and linker wrappers (or parameters). I have no clue how to set that up.
]> ]> So for now we'll have to stick with MDEPKG_NDEBUG I guess.
]> ]
]> ]I've been trying to get -flto working this week, but without success (yet!).
]> ]I'm currently running into a problem of ld (built from source) apparently not
]> ]knowing about the lto plugin (and passing the liblto.so file via -plugin causes
]> ]an assert failure), but I'll keep trying.
]>
]> Last year I spent some time to get gcc -flto working properly with EDK2.
]> At that time, the people here were busy with other things and there didn't
]> seem to be a lot of interest in gcc link time optimization. So I never
]> submitted a patch. Maybe it is time to revive this effort. Here is what I
]>
]> http://notabs.org/uefi/gcc-lto.htm
]>
]> So 6 conditions need to be met for ghcc link time optimization to work
]>
]> 1) Add -flto to the compile flags
]> 2) Use gcc to launch ld instead of invoking ld directly
]> 3) Include the compile flags on the link command line
]> 4) Use gcc-ar in place of ar when building static libraries
]> 5) Library code that resolves helper function calls generated by the compiler must be compiled ]without the -flto flag
]> 6) These libraries must be prefixed with -Wl,-plugin-opt=-pass-through= on the link command line.
]>
]> http://sourceforge.net/projects/edk2developertoolsforwindows/files/Patches/Link%20Time%20Optimization/
]
]BaseTools/Contributions.txt (Contributed-under...)
]
]If you don't submit it edk2-devel, and continue to follow up on it,
]then work can easily slip through the cracks.
]
]That said, I've spent a little time looking at your work, so it's not
]true that there was no interest. But, if you have something that is
]*actually working* for DuetPkg, EmulatorPkg and OvmfPkg, can you put
]together a patch series?
The patch isn't ready for review. One reason is that it exposes
a problem that has no simple solution that I can find. A BaseTools
change may be needed. Link time code generation allows gcc to warn
extern const VOID* _gPcd_FixedAtBuild_PcdShellSupplier[];
AutoGen.c:207:15: note: previously declared here
GLOBAL_REMOVE_IF_UNREFERENCED const UINT16
_gPcd_FixedAtBuild_PcdShellSupplier[7] = {69, 68, 75, 32, 73, 73, 0 };
Both definitions are generated so they can't easily be changed.
This slight inconsistency in the array definition stops the build
because of warnings as errors. For this -flto proof of concept
experiment, I disable warning as errors. But the problem needs
to be resolved before a patch can be submitted. It was discussed
in a thread titled "Status of gcc link time optimization for EDK2 use".
Sergey made a python patch, but as I remember it didn't quite fix
every occurrence.
]I don't see how we can manage 5 & 6 with our current BaseTools. How
]did you manage it? Searching for 'pass-through' in your patch doesn't
]yield any results.
I punted and omitted -flto for that case. It is only a serious problem
"While making the EDK2 build meet requirement 5 is easy, the same
is not true for requirement 6 in some cases. For example, the ARM
build resolves compiler generated helper function calls with
CompilerIntrinsicsLib.lib and BaseStackCheckLib.lib. The path to
these libraries varies with the package name and build options.
Without a significant modification to the EDK2 build tools, there
is no way to generate the requires prefixed path for use on the
linker command line. This limitation affects ARM builds the most
because gcc ARM code uses more compiler generated helper function
calls than other target architectures. For this reason, link time
optimization is not enabled for ARM in the gcc LTO EDK2 patch. The
use of compiler generated helper function calls is kept to a minimum
in x86 code. For the IA32 builds the compiler generates a floating
point helper call when building stdlib. The patch disables link time
optimization for IA32 stdlib to avoid the
-Wl,-plugin-opt=-pass-through= requirement."
I will update the patch to work with the current EDK2 code. Here are
another couple of things to decide.
1) Which gcc version(s) get -flto? Probably 4.9 and later make sense.

I think it should work with the gcc 4.8 version shipped in RHEL-7.1. The
test written up on your website succeeded on my RHEL-7.1 laptop (with
"gcc (GCC) 4.8.3 20140911 (Red Hat 4.8.3-9)"). It would be great if I
could use -flto too. :)

Thanks!
Laszlo

Post by Scott Duplichan
2) Use of gcc -flto probably needs to be optional. We could make
two tool chain definitions, such as GCC49 and GCC49LTO. But that
creates a lot of duplication in an already big template file.
For the proof of concept experiment, -flto can be disabled by
setting an environment variable.
]Thanks,
]
]-Jordan
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Olivier Martin

2015-07-09 10:27:33 UTC

About the build infrastructure.
An option would be to investigate solution like travis-ci (https://travis-ci.com/). The little I know about travis-ci is it is free for open-source project, it can follow your github project and report if it builds or not (as soon as your build does not take more than 10-15min). When you send github 'push-request' it can also say if you break the build.
The build script is part of your repository (*.travis.yml), so everyone can contribute to it.

-----Original Message-----
From: Laszlo Ersek [mailto:***@redhat.com]
Sent: 08 July 2015 20:02
To: Bill Paul
Cc: edk2-***@lists.sourceforge.net; Scott Duplichan; Olivier Martin; ***@linaro.org; ***@intel.com
Subject: Re: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables

Post by Bill Paul
Of all the gin joints in all the towns in all the world, Laszlo Ersek

Post by Laszlo Ersek
In the above pattern, ASSERT_EFI_ERROR() is not necessarily used for
error handling.

You are absolutely right: it's not. It's being used as a panic().

Good point! I guess I've always used ASSERT() in the sense of PANIC_UNLESS().

Post by Bill Paul
[...] There is also DEBUG(). You will find several instances of the
int SomeInterestingValue;
/* Populate SomeInterestingValue */
SomeInterestingValue = GetSomeInterestingStatus (handle);
/* SomeInterestingValue is never referenced again */
DEBUG(EFI_D_INFO, "INTERESTING VALUE IS: %d\n",
SomeInterestingValue);
DEBUG() is also disabled when MDEPKG_NDEBUG is enabled, which creates
the same problem. What would be the correct way to handle this case?

Honestly, I don't know. I only concern myself with the case when even DEBUG_VERBOSE is enabled (PcdDebugPrintErrorLevel == 0x8040004F). The case when I have to look at debug messages and analyze behavior is the *norm*, not the exception (even if OVMF is not at fault -- the user may have passed wrong QEMU options, for example).

It's an incredible chore when a user reports behavior he doesn't understand or expect, and I first have to ask him to reproduce on a DEBUG_VERBOSE build, and that he send me *that* log. Usually he has no access to such a build, cannot build one himself, so I get to build it for him. Blech. (I think Gerd's packages do enable DEBUG_VERBOSE, but I'm not sure.)

Post by Bill Paul
It should be noted that once in a while I run into this sort of thing
in VxWorks as well, only it's often the other way around. We typically
have DEBUG()-style messages turned off by default, and sometimes there
are debug messages that refer to variables which no longer exist in
the code. (The variables were removed but the debug messages were
never updated to match.)

SeaBIOS employs the following trick to avoid this: all CONFIG_XXXX flags are checked in regular if() statements, not in #if macros. That is, any elmination happens during compilation / optimization / linking, not in preprocessing. This allows the compiler to catch such errors even when a CONFIG_XXXX option is predominantly false (or predominantly true).

Post by Bill Paul

Post by Laszlo Ersek
Right. I do agree with you that the optimal solution would be a build
farm where all developers could test-build against all supported
compilers. I doubt it's ever going to happen.

Given that UEFI firmware is supposed to be the standard for a large
number of commercially available computer platforms, I find your lack of faith...
disturbing.

My lack of faith is based on "experience" :) We've been complaining about this for ages on the list. I think it is safe to assume that the primary "participant" that has legal access to all supported Microsoft toolchains is Intel. As described (and provided!) by Scott, gcc-for-Windows is almost trivially available (all supported gcc versions). It seems to follow that Intel should operate such a build farm. Based on how long we've been whining about this, I don't think it's going to happen any time soon.

Post by Bill Paul
Lastly, yesterday I actually spent some time updating the
mingw-gcc-build.py to support binutils 2.25 and GCC 4.9.3, and I ran
into this exact issue. I had to add -Wno-unused-but-set-variable to
the UNIXGCC instances of CC_FLAGS in tools_def.template in order to
get the OVMF build to work. But along the way, I noticed about a dozen
instances of this anti-pattern. There are at least as many cases that stem from DEBUG() rather than ASSERT() or ASSERT_EFI_ERROR().
However I also found at least one legitimate error case of a useless
"set but never used" variable too (i.e. a case that didn't involve
conditional compilation effects). This means that now that GCC has
been muzzled to disguise intentional abuses of this pattern, some
unintentional abuses are now being hidden too.

Optimally, we shouldn't conflate the different (deeper) reasons this warning gets emitted for. Unfortunately, it's not practical even for me to build OVMF in all possible configurations:
- DEBUG and RELEASE (2)
- gcc 4.4 to 4.9 (6)
- Ia32, X64, Ia32X64 for OVMF (3)

That's 36 configs (and yes, the data flow analysis is specific to both target architecture and gcc version, so some warnings would only be found in configs that I can't test).

Add ARM and AARCH64 for ArmVirtPkg (QEMU and Xen separately)... Secure Boot enabled vs. disabled... ARM BDS vs. Intel BDS... For OVMF, CSM enabled vs. disabled; IPv6 enabled vs. disabled... It's really unmanageable without a build farm.

Yes, someone could say "hey Laszlo why don't you create a number of virtual machines, with different gcc versions installed, and implement at least the 'free software' build farm?" -- Because I don't have time for it. (And that's the reason I can't really blame Intel people either, for not setting up the MSVC+gcc build farm. Someone would have to take a
*personal* hit.)

For now we can say that we'll fix up such errors when we find out about them (and thus we don't patch the warning flags now). Annoying, but it does lead to a better code base.

Thanks
Laszlo

Post by Bill Paul
-Bill

Post by Laszlo Ersek
Thanks
Laszlo

Post by Scott Duplichan
Thanks,
Scott
]So let's allow this pattern, by passing the appropriate GCC command
]line option.
]
]Contributed-under: TianoCore Contribution Agreement 1.0
]---
] BaseTools/Conf/tools_def.template | 2 +- ] 1 file changed, 1
insertion(+), 1 deletion(-) ] ]diff --git
a/BaseTools/Conf/tools_def.template
b/BaseTools/Conf/tools_def.template ]index
7edd7590956b..15d8db04232f
100644
]--- a/BaseTools/Conf/tools_def.template
]+++ b/BaseTools/Conf/tools_def.template
/NODEFAULTLIB /LTCG /DLL /OPT:REF ] DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG =
--add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug ]
RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
]
]-DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar
-fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include
AutoGen.h ]+DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar
-fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include
AutoGen.h -Wno-error=unused-but-set-variable ] DEFINE GCC_IA32_CC_FLAGS
= DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double
-freorder-blocks -freorder-]blocks-and-partition -O2 -mno-stack-arg-probe ] DEFINE
GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone
-Wno-address -mno-stack-arg-probe ] DEFINE GCC_IPF_CC_FLAGS =
DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency ]--
]1.9.1

---------------------------------------------------------------------
------
--- Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782

Bruce Cran

2015-07-09 15:29:36 UTC

Post by Olivier Martin
About the build infrastructure.
An option would be to investigate solution like travis-ci (https://travis-ci.com/). The little I know about travis-ci is it is free for open-source project, it can follow your github project and report if it builds or not (as soon as your build does not take more than 10-15min). When you send github 'push-request' it can also say if you break the build.
The build script is part of your repository (*.travis.yml), so everyone can contribute to it.

I've started setting up a Jenkins instance at https://edk2.bluestop.org:8000/jenkins - I'm
currently hoping to use AWS/EC2 for the build nodes. No idea how far I'll get
or if it'll ever be functional though!

One issue I've noticed with https://github.com/tianocore/edk2 is that it seems
to lag behind svn by up to about a day.

--
Bruce

Bruce Cran

2015-07-12 02:53:39 UTC

Post by Laszlo Ersek
My lack of faith is based on "experience" :) We've been complaining
about this for ages on the list. I think it is safe to assume that the
primary "participant" that has legal access to all supported Microsoft
toolchains is Intel. As described (and provided!) by Scott,
gcc-for-Windows is almost trivially available (all supported gcc
versions). It seems to follow that Intel should operate such a build
farm. Based on how long we've been whining about this, I don't think
it's going to happen any time soon.

Since I had a few days off from the daily grind, I setup Jenkins and a few VMs
to build edk2 trunk, polling svn for changes. At the moment it builds debug and
release, 32-bit and 64-bit OVMF on Linux gcc 4.6, 4.7, 4.8, 4.9 and 5.1, and
MSVC 2008, 2010, 2012 and 2013. gcc 4.4 and 4.5 need more investigation since
the compilers don't build.
The Jenkins URL to see build status etc. is https://edk2.bluestop.org:8000/jenkins .

I've also set up an EDKSMOKE repository at https://edk2.bluestop.org/diffusion/EDKSMOKE
which currently lets anyone with an account push changesets to. I've not done
the integration yet, but the idea is that I'll hook it into Jenkins to let
people smoke test changesets on the various configurations I'll setup.

--
Bruce

Laszlo Ersek

2015-07-13 11:21:11 UTC

Post by Bruce Cran

Post by Laszlo Ersek
My lack of faith is based on "experience" :) We've been complaining
about this for ages on the list. I think it is safe to assume that the
primary "participant" that has legal access to all supported Microsoft
toolchains is Intel. As described (and provided!) by Scott,
gcc-for-Windows is almost trivially available (all supported gcc
versions). It seems to follow that Intel should operate such a build
farm. Based on how long we've been whining about this, I don't think
it's going to happen any time soon.

Since I had a few days off from the daily grind, I setup Jenkins and a few VMs
to build edk2 trunk, polling svn for changes. At the moment it builds debug and
release, 32-bit and 64-bit OVMF on Linux gcc 4.6, 4.7, 4.8, 4.9 and 5.1, and
MSVC 2008, 2010, 2012 and 2013. gcc 4.4 and 4.5 need more investigation since
the compilers don't build.
The Jenkins URL to see build status etc. is https://edk2.bluestop.org:8000/jenkins .
I've also set up an EDKSMOKE repository at https://edk2.bluestop.org/diffusion/EDKSMOKE
which currently lets anyone with an account push changesets to. I've not done
the integration yet, but the idea is that I'll hook it into Jenkins to let
people smoke test changesets on the various configurations I'll setup.

Thank you very much.

I also spent quite a few hours during the weekend on setting up 6
virtual machines locally (various Fedora releases that have gcc 4.4 to
4.9, inclusive, as their "native" compiler). I got it working, but it
was quite an awful experience. (For some reason I don't seem to be
attracted to "system administration".) In any case, your solution
appears much superior. I'd like to return to it (and hopefully start
using it regularly!) later on.

Thanks!
Laszlo

Jordan Justen

2015-07-12 05:42:49 UTC

Post by Laszlo Ersek
It seems to follow that Intel should operate such a build
farm. Based on how long we've been whining about this, I don't think
it's going to happen any time soon.

Intel has had such a build farm for years. It does usually catch these
issues... after the fact.

Even then it'll take some time for someone to see the issue and
attempt to fix it.

But, it is a lot more likely that someone that uses GCC will notice
the build break and submit a patch way before that process completes.

Similarly, if we break the MSVC build (this is a two way street), then
it'll probably be found and patched fairly quickly.

It could be that the move to git will make it more feasible to have a
testing branch that could allow all patches to first be tested against
all toolchains. But, personally I'm not really sure that a separate
branch would be worth the hassle.

-Jordan

B Cran

2015-07-12 06:24:51 UTC

Post by Jordan Justen
It could be that the move to git will make it more feasible to have a
testing branch that could allow all patches to first be tested against
all toolchains. But, personally I'm not really sure that a separate
branch would be worth the hassle.

I don’t think you want a separate _branch_, but probably a separate _repository_ that mirrors
the official one and that people can create new heads on. Pass the changeset hash to the
build farm and it can pull and build the changes. Then, once a day destroy the clone and
reset to upstream.

—
Bruce

Laszlo Ersek

2015-07-13 11:10:07 UTC

Post by B Cran

Post by Jordan Justen
It could be that the move to git will make it more feasible to have a
testing branch that could allow all patches to first be tested against
all toolchains. But, personally I'm not really sure that a separate
branch would be worth the hassle.

I don’t think you want a separate _branch_, but probably a separate _repository_ that mirrors
the official one and that people can create new heads on. Pass the changeset hash to the
build farm and it can pull and build the changes. Then, once a day destroy the clone and
reset to upstream.

That would be great.

Laszlo

Bruce Cran

2015-07-08 21:43:23 UTC

Post by Bill Paul
Lastly, yesterday I actually spent some time updating the mingw-gcc-build.py
to support binutils 2.25 and GCC 4.9.3, and I ran into this exact issue.

I also spent a few minutes updating it today (to binutils 2.25 and gcc 5.1,
though 5.1.0 is way too new to be put into a changeset to be committed).

Did you come across the missing objcopy symlink in BaseTools/gcc/symlinks/{ia32,x64}?

I also had to add a UNIXGCC_IPF_PETOOLS_PREFIX line to tools_def.txt, and also
change ENV(WORKSPACE)/BaseTools/Bin/gcc/Ia32/ to ENV(WORKSPACE)/BaseTools/gcc/symlinks/ia32/
(and the corresponding x64 define). Should these changes be committed to the
template, or do people commonly have gcc in BaseTools/Bin/gcc/{Ia32,X64}?

--
Bruce

Bill Paul

2015-07-08 22:50:11 UTC

Of all the gin joints in all the towns in all the world, Bruce Cran had to

Post by Bruce Cran

Post by Bill Paul
Lastly, yesterday I actually spent some time updating the
mingw-gcc-build.py to support binutils 2.25 and GCC 4.9.3, and I ran
into this exact issue.

I also spent a few minutes updating it today (to binutils 2.25 and gcc 5.1,
though 5.1.0 is way too new to be put into a changeset to be committed).

When I tried GCC 5.1.0 on my FreeBSD 9.1 machine in my office, it bombed with
an internal compiler error on some files. I'm not sure why this happened; the
cross compiler build succeeded without error. I tried 4.8.5 and 4.9.3 and both
of those worked fine, so I stuck with the latter.

Post by Bruce Cran
Did you come across the missing objcopy symlink in
BaseTools/gcc/symlinks/{ia32,x64}?

Er... no?

I put my modified copies of mingw-gcc-build.py and tools_def.template at:

http://people.freebsd.org/~wpaul/edk2

The problems I ran into were:

1) When building gcc, the fixincludes stage insists that there be a
mingw/include directory present in the --prefix= path. This was not
the case with 4.3.0, but 4.8.x and 4.9.x seem to want it. When I tried
5.1.0 it didn't seem to care about it. I modified the mingw-gcc-build.py
to create this directory and that shut it up. (The directory can be empty,
but it must be present.)

2) The UNIXGCC rules now need -Wno-unused-but-set-variable, for reasons
already discussed.

3) The DLINK and ASLDLINK rules for X64 builds need to be changed. Currently
the UNIXGCC rules use these generic GCC rules:

DEFINE GCC_IA32_X64_ASLDLINK_FLAGS = DEF(GCC_IA32_X64_DLINK_COMMON) --entry
_ReferenceAcpiTable -u $(IMAGE_ENTRY_POINT)

DEFINE GCC_IA32_X64_DLINK_FLAGS = DEF(GCC_IA32_X64_DLINK_COMMON) --entry
_$(IMAGE_ENTRY_POINT) --file-alignment 0x20 --section-alignment 0x20 -Map
$(DEST_DIR_DEBUG)/$(BASE_NAME).map

Note the leading underscore before ReferenceAcpiTable and
$(IMAGE_ENTRY_POINT). This rule is used for both IA32 and X64 builds
(hence the name), under the assumption that both the 32-bit and 64-bit
exhibit the same behavior. But the newer versions of GCC built for MinGW
do _NOT_ need the leading undercore for symbol names on for the x86-64
architecture, so using these rules results in non-functional X64 images.

Frustratingly, this does not cause the build to halt and catch fire to
make the problem obvious. Instead the linker emits a warning and then
merrily proceeds to create invalid objects, and you end up with an
OVMF.fd that doesn't boot. (It took me half an hour before I figured
it out.)

I added X64-specific instances of these rules that do not include the
underscore and switched the UNIXGCC X64 rules to use them.

4) I added -m32 and -m64 to the UNIXGCC assembler flags in a few places to
match existing cases for GCC 4.9. (This change may be superfluous.)

Once I did the above, I was able to build functional cross compilers and
generate working OVMF images for IA32 and X64 architectures which I tested
with QEMU 2.3.0. I was also able to compile working versions of the VxWorks
UEFI boot loader.

Post by Bruce Cran
I also had to add a UNIXGCC_IPF_PETOOLS_PREFIX line to tools_def.txt, and
also change ENV(WORKSPACE)/BaseTools/Bin/gcc/Ia32/ to
ENV(WORKSPACE)/BaseTools/gcc/symlinks/ia32/ (and the corresponding x64
define). Should these changes be committed to the template, or do people
commonly have gcc in BaseTools/Bin/gcc/{Ia32,X64}?

I did not do anything with the IPF platform rules. You can't build OVMF for
the Itanium architecture anyway. I tend to modify the "Option 5" rules in
tools_def.txt to specify my custom path to the cross-compiler toolchain.

The changes I made to fix the rules for the newer version of GCC should go in
the template. Changes to specify your local toolchain path should be done in
your own tools_def.txt file.

-Bill

--
=============================================================================
-Bill Paul (510) 749-2329 | Senior Member of Technical Staff,
***@windriver.com | Master of Unix-Fu - Wind River Systems
=============================================================================
"I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

Bruce Cran

2015-07-08 23:03:45 UTC

Post by Bill Paul
I did not do anything with the IPF platform rules. You can't build OVMF for
the Itanium architecture anyway. I tend to modify the "Option 5" rules in
tools_def.txt to specify my custom path to the cross-compiler toolchain.

It's maybe because I'm working against the UDK2014.SP1 branch (since that's
what the Minnowboard MAX supports) but despite not doing anything with Itanium
the build system complained that I hadn't defined it.

--
Bruce

34 Replies
9 Views
Permalink to this page
Disable enhanced parsing

Thread Navigation

Ard Biesheuvel 2015-07-08 13:24:18 UTC

Olivier Martin 2015-07-08 14:59:10 UTC

Ard Biesheuvel 2015-07-08 15:02:30 UTC

Laszlo Ersek 2015-07-08 15:58:04 UTC

Jordan Justen 2015-07-08 23:42:25 UTC

Laszlo Ersek 2015-07-09 04:56:36 UTC

Ard Biesheuvel 2015-07-09 07:16:16 UTC

Laszlo Ersek 2015-07-08 15:54:46 UTC

Scott Duplichan 2015-07-08 15:28:41 UTC

Laszlo Ersek 2015-07-08 16:17:08 UTC

Bill Paul 2015-07-08 18:15:44 UTC

Laszlo Ersek 2015-07-08 19:02:10 UTC

Andrew Fish 2015-07-08 19:26:32 UTC

Laszlo Ersek 2015-07-08 20:43:38 UTC

Andrew Fish 2015-07-08 20:53:50 UTC

Laszlo Ersek 2015-07-08 21:21:24 UTC

Andrew Fish 2015-07-08 21:34:16 UTC

Bruce Cran 2015-07-08 22:12:06 UTC

Scott Duplichan 2015-07-09 02:31:09 UTC

Laszlo Ersek 2015-07-09 05:35:37 UTC

Bill Paul 2015-07-09 17:35:11 UTC

Laszlo Ersek 2015-07-09 18:19:04 UTC

Jordan Justen 2015-07-10 07:23:55 UTC

Scott Duplichan 2015-07-11 07:43:09 UTC

Laszlo Ersek 2015-07-13 11:52:10 UTC

Olivier Martin 2015-07-09 10:27:33 UTC

Bruce Cran 2015-07-09 15:29:36 UTC

Bruce Cran 2015-07-12 02:53:39 UTC

Laszlo Ersek 2015-07-13 11:21:11 UTC

Jordan Justen 2015-07-12 05:42:49 UTC

B Cran 2015-07-12 06:24:51 UTC

Laszlo Ersek 2015-07-13 11:10:07 UTC

Bruce Cran 2015-07-08 21:43:23 UTC

Bill Paul 2015-07-08 22:50:11 UTC

Bruce Cran 2015-07-08 23:03:45 UTC

about - legalese

Loading...