OvmfPkg/IoMmuDxe: unused-const-variable warning fails release build with GCC 6.3

OvmfPkg/IoMmuDxe: unused-const-variable warning fails release build with GCC 6.3

[Thomas Lamprecht]

Hi,

commit 2ad6ba80a1bd58382bde6b994070f7c01d2fb48d
Author: Laszlo Ersek <lersek@redhat.com>
Date:   Wed Aug 30 14:00:58 2017 +0200

     OvmfPkg/IoMmuDxe: IoMmuMap(): log nicer and more informative DEBUG msgs

triggers a unused-const-variable warning for mBusMasterOperationName,
and thus for releases with warnings-as-errors a build error.

I'm using a quite vanilla Debian Stretch machine on amd64/x86_64 (running
in qemu/KVM) as build host, gcc version is 6.3.0 20170516 (Debian 6.3.0-18)

My build procedure looks like:

# make -C BaseTools/
# . edksetup.sh
# OvmfPkg/build.sh -a X64 -b RELEASE -n 4 -t GCC5

With current master (12cfc9009e7cf1a69ca675110c2cf6e21b152992) checked out.

I suspect that gcc, at least in this version, cannot track the usage of the
variable in crime in the DEBUG macros, and thus (falsely?) detects this warning.

So I'm not quite sure if this is a problem with edk2/Ovmf itself or a problem
stemming from gcc.

The following patch fixes the problem quite nonchalantly by adding an unused
attribute, which is highly probably not what is wanted as a clean fix -
I guess - but it achieves my desired result :)

----8<----
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index bc57de5b57..8c0b8b0931 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -48,7 +48,7 @@ STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
  // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
  //
  STATIC CONST CHAR8 * CONST
-mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
+mBusMasterOperationName[EdkiiIoMmuOperationMaximum] __attribute__ ((unused)) = {
    "Read",
    "Write",
    "CommonBuffer",
---->8----

It naturally could be that I've got something wrong, wouldn't be the first time,
but I suspect that it's edk2 in combination with my GCC version this time.
As I lack in depth knowledge of Ovmf it'd be nice if someone could confirm my
suspicion.

CC'ing Laszlo as he is the author of the patch which lets this problem
trigger and following this mailing list since a bit it seems that he surely
has the in depth knowledge.
Sorry that I wasn't patient enough to test other compiler version before
posting here.

cheers,
Thomas

Re: OvmfPkg/IoMmuDxe: unused-const-variable warning fails release build with GCC 6.3

[Laszlo Ersek]

Hello Thomas,

(CC Ard, Liming, Yonghong)

On 09/06/17 09:44, Thomas Lamprecht wrote:
> Hi,
>
> commit 2ad6ba80a1bd58382bde6b994070f7c01d2fb48d
> Author: Laszlo Ersek <lersek@redhat.com>
> Date:   Wed Aug 30 14:00:58 2017 +0200
>
>     OvmfPkg/IoMmuDxe: IoMmuMap(): log nicer and more informative DEBUG msgs
>
> triggers a unused-const-variable warning for mBusMasterOperationName,
> and thus for releases with warnings-as-errors a build error.
>
> I'm using a quite vanilla Debian Stretch machine on amd64/x86_64
> (running in qemu/KVM) as build host, gcc version is 6.3.0 20170516
> (Debian 6.3.0-18)
>
> My build procedure looks like:
>
> # make -C BaseTools/
> # . edksetup.sh
> # OvmfPkg/build.sh -a X64 -b RELEASE -n 4 -t GCC5
>
> With current master (12cfc9009e7cf1a69ca675110c2cf6e21b152992) checked
> out.
>
> I suspect that gcc, at least in this version, cannot track the usage
> of the variable in crime in the DEBUG macros, and thus (falsely?)
> detects this warning.
>
> So I'm not quite sure if this is a problem with edk2/Ovmf itself or a
> problem stemming from gcc.
>
> The following patch fixes the problem quite nonchalantly by adding an
> unused attribute, which is highly probably not what is wanted as a
> clean fix - I guess - but it achieves my desired result :)
>
> ----8<----
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index bc57de5b57..8c0b8b0931 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -48,7 +48,7 @@ STATIC LIST_ENTRY mRecycledMapInfos =
> INITIALIZE_LIST_HEAD_VARIABLE (
>  // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
>  //
>  STATIC CONST CHAR8 * CONST
> -mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
> +mBusMasterOperationName[EdkiiIoMmuOperationMaximum] __attribute__
> ((unused)) = {
>    "Read",
>    "Write",
>    "CommonBuffer",
> ---->8----
>
> It naturally could be that I've got something wrong, wouldn't be the
> first time, but I suspect that it's edk2 in combination with my GCC
> version this time. As I lack in depth knowledge of Ovmf it'd be nice
> if someone could confirm my suspicion.
>
> CC'ing Laszlo as he is the author of the patch which lets this problem
> trigger and following this mailing list since a bit it seems that he
> surely has the in depth knowledge. Sorry that I wasn't patient enough
> to test other compiler version before posting here.

Thanks for the report.

When I (recently) wrote the above commit, I was fully aware that the
variable would become unused if DEBUG macro invocations were compiled
out of the code (i.e., in the RELEASE build target).

However, that didn't worry me because we had already introduced a
distinction for such variables, between RELEASE and DEBUG builds. As you
can expect, this phenomenon is quite wide-spread; for example a Status
variable may capture a function return value, only to be ASSERT()-ed
upon. In a RELEASE build, the ASSERT() disappears, and we don't want the
compiler to yell at us for setting, but never checking, Status.

... Please refer to git commit 20d00edf21d2 ("BaseTools/GCC: set
-Wno-unused-but-set-variables only on RELEASE builds", 2016-03-24).

Looks like gcc-6 has a knob specific to const variables that we should
give the same treatment. Looking at the following pages:

  https://gcc.gnu.org/onlinedocs/gcc-4.9.4/gcc/Warning-Options.html
  https://gcc.gnu.org/onlinedocs/gcc-5.4.0/gcc/Warning-Options.html
  https://gcc.gnu.org/onlinedocs/gcc-6.4.0/gcc/Warning-Options.html
  https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Warning-Options.html

it appears that "-Wunused-const-variable" has first appeared in gcc-6.

Now, that's a problem. In edk2, we currently do not have a specific GCC6
toolchain setting. We have several GCC4x toolchains, and one GCC5
toolchain. Normally I would have asked you to send a patch, similar to
commit 20d00edf21d2 above, to add "-Wno-unused-const-variable" to the
"oldest" applicable macros in "BaseTools/Conf/tools_def.template", for
example:

  RELEASE_GCC5_IA32_CC_FLAGS
  RELEASE_GCC5_X64_CC_FLAGS
  RELEASE_GCC5_ARM_CC_FLAGS
  RELEASE_GCC5_AARCH64_CC_FLAGS

But gcc-5 would not recognize this option. I think we might have to
introduce the GCC6 toolchain for this.

Here's what I suggest: please file a TianoCore BZ about this issue, at
<https://bugzilla.tianocore.org/>. Select "BaseTools" as Package. Feel
free to reference the mailing list thread in the BZ:

https://lists.01.org/pipermail/edk2-devel/2017-September/014225.html

Now, addressing that will take a while. Meanwhile, can you please check
if simply removing the CONST suppresses the issue? (Note, the CONST
should be removed from the "mBusMasterOperationName" variable, *not*
from the pointed-to CHAR8.)

If that works, can you please submit a patch like this?

> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index bc57de5b572b..45d1c909b5ad 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -47,7 +47,9 @@ STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
>  //
>  // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
>  //
> -STATIC CONST CHAR8 * CONST
> +// Not CONST-qualified in order to suppress gcc-6+ warnings.
> +//
> +STATIC CONST CHAR8 *
>  mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>    "Read",
>    "Write",

We could commit this patch for now. Once the BZ you file for BaseTools
is fixed, we can revert this patch.

Thank you!
Laszlo

Re: OvmfPkg/IoMmuDxe: unused-const-variable warning fails release build with GCC 6.3

[Thomas Lamprecht]

On 09/06/2017 01:15 PM, Laszlo Ersek wrote:
> Hello Thomas,
> 
> (CC Ard, Liming, Yonghong)
> 
> On 09/06/17 09:44, Thomas Lamprecht wrote:
>> Hi,
>>
>> commit 2ad6ba80a1bd58382bde6b994070f7c01d2fb48d
>> Author: Laszlo Ersek <lersek@redhat.com>
>> Date:   Wed Aug 30 14:00:58 2017 +0200
>>
>>      OvmfPkg/IoMmuDxe: IoMmuMap(): log nicer and more informative DEBUG msgs
>>
>> triggers a unused-const-variable warning for mBusMasterOperationName,
>> and thus for releases with warnings-as-errors a build error.
>>
>> I'm using a quite vanilla Debian Stretch machine on amd64/x86_64
>> (running in qemu/KVM) as build host, gcc version is 6.3.0 20170516
>> (Debian 6.3.0-18)
>>
>> My build procedure looks like:
>>
>> # make -C BaseTools/
>> # . edksetup.sh
>> # OvmfPkg/build.sh -a X64 -b RELEASE -n 4 -t GCC5
>>
>> With current master (12cfc9009e7cf1a69ca675110c2cf6e21b152992) checked
>> out.
>>
>> I suspect that gcc, at least in this version, cannot track the usage
>> of the variable in crime in the DEBUG macros, and thus (falsely?)
>> detects this warning.
>>
>> So I'm not quite sure if this is a problem with edk2/Ovmf itself or a
>> problem stemming from gcc.
>>
>> The following patch fixes the problem quite nonchalantly by adding an
>> unused attribute, which is highly probably not what is wanted as a
>> clean fix - I guess - but it achieves my desired result :)
>>
>> ----8<----
>> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> index bc57de5b57..8c0b8b0931 100644
>> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> @@ -48,7 +48,7 @@ STATIC LIST_ENTRY mRecycledMapInfos =
>> INITIALIZE_LIST_HEAD_VARIABLE (
>>   // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
>>   //
>>   STATIC CONST CHAR8 * CONST
>> -mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>> +mBusMasterOperationName[EdkiiIoMmuOperationMaximum] __attribute__
>> ((unused)) = {
>>     "Read",
>>     "Write",
>>     "CommonBuffer",
>> ---->8----
>>
>> It naturally could be that I've got something wrong, wouldn't be the
>> first time, but I suspect that it's edk2 in combination with my GCC
>> version this time. As I lack in depth knowledge of Ovmf it'd be nice
>> if someone could confirm my suspicion.
>>
>> CC'ing Laszlo as he is the author of the patch which lets this problem
>> trigger and following this mailing list since a bit it seems that he
>> surely has the in depth knowledge. Sorry that I wasn't patient enough
>> to test other compiler version before posting here.
> 
> Thanks for the report.
> 
> When I (recently) wrote the above commit, I was fully aware that the
> variable would become unused if DEBUG macro invocations were compiled
> out of the code (i.e., in the RELEASE build target).
> 
> However, that didn't worry me because we had already introduced a
> distinction for such variables, between RELEASE and DEBUG builds. As you
> can expect, this phenomenon is quite wide-spread; for example a Status
> variable may capture a function return value, only to be ASSERT()-ed
> upon. In a RELEASE build, the ASSERT() disappears, and we don't want the
> compiler to yell at us for setting, but never checking, Status.
> 

Ah yes, that makes sense.

> ... Please refer to git commit 20d00edf21d2 ("BaseTools/GCC: set
> -Wno-unused-but-set-variables only on RELEASE builds", 2016-03-24).
> 
> Looks like gcc-6 has a knob specific to const variables that we should
> give the same treatment. Looking at the following pages:
> 
>    https://gcc.gnu.org/onlinedocs/gcc-4.9.4/gcc/Warning-Options.html
>    https://gcc.gnu.org/onlinedocs/gcc-5.4.0/gcc/Warning-Options.html
>    https://gcc.gnu.org/onlinedocs/gcc-6.4.0/gcc/Warning-Options.html
>    https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Warning-Options.html
> 
> it appears that "-Wunused-const-variable" has first appeared in gcc-6.
> 
> Now, that's a problem. In edk2, we currently do not have a specific GCC6
> toolchain setting. We have several GCC4x toolchains, and one GCC5
> toolchain. Normally I would have asked you to send a patch, similar to
> commit 20d00edf21d2 above, to add "-Wno-unused-const-variable" to the
> "oldest" applicable macros in "BaseTools/Conf/tools_def.template", for
> example:
> 
>    RELEASE_GCC5_IA32_CC_FLAGS
>    RELEASE_GCC5_X64_CC_FLAGS
>    RELEASE_GCC5_ARM_CC_FLAGS
>    RELEASE_GCC5_AARCH64_CC_FLAGS
> 
> But gcc-5 would not recognize this option. I think we might have to
> introduce the GCC6 toolchain for this.
> 

After giving the "BaseTools/Conf/tools_def.template" another look, it
seems that adding the target would not be the main work here, but
testing it on all platforms and verifying that all other flags are still
correct and that for all platforms would be, or?

I could add a GCC6 one which mainly inherits from GCC5, i.e. in the same
way as GCC49 is already the "parent" for GCC5.
But I could currently only compile and test OVMF for amd64 and aarch64.

I mean from a developer perspective it seems OK to do that and add the
target even without full coverage and the adapt the target toolchain -
or the platform where appropriate,  on a case per case basis, once the
new toolchain will see more use there.

> Here's what I suggest: please file a TianoCore BZ about this issue, at
> <https://bugzilla.tianocore.org/>. Select "BaseTools" as Package. Feel
> free to reference the mailing list thread in the BZ:
> 
> https://lists.01.org/pipermail/edk2-devel/2017-September/014225.html
> 

Done, I got #700 :
<https://bugzilla.tianocore.org/show_bug.cgi?id=700>

> Now, addressing that will take a while. Meanwhile, can you please check
> if simply removing the CONST suppresses the issue? (Note, the CONST
> should be removed from the "mBusMasterOperationName" variable, *not*
> from the pointed-to CHAR8.)
> 
> If that works, can you please submit a patch like this?
> 

No, it does not work, it then does not falls under the
"-Wno-unused-but-set-variable" case as its static, if I understand the gcc
Warning Options doc correctly:

  > -Wunused-but-set-variable:
  >     Warn whenever a local variable is assigned to, ...

Whereas:

  > -Wunused-variable
  >     Warn whenever a local or static variable is unused aside from its
  >     declaration. This option implies -Wunused-const-variable=1 for C, ...

So "no-unused-but-set" allows the local variable case but not the static
variable case.


>> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> index bc57de5b572b..45d1c909b5ad 100644
>> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> @@ -47,7 +47,9 @@ STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
>>   //
>>   // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
>>   //
>> -STATIC CONST CHAR8 * CONST
>> +// Not CONST-qualified in order to suppress gcc-6+ warnings.
>> +//
>> +STATIC CONST CHAR8 *
>>   mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>>     "Read",
>>     "Write",
> 
> We could commit this patch for now. Once the BZ you file for BaseTools
> is fixed, we can revert this patch.
> 

With your proposed workaround changes my compiler emits:

[...]
/root/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c:51:1: error: ‘mBusMasterOperationName’ defined but not used [-Werror=unused-variable]
  mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
  ^~~~~~~~~~~~~~~~~~~~~~~

Another idea, why not also compile out the declaration of the
"mBusMasterOperationName" variable when building a RELEASE?

Something like:

----8<----
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index bc57de5b57..5af5ce258f 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -47,6 +47,7 @@ STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
  //
  // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
  //
+#ifdef EFI_DEBUG^M
  STATIC CONST CHAR8 * CONST
  mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
    "Read",
@@ -56,6 +57,7 @@ mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
    "Write64",
    "CommonBuffer64"
  };
+#endif^M

  //
  // The following structure enables Map() and Unmap() to perform in-place
---->8----

works here, not sure if its up to your coding standards.

cheers,
Thomas

Re: OvmfPkg/IoMmuDxe: unused-const-variable warning fails release build with GCC 6.3

[Laszlo Ersek]

On 09/06/17 15:35, Thomas Lamprecht wrote:
> On 09/06/2017 01:15 PM, Laszlo Ersek wrote:
>> Hello Thomas,
>>
>> (CC Ard, Liming, Yonghong)
>>
>> On 09/06/17 09:44, Thomas Lamprecht wrote:
>>> Hi,
>>>
>>> commit 2ad6ba80a1bd58382bde6b994070f7c01d2fb48d
>>> Author: Laszlo Ersek <lersek@redhat.com>
>>> Date:   Wed Aug 30 14:00:58 2017 +0200
>>>
>>>      OvmfPkg/IoMmuDxe: IoMmuMap(): log nicer and more informative
>>> DEBUG msgs
>>>
>>> triggers a unused-const-variable warning for mBusMasterOperationName,
>>> and thus for releases with warnings-as-errors a build error.
>>>
>>> I'm using a quite vanilla Debian Stretch machine on amd64/x86_64
>>> (running in qemu/KVM) as build host, gcc version is 6.3.0 20170516
>>> (Debian 6.3.0-18)
>>>
>>> My build procedure looks like:
>>>
>>> # make -C BaseTools/
>>> # . edksetup.sh
>>> # OvmfPkg/build.sh -a X64 -b RELEASE -n 4 -t GCC5
>>>
>>> With current master (12cfc9009e7cf1a69ca675110c2cf6e21b152992) checked
>>> out.
>>>
>>> I suspect that gcc, at least in this version, cannot track the usage
>>> of the variable in crime in the DEBUG macros, and thus (falsely?)
>>> detects this warning.
>>>
>>> So I'm not quite sure if this is a problem with edk2/Ovmf itself or a
>>> problem stemming from gcc.
>>>
>>> The following patch fixes the problem quite nonchalantly by adding an
>>> unused attribute, which is highly probably not what is wanted as a
>>> clean fix - I guess - but it achieves my desired result :)
>>>
>>> ----8<----
>>> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> index bc57de5b57..8c0b8b0931 100644
>>> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> @@ -48,7 +48,7 @@ STATIC LIST_ENTRY mRecycledMapInfos =
>>> INITIALIZE_LIST_HEAD_VARIABLE (
>>>   // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
>>>   //
>>>   STATIC CONST CHAR8 * CONST
>>> -mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>>> +mBusMasterOperationName[EdkiiIoMmuOperationMaximum] __attribute__
>>> ((unused)) = {
>>>     "Read",
>>>     "Write",
>>>     "CommonBuffer",
>>> ---->8----
>>>
>>> It naturally could be that I've got something wrong, wouldn't be the
>>> first time, but I suspect that it's edk2 in combination with my GCC
>>> version this time. As I lack in depth knowledge of Ovmf it'd be nice
>>> if someone could confirm my suspicion.
>>>
>>> CC'ing Laszlo as he is the author of the patch which lets this problem
>>> trigger and following this mailing list since a bit it seems that he
>>> surely has the in depth knowledge. Sorry that I wasn't patient enough
>>> to test other compiler version before posting here.
>>
>> Thanks for the report.
>>
>> When I (recently) wrote the above commit, I was fully aware that the
>> variable would become unused if DEBUG macro invocations were compiled
>> out of the code (i.e., in the RELEASE build target).
>>
>> However, that didn't worry me because we had already introduced a
>> distinction for such variables, between RELEASE and DEBUG builds. As you
>> can expect, this phenomenon is quite wide-spread; for example a Status
>> variable may capture a function return value, only to be ASSERT()-ed
>> upon. In a RELEASE build, the ASSERT() disappears, and we don't want the
>> compiler to yell at us for setting, but never checking, Status.
>>
> 
> Ah yes, that makes sense.
> 
>> ... Please refer to git commit 20d00edf21d2 ("BaseTools/GCC: set
>> -Wno-unused-but-set-variables only on RELEASE builds", 2016-03-24).
>>
>> Looks like gcc-6 has a knob specific to const variables that we should
>> give the same treatment. Looking at the following pages:
>>
>>    https://gcc.gnu.org/onlinedocs/gcc-4.9.4/gcc/Warning-Options.html
>>    https://gcc.gnu.org/onlinedocs/gcc-5.4.0/gcc/Warning-Options.html
>>    https://gcc.gnu.org/onlinedocs/gcc-6.4.0/gcc/Warning-Options.html
>>    https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Warning-Options.html
>>
>> it appears that "-Wunused-const-variable" has first appeared in gcc-6.
>>
>> Now, that's a problem. In edk2, we currently do not have a specific GCC6
>> toolchain setting. We have several GCC4x toolchains, and one GCC5
>> toolchain. Normally I would have asked you to send a patch, similar to
>> commit 20d00edf21d2 above, to add "-Wno-unused-const-variable" to the
>> "oldest" applicable macros in "BaseTools/Conf/tools_def.template", for
>> example:
>>
>>    RELEASE_GCC5_IA32_CC_FLAGS
>>    RELEASE_GCC5_X64_CC_FLAGS
>>    RELEASE_GCC5_ARM_CC_FLAGS
>>    RELEASE_GCC5_AARCH64_CC_FLAGS
>>
>> But gcc-5 would not recognize this option. I think we might have to
>> introduce the GCC6 toolchain for this.
>>
> 
> After giving the "BaseTools/Conf/tools_def.template" another look, it
> seems that adding the target would not be the main work here, but
> testing it on all platforms and verifying that all other flags are still
> correct and that for all platforms would be, or?
> 
> I could add a GCC6 one which mainly inherits from GCC5, i.e. in the same
> way as GCC49 is already the "parent" for GCC5.
> But I could currently only compile and test OVMF for amd64 and aarch64.

Hmm, if you can compile for X64, how can you not compile it for IA32? It
should not require different (native) compiler + binutils packages on
your system.

If you are cross-compiling for AARCH64, the situation is then different,
because for cross-compiling to ARM, separate compiler + binutils
packages could be required.

> 
> I mean from a developer perspective it seems OK to do that and add the
> target even without full coverage and the adapt the target toolchain -
> or the platform where appropriate,  on a case per case basis, once the
> new toolchain will see more use there.

If you want to explore adding the new toolchain, I guess you could
submit a patch with the whole thing (all arches), and ask for help with
testing (always specifying "-t GCC6" explicitly). I can see the
following test cases:

  { DEBUG, RELEASE } x { IA32, X64, ARM, AARCH64 } x { gcc-6, gcc-7 }

     1  gcc-6  AARCH64  DEBUG
     2  gcc-6  AARCH64  RELEASE
     3  gcc-6  ARM      DEBUG
     4  gcc-6  ARM      RELEASE
     5  gcc-6  IA32     DEBUG
     6  gcc-6  IA32     RELEASE
     7  gcc-6  X64      DEBUG
     8  gcc-6  X64      RELEASE
     9  gcc-7  AARCH64  DEBUG
    10  gcc-7  AARCH64  RELEASE
    11  gcc-7  ARM      DEBUG
    12  gcc-7  ARM      RELEASE
    13  gcc-7  IA32     DEBUG
    14  gcc-7  IA32     RELEASE
    15  gcc-7  X64      DEBUG
    16  gcc-7  X64      RELEASE

You should be able to build-test cases 1-2, 5-8.

I can help you build-test cases 3-4 (already have that cross-compiler
installed).

Cases 9-16 are testable, for example, within a Fedora 26 virtual
machine. Unless someone on the list runs Fedora 26 on a permanent basis,
the effort of setting up a VM would be the same for them as for you.
But, you might get lucky.

Boot testing is secondary in this case, IMO. If your build passes a
smoke test for booting, on your side, that should be sufficient.
Miscompilations due to new GCC versions have occurred, but they are
extremely time consuming to figure out, so we generally investigate them
when they occur, not in advance. The point is that older toolchains are
not regressed by adding newer ones.

>> Here's what I suggest: please file a TianoCore BZ about this issue, at
>> <https://bugzilla.tianocore.org/>. Select "BaseTools" as Package. Feel
>> free to reference the mailing list thread in the BZ:
>>
>> https://lists.01.org/pipermail/edk2-devel/2017-September/014225.html
>>
> 
> Done, I got #700 :
> <https://bugzilla.tianocore.org/show_bug.cgi?id=700>
> 
>> Now, addressing that will take a while. Meanwhile, can you please check
>> if simply removing the CONST suppresses the issue? (Note, the CONST
>> should be removed from the "mBusMasterOperationName" variable, *not*
>> from the pointed-to CHAR8.)
>>
>> If that works, can you please submit a patch like this?
>>
> 
> No, it does not work, it then does not falls under the
> "-Wno-unused-but-set-variable" case as its static, if I understand the gcc
> Warning Options doc correctly:
> 
>  > -Wunused-but-set-variable:
>  >     Warn whenever a local variable is assigned to, ...
> 
> Whereas:
> 
>  > -Wunused-variable
>  >     Warn whenever a local or static variable is unused aside from its
>  >     declaration. This option implies -Wunused-const-variable=1 for C,
> ...
> 
> So "no-unused-but-set" allows the local variable case but not the static
> variable case.
> 
> 
>>> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> index bc57de5b572b..45d1c909b5ad 100644
>>> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>> @@ -47,7 +47,9 @@ STATIC LIST_ENTRY mRecycledMapInfos =
>>> INITIALIZE_LIST_HEAD_VARIABLE (
>>>   //
>>>   // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
>>>   //
>>> -STATIC CONST CHAR8 * CONST
>>> +// Not CONST-qualified in order to suppress gcc-6+ warnings.
>>> +//
>>> +STATIC CONST CHAR8 *
>>>   mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>>>     "Read",
>>>     "Write",
>>
>> We could commit this patch for now. Once the BZ you file for BaseTools
>> is fixed, we can revert this patch.
>>
> 
> With your proposed workaround changes my compiler emits:
> 
> [...]
> /root/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c:51:1: error:
> ‘mBusMasterOperationName’ defined but not used [-Werror=unused-variable]
>  mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>  ^~~~~~~~~~~~~~~~~~~~~~~

Thank you for trying it.

How incredibly annoying. :/

I'm unsure if we want to disble "-Wunused-variable" even for RELEASE
builds. Probably not. Needs more discussion by others too.

> Another idea, why not also compile out the declaration of the
> "mBusMasterOperationName" variable when building a RELEASE?

Code that depends on DEBUG vs. RELEASE should generally be evicted by
the compiler, not the preprocessor. This makes for much better source
code coverage.

> Something like:
> 
> ----8<----
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index bc57de5b57..5af5ce258f 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -47,6 +47,7 @@ STATIC LIST_ENTRY mRecycledMapInfos =
> INITIALIZE_LIST_HEAD_VARIABLE (
>  //
>  // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
>  //
> +#ifdef EFI_DEBUG^M
>  STATIC CONST CHAR8 * CONST
>  mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>    "Read",
> @@ -56,6 +57,7 @@ mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>    "Write64",
>    "CommonBuffer64"
>  };
> +#endif^M
> 
>  //
>  // The following structure enables Map() and Unmap() to perform in-place
> ---->8----
> 
> works here, not sure if its up to your coding standards.

It's not.

We already have macros like DEBUG_CODE(), DEBUG_CODE_BEGIN() and
DEBUG_CODE_END() -- they provide the above coverage --, but they only
work in block scope, not file scope.

Sigh. Let me submit another OvmfPkg patch for this. I'll CC you. Please
report back if it works.

Thanks
Laszlo

Re: OvmfPkg/IoMmuDxe: unused-const-variable warning fails release build with GCC 6.3

[Thomas Lamprecht]

On 09/06/2017 04:22 PM, Laszlo Ersek wrote:
> On 09/06/17 15:35, Thomas Lamprecht wrote:
>> On 09/06/2017 01:15 PM, Laszlo Ersek wrote:
>> [snip]
>>> But gcc-5 would not recognize this option. I think we might have to
>>> introduce the GCC6 toolchain for this.
>>>
>>
>> After giving the "BaseTools/Conf/tools_def.template" another look, it
>> seems that adding the target would not be the main work here, but
>> testing it on all platforms and verifying that all other flags are still
>> correct and that for all platforms would be, or?
>>
>> I could add a GCC6 one which mainly inherits from GCC5, i.e. in the same
>> way as GCC49 is already the "parent" for GCC5.
>> But I could currently only compile and test OVMF for amd64 and aarch64.
> 
> Hmm, if you can compile for X64, how can you not compile it for IA32? It
> should not require different (native) compiler + binutils packages on
> your system.
> 

Yes, I can test IA32 naturally too, just forgot to mention it.

> If you are cross-compiling for AARCH64, the situation is then different,
> because for cross-compiling to ARM, separate compiler + binutils
> packages could be required.
> 

As I have an APM x-gene here I can compile aarch64 natively running our
Debian derivated distro.
Did not configured a 32-bit environment yet, but that should be possible.

>>
>> I mean from a developer perspective it seems OK to do that and add the
>> target even without full coverage and the adapt the target toolchain -
>> or the platform where appropriate,  on a case per case basis, once the
>> new toolchain will see more use there.
> 
> If you want to explore adding the new toolchain, I guess you could
> submit a patch with the whole thing (all arches), and ask for help with
> testing (always specifying "-t GCC6" explicitly). I can see the
> following test cases:
> 
>    { DEBUG, RELEASE } x { IA32, X64, ARM, AARCH64 } x { gcc-6, gcc-7 }
> 
>       1  gcc-6  AARCH64  DEBUG
>       2  gcc-6  AARCH64  RELEASE
>       3  gcc-6  ARM      DEBUG
>       4  gcc-6  ARM      RELEASE
>       5  gcc-6  IA32     DEBUG
>       6  gcc-6  IA32     RELEASE
>       7  gcc-6  X64      DEBUG
>       8  gcc-6  X64      RELEASE
>       9  gcc-7  AARCH64  DEBUG
>      10  gcc-7  AARCH64  RELEASE
>      11  gcc-7  ARM      DEBUG
>      12  gcc-7  ARM      RELEASE
>      13  gcc-7  IA32     DEBUG
>      14  gcc-7  IA32     RELEASE
>      15  gcc-7  X64      DEBUG
>      16  gcc-7  X64      RELEASE
> 
> You should be able to build-test cases 1-2, 5-8.
> 
> I can help you build-test cases 3-4 (already have that cross-compiler
> installed).
> 
> Cases 9-16 are testable, for example, within a Fedora 26 virtual
> machine. Unless someone on the list runs Fedora 26 on a permanent basis,
> the effort of setting up a VM would be the same for them as for you.
> But, you might get lucky.
> 

I got a Fedora VM here already, so this should be also no problem for
me then, great!

> Boot testing is secondary in this case, IMO. If your build passes a
> smoke test for booting, on your side, that should be sufficient.
> Miscompilations due to new GCC versions have occurred, but they are
> extremely time consuming to figure out, so we generally investigate them
> when they occur, not in advance. The point is that older toolchains are
> not regressed by adding newer ones.
> 

OK.

>>> Here's what I suggest: please file a TianoCore BZ about this issue, at
>>> <https://bugzilla.tianocore.org/>. Select "BaseTools" as Package. Feel
>>> free to reference the mailing list thread in the BZ:
>>>
>>> https://lists.01.org/pipermail/edk2-devel/2017-September/014225.html
>>>
>>
>> Done, I got #700 :
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=700>
>>
>>> Now, addressing that will take a while. Meanwhile, can you please check
>>> if simply removing the CONST suppresses the issue? (Note, the CONST
>>> should be removed from the "mBusMasterOperationName" variable, *not*
>>> from the pointed-to CHAR8.)
>>>
>>> If that works, can you please submit a patch like this?
>>>
>>
>> No, it does not work, it then does not falls under the
>> "-Wno-unused-but-set-variable" case as its static, if I understand the gcc
>> Warning Options doc correctly:
>>
>>   > -Wunused-but-set-variable:
>>   >     Warn whenever a local variable is assigned to, ...
>>
>> Whereas:
>>
>>   > -Wunused-variable
>>   >     Warn whenever a local or static variable is unused aside from its
>>   >     declaration. This option implies -Wunused-const-variable=1 for C,
>> ...
>>
>> So "no-unused-but-set" allows the local variable case but not the static
>> variable case.
>>
>>
>>>> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>>> b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>>> index bc57de5b572b..45d1c909b5ad 100644
>>>> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>>> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>>>> @@ -47,7 +47,9 @@ STATIC LIST_ENTRY mRecycledMapInfos =
>>>> INITIALIZE_LIST_HEAD_VARIABLE (
>>>>    //
>>>>    // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
>>>>    //
>>>> -STATIC CONST CHAR8 * CONST
>>>> +// Not CONST-qualified in order to suppress gcc-6+ warnings.
>>>> +//
>>>> +STATIC CONST CHAR8 *
>>>>    mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>>>>      "Read",
>>>>      "Write",
>>>
>>> We could commit this patch for now. Once the BZ you file for BaseTools
>>> is fixed, we can revert this patch.
>>>
>>
>> With your proposed workaround changes my compiler emits:
>>
>> [...]
>> /root/edk2/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c:51:1: error:
>> ‘mBusMasterOperationName’ defined but not used [-Werror=unused-variable]
>>   mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>>   ^~~~~~~~~~~~~~~~~~~~~~~
> 
> Thank you for trying it.
> 
> How incredibly annoying. :/
> 
> I'm unsure if we want to disble "-Wunused-variable" even for RELEASE
> builds. Probably not. Needs more discussion by others too.
> 
>> Another idea, why not also compile out the declaration of the
>> "mBusMasterOperationName" variable when building a RELEASE?
> 
> Code that depends on DEBUG vs. RELEASE should generally be evicted by
> the compiler, not the preprocessor. This makes for much better source
> code coverage.
> 

Uh, ok now looking again I see why. I first just saw:
"EdkCompatibilityPkg/Foundation/Include/EfiDebug.h"
Where both cases, mine and DEBUG(expr), would get removed by the
preprocessor. But the here actual used "MdePkg/Include/Library/DebugLib.h"
tells me otherwise.

>> Something like:
>>
>> ----8<----
>> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> index bc57de5b57..5af5ce258f 100644
>> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> @@ -47,6 +47,7 @@ STATIC LIST_ENTRY mRecycledMapInfos =
>> INITIALIZE_LIST_HEAD_VARIABLE (
>>   //
>>   // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
>>   //
>> +#ifdef EFI_DEBUG^M
>>   STATIC CONST CHAR8 * CONST
>>   mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>>     "Read",
>> @@ -56,6 +57,7 @@ mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>>     "Write64",
>>     "CommonBuffer64"
>>   };
>> +#endif^M
>>
>>   //
>>   // The following structure enables Map() and Unmap() to perform in-place
>> ---->8----
>>
>> works here, not sure if its up to your coding standards.
> 
> It's not.
> 
> We already have macros like DEBUG_CODE(), DEBUG_CODE_BEGIN() and
> DEBUG_CODE_END() -- they provide the above coverage --, but they only
> work in block scope, not file scope.
> 
> Sigh. Let me submit another OvmfPkg patch for this. I'll CC you. Please
> report back if it works.
> 

OK.

cheers,
Thomas