* [PATCH v2 1/2] ShellPkg/UefiShellDebug1CommandsLib: sync Compress() definition with decl.
2018-02-07 22:44 [PATCH v2 0/2] sync some function definitions with their declarations Laszlo Ersek
@ 2018-02-07 22:44 ` Laszlo Ersek
2018-02-08 3:25 ` Gary Lin
2018-02-07 22:44 ` [PATCH v2 2/2] OvmfPkg/PlatformPei: sync AmdSevInitialize() definition with declaration Laszlo Ersek
2018-02-08 15:14 ` [PATCH v2 0/2] sync some function definitions with their declarations Carsey, Jaben
2 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2018-02-07 22:44 UTC (permalink / raw)
To: edk2-devel; +Cc: Gary Lin, Jaben Carsey, Ruiyu Ni
"Compress.h" declares the Compress() function as EFIAPI, but the
definition in "Compress.c" lacks EFIAPI.
GCC toolchains without LTO do not catch this error because "Compress.c"
does not include "Compress.h"; i.e. the declaration used by callers such
as "EfiCompress.c" is not actually matched against the function definition
at build time.
With LTO enabled, the mismatch is found -- however, as a warning only, due
to commit f8d0b9662993 ("BaseTools GCC5: disable warnings-as-errors for
now", 2016-08-03).
Include the header in the C file (which turns the issue into a hard build
error on all GCC toolchains), plus sync the declaration from the header
file to the C file. Finally, remove EFIAPI from both declaration and
definition -- this was the original intent of commit c4e74e9b814c
("ShellPkg/UefiShellDebug1CommandsLib: Remove unnecessary EFIAPI",
2016-10-09), but it missed the header file.
(Gary meant to address that omission in Oct 2017:
[edk2] [PATCH] ShellPkg/UefiShellDebug1CommandsLib: Remove EFIAPI from
Compress()
http://mid.mail-archive.com/20171026065329.32311-1-glin@suse.com
and Ray reviewed the patch, but then the patch was never committed.)
So do the sync and drop EFIAPI now.
This happens to fix the EFICOMPRESS shell command, when built with GCC for
X64.
Cc: Gary Lin <glin@suse.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Fixes: c4e74e9b814cfb4b51cf832f3bb218cd2aba348b
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- drop EFIAPI based on Gary's earlier patch which I just stumbled upon
- do not pick up Jaben's R-b from v1, due to inverted EFIAPI handling
- add "Fixes:" tag
ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h | 1 -
ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c | 14 ++++++++------
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h b/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h
index 39a997178fb7..7fe844e212a8 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h
@@ -28,7 +28,6 @@
@retval EFI_BUFFER_TOO_SMALL The buffer was too small. DstSize is required.
**/
EFI_STATUS
-EFIAPI
Compress (
IN VOID *SrcBuffer,
IN UINT64 SrcSize,
diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c
index 736d2a35b3ac..cde2c54f1b45 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c
@@ -23,6 +23,8 @@
#include <Library/DebugLib.h>
#include <Library/ShellLib.h>
+#include "Compress.h"
+
//
// Macro Definitions
//
@@ -1307,20 +1309,20 @@ Encode (
The compression routine.
@param[in] SrcBuffer The buffer containing the source data.
- @param[in] SrcSize The number of bytes in SrcBuffer.
+ @param[in] SrcSize Number of bytes in SrcBuffer.
@param[in] DstBuffer The buffer to put the compressed image in.
@param[in, out] DstSize On input the size (in bytes) of DstBuffer, on
- return the number of bytes placed in DstBuffer.
+ return the number of bytes placed in DstBuffer.
@retval EFI_SUCCESS The compression was sucessful.
@retval EFI_BUFFER_TOO_SMALL The buffer was too small. DstSize is required.
**/
EFI_STATUS
Compress (
- IN VOID *SrcBuffer,
- IN UINT64 SrcSize,
- IN VOID *DstBuffer,
- IN OUT UINT64 *DstSize
+ IN VOID *SrcBuffer,
+ IN UINT64 SrcSize,
+ IN VOID *DstBuffer,
+ IN OUT UINT64 *DstSize
)
{
EFI_STATUS Status;
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 1/2] ShellPkg/UefiShellDebug1CommandsLib: sync Compress() definition with decl.
2018-02-07 22:44 ` [PATCH v2 1/2] ShellPkg/UefiShellDebug1CommandsLib: sync Compress() definition with decl Laszlo Ersek
@ 2018-02-08 3:25 ` Gary Lin
0 siblings, 0 replies; 6+ messages in thread
From: Gary Lin @ 2018-02-08 3:25 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel, Jaben Carsey, Ruiyu Ni
On Wed, Feb 07, 2018 at 11:44:34PM +0100, Laszlo Ersek wrote:
> "Compress.h" declares the Compress() function as EFIAPI, but the
> definition in "Compress.c" lacks EFIAPI.
>
> GCC toolchains without LTO do not catch this error because "Compress.c"
> does not include "Compress.h"; i.e. the declaration used by callers such
> as "EfiCompress.c" is not actually matched against the function definition
> at build time.
>
> With LTO enabled, the mismatch is found -- however, as a warning only, due
> to commit f8d0b9662993 ("BaseTools GCC5: disable warnings-as-errors for
> now", 2016-08-03).
>
> Include the header in the C file (which turns the issue into a hard build
> error on all GCC toolchains), plus sync the declaration from the header
> file to the C file. Finally, remove EFIAPI from both declaration and
> definition -- this was the original intent of commit c4e74e9b814c
> ("ShellPkg/UefiShellDebug1CommandsLib: Remove unnecessary EFIAPI",
> 2016-10-09), but it missed the header file.
>
> (Gary meant to address that omission in Oct 2017:
>
> [edk2] [PATCH] ShellPkg/UefiShellDebug1CommandsLib: Remove EFIAPI from
> Compress()
>
> http://mid.mail-archive.com/20171026065329.32311-1-glin@suse.com
>
> and Ray reviewed the patch, but then the patch was never committed.)
>
I forgot to track the commit. Thanks for picking it up!
Reviewed-by Gary Lin <glin@suse.com>
> So do the sync and drop EFIAPI now.
>
> This happens to fix the EFICOMPRESS shell command, when built with GCC for
> X64.
>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Fixes: c4e74e9b814cfb4b51cf832f3bb218cd2aba348b
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> v2:
> - drop EFIAPI based on Gary's earlier patch which I just stumbled upon
> - do not pick up Jaben's R-b from v1, due to inverted EFIAPI handling
> - add "Fixes:" tag
>
> ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h | 1 -
> ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c | 14 ++++++++------
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h b/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h
> index 39a997178fb7..7fe844e212a8 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h
> @@ -28,7 +28,6 @@
> @retval EFI_BUFFER_TOO_SMALL The buffer was too small. DstSize is required.
> **/
> EFI_STATUS
> -EFIAPI
> Compress (
> IN VOID *SrcBuffer,
> IN UINT64 SrcSize,
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c
> index 736d2a35b3ac..cde2c54f1b45 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c
> @@ -23,6 +23,8 @@
> #include <Library/DebugLib.h>
> #include <Library/ShellLib.h>
>
> +#include "Compress.h"
> +
> //
> // Macro Definitions
> //
> @@ -1307,20 +1309,20 @@ Encode (
> The compression routine.
>
> @param[in] SrcBuffer The buffer containing the source data.
> - @param[in] SrcSize The number of bytes in SrcBuffer.
> + @param[in] SrcSize Number of bytes in SrcBuffer.
> @param[in] DstBuffer The buffer to put the compressed image in.
> @param[in, out] DstSize On input the size (in bytes) of DstBuffer, on
> - return the number of bytes placed in DstBuffer.
> + return the number of bytes placed in DstBuffer.
>
> @retval EFI_SUCCESS The compression was sucessful.
> @retval EFI_BUFFER_TOO_SMALL The buffer was too small. DstSize is required.
> **/
> EFI_STATUS
> Compress (
> - IN VOID *SrcBuffer,
> - IN UINT64 SrcSize,
> - IN VOID *DstBuffer,
> - IN OUT UINT64 *DstSize
> + IN VOID *SrcBuffer,
> + IN UINT64 SrcSize,
> + IN VOID *DstBuffer,
> + IN OUT UINT64 *DstSize
> )
> {
> EFI_STATUS Status;
> --
> 2.14.1.3.gb7cf6e02401b
>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] OvmfPkg/PlatformPei: sync AmdSevInitialize() definition with declaration
2018-02-07 22:44 [PATCH v2 0/2] sync some function definitions with their declarations Laszlo Ersek
2018-02-07 22:44 ` [PATCH v2 1/2] ShellPkg/UefiShellDebug1CommandsLib: sync Compress() definition with decl Laszlo Ersek
@ 2018-02-07 22:44 ` Laszlo Ersek
2018-02-08 15:14 ` [PATCH v2 0/2] sync some function definitions with their declarations Carsey, Jaben
2 siblings, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2018-02-07 22:44 UTC (permalink / raw)
To: edk2-devel; +Cc: Ard Biesheuvel, Brijesh Singh, Jordan Justen
"Platform.h" declares the AmdSevInitialize() function without EFIAPI, but
the definition in "AmdSev.c" includes EFIAPI.
GCC toolchains without LTO do not catch this error because "AmdSev.c" does
not include "Platform.h"; i.e. the declaration used by callers such as
"Platform.c" is not actually matched against the function definition at
build time.
With LTO enabled, the mismatch is found -- however, as a warning only, due
to commit f8d0b9662993 ("BaseTools GCC5: disable warnings-as-errors for
now", 2016-08-03).
Include the header in the C file (which turns the issue into a hard build
error on all GCC toolchains), plus sync the declaration from the header
file to the C file.
There's been no functional breakage because AmdSevInitialize() takes no
parameters.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Fixes: 13b5d743c87a22dfcd94e8475d943dee5712b62d
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Notes:
v2:
- no code changes, just pick up feedback tags
- add "Fixes:" tag to the commit message
OvmfPkg/PlatformPei/AmdSev.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index 1539e5b5cdce..ad31b69fb032 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -23,6 +23,8 @@
#include <Register/Amd/Cpuid.h>
#include <Library/MemEncryptSevLib.h>
+#include "Platform.h"
+
/**
Function checks if SEV support is available, if present then it sets
@@ -30,7 +32,6 @@
**/
VOID
-EFIAPI
AmdSevInitialize (
VOID
)
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 0/2] sync some function definitions with their declarations
2018-02-07 22:44 [PATCH v2 0/2] sync some function definitions with their declarations Laszlo Ersek
2018-02-07 22:44 ` [PATCH v2 1/2] ShellPkg/UefiShellDebug1CommandsLib: sync Compress() definition with decl Laszlo Ersek
2018-02-07 22:44 ` [PATCH v2 2/2] OvmfPkg/PlatformPei: sync AmdSevInitialize() definition with declaration Laszlo Ersek
@ 2018-02-08 15:14 ` Carsey, Jaben
2018-02-08 17:26 ` Laszlo Ersek
2 siblings, 1 reply; 6+ messages in thread
From: Carsey, Jaben @ 2018-02-08 15:14 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Brijesh Singh, Ard Biesheuvel, Justen, Jordan L,
Gary Lin
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, February 07, 2018 2:45 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Gary Lin <glin@suse.com>; Carsey, Jaben
> <jaben.carsey@intel.com>
> Subject: [edk2] [PATCH v2 0/2] sync some function definitions with their
> declarations
> Importance: High
>
> Repo: https://github.com/lersek/edk2.git
> Branch: sync_defs_to_decls_v2
>
> This is version 2 of the series that I posted earlier. Because that
> series didn't reach the list at all, due to SMTP issues on my end, I'm
> including the v1 blurb here:
>
> > The patches say it all, just a side remark up here:
> >
> > These errors would have been caught long ago if we had enabled the
> > "-Wmissing-prototypes" gcc option.
> >
> > (Unfortunately, we can't enable that option even now, because it --
> > laudably -- forces the programmer to give internal linkage ("STATIC")
> > to their helper / local functions, and -- as we've learned -- some
> > versions of the the Visual Studio debugger choke on STATIC functions.
> > Thus, core modules basically never make their internal functions
> > STATIC.)
>
> The v2 changes are noted per patch.
>
> (This posting should hopefully reach the list; while the original SMTP
> issue remains undiagnosed, I attempted to put a work-around in place.
> Let's see if it works.)
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (2):
> ShellPkg/UefiShellDebug1CommandsLib: sync Compress() definition with
> decl.
> OvmfPkg/PlatformPei: sync AmdSevInitialize() definition with
> declaration
>
> OvmfPkg/PlatformPei/AmdSev.c | 3 ++-
> ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c | 14
> ++++++++------
> ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h | 1 -
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
> --
> 2.14.1.3.gb7cf6e02401b
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 0/2] sync some function definitions with their declarations
2018-02-08 15:14 ` [PATCH v2 0/2] sync some function definitions with their declarations Carsey, Jaben
@ 2018-02-08 17:26 ` Laszlo Ersek
0 siblings, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2018-02-08 17:26 UTC (permalink / raw)
To: Carsey, Jaben, edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Justen, Jordan L, Brijesh Singh, Gary Lin,
Ard Biesheuvel
On 02/08/18 16:14, Carsey, Jaben wrote:
> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
Thank you all; series pushed as commit range de8373fa07f8..c0d221a34854.
Laszlo
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Wednesday, February 07, 2018 2:45 PM
>> To: edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
>> Ard Biesheuvel <ard.biesheuvel@linaro.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Gary Lin <glin@suse.com>; Carsey, Jaben
>> <jaben.carsey@intel.com>
>> Subject: [edk2] [PATCH v2 0/2] sync some function definitions with their
>> declarations
>> Importance: High
>>
>> Repo: https://github.com/lersek/edk2.git
>> Branch: sync_defs_to_decls_v2
>>
>> This is version 2 of the series that I posted earlier. Because that
>> series didn't reach the list at all, due to SMTP issues on my end, I'm
>> including the v1 blurb here:
>>
>>> The patches say it all, just a side remark up here:
>>>
>>> These errors would have been caught long ago if we had enabled the
>>> "-Wmissing-prototypes" gcc option.
>>>
>>> (Unfortunately, we can't enable that option even now, because it --
>>> laudably -- forces the programmer to give internal linkage ("STATIC")
>>> to their helper / local functions, and -- as we've learned -- some
>>> versions of the the Visual Studio debugger choke on STATIC functions.
>>> Thus, core modules basically never make their internal functions
>>> STATIC.)
>>
>> The v2 changes are noted per patch.
>>
>> (This posting should hopefully reach the list; while the original SMTP
>> issue remains undiagnosed, I attempted to put a work-around in place.
>> Let's see if it works.)
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Gary Lin <glin@suse.com>
>> Cc: Jaben Carsey <jaben.carsey@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (2):
>> ShellPkg/UefiShellDebug1CommandsLib: sync Compress() definition with
>> decl.
>> OvmfPkg/PlatformPei: sync AmdSevInitialize() definition with
>> declaration
>>
>> OvmfPkg/PlatformPei/AmdSev.c | 3 ++-
>> ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.c | 14
>> ++++++++------
>> ShellPkg/Library/UefiShellDebug1CommandsLib/Compress.h | 1 -
>> 3 files changed, 10 insertions(+), 8 deletions(-)
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 6+ messages in thread