ArmPkg,SecurityPkg,BaseTools: Remove no longer needed warning suppression

[mikebeaton]

Description

Since ae83c6b -Wno-unused-but-set-variable and -Wno-unused-variable warning suppression is no longer needed in any builds, and the warnings can be re-enabled to catch real errors.

On pushing this to CI we find two such minors errors introduced in 1634dd5 and 9c651ef, so we fix these in additional commits.

• Breaking change?
  • NO.
• Impacts security?
  • NO.
• Includes tests?
  • NO.

How This Was Tested

EDK 2 CI (GCC only)
Acidanthera AUDK CI (also tests CLANGDWARF and CLANGPDB)

Integration Instructions

N/A

[leiflindholm]

I think there is a reasonable chance this will break many out of tree builds. But since this would get up being merged shortly after the stable tag, I don't have an issue with that. These suppression flags let real bugs slip through.

[ardbiesheuvel]

Often, hacks such as these exist because of bugs in the compiler, not bugs in our code.

IOW, dropping this without checking whether or not GCC 4.8 still works is not a great idea. Bonus points for finding out that GCC 4.8 doesn't even build to being with, in which case we can consider dropping GCC 4.8 support altogether. Or did you do a test build with GCC 4.8 already?

[mikebeaton]

No, will test.

[leiflindholm]

These days, we tend to see them because our codebase is crammed full of several-hundred-line functions with 30+ live variables throughout with overlapping meanings across.

The last GCC 4.8 point release was made over 10 years ago.
And we do have this note, from May 2023:
https://github.com/tianocore/edk2/blob/master/BaseTools/Conf/tools_def.template#L22

In fact, I was surprised to notice recently we still had GCC 4.8 support in at all. I had mistakenly thought we threw that out when we decided to reinterpret GCC5 as LTO with GCC49 as NOLTO.

Which is a long-winded way of saying:
a) I think it's past time to yeet 4.8 (and maybe 4.9, 5.0)
b) I would quite like to see what happens if we merge this

As always, we can revert (for now) if it causes significant impact and those affected are willing to start working towards removing their dependencies on outdated tools.

[mikebeaton]

So on Ubuntu 14.04.1, GCC 4.8.4, Python 3.5.2, it is struggling to make BaseTools, before I ever get to trying the toolchain.

Here's what I'm seeing - let me know if I'm missing something and there's a quick way to get past this!

---

Initially we get:

In file included from BrotliCompress.c:21:0:
./brotli/c/common/constants.h:196:1: error: ‘model’ attribute directive ignored [-Werror=attributes]
 BrotliPrefixCodeRange _kBrotliPrefixCodeRanges[BROTLI_NUM_BLOCK_LEN_SYMBOLS];
 ^
cc1: error: unrecognized command line option "-Wno-restrict" [-Werror]
cc1: error: unrecognized command line option "-Wno-stringop-truncation" [-Werror]

If I just remove those options from BaseTools/Source/C/Makefiles/header.makefile then I get to:

In file included from BrotliCompress.c:21:0:
./brotli/c/common/constants.h:196:1: error: ‘model’ attribute directive ignored [-Werror=attributes]
 BrotliPrefixCodeRange _kBrotliPrefixCodeRanges[BROTLI_NUM_BLOCK_LEN_SYMBOLS];
 ^

If I then add -Wno-error=attributes, we get to:

.
.
.
In file included from brotli/c/enc/encoder_dict.h:15:0,
                 from brotli/c/enc/params.h:13,
                 from brotli/c/enc/command.h:15,
                 from brotli/c/enc/backward_references_hq.h:14,
                 from brotli/c/enc/encode.c:16:
brotli/c/enc/static_dict_lut.h:40:5: warning: ‘model’ attribute directive ignored [-Wattributes]
     kStaticDictionaryBuckets[BROTLI_ENC_STATIC_DICT_LUT_NUM_BUCKETS];
     ^
brotli/c/enc/static_dict_lut.h:42:5: warning: ‘model’ attribute directive ignored [-Wattributes]
     kStaticDictionaryWords[BROTLI_ENC_STATIC_DICT_LUT_NUM_ITEMS];
     ^
In file included from brotli/c/enc/backward_references_hq.h:14:0,
                 from brotli/c/enc/encode.c:16:
brotli/c/enc/command.h:23:1: warning: ‘model’ attribute directive ignored [-Wattributes]
 uint32_t kBrotliInsBase[BROTLI_NUM_INS_COPY_CODES];
 ^
brotli/c/enc/command.h:25:1: warning: ‘model’ attribute directive ignored [-Wattributes]
 uint32_t kBrotliInsExtra[BROTLI_NUM_INS_COPY_CODES];
 ^
brotli/c/enc/command.h:27:1: warning: ‘model’ attribute directive ignored [-Wattributes]
 uint32_t kBrotliCopyBase[BROTLI_NUM_INS_COPY_CODES];
 ^
brotli/c/enc/command.h:29:1: warning: ‘model’ attribute directive ignored [-Wattributes]
 uint32_t kBrotliCopyExtra[BROTLI_NUM_INS_COPY_CODES];
 ^
In file included from brotli/c/enc/brotli_bit_stream.h:22:0,
                 from brotli/c/enc/encode.c:19:
brotli/c/enc/entropy_encode.h:78:1: warning: ‘model’ attribute directive ignored [-Wattributes]
 BROTLI_INTERNAL extern BROTLI_MODEL("small") const size_t kBrotliShellGaps[6];
 ^
In file included from brotli/c/enc/encode.c:24:0:
brotli/c/enc/dictionary_hash.h:36:5: warning: ‘model’ attribute directive ignored [-Wattributes]
     kStaticDictionaryHashWords[BROTLI_ENC_NUM_HASH_BUCKETS];
     ^
brotli/c/enc/dictionary_hash.h:38:5: warning: ‘model’ attribute directive ignored [-Wattributes]
     kStaticDictionaryHashLengths[BROTLI_ENC_NUM_HASH_BUCKETS];
     ^
brotli/c/enc/encode.c: In function ‘InitCommandPrefixCodes’:
brotli/c/enc/encode.c:203:3: warning: ‘model’ attribute directive ignored [-Wattributes]
   static const BROTLI_MODEL("small") uint8_t kDefaultCommandDepths[128] = {
   ^
brotli/c/enc/encode.c:213:3: warning: ‘model’ attribute directive ignored [-Wattributes]
   static const BROTLI_MODEL("small") uint16_t kDefaultCommandBits[128] = {
   ^
brotli/c/enc/encode.c:227:3: warning: ‘model’ attribute directive ignored [-Wattributes]
   static const BROTLI_MODEL("small") uint8_t kDefaultCommandCode[] = {
   ^
brotli/c/enc/encode.c: In function ‘EstimateEntropy’:
brotli/c/enc/encode.c:248:3: error: ‘for’ loop initial declarations are only allowed in C99 mode
   for (size_t i = 0; i < size; ++i) {
   ^
brotli/c/enc/encode.c:248:3: note: use option -std=c99 or -std=gnu99 to compile your code
brotli/c/enc/encode.c: In function ‘ChooseContextMap’:
brotli/c/enc/encode.c:269:3: warning: ‘model’ attribute directive ignored [-Wattributes]
   uint32_t kStaticContextMapContinuation[64] = {
   ^
brotli/c/enc/encode.c:276:3: warning: ‘model’ attribute directive ignored [-Wattributes]
   uint32_t kStaticContextMapSimpleUTF8[64] = {
   ^
brotli/c/enc/encode.c: In function ‘ShouldUseComplexStaticContextMap’:
brotli/c/enc/encode.c:333:3: warning: ‘model’ attribute directive ignored [-Wattributes]
   uint32_t kStaticContextMapComplexUTF8[64] = {
   ^
make[2]: *** [brotli/c/enc/encode.o] Error 1
make[2]: Leaving directory `/home/mjsbeaton/OpenSource/edk2/BaseTools/Source/C/BrotliCompress'
make[1]: *** [BrotliCompress] Error 2
make[1]: Leaving directory `/home/mjsbeaton/OpenSource/edk2/BaseTools/Source/C'
make: *** [Source/C] Error 2

with several pages more similar above what I've pasted.

[ardbiesheuvel]

Thanks for doing the due diligence. Let's drop GCC 4.8 support rather than waste any more time on this.

[mikebeaton]

Thanks for doing the due diligence. Let's drop GCC 4.8 support rather than waste any more time on this.

Would that be and drop 4.9 as well? (I don't have 'skin in the game', just wondering!)

[mikebeaton]

Hmmm... for GCC5, the newest version of Ubuntu which comes with GCC 5 by default is 16.04, but that comes with Python 3.5 by default and no easy access to Python 3.6, so we can build BaseTools, but when we come to build a package we get:

$ build -a X64 -t GCC5 -p MdePkg/MdePkg.dsc
Traceback (most recent call last):
  File "/home/mjsbeaton/OpenSource/edk2/BaseTools/BinWrappers/PosixLike/../../Source/Python/build/build.py", line 33, in <module>
    import secrets
ImportError: No module named 'secrets'

Apparently 'secrets' is a built-in thing, in Python 3.6+.

Presumably we (I!) can work round this one by going to a newer Linux and installing gcc 5.

[ardbiesheuvel]

Again, thanks for doing the due diligence.

A while ago, we cloned GCC49 to GCCNOLTO and GCC5 to GCC, in order to decouple our GCC support from the major/minor GCC versions.

Perhaps by the time the upcoming stable tag lands, it's time to
a) get rid of GCC48 and GCC49
b) decide which version of GCC we still want to support, as I suspect that there is little point to supporting anything older than 8 at this time
c) get rid of GCC5, or keep it out of nostalgia, but document that only GCC 8 and older are still supported.

[mikebeaton]

FWIW with gcc 7 these changes don't break sample test builds X46 RELEASE of MdePkg, MdeModulePkg, SecurityPkg, UefiCpuPkg with GCC5.

The changes in this PR also don't break GCC48 or GCC49 X46 RELEASE MdePkg with gcc 7. MdeModulePkg, SecurityPkg and UefiCpuPkg fail with GCC48 and GCC49 on the same test setup, but not for reasons to do with this PR.

Worth noting we are now reliant on nasm 2.15.01+ (due to use of shadow stack instructions https://github.com/netwide-assembler/nasm/blob/master/doc/changes.src#L479), which is also not found naturally with older gcc (e.g. I compiled 3.01 from source).

[mikebeaton]

Perhaps by the time the upcoming stable tag lands, it's time to a) get rid of GCC48 and GCC49 b) decide which version of GCC we still want to support, as I suspect that there is little point to supporting anything older than 8 at this time c) get rid of GCC5, or keep it out of nostalgia, but document that only GCC 8 and older are still supported.

One more quick point: it may be worth noting that there is a Ubuntu LTS with gcc 7 as the default (18.04), but not one with 8 (20.04 skips to 9). Since that is one (of several) ways of quickly accessing a test environment, that might be some degree of a reason to prefer 7 rather than 8 as a minimal supported gcc version, depending on what the other reasons are.

[mikebeaton]

After discussion and due diligence as above, is this one agreed to merge as is now?

[mikebeaton]

I'll have a poke around in edk2-platforms, and see what changes this would require, there.

[mikebeaton]

Rebased, and fixed very minor conflict in two places where changed lines here were right next to lines added for LOONGARCH64 in c33aa63. Rebase has also dropped one of the two previously required preceding commits, which has now been merged as 6e5835d.

[mikebeaton]

I've successfully built multiple platforms in edk2-platforms with GCC, with this change, with almost no problems. Including Arm, Intel, etc. As usual, I think, not all packages in there build properly, but not for reasons related to this change.

TL/DR of my comments above, this doesn't even break GCC48/GCC49 with old (but not as old as 4.8 and 4.9) gcc, so possibly finally removing these doesn't need to be a blocker to this PR? Or were you hoping that would be done too, in this PR?

decide which version of GCC we still want to support, as I suspect that there is little point to supporting anything older than 8 at this time

I also suggested above that 7 might be a better choice (due to what different Ubuntu LTS versions support).