Laszlo Ersek [mailto:***@redhat.com] wrote:
]Sent: Wednesday, May 27, 2015 01:43 PM
]To: Scott Duplichan
]Cc: 'edk2-devel list'; 'Gerd Hoffmann'; 'Maurice Ma'; 'Dong Guo'
]Subject: Re: CorebootModulePkg: gcc reports conflicting types for 'CbParseAcpiTable'
]
]On 05/27/15 18:36, Scott Duplichan wrote:
]> Laszlo Ersek [mailto:***@redhat.com] wrote:
]>
]> ]Sent: Tuesday, May 26, 2015 03:48 AM
]> ]To: edk2-devel list
]> ]Cc: Gerd Hoffmann; 'Maurice Ma'; Dong Guo; Scott Duplichan
]> ]Subject: CorebootModulePkg: gcc reports conflicting types for 'CbParseAcpiTable'
]> ]
]> ]Hi,
]> ]
]> ]CorebootModulePkg fails to build with gcc. See error msg below.
]> ]
]> ]Thanks
]> ]Laszlo
]> ]
]> ]CorebootModulePkg/Library/CbParseLib/CbParseLib.c:363:1: error: conflicting types for 'CbParseAcpiTable'
]> ] CbParseAcpiTable (
]> ] ^
]> ]In file included from CorebootModulePkg/Library/CbParseLib/CbParseLib.c:21:0:
]> ]CorebootModulePkg/Include/Library/CbParseLib.h:65:1: note: previous declaration of 'CbParseAcpiTable' ]was ]here
]> ] CbParseAcpiTable (
]> ] ^
]> ]CorebootModulePkg/Library/CbParseLib/CbParseLib.c: In function 'CbParseFadtInfo':
]> ]CorebootModulePkg/Library/CbParseLib/CbParseLib.c:425:3: error: passing argument 1 of 'CbParseAcpiTable' ]]from incompatible pointer type [-Werror]
]> ] Status = CbParseAcpiTable (&Rsdp, NULL);
]> ] ^
]> ]CorebootModulePkg/Library/CbParseLib/CbParseLib.c:363:1: note: expected 'void **' but argument is of ]type ]'struct EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER **'
]> ] CbParseAcpiTable (
]> ] ^
]> ]cc1: all warnings being treated as errors
]>
]> This gcc build fail is caused by SVN rev 17486:
]> http://permalink.gmane.org/gmane.comp.bios.tianocore.scm/3924
]>
]> It accidentally reverts the very change that was added to fix
]> the gcc build fail:
]> http://sourceforge.net/p/tianocore/edk2/ci/3b17ae9e51998a7002d7e00cb25243969c570215/
]> Reverting SVN rev 17486 solves the problem.
]
]Something seems fishy. I haven't tried to understand the code until now,
]but I guess I can't avoid looking at it after all:
]
]SVN r17144: Changes pMemTable of CbParseAcpiTable() from VOID** to
] VOID*. Callers are not updated. In order to compensate for
] that, CbParseAcpiTable() is updated internally.
]
]SVN r17486: Practically reverts the above. Triggers the gcc build
] failure that SVN r17144 was supposed to fix.
]
]SVN r17519: Current fix. Leaves CbParseAcpiTable() intact and adapts
] the caller instead.
]
]I think we can live with r17519 now. It matches the edk2 coding style.
]
]Strictly speaking (in ISO C standardese), the pattern visible in r17519
](and everywhere else in edk2) is not correct, because "pointer to
]structure" and "pointer to pointer" can have different representations.
](This is precisely why gcc complains without a forceful cast.)
]
]The correct way would be:
]
]> diff --git a/CorebootModulePkg/Library/CbParseLib/CbParseLib.c ]b/CorebootModulePkg/Library/CbParseLib/CbParseLib.c
]> index 66374b5..acb796a 100644
]> --- a/CorebootModulePkg/Library/CbParseLib/CbParseLib.c
]> +++ b/CorebootModulePkg/Library/CbParseLib/CbParseLib.c
]> @@ -408,6 +408,7 @@ CbParseFadtInfo (
]> OUT UINTN *pResetValue
]> )
]> {
]> + VOID *Tmp;
]> EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER *Rsdp;
]> EFI_ACPI_DESCRIPTION_HEADER *Rsdt;
]> UINT32 *Entry32;
]> @@ -422,10 +423,11 @@ CbParseFadtInfo (
]> Rsdp = NULL;
]> Status = RETURN_SUCCESS;
]>
]> - Status = CbParseAcpiTable ((VOID **)&Rsdp, NULL);
]> + Status = CbParseAcpiTable (&Tmp, NULL);
]> if (RETURN_ERROR(Status)) {
]> return Status;
]> }
]> + Rsdp = Tmp;
]>
]> if (Rsdp == NULL) {
]> return RETURN_NOT_FOUND;
]
]This would involve a pointer conversion (in the Rsdp=Tmp assignment)
]which could update the representation, in theory.
]
]In practice the representation is identical; hence the edk2 pattern (and
]the -fno-strict-aliasing flag as well).
I do like this approach because it reduces the use of type casts.
Another possibility is to make the function work like malloc() and
return a void pointer. If return status information beyond pass/fail
is needed, the caller could pass the address of an error status
variable and the function could update it. Otherwise the function can
return NULL for fail status. This is not a completely general solution
though, because it does not work in cases where a function needs to
return two or more pointers.
]> With the large number of tool chains supported by EDK2, it is not surprising
]> to see these kinds of problems. While the EDK2 project really needs a build server,
]> contributors building from Windows can easily test gcc builds. Windows hosted
]> gcc tool chains are here:
]> http://sourceforge.net/projects/edk2developertoolsforwindows/
]
]Cool :)
]
]> These tool chain downloads are small and require no installation. Just decompress
]> and set environment variables to identify their location. Building with these
]> tool chains does not involve running any bash shell. It works exactly the same
]> as building with Microsoft tool chains, so there is no learning curve for
]> Windows users.
]>
]> The reverse situation of running Microsoft tool chains from Linux is impossible
]> or impractical. Linux users need to test Microsoft tool chain builds using a
]> private or public build server.
]>
]> I ran a few builds just to see what to expect from a build server run. I used
]> SVN rev 17512 with change 17486 reverted. I added openssl so that ovmf could
]> be tested using SECURE_BOOT_ENABLE.
]>
]> The builds use combinations of:
]> Ovmf App IntelFsp Duet Shell BeagleBoard ArmPlatform ArmJuno Emulator CorebootPayload
]> DDK3790 VS2005 VS2008 VS2010 VS2012 VS2013 GCC44 GCC45 GCC46 GCC47 GCC48 GCC49
]> DEBUG RELEASE
]> X64 IA32 IA32X64 ARM AARCH64
]>
]> NOOPT build was not tested, though it sometimes finds problems not seen with
]> DEBUG or RELEASE builds.
]>
]> IPF was not tested due to lack of new development for that platform.
]>
]> VS2003 was not tested because it is old and causes unwanted warnings that
]> are not consistent with newer Microsoft tool chains. VS2003 also lacks
]> X64 support.
]>
]> AppPkg builds for ARM or AARCH64 were not attempted. These failed in the
]> past and probably fail today. If I remember correctly, the failure is due
]> to missing library functions needed to satisfy calls generated by gcc.
]> ArmJunoPkg builds for ARM were not attempted for the same reason. ArmJunoPkg
]> for AARCH64 is OK.
]>
]> Of the 348 combinations remaining, 314 pass and 34 fail:
]>
]> build.exe -p D:\edk2build\edk2\OvmfPkg\OvmfPkgX64.dsc -b DEBUG -t DDK3790 -n 16 -a X64 -]DSECURE_BOOT_ENABLE -DFD_SIZE_2MB
]> OvmfPkg\Library\XenHypercallLib\X86XenHypercall.c(38) : warning C4244: 'return' : conversion from 'int' ]to 'BOOLEAN', possible loss of data
]> This fail occurs only for DDK3790 and VS2005. Maybe time to retire these
]> especially old tool chains.
]
]I guess we'll fix this when the build server complains about it, or
]someone runs into it in the wild ;)
]
]Thanks!
]Laszlo
]
]>
]>
]> build.exe -p D:\edk2build\edk2\AppPkg\AppPkg.dsc -b DEBUG -t DDK3790 -n 16 -a X64
]> d:\edk2build\edk2\StdLib\LibC\Uefi\Devices\Console\daConsole.c(305) : warning C4244: '=' : conversion ]from 'int' to 'wchar_t', possible loss of data
]> This fails only with Microsoft DDK3790, an old tool chain.
]>
]>
]> build.exe -p D:\edk2build\edk2\AppPkg\AppPkg.dsc -b DEBUG -t VS2013x86 -n 16 -a IA32
]> LibGdtoa.lib(strtod.obj) : error LNK2001: unresolved external symbol __dtoui3
]> This fail happens with VS2013 or newer Microsoft tool chains. This problem was
]> discussed on the mailing list in 2014.
]>
]>
]> build.exe -p D:\edk2build\edk2\OvmfPkg\OvmfPkgX64.dsc -b DEBUG -t DDK3790 -n 16 -a X64 -]DSECURE_BOOT_ENABLE -DFD_SIZE_2MB
]> d:\edk2build\edk2\MdePkg\Include\Library\DebugLib.h(264) : error C2010: '.' : unexpected in macro formal ]parameter list
]> This fail is caused because the logic that checks for missing Microsoft
]> compiler support for variadic macros uses compiler predefined macro
]> _MSC_VER. The logic is correct, but fails because OpensslLib.inf undefines
]> _MSC_VER. Removing all occurrences of -U_MSC_VER from OpensslLib.inf solves
]> the build problem. If it is essential to undefined _MSC_VER for some not so
]> obvious reason, the DebugLib.h logic will have to change.
]>
]> Thanks,
]> Scott
------------------------------------------------------------------------------