[edk2-devel] RELEASE CLANGPDB OVMF currently does not compile

[edk2-devel] RELEASE CLANGPDB OVMF currently does not compile

[Mike Beaton]

Dear Ard,

Thanks for your attention on my other issue, about NOOPT CLANGDWARF OVMF.

This one is about RELEASE CLANGPDB OVMF, which currently does not
compile, giving:

```
/home/mjsbeaton/OpenSource/edk2/OvmfPkg/VirtioSerialDxe/VirtioSerial.c:28:22:
error: variable 'EventNames' is not needed and will not be emitted
[-Werror,-Wunneeded-internal-declaration]
STATIC CONST CHAR8  *EventNames[] = {
                     ^
1 error generated.
```

I logged this at https://bugzilla.tianocore.org/show_bug.cgi?id=4620

This _can_ be fixed by:

```
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -1754,7 +1754,7 @@ DEFINE CLANGPDB_X64_PREFIX           = ENV(CLANG_BIN)
 DEFINE CLANGPDB_IA32_TARGET          = -target i686-unknown-windows-gnu
 DEFINE CLANGPDB_X64_TARGET           = -target x86_64-unknown-windows-gnu

-DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality
-Wno-tautological-compare
-Wno-tautological-constant-out-of-range-compare -Wno-empty-body
-Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option
-Wno-unused-but-set-variable -Wno-unused-const-variable
-Wno-unaligned-access -Wno-microsoft-enum-forward-reference
+DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality
-Wno-tautological-compare
-Wno-tautological-constant-out-of-range-compare -Wno-empty-body
-Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option
-Wno-unused-but-set-variable -Wno-unused-const-variable
-Wno-unaligned-access -Wno-unneeded-internal-declaration
-Wno-microsoft-enum-forward-reference
 DEFINE CLANGPDB_ALL_CC_FLAGS         = DEF(GCC48_ALL_CC_FLAGS)
DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -funsigned-char
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
-Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
-Wno-incompatible-library-redeclaration -Wno-null-dereference
-mno-implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib
-nostdlibinc -fseh-exceptions

 ###########################
```

which duplicates https://github.com/tianocore/edk2/commit/d3225577123,
which already applied the same change to CLANGDWARF.

That change was discussed and approved here:
https://edk2.groups.io/g/devel/topic/98807494 - however, I'd like to
belatedly object, if I may!

There is currently exactly one line of code which needs this warning
to be disabled (at least, in OVMF), the line mentioned above. So if we
were going to bring the warning back, now rather than later would be
the time to do it. The issue at that line can, of course, be worked
around by removing the STATIC (and presumably adding a comment, so
that someone doesn't mistakenly add it back again).

I would guess that there must be several other places where people
_have_ already tiptoed round this issue before, in EDK-2 code, though
a quick search for the warning name itself only throws up one such
instance in OpenSslLib.

The advantage of tiptoeing round the issue (in the reasonably rare
cases where needed) instead of disabling the error check, is that the
check may well show up genuine issues in code (perhaps most obviously,
variables left unused after code changes).

For that reason, I'd propose re-enabling the warning in CLANGDWARF,
and removing the STATIC (and adding a comment) in the relevant line in
VirtioSerialDxe.


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

Re: [edk2-devel] RELEASE CLANGPDB OVMF currently does not compile

[Ard Biesheuvel]

On Sun, 10 Dec 2023 at 01:26, Mike Beaton <mjsbeaton@gmail.com> wrote:
>
> Dear Ard,
>
> This one is about RELEASE CLANGPDB OVMF, which currently does not
> compile, giving:
>
> ```
> /home/mjsbeaton/OpenSource/edk2/OvmfPkg/VirtioSerialDxe/VirtioSerial.c:28:22:
> error: variable 'EventNames' is not needed and will not be emitted
> [-Werror,-Wunneeded-internal-declaration]
> STATIC CONST CHAR8  *EventNames[] = {
>                      ^
> 1 error generated.
> ```
>
> I logged this at https://bugzilla.tianocore.org/show_bug.cgi?id=4620
>
> This _can_ be fixed by:
>
> ```
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -1754,7 +1754,7 @@ DEFINE CLANGPDB_X64_PREFIX           = ENV(CLANG_BIN)
>  DEFINE CLANGPDB_IA32_TARGET          = -target i686-unknown-windows-gnu
>  DEFINE CLANGPDB_X64_TARGET           = -target x86_64-unknown-windows-gnu
>
> -DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality
> -Wno-tautological-compare
> -Wno-tautological-constant-out-of-range-compare -Wno-empty-body
> -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option
> -Wno-unused-but-set-variable -Wno-unused-const-variable
> -Wno-unaligned-access -Wno-microsoft-enum-forward-reference
> +DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality
> -Wno-tautological-compare
> -Wno-tautological-constant-out-of-range-compare -Wno-empty-body
> -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option
> -Wno-unused-but-set-variable -Wno-unused-const-variable
> -Wno-unaligned-access -Wno-unneeded-internal-declaration
> -Wno-microsoft-enum-forward-reference
>  DEFINE CLANGPDB_ALL_CC_FLAGS         = DEF(GCC48_ALL_CC_FLAGS)
> DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -funsigned-char
> -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
> -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
> -Wno-incompatible-library-redeclaration -Wno-null-dereference
> -mno-implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib
> -nostdlibinc -fseh-exceptions
>
>  ###########################
> ```
>
> which duplicates https://github.com/tianocore/edk2/commit/d3225577123,
> which already applied the same change to CLANGDWARF.
>
> That change was discussed and approved here:
> https://edk2.groups.io/g/devel/topic/98807494 - however, I'd like to
> belatedly object, if I may!
>
> There is currently exactly one line of code which needs this warning
> to be disabled (at least, in OVMF), the line mentioned above. So if we
> were going to bring the warning back, now rather than later would be
> the time to do it. The issue at that line can, of course, be worked
> around by removing the STATIC (and presumably adding a comment, so
> that someone doesn't mistakenly add it back again).
>
> I would guess that there must be several other places where people
> _have_ already tiptoed round this issue before, in EDK-2 code, though
> a quick search for the warning name itself only throws up one such
> instance in OpenSslLib.
>
> The advantage of tiptoeing round the issue (in the reasonably rare
> cases where needed) instead of disabling the error check, is that the
> check may well show up genuine issues in code (perhaps most obviously,
> variables left unused after code changes).
>
> For that reason, I'd propose re-enabling the warning in CLANGDWARF,
> and removing the STATIC (and adding a comment) in the relevant line in
> VirtioSerialDxe.
>

Removing STATIC means that (modulo LTO) the compiler will not know
whether or not the definition can be dropped. It also pollutes the
global namespace.

IMO, lack of the use of STATIC where appropriate is a severe issue
with the EDK2 codebase. Removing STATIC to appease compiler
diagnostics is *not* the way to solve this.

The patch you proposed looks fine to me:

Acked-by: Ard Biesheuvel <ardb@kernel.org>


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

Re: [edk2-devel] RELEASE CLANGPDB OVMF currently does not compile

[Mike Beaton]

> Removing STATIC means that (modulo LTO) the compiler will not know
> whether or not the definition can be dropped. It also pollutes the
> global namespace.
>
> IMO, lack of the use of STATIC where appropriate is a severe issue
> with the EDK2 codebase. Removing STATIC to appease compiler
> diagnostics is *not* the way to solve this.

Thank you for your feedback. On reflection, since gcc still _can_
distinguish between genuinely unused variables and variables who usage
was optimized away like this (I think that's well known; but I just
double-checked by adding a similar, but entirely unused variable to
the same file - gcc then complains), and since all code in the project
is going to end up being compiled under gcc as well clang, then just
squelching the (slightly broken) warning under clang is not really
losing useful information after all, in this case. So thank you for
the ack, and agreed, now, that it is the best way to proceed.


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

Re: [edk2-devel] RELEASE CLANGPDB OVMF currently does not compile

[Mike Beaton]

I've sent a patch to this list (with some kind of proposed comment
about the issues!) under separate cover.


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