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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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