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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
