Laszlo,
Thank you very much for this work.
They are quite helpful to detect potential issues.
But without this specific patch being checked in, future break will still
happen.
I don't want it to be checked in ASAP because I know that there are quite a lot
of close source code that may get build break due to this change.
Besides that, what prevent you make the decision to check in the changes?
Thanks,
Ray
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Tuesday, September 17, 2019 12:49 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Achin Gupta <achin.gu...@arm.com>; Andrew Fish <af...@apple.com>; Anthony
> Perard <anthony.per...@citrix.com>;
> Ard Biesheuvel <ard.biesheu...@linaro.org>; You, Benjamin
> <benjamin....@intel.com>; Zhang, Chao B
> <chao.b.zh...@intel.com>; Bi, Dandan <dandan...@intel.com>; David Woodhouse
> <dw...@infradead.org>; Dong, Eric
> <eric.d...@intel.com>; Dong, Guo <guo.d...@intel.com>; Wu, Hao A
> <hao.a...@intel.com>; Carsey, Jaben
> <jaben.car...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Wu, Jiaxin
> <jiaxin...@intel.com>; Yao, Jiewen
> <jiewen....@intel.com>; Justen, Jordan L <jordan.l.jus...@intel.com>; Julien
> Grall <julien.gr...@arm.com>; Leif Lindholm
> <leif.lindh...@linaro.org>; Gao, Liming <liming....@intel.com>; Ma, Maurice
> <maurice...@intel.com>; Kinney, Michael
> D <michael.d.kin...@intel.com>; Ni, Ray <ray...@intel.com>; Fu, Siyuan
> <siyuan...@intel.com>; Supreeth Venkatesh
> <supreeth.venkat...@arm.com>; Gao, Zhichao <zhichao....@intel.com>
> Subject: [edk2-devel] [PATCH 01/35] DO NOT APPLY: edk2: turn standard handle
> types into pointers to non-VOID
>
> Unfortunately, the UEFI / PI / Shell specs define a number of handle types
> as pointers to VOID. This is a design mistake; those types should have
> been pointers to incomplete union or structure types. Any
> pointer-to-object type converts implicitly to, and from, pointer-to-void,
> which prevents compilers from catching at least the following two types of
> mistakes:
>
> - mixing up one handle type with another (for example, EFI_HANDLE with
> EFI_EVENT),
>
> - getting the depth of indirection wrong (for example, mixing up
> (EFI_HANDLE*) with EFI_HANDLE).
>
> In order to root out such mistakes in the edk2 codebase, introduce
> incomplete structure types with unique tags, such as:
>
> struct EFI_FOOBAR_OBJECT;
> typedef struct EFI_FOOBAR_OBJECT *EFI_FOOBAR_HANDLE;
>
> replacing the spec mandated
>
> typedef VOID *EFI_FOOBAR_HANDLE;
>
> (For some types, such as:
>
> - EFI_ACPI_HANDLE,
> - EFI_EVENT,
> - EFI_FONT_HANDLE,
> - EFI_HANDLE,
> - EFI_HII_HANDLE,
> - EFI_S3_BOOT_SCRIPT_POSITION,
> - SHELL_FILE_HANDLE,
>
> we connect the actual complete type (the internal, implementation-specific
> type) to the typedef. Some of these also demonstrate how the code could
> have looked in practice if the specs had used proper opaque (=incomplete)
> types.)
>
> Then, unleash "build" on the package DSC files. This causes the compiler
> to warn about incompatible pointer assignments, and to stop the build.
>
> The rest of the series addresses the resultant warnings. Each patch
> belongs in one of two categories:
>
> - semantic cleanups (no functional / behavioral changes),
> - actual bugfixes.
>
> As the subject line of this patch states, this specific patch is *not*
> meant to be applied. It is just a "what if" patch that temporarily
> isolates the standard types from each other, the way the specs should
> have, so that the compiler have more information to work with.
>
> Cc: Achin Gupta <achin.gu...@arm.com>
> Cc: Andrew Fish <af...@apple.com>
> Cc: Anthony Perard <anthony.per...@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Benjamin You <benjamin....@intel.com>
> Cc: Chao Zhang <chao.b.zh...@intel.com>
> Cc: Dandan Bi <dandan...@intel.com>
> Cc: David Woodhouse <dw...@infradead.org>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Guo Dong <guo.d...@intel.com>
> Cc: Hao A Wu <hao.a...@intel.com>
> Cc: Jaben Carsey <jaben.car...@intel.com>
> Cc: Jian J Wang <jian.j.w...@intel.com>
> Cc: Jian Wang <jian.j.w...@intel.com>
> Cc: Jiaxin Wu <jiaxin...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Julien Grall <julien.gr...@arm.com>
> Cc: Leif Lindholm <leif.lindh...@linaro.org>
> Cc: Liming Gao <liming....@intel.com>
> Cc: Maurice Ma <maurice...@intel.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Siyuan Fu <siyuan...@intel.com>
> Cc: Supreeth Venkatesh <supreeth.venkat...@arm.com>
> Cc: Zhichao Gao <zhichao....@intel.com>
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
> MdePkg/Include/Pi/PiPeiCis.h | 6 ++++--
> MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h | 3 ++-
> MdePkg/Include/Protocol/Bis.h | 3 ++-
> MdePkg/Include/Protocol/Eap.h | 3 ++-
> MdePkg/Include/Protocol/HiiFont.h | 3 +--
> MdePkg/Include/Protocol/MmMp.h | 3 ++-
> MdePkg/Include/Protocol/S3SaveState.h | 2 +-
> MdePkg/Include/Protocol/Shell.h | 3 ++-
> MdePkg/Include/Protocol/UserManager.h | 9 ++++++---
> MdePkg/Include/Uefi/UefiBaseType.h | 6 ++++--
> MdePkg/Include/Uefi/UefiInternalFormRepresentation.h | 3 ++-
> MdeModulePkg/Core/Dxe/Event/Event.h | 2 +-
> MdeModulePkg/Core/Dxe/Hand/Handle.h | 2 +-
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 2 +-
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h | 2 +-
> MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h | 2 +-
> StandaloneMmPkg/Core/StandaloneMmCore.h | 2 +-
> 17 files changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/MdePkg/Include/Pi/PiPeiCis.h b/MdePkg/Include/Pi/PiPeiCis.h
> index d9d4ed7d413a..3e9e82b62ae9 100644
> --- a/MdePkg/Include/Pi/PiPeiCis.h
> +++ b/MdePkg/Include/Pi/PiPeiCis.h
> @@ -18,12 +18,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> ///
> /// The handles of EFI FV.
> ///
> -typedef VOID *EFI_PEI_FV_HANDLE;
> +struct EFI_PEI_FV_OBJECT;
> +typedef struct EFI_PEI_FV_OBJECT *EFI_PEI_FV_HANDLE;
>
> ///
> /// The handles of EFI FFS.
> ///
> -typedef VOID *EFI_PEI_FILE_HANDLE;
> +struct EFI_PEI_FILE_OBJECT;
> +typedef struct EFI_PEI_FILE_OBJECT *EFI_PEI_FILE_HANDLE;
>
> ///
> /// Declare the forward reference data structure for EFI_PEI_SERVICE.
> diff --git a/MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
> b/MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
> index a8e0b24c6c8d..8a1863f3e03d 100644
> --- a/MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
> +++ b/MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
> @@ -16,7 +16,8 @@
> { 0xeb97088e, 0xcfdf, 0x49c6, { 0xbe, 0x4b, 0xd9, 0x6, 0xa5, 0xb2, 0xe,
> 0x86 }}
>
> typedef UINT32 EFI_ACPI_TABLE_VERSION;
> -typedef VOID *EFI_ACPI_HANDLE;
> +struct EFI_ACPI_OBJECT;
> +typedef struct EFI_ACPI_OBJECT *EFI_ACPI_HANDLE;
>
> #define EFI_ACPI_TABLE_VERSION_NONE (1 << 0)
> #define EFI_ACPI_TABLE_VERSION_1_0B (1 << 1)
> diff --git a/MdePkg/Include/Protocol/Bis.h b/MdePkg/Include/Protocol/Bis.h
> index 2be6718f4bc2..8eca94512d03 100644
> --- a/MdePkg/Include/Protocol/Bis.h
> +++ b/MdePkg/Include/Protocol/Bis.h
> @@ -37,7 +37,8 @@ typedef struct _EFI_BIS_PROTOCOL EFI_BIS_PROTOCOL;
> //
> // Basic types
> //
> -typedef VOID *BIS_APPLICATION_HANDLE;
> +struct BIS_APPLICATION_OBJECT;
> +typedef struct BIS_APPLICATION_OBJECT *BIS_APPLICATION_HANDLE;
> typedef UINT16 BIS_ALG_ID;
> typedef UINT32 BIS_CERT_ID;
>
> diff --git a/MdePkg/Include/Protocol/Eap.h b/MdePkg/Include/Protocol/Eap.h
> index 203d0f40b0dd..06584ef409d0 100644
> --- a/MdePkg/Include/Protocol/Eap.h
> +++ b/MdePkg/Include/Protocol/Eap.h
> @@ -28,7 +28,8 @@ typedef struct _EFI_EAP_PROTOCOL EFI_EAP_PROTOCOL;
> /// Type for the identification number assigned to the Port by the
> /// System in which the Port resides.
> ///
> -typedef VOID * EFI_PORT_HANDLE;
> +struct EFI_PORT_OBJECT;
> +typedef struct EFI_PORT_OBJECT *EFI_PORT_HANDLE;
>
> ///
> /// EAP Authentication Method Type (RFC 3748)
> diff --git a/MdePkg/Include/Protocol/HiiFont.h
> b/MdePkg/Include/Protocol/HiiFont.h
> index 1f2e321ea4e2..450cad9ada70 100644
> --- a/MdePkg/Include/Protocol/HiiFont.h
> +++ b/MdePkg/Include/Protocol/HiiFont.h
> @@ -19,8 +19,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> { 0xe9ca4775, 0x8657, 0x47fc, { 0x97, 0xe7, 0x7e, 0xd6, 0x5a, 0x8, 0x43,
> 0x24 } }
>
> typedef struct _EFI_HII_FONT_PROTOCOL EFI_HII_FONT_PROTOCOL;
> -
> -typedef VOID *EFI_FONT_HANDLE;
> +typedef LIST_ENTRY *EFI_FONT_HANDLE;
>
> ///
> /// EFI_HII_OUT_FLAGS.
> diff --git a/MdePkg/Include/Protocol/MmMp.h b/MdePkg/Include/Protocol/MmMp.h
> index beace1386cbe..cd4e0db47e08 100644
> --- a/MdePkg/Include/Protocol/MmMp.h
> +++ b/MdePkg/Include/Protocol/MmMp.h
> @@ -36,7 +36,8 @@
> //
> // Completion token
> //
> -typedef VOID* MM_COMPLETION;
> +struct MM_COMPLETION_OBJECT;
> +typedef struct MM_COMPLETION_OBJECT* MM_COMPLETION;
>
> typedef struct {
> MM_COMPLETION Completion;
> diff --git a/MdePkg/Include/Protocol/S3SaveState.h
> b/MdePkg/Include/Protocol/S3SaveState.h
> index c1b8f8b9e08d..235c36be6737 100644
> --- a/MdePkg/Include/Protocol/S3SaveState.h
> +++ b/MdePkg/Include/Protocol/S3SaveState.h
> @@ -21,7 +21,7 @@
> { 0xe857caf6, 0xc046, 0x45dc, { 0xbe, 0x3f, 0xee, 0x7, 0x65, 0xfb, 0xa8,
> 0x87 }}
>
>
> -typedef VOID *EFI_S3_BOOT_SCRIPT_POSITION;
> +typedef UINT8 *EFI_S3_BOOT_SCRIPT_POSITION;
>
> typedef struct _EFI_S3_SAVE_STATE_PROTOCOL EFI_S3_SAVE_STATE_PROTOCOL;
>
> diff --git a/MdePkg/Include/Protocol/Shell.h b/MdePkg/Include/Protocol/Shell.h
> index cfb7878228c5..bf791792b4f2 100644
> --- a/MdePkg/Include/Protocol/Shell.h
> +++ b/MdePkg/Include/Protocol/Shell.h
> @@ -11,12 +11,13 @@
> #define __EFI_SHELL_PROTOCOL_H__
>
> #include <Guid/FileInfo.h>
> +#include <Protocol/SimpleFileSystem.h>
>
> #define EFI_SHELL_PROTOCOL_GUID \
> { \
> 0x6302d008, 0x7f9b, 0x4f30, { 0x87, 0xac, 0x60, 0xc9, 0xfe, 0xf5, 0xda,
> 0x4e } \
> }
> -typedef VOID *SHELL_FILE_HANDLE;
> +typedef EFI_FILE_PROTOCOL *SHELL_FILE_HANDLE;
>
> typedef enum {
> ///
> diff --git a/MdePkg/Include/Protocol/UserManager.h
> b/MdePkg/Include/Protocol/UserManager.h
> index 26ac4955f1ec..9abfcffbeebf 100644
> --- a/MdePkg/Include/Protocol/UserManager.h
> +++ b/MdePkg/Include/Protocol/UserManager.h
> @@ -24,8 +24,10 @@
> 0xbaf1e6de, 0x209e, 0x4adb, { 0x8d, 0x96, 0xfd, 0x8b, 0x71, 0xf3, 0xf6,
> 0x83 } \
> }
>
> -typedef VOID *EFI_USER_PROFILE_HANDLE;
> -typedef VOID *EFI_USER_INFO_HANDLE;
> +struct EFI_USER_PROFILE_OBJECT;
> +typedef struct EFI_USER_PROFILE_OBJECT *EFI_USER_PROFILE_HANDLE;
> +struct EFI_USER_INFO_OBJECT;
> +typedef struct EFI_USER_INFO_OBJECT *EFI_USER_INFO_HANDLE;
>
> ///
> /// The attributes of the user profile information.
> @@ -157,7 +159,8 @@ typedef CHAR16 *EFI_USER_INFO_CREDENTIAL_PROVIDER_NAME;
> /// Biometric Exchange Formats Framework) specification.
> ///
> #define EFI_USER_INFO_CBEFF_RECORD 0x0B
> -typedef VOID *EFI_USER_INFO_CBEFF;
> +struct EFI_USER_INFO_CBEFF_OBJECT;
> +typedef struct EFI_USER_INFO_CBEFF_OBJECT *EFI_USER_INFO_CBEFF;
> ///
> /// Indicates how close of a match the fingerprint must be in order to be
> considered a match.
> ///
> diff --git a/MdePkg/Include/Uefi/UefiBaseType.h
> b/MdePkg/Include/Uefi/UefiBaseType.h
> index a62f13dd064f..be5831991b52 100644
> --- a/MdePkg/Include/Uefi/UefiBaseType.h
> +++ b/MdePkg/Include/Uefi/UefiBaseType.h
> @@ -28,11 +28,13 @@ typedef RETURN_STATUS EFI_STATUS;
> ///
> /// A collection of related interfaces.
> ///
> -typedef VOID *EFI_HANDLE;
> +struct EFI_OBJECT;
> +typedef struct EFI_OBJECT *EFI_HANDLE;
> ///
> /// Handle to an event structure.
> ///
> -typedef VOID *EFI_EVENT;
> +struct EFI_EVENT_OBJECT;
> +typedef struct EFI_EVENT_OBJECT *EFI_EVENT;
> ///
> /// Task priority level.
> ///
> diff --git a/MdePkg/Include/Uefi/UefiInternalFormRepresentation.h
> b/MdePkg/Include/Uefi/UefiInternalFormRepresentation.h
> index 4a1346a599d0..93bf9e9e0f13 100644
> --- a/MdePkg/Include/Uefi/UefiInternalFormRepresentation.h
> +++ b/MdePkg/Include/Uefi/UefiInternalFormRepresentation.h
> @@ -20,7 +20,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> ///
> /// The following types are currently defined:
> ///
> -typedef VOID* EFI_HII_HANDLE;
> +struct EFI_HII_OBJECT;
> +typedef struct EFI_HII_OBJECT* EFI_HII_HANDLE;
> typedef CHAR16* EFI_STRING;
> typedef UINT16 EFI_IMAGE_ID;
> typedef UINT16 EFI_QUESTION_ID;
> diff --git a/MdeModulePkg/Core/Dxe/Event/Event.h
> b/MdeModulePkg/Core/Dxe/Event/Event.h
> index 8141c5003eec..42590cb1dd09 100644
> --- a/MdeModulePkg/Core/Dxe/Event/Event.h
> +++ b/MdeModulePkg/Core/Dxe/Event/Event.h
> @@ -37,7 +37,7 @@ typedef struct {
> } TIMER_EVENT_INFO;
>
> #define EVENT_SIGNATURE SIGNATURE_32('e','v','n','t')
> -typedef struct {
> +typedef struct EFI_EVENT_OBJECT {
> UINTN Signature;
> UINT32 Type;
> UINT32 SignalCount;
> diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.h
> b/MdeModulePkg/Core/Dxe/Hand/Handle.h
> index 83eb2b9f3afe..1f1ab3274e8a 100644
> --- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
> +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
> @@ -15,7 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> ///
> /// IHANDLE - contains a list of protocol handles
> ///
> -typedef struct {
> +typedef struct EFI_OBJECT {
> UINTN Signature;
> /// All handles list of IHANDLE
> LIST_ENTRY AllHandles;
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> index 0908e7f4e9e7..c55da58d465e 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> @@ -145,7 +145,7 @@ typedef struct {
> ///
> /// IHANDLE - contains a list of protocol handles
> ///
> -typedef struct {
> +typedef struct EFI_OBJECT {
> UINTN Signature;
> /// All handles list of IHANDLE
> LIST_ENTRY AllHandles;
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h
> index 50d4c96edb63..bfebbb1f8182 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h
> @@ -88,7 +88,7 @@ struct _EFI_AML_NODE_LIST {
> // This buffer should not be freed.
> // Size is the total size of this ACPI node buffer.
> //
> -typedef struct {
> +typedef struct EFI_ACPI_OBJECT {
> UINT32 Signature;
> UINT8 *Buffer;
> UINTN Size;
> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h
> b/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h
> index 4a3feab94df5..48972d0fcad6 100644
> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h
> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h
> @@ -274,7 +274,7 @@ typedef struct _HII_DATABASE_PACKAGE_LIST_INSTANCE {
>
> #define HII_HANDLE_SIGNATURE SIGNATURE_32 ('h','i','h','l')
>
> -typedef struct {
> +typedef struct EFI_HII_OBJECT {
> UINTN Signature;
> LIST_ENTRY Handle;
> UINTN Key;
> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.h
> b/StandaloneMmPkg/Core/StandaloneMmCore.h
> index 4d0eed273f50..dcf91bc5e916 100644
> --- a/StandaloneMmPkg/Core/StandaloneMmCore.h
> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.h
> @@ -105,7 +105,7 @@ typedef struct {
> ///
> /// IHANDLE - contains a list of protocol handles
> ///
> -typedef struct {
> +typedef struct EFI_OBJECT {
> UINTN Signature;
> /// All handles list of IHANDLE
> LIST_ENTRY AllHandles;
> --
> 2.19.1.3.g30247aa5d201
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#47388): https://edk2.groups.io/g/devel/message/47388
> Mute This Topic: https://groups.io/mt/34180199/1712937
> Group Owner: devel+ow...@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray...@intel.com]
> -=-=-=-=-=-=
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47425): https://edk2.groups.io/g/devel/message/47425
Mute This Topic: https://groups.io/mt/34180199/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
