[PATCH 0/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE

[PATCH 0/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: iommu_unused_const_variable

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Thomas Lamprecht <t.lamprecht@proxmox.com>

Laszlo Ersek (1):
  OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in
    RELEASE

 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 29 +++++++++++---------
 1 file changed, 16 insertions(+), 13 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 1/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE

[Laszlo Ersek]

Starting with gcc-6, a new warning option called "-Wunused-const-variable"
has become available (and enabled by default under our build settings):

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

We should give it the same treatment as Ard gave "unused-but-set-variable"
in commit 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables
only on RELEASE builds", 2016-03-24); i.e., we should restrict the warning
to the DEBUG build target.

However, because the new warning is gcc-6+ only, we cannot add
"-Wno-unused-const-variable" to any GCC5 macros in
"BaseTools/Conf/tools_def.template". While the GCC6 toolchain and/or the
desired handling of the new warning are investigated in
<https://bugzilla.tianocore.org/show_bug.cgi?id=700>, suppress the warning
for gcc-6+ (in RELEASE builds) as follows:

- Replace the "mBusMasterOperationName" array with the
  BUS_MASTER_OPERATION_NAME(Expression, ShortName) macro that compares
  Expression against EdkiiIoMmuOperationBusMaster<ShortName>, and in case
  of a match, evaluates to "ShortName", stringified,

- when composing the DEBUG message -- which the preprocessor might
  eliminate from RELEASE builds, thereby removing its references to
  variables --, build a ladder of comparisons with
  BUS_MASTER_OPERATION_NAME().

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Thomas Lamprecht <t.lamprecht@proxmox.com>
Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 29 +++++++++++---------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index bc57de5b572b..dcd03d8f0474 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -45,17 +45,17 @@ STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
 #define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R')
 
 //
-// ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
+// The following macro builds the first two operands of a conditional (ternary)
+// operator that matches Expression against the EDKII_IOMMU_OPERATION constant
+// derived from ShortName. In case of a match, the replacement text evaluates
+// to an ASCII string literal that stands for the constant. The replacement
+// text stops before the colon (":"). The macro invocation site is supposed to
+// provide the colon and the third operand, either as another invocation of the
+// same macro, or as a default (fallback) string literal.
 //
-STATIC CONST CHAR8 * CONST
-mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
-  "Read",
-  "Write",
-  "CommonBuffer",
-  "Read64",
-  "Write64",
-  "CommonBuffer64"
-};
+#define BUS_MASTER_OPERATION_NAME(Expression, ShortName) \
+          ((Expression) == (EdkiiIoMmuOperationBusMaster ## ShortName)) ? \
+          (# ShortName)
 
 //
 // The following structure enables Map() and Unmap() to perform in-place
@@ -133,9 +133,12 @@ IoMmuMap (
     DEBUG_VERBOSE,
     "%a: Operation=%a Host=0x%p Bytes=0x%Lx\n",
     __FUNCTION__,
-    ((Operation >= 0 &&
-      Operation < ARRAY_SIZE (mBusMasterOperationName)) ?
-     mBusMasterOperationName[Operation] :
+    (BUS_MASTER_OPERATION_NAME (Operation, Read) :
+     BUS_MASTER_OPERATION_NAME (Operation, Write) :
+     BUS_MASTER_OPERATION_NAME (Operation, CommonBuffer) :
+     BUS_MASTER_OPERATION_NAME (Operation, Read64) :
+     BUS_MASTER_OPERATION_NAME (Operation, Write64) :
+     BUS_MASTER_OPERATION_NAME (Operation, CommonBuffer64) :
      "Invalid"),
     HostAddress,
     (UINT64)((NumberOfBytes == NULL) ? 0 : *NumberOfBytes)
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 1/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE

[Ard Biesheuvel]

On 6 September 2017 at 17:48, Laszlo Ersek <lersek@redhat.com> wrote:
> Starting with gcc-6, a new warning option called "-Wunused-const-variable"
> has become available (and enabled by default under our build settings):
>
>   https://gcc.gnu.org/onlinedocs/gcc-6.4.0/gcc/Warning-Options.html
>
> We should give it the same treatment as Ard gave "unused-but-set-variable"
> in commit 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables
> only on RELEASE builds", 2016-03-24); i.e., we should restrict the warning
> to the DEBUG build target.
>
> However, because the new warning is gcc-6+ only, we cannot add
> "-Wno-unused-const-variable" to any GCC5 macros in
> "BaseTools/Conf/tools_def.template". While the GCC6 toolchain and/or the
> desired handling of the new warning are investigated in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=700>, suppress the warning
> for gcc-6+ (in RELEASE builds) as follows:
>
> - Replace the "mBusMasterOperationName" array with the
>   BUS_MASTER_OPERATION_NAME(Expression, ShortName) macro that compares
>   Expression against EdkiiIoMmuOperationBusMaster<ShortName>, and in case
>   of a match, evaluates to "ShortName", stringified,
>
> - when composing the DEBUG message -- which the preprocessor might
>   eliminate from RELEASE builds, thereby removing its references to
>   variables --, build a ladder of comparisons with
>   BUS_MASTER_OPERATION_NAME().
>

I wonder if we should put this in a header file somewhere. I really
hate that we need to create new GCCx versions just for small
deviations between versions of the compiler.

I.e., add this to MdePkg/Include/Base.h

#ifdef MDEPKG_NDEBUG
  //
  // Don't warn about unused but set variables: they may only be
  // referenced by DEBUG code.
  //
  #pragma GCC diagnostic ignored "-Wunused-but-set-variable"
  #if __GNUC__ >= 6
    #pragma GCC diagnostic ignored "-Wunused-const-variable"
  #endif
#endif


> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 29 +++++++++++---------
>  1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index bc57de5b572b..dcd03d8f0474 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -45,17 +45,17 @@ STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
>  #define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R')
>
>  //
> -// ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
> +// The following macro builds the first two operands of a conditional (ternary)
> +// operator that matches Expression against the EDKII_IOMMU_OPERATION constant
> +// derived from ShortName. In case of a match, the replacement text evaluates
> +// to an ASCII string literal that stands for the constant. The replacement
> +// text stops before the colon (":"). The macro invocation site is supposed to
> +// provide the colon and the third operand, either as another invocation of the
> +// same macro, or as a default (fallback) string literal.
>  //
> -STATIC CONST CHAR8 * CONST
> -mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
> -  "Read",
> -  "Write",
> -  "CommonBuffer",
> -  "Read64",
> -  "Write64",
> -  "CommonBuffer64"
> -};
> +#define BUS_MASTER_OPERATION_NAME(Expression, ShortName) \
> +          ((Expression) == (EdkiiIoMmuOperationBusMaster ## ShortName)) ? \
> +          (# ShortName)
>
>  //
>  // The following structure enables Map() and Unmap() to perform in-place
> @@ -133,9 +133,12 @@ IoMmuMap (
>      DEBUG_VERBOSE,
>      "%a: Operation=%a Host=0x%p Bytes=0x%Lx\n",
>      __FUNCTION__,
> -    ((Operation >= 0 &&
> -      Operation < ARRAY_SIZE (mBusMasterOperationName)) ?
> -     mBusMasterOperationName[Operation] :
> +    (BUS_MASTER_OPERATION_NAME (Operation, Read) :
> +     BUS_MASTER_OPERATION_NAME (Operation, Write) :
> +     BUS_MASTER_OPERATION_NAME (Operation, CommonBuffer) :
> +     BUS_MASTER_OPERATION_NAME (Operation, Read64) :
> +     BUS_MASTER_OPERATION_NAME (Operation, Write64) :
> +     BUS_MASTER_OPERATION_NAME (Operation, CommonBuffer64) :
>       "Invalid"),
>      HostAddress,
>      (UINT64)((NumberOfBytes == NULL) ? 0 : *NumberOfBytes)
> --
> 2.14.1.3.gb7cf6e02401b
>

Re: [PATCH 1/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE

[Laszlo Ersek]

On 09/06/17 18:56, Ard Biesheuvel wrote:
> On 6 September 2017 at 17:48, Laszlo Ersek <lersek@redhat.com> wrote:
>> Starting with gcc-6, a new warning option called "-Wunused-const-variable"
>> has become available (and enabled by default under our build settings):
>>
>>   https://gcc.gnu.org/onlinedocs/gcc-6.4.0/gcc/Warning-Options.html
>>
>> We should give it the same treatment as Ard gave "unused-but-set-variable"
>> in commit 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables
>> only on RELEASE builds", 2016-03-24); i.e., we should restrict the warning
>> to the DEBUG build target.
>>
>> However, because the new warning is gcc-6+ only, we cannot add
>> "-Wno-unused-const-variable" to any GCC5 macros in
>> "BaseTools/Conf/tools_def.template". While the GCC6 toolchain and/or the
>> desired handling of the new warning are investigated in
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=700>, suppress the warning
>> for gcc-6+ (in RELEASE builds) as follows:
>>
>> - Replace the "mBusMasterOperationName" array with the
>>   BUS_MASTER_OPERATION_NAME(Expression, ShortName) macro that compares
>>   Expression against EdkiiIoMmuOperationBusMaster<ShortName>, and in case
>>   of a match, evaluates to "ShortName", stringified,
>>
>> - when composing the DEBUG message -- which the preprocessor might
>>   eliminate from RELEASE builds, thereby removing its references to
>>   variables --, build a ladder of comparisons with
>>   BUS_MASTER_OPERATION_NAME().
>>
> 
> I wonder if we should put this in a header file somewhere. I really
> hate that we need to create new GCCx versions just for small
> deviations between versions of the compiler.
> 
> I.e., add this to MdePkg/Include/Base.h
> 
> #ifdef MDEPKG_NDEBUG
>   //
>   // Don't warn about unused but set variables: they may only be
>   // referenced by DEBUG code.
>   //
>   #pragma GCC diagnostic ignored "-Wunused-but-set-variable"
>   #if __GNUC__ >= 6
>     #pragma GCC diagnostic ignored "-Wunused-const-variable"
>   #endif
> #endif

The option in question is "-Wno-unused-const-variable". But, that's a
side question.

If the above is acceptable to the MdePkg maintainers, it is certainly
good enough for me. In that case, I will reassign TianoCore BZ#700 to
MdePkg. (I was careful to write "desired handling of the new warning" in
the commit message, because I expected that the introduction of the GCC6
toolchain just for this might not be universally liked.)

Personally I wouldn't like to submit the MdePkg patch myself, as GCC
pragmas are another delicacy that doesn't fit on my plate right now. (If
I happen to break something with it, I get to fix it too... Like I'm
fixing it now within OvmfPkg.)

The point of this patch is to get OVMF back to a building state ASAP.
Once we fix BZ#700, I'm more than happy to revert the patch. I sought to
write the patch in a way that would be tolerable both short-term and
mid/long-term.

Thanks!
Laszlo


> 
> 
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Thomas Lamprecht <t.lamprecht@proxmox.com>
>> Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 29 +++++++++++---------
>>  1 file changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> index bc57de5b572b..dcd03d8f0474 100644
>> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
>> @@ -45,17 +45,17 @@ STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
>>  #define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R')
>>
>>  //
>> -// ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
>> +// The following macro builds the first two operands of a conditional (ternary)
>> +// operator that matches Expression against the EDKII_IOMMU_OPERATION constant
>> +// derived from ShortName. In case of a match, the replacement text evaluates
>> +// to an ASCII string literal that stands for the constant. The replacement
>> +// text stops before the colon (":"). The macro invocation site is supposed to
>> +// provide the colon and the third operand, either as another invocation of the
>> +// same macro, or as a default (fallback) string literal.
>>  //
>> -STATIC CONST CHAR8 * CONST
>> -mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
>> -  "Read",
>> -  "Write",
>> -  "CommonBuffer",
>> -  "Read64",
>> -  "Write64",
>> -  "CommonBuffer64"
>> -};
>> +#define BUS_MASTER_OPERATION_NAME(Expression, ShortName) \
>> +          ((Expression) == (EdkiiIoMmuOperationBusMaster ## ShortName)) ? \
>> +          (# ShortName)
>>
>>  //
>>  // The following structure enables Map() and Unmap() to perform in-place
>> @@ -133,9 +133,12 @@ IoMmuMap (
>>      DEBUG_VERBOSE,
>>      "%a: Operation=%a Host=0x%p Bytes=0x%Lx\n",
>>      __FUNCTION__,
>> -    ((Operation >= 0 &&
>> -      Operation < ARRAY_SIZE (mBusMasterOperationName)) ?
>> -     mBusMasterOperationName[Operation] :
>> +    (BUS_MASTER_OPERATION_NAME (Operation, Read) :
>> +     BUS_MASTER_OPERATION_NAME (Operation, Write) :
>> +     BUS_MASTER_OPERATION_NAME (Operation, CommonBuffer) :
>> +     BUS_MASTER_OPERATION_NAME (Operation, Read64) :
>> +     BUS_MASTER_OPERATION_NAME (Operation, Write64) :
>> +     BUS_MASTER_OPERATION_NAME (Operation, CommonBuffer64) :
>>       "Invalid"),
>>      HostAddress,
>>      (UINT64)((NumberOfBytes == NULL) ? 0 : *NumberOfBytes)
>> --
>> 2.14.1.3.gb7cf6e02401b
>>

Re: [PATCH 1/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE

[Ard Biesheuvel]

On 6 September 2017 at 18:09, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/06/17 18:56, Ard Biesheuvel wrote:
>> On 6 September 2017 at 17:48, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Starting with gcc-6, a new warning option called "-Wunused-const-variable"
>>> has become available (and enabled by default under our build settings):
>>>
>>>   https://gcc.gnu.org/onlinedocs/gcc-6.4.0/gcc/Warning-Options.html
>>>
>>> We should give it the same treatment as Ard gave "unused-but-set-variable"
>>> in commit 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables
>>> only on RELEASE builds", 2016-03-24); i.e., we should restrict the warning
>>> to the DEBUG build target.
>>>
>>> However, because the new warning is gcc-6+ only, we cannot add
>>> "-Wno-unused-const-variable" to any GCC5 macros in
>>> "BaseTools/Conf/tools_def.template". While the GCC6 toolchain and/or the
>>> desired handling of the new warning are investigated in
>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=700>, suppress the warning
>>> for gcc-6+ (in RELEASE builds) as follows:
>>>
>>> - Replace the "mBusMasterOperationName" array with the
>>>   BUS_MASTER_OPERATION_NAME(Expression, ShortName) macro that compares
>>>   Expression against EdkiiIoMmuOperationBusMaster<ShortName>, and in case
>>>   of a match, evaluates to "ShortName", stringified,
>>>
>>> - when composing the DEBUG message -- which the preprocessor might
>>>   eliminate from RELEASE builds, thereby removing its references to
>>>   variables --, build a ladder of comparisons with
>>>   BUS_MASTER_OPERATION_NAME().
>>>
>>
>> I wonder if we should put this in a header file somewhere. I really
>> hate that we need to create new GCCx versions just for small
>> deviations between versions of the compiler.
>>
>> I.e., add this to MdePkg/Include/Base.h
>>
>> #ifdef MDEPKG_NDEBUG
>>   //
>>   // Don't warn about unused but set variables: they may only be
>>   // referenced by DEBUG code.
>>   //
>>   #pragma GCC diagnostic ignored "-Wunused-but-set-variable"
>>   #if __GNUC__ >= 6
>>     #pragma GCC diagnostic ignored "-Wunused-const-variable"
>>   #endif
>> #endif
>
> The option in question is "-Wno-unused-const-variable". But, that's a
> side question.
>

No. The option is "-Wunused-const-variable", and adding the 'no-'
prefix is the same as ignoring it.

> If the above is acceptable to the MdePkg maintainers, it is certainly
> good enough for me. In that case, I will reassign TianoCore BZ#700 to
> MdePkg. (I was careful to write "desired handling of the new warning" in
> the commit message, because I expected that the introduction of the GCC6
> toolchain just for this might not be universally liked.)
>

No. I think we should have fewer rather than more. I think the current
collection in tools_def is ridiculous, but that is my personal
opinion.

> Personally I wouldn't like to submit the MdePkg patch myself, as GCC
> pragmas are another delicacy that doesn't fit on my plate right now. (If
> I happen to break something with it, I get to fix it too... Like I'm
> fixing it now within OvmfPkg.)
>
> The point of this patch is to get OVMF back to a building state ASAP.
> Once we fix BZ#700, I'm more than happy to revert the patch. I sought to
> write the patch in a way that would be tolerable both short-term and
> mid/long-term.
>

Fair enough.

Re: [PATCH 1/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE

[Laszlo Ersek]

On 09/06/17 19:12, Ard Biesheuvel wrote:
> On 6 September 2017 at 18:09, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/06/17 18:56, Ard Biesheuvel wrote:
>>> On 6 September 2017 at 17:48, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> Starting with gcc-6, a new warning option called "-Wunused-const-variable"
>>>> has become available (and enabled by default under our build settings):
>>>>
>>>>   https://gcc.gnu.org/onlinedocs/gcc-6.4.0/gcc/Warning-Options.html
>>>>
>>>> We should give it the same treatment as Ard gave "unused-but-set-variable"
>>>> in commit 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables
>>>> only on RELEASE builds", 2016-03-24); i.e., we should restrict the warning
>>>> to the DEBUG build target.
>>>>
>>>> However, because the new warning is gcc-6+ only, we cannot add
>>>> "-Wno-unused-const-variable" to any GCC5 macros in
>>>> "BaseTools/Conf/tools_def.template". While the GCC6 toolchain and/or the
>>>> desired handling of the new warning are investigated in
>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=700>, suppress the warning
>>>> for gcc-6+ (in RELEASE builds) as follows:
>>>>
>>>> - Replace the "mBusMasterOperationName" array with the
>>>>   BUS_MASTER_OPERATION_NAME(Expression, ShortName) macro that compares
>>>>   Expression against EdkiiIoMmuOperationBusMaster<ShortName>, and in case
>>>>   of a match, evaluates to "ShortName", stringified,
>>>>
>>>> - when composing the DEBUG message -- which the preprocessor might
>>>>   eliminate from RELEASE builds, thereby removing its references to
>>>>   variables --, build a ladder of comparisons with
>>>>   BUS_MASTER_OPERATION_NAME().
>>>>
>>>
>>> I wonder if we should put this in a header file somewhere. I really
>>> hate that we need to create new GCCx versions just for small
>>> deviations between versions of the compiler.
>>>
>>> I.e., add this to MdePkg/Include/Base.h
>>>
>>> #ifdef MDEPKG_NDEBUG
>>>   //
>>>   // Don't warn about unused but set variables: they may only be
>>>   // referenced by DEBUG code.
>>>   //
>>>   #pragma GCC diagnostic ignored "-Wunused-but-set-variable"
>>>   #if __GNUC__ >= 6
>>>     #pragma GCC diagnostic ignored "-Wunused-const-variable"
>>>   #endif
>>> #endif
>>
>> The option in question is "-Wno-unused-const-variable". But, that's a
>> side question.
>>
> 
> No. The option is "-Wunused-const-variable", and adding the 'no-'
> prefix is the same as ignoring it.

The best place to commit a typographical error is in the correction for
an earlier typographical error! :)

> 
>> If the above is acceptable to the MdePkg maintainers, it is certainly
>> good enough for me. In that case, I will reassign TianoCore BZ#700 to
>> MdePkg. (I was careful to write "desired handling of the new warning" in
>> the commit message, because I expected that the introduction of the GCC6
>> toolchain just for this might not be universally liked.)
>>
> 
> No. I think we should have fewer rather than more. I think the current
> collection in tools_def is ridiculous, but that is my personal
> opinion.
> 
>> Personally I wouldn't like to submit the MdePkg patch myself, as GCC
>> pragmas are another delicacy that doesn't fit on my plate right now. (If
>> I happen to break something with it, I get to fix it too... Like I'm
>> fixing it now within OvmfPkg.)
>>
>> The point of this patch is to get OVMF back to a building state ASAP.
>> Once we fix BZ#700, I'm more than happy to revert the patch. I sought to
>> write the patch in a way that would be tolerable both short-term and
>> mid/long-term.
>>
> 
> Fair enough.
> 

Thanks.
Laszlo

Re: [PATCH 1/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE

[Gao, Liming]

Laszlo:
  I add -Wno-unused-const-variable option in GCC5 RELEASE option, and build OvmfPkg with GCC5.3. I don't meet any warning or error. Do you meet any issue? So, you think we can't add this option in GCC5 tool chain. 

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, September 7, 2017 12:48 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; Thomas Lamprecht
> <t.lamprecht@proxmox.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [edk2] [PATCH 1/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE
> 
> Starting with gcc-6, a new warning option called "-Wunused-const-variable"
> has become available (and enabled by default under our build settings):
> 
>   https://gcc.gnu.org/onlinedocs/gcc-6.4.0/gcc/Warning-Options.html
> 
> We should give it the same treatment as Ard gave "unused-but-set-variable"
> in commit 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables
> only on RELEASE builds", 2016-03-24); i.e., we should restrict the warning
> to the DEBUG build target.
> 
> However, because the new warning is gcc-6+ only, we cannot add
> "-Wno-unused-const-variable" to any GCC5 macros in
> "BaseTools/Conf/tools_def.template". While the GCC6 toolchain and/or the
> desired handling of the new warning are investigated in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=700>, suppress the warning
> for gcc-6+ (in RELEASE builds) as follows:
> 
> - Replace the "mBusMasterOperationName" array with the
>   BUS_MASTER_OPERATION_NAME(Expression, ShortName) macro that compares
>   Expression against EdkiiIoMmuOperationBusMaster<ShortName>, and in case
>   of a match, evaluates to "ShortName", stringified,
> 
> - when composing the DEBUG message -- which the preprocessor might
>   eliminate from RELEASE builds, thereby removing its references to
>   variables --, build a ladder of comparisons with
>   BUS_MASTER_OPERATION_NAME().
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 29 +++++++++++---------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index bc57de5b572b..dcd03d8f0474 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -45,17 +45,17 @@ STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
>  #define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R')
> 
>  //
> -// ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
> +// The following macro builds the first two operands of a conditional (ternary)
> +// operator that matches Expression against the EDKII_IOMMU_OPERATION constant
> +// derived from ShortName. In case of a match, the replacement text evaluates
> +// to an ASCII string literal that stands for the constant. The replacement
> +// text stops before the colon (":"). The macro invocation site is supposed to
> +// provide the colon and the third operand, either as another invocation of the
> +// same macro, or as a default (fallback) string literal.
>  //
> -STATIC CONST CHAR8 * CONST
> -mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
> -  "Read",
> -  "Write",
> -  "CommonBuffer",
> -  "Read64",
> -  "Write64",
> -  "CommonBuffer64"
> -};
> +#define BUS_MASTER_OPERATION_NAME(Expression, ShortName) \
> +          ((Expression) == (EdkiiIoMmuOperationBusMaster ## ShortName)) ? \
> +          (# ShortName)
> 
>  //
>  // The following structure enables Map() and Unmap() to perform in-place
> @@ -133,9 +133,12 @@ IoMmuMap (
>      DEBUG_VERBOSE,
>      "%a: Operation=%a Host=0x%p Bytes=0x%Lx\n",
>      __FUNCTION__,
> -    ((Operation >= 0 &&
> -      Operation < ARRAY_SIZE (mBusMasterOperationName)) ?
> -     mBusMasterOperationName[Operation] :
> +    (BUS_MASTER_OPERATION_NAME (Operation, Read) :
> +     BUS_MASTER_OPERATION_NAME (Operation, Write) :
> +     BUS_MASTER_OPERATION_NAME (Operation, CommonBuffer) :
> +     BUS_MASTER_OPERATION_NAME (Operation, Read64) :
> +     BUS_MASTER_OPERATION_NAME (Operation, Write64) :
> +     BUS_MASTER_OPERATION_NAME (Operation, CommonBuffer64) :
>       "Invalid"),
>      HostAddress,
>      (UINT64)((NumberOfBytes == NULL) ? 0 : *NumberOfBytes)
> --
> 2.14.1.3.gb7cf6e02401b
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 1/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE

[Thomas Lamprecht]

On 09/07/2017 05:38 AM, Gao, Liming wrote:
> Laszlo:
>    I add -Wno-unused-const-variable option in GCC5 RELEASE option, and build OvmfPkg with GCC5.3. I don't meet any warning or error. Do you meet any issue? So, you think we can't add this option in GCC5 tool chain.
> 

Yes, he is right, looking at:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options

> When an unrecognized warning option is requested (e.g.,
> -Wunknown-warning), GCC emits a diagnostic stating that the option is
> not recognized. However, if the -Wno- form is used, the behavior is
> slightly different: no diagnostic is produced for -Wno-unknown-warning
> unless other diagnostics are being produced. This allows the use of new
> -Wno- options with old compilers, but if something goes wrong, the
> compiler warns that an unrecognized option is present.

Also the older ones support it:
https://gcc.gnu.org/onlinedocs/gcc-4.9.4/gcc/Warning-Options.html#Warning-Options
https://gcc.gnu.org/onlinedocs/gcc-5.4.0/gcc/Warning-Options.html#Warning-Options

I verified this behavior on gcc 5.4 and on gcc version 4.4.5 (Debian 4.4.5-8)
(spun up an squeeze container, fun times)

This seems like an easy solution, hopefully also a valid one in the edk2 context.
The gcc rationale seems good, if there is such a warning suppress it, if not we
wouldn't output any warning anyhow, so there is nothing to suppress.

The only drawback seems that if something goes wrong the user could wonder if
those "unrecognized command line options" have something to do with his problem.

It looks like this (gcc 4.4.5):

tom@olddeb:~# gcc -Wall -Wno-foo-bar test.c
test.c: In function 'main':
test.c:4: warning: unused variable 'test'
At top level:
cc1: warning: unrecognized command line option "-Wno-foo-bar"

And very similar on gcc 6:

tom@plain-stretch:~# gcc -Wall -Wno-foo-bar test.c
test.c: In function ‘main’:
test.c:7:9: warning: unused variable ‘test’ [-Wunused-variable]
      int test;
          ^~~~
test.c: At top level:
cc1: warning: unrecognized command line option ‘-Wno-foo-bar’

cheers,
Thomas

Re: [PATCH 1/1] OvmfPkg/IoMmuDxe: shut up "unused-const-variable" gcc-6 warning in RELEASE

[Laszlo Ersek]

On 09/07/17 07:47, Thomas Lamprecht wrote:
> On 09/07/2017 05:38 AM, Gao, Liming wrote:
>> Laszlo:
>>    I add -Wno-unused-const-variable option in GCC5 RELEASE option, and
>> build OvmfPkg with GCC5.3. I don't meet any warning or error. Do you
>> meet any issue? So, you think we can't add this option in GCC5 tool
>> chain.
>>
> 
> Yes, he is right, looking at:
> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options
> 
>> When an unrecognized warning option is requested (e.g.,
>> -Wunknown-warning), GCC emits a diagnostic stating that the option is
>> not recognized. However, if the -Wno- form is used, the behavior is
>> slightly different: no diagnostic is produced for -Wno-unknown-warning
>> unless other diagnostics are being produced. This allows the use of new
>> -Wno- options with old compilers, but if something goes wrong, the
>> compiler warns that an unrecognized option is present.
> 
> Also the older ones support it:
> https://gcc.gnu.org/onlinedocs/gcc-4.9.4/gcc/Warning-Options.html#Warning-Options
> 
> https://gcc.gnu.org/onlinedocs/gcc-5.4.0/gcc/Warning-Options.html#Warning-Options
> 
> 
> I verified this behavior on gcc 5.4 and on gcc version 4.4.5 (Debian
> 4.4.5-8)
> (spun up an squeeze container, fun times)
> 
> This seems like an easy solution, hopefully also a valid one in the edk2
> context.
> The gcc rationale seems good, if there is such a warning suppress it, if
> not we
> wouldn't output any warning anyhow, so there is nothing to suppress.
> 
> The only drawback seems that if something goes wrong the user could
> wonder if
> those "unrecognized command line options" have something to do with his
> problem.
> 
> It looks like this (gcc 4.4.5):
> 
> tom@olddeb:~# gcc -Wall -Wno-foo-bar test.c
> test.c: In function 'main':
> test.c:4: warning: unused variable 'test'
> At top level:
> cc1: warning: unrecognized command line option "-Wno-foo-bar"
> 
> And very similar on gcc 6:
> 
> tom@plain-stretch:~# gcc -Wall -Wno-foo-bar test.c
> test.c: In function ‘main’:
> test.c:7:9: warning: unused variable ‘test’ [-Wunused-variable]
>      int test;
>          ^~~~
> test.c: At top level:
> cc1: warning: unrecognized command line option ‘-Wno-foo-bar’

Sounds great, thank you guys for discovering this. I believe this would
be a simple solution to BZ#700. Please feel free to update the subject
of that bug as appropriate.

Correspondingly, this patch can be ignored.

Thanks!
Laszlo