EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c | 2 +- EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c | 1 + EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 1 + EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h | 32 ++++++-- EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c | 8 ++ EmbeddedPkg/GdbStub/GdbStubInternal.h | 9 +++ EmbeddedPkg/MetronomeDxe/Metronome.c | 1 + EmbeddedPkg/Universal/MmcDxe/Mmc.c | 5 +- EmulatorPkg/EmuGopDxe/GopInput.c | 4 +- EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c | 2 +- MdeModulePkg/Bus/I2c/I2cDxe/I2cBus.c | 2 +- MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c | 2 +- MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 6 +- MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c | 2 +- MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.c | 2 +- MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 2 +- MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c | 2 +- MdeModulePkg/Core/Dxe/Event/Event.c | 8 ++ MdeModulePkg/Core/Dxe/Event/Event.h | 2 +- MdeModulePkg/Core/Dxe/Hand/Handle.h | 2 +- MdeModulePkg/Core/Pei/FwVol/FwVol.c | 2 +- MdeModulePkg/Core/Pei/FwVol/FwVol.h | 2 +- MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 2 +- MdeModulePkg/Core/PiSmmCore/Smi.c | 8 +- MdeModulePkg/Core/RuntimeDxe/Runtime.c | 10 ++- MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c | 12 +-- MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c | 6 +- MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c | 8 +- MdeModulePkg/Library/UefiHiiLib/HiiString.c | 4 +- MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h | 2 +- MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c | 4 +- MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.c | 4 +- MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 2 +- MdeModulePkg/Universal/DriverSampleDxe/DriverSample.c | 4 +- MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.c | 2 +- MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c | 2 +- MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h | 2 +- MdeModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.c | 2 +- MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 2 +- 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 +- MdePkg/Library/DxeServicesLib/DxeServicesLib.c | 2 +- NetworkPkg/DnsDxe/DnsDriver.c | 4 +- NetworkPkg/IScsiDxe/IScsiConfig.c | 2 +- NetworkPkg/Ip4Dxe/Ip4Driver.c | 2 +- NetworkPkg/Ip4Dxe/Ip4If.c | 4 +- NetworkPkg/Ip6Dxe/Ip6Driver.c | 2 +- NetworkPkg/Library/DxeNetLib/DxeNetLib.c | 2 +- NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c | 2 +- NetworkPkg/TcpDxe/SockImpl.c | 4 +- NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c | 2 +- OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c | 6 +- OvmfPkg/PlatformDxe/Platform.c | 4 +- OvmfPkg/VirtioNetDxe/Events.c | 2 +- OvmfPkg/XenBusDxe/XenBus.c | 2 +- SecurityPkg/HddPassword/HddPasswordDxe.c | 2 +- SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c | 2 +- SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c | 2 +- SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDriver.c | 2 +- ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c | 6 +- ShellPkg/DynamicCommand/DpDynamicCommand/Dp.h | 4 +- ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c | 6 +- ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.h | 4 +- ShellPkg/Include/Library/ShellCommandLib.h | 2 +- ShellPkg/Include/Library/ShellLib.h | 4 +- ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 2 +- ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 2 +- ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c | 2 +- ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.h | 2 +- ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/FileBuffer.c | 2 +- ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c | 2 +- ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.h | 2 +- ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c | 4 +- ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.c | 2 +- ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.h | 2 +- ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.c | 2 +- ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.h | 2 +- ShellPkg/Library/UefiShellLevel2CommandsLib/Rm.c | 2 +- ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.c | 2 +- ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.h | 2 +- ShellPkg/Library/UefiShellLevel3CommandsLib/Touch.c | 2 +- ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.c | 2 +- ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.h | 2 +- ShellPkg/Library/UefiShellLib/UefiShellLib.c | 26 ++++++- ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.c | 2 +- ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.h | 2 +- ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.c | 2 +- ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.h | 2 +- StandaloneMmPkg/Core/Dispatcher.c | 80 +++++++++++--------- StandaloneMmPkg/Core/FwVol.c | 16 ++-- StandaloneMmPkg/Core/StandaloneMmCore.h | 4 +- UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 +- UefiPayloadPkg/BlSupportPei/BlSupportPei.c | 19 +++-- 102 files changed, 294 insertions(+), 194 deletions(-)
Repository: https://github.com/lersek/edk2.git
Branch: voidptr
The UEFI / PI / Shell specifications define a number of standard types
as pointers to VOID. This is arguably a design mistake; those types
should have been pointers to distinct incomplete union or structure
types. Here's why:
Roughly paraphrasing the constraints from ISO C99 "6.5.16.1 Simple
assignment" and "6.5.4 Cast operators", any pointer-to-object type
converts implicitly to, and from, pointer-to-void, provided const /
volatile qualifications are not relaxed. Such implicit conversions
prevent compilers from catching at least the following two kinds of
coding mistakes:
- mixing up one 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).
This series first separates these standard types from each other, in the
first patch, which is *not* being proposed for merging. This unmasks a
number of warts (semantic issues, or actual bugs) in the source code, in
the form of build breakages. The rest of the series works through those
breakages, cleaning and fixing the code.
Every DSC file in the edk2 tree was built for at least one of the NOOPT,
DEBUG, RELEASE targets (NOOPT being preferred), with the GCC48 toolchain
(for IA32 / X64) and the GCC5 toolchain (for ARM / AARCH64). Of course,
the build arches were restricted to the SUPPORTED_ARCHITECTURES stated
in the individual DSC files.
There were two exceptions to the above rule: DynamicTablesPkg was only
build-tested with AARCH64 (despite its SUPPORTED_ARCHITECTURES), given
that 32-bit ARM has no ACPI bindings. StandaloneMmPkg too was only
build-tested with AARCH64; it doesn't actually support IA32/X64 yet.
Regarding boot & runtime tests, ArmVirtQemu on AARCH64 was tested with
booting to the OS (RHEL7). Furthermore, I exercised OVMF with my usual
boot and S3 tests, covering IA32, IA32X64, and X64. Finally, some other
individual tests (noted per patch) were done with OVMF.
Cc: Achin Gupta <achin.gupta@arm.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jian Wang <jian.j.wang@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Thanks
Laszlo
Laszlo Ersek (35):
DO NOT APPLY: edk2: turn standard handle types into pointers to
non-VOID
EmbeddedPkg: add missing EFIAPI calling convention specifiers
EmbeddedPkg/AndroidFastbootTransportTcpDxe: fix DestroyChild() call
EmbeddedPkg/Universal/MmcDxe: "fix" CloseProtocol() call in
BindingStop()
EmulatorPkg/DxeTimerLib: drop superfluous cast
EmulatorPkg: stop abusing EFI_HANDLE for keystroke notify registration
MdeModulePkg: fix cast in GetModuleInfoFromHandle() calls
MdeModulePkg/UefiHiiLib: stop using EFI_HANDLE in place of
EFI_HII_HANDLE
MdeModulePkg: stop abusing EFI_EVENT for protocol notify registration
MdeModulePkg/PlatformVarCleanupLib: fix HiiConstructConfigHdr() call
MdeModulePkg: document workaround for EFI_RUNTIME_EVENT_ENTRY PI spec
bug
MdeModulePkg: stop abusing EFI_HANDLE for keystroke notify
registration
MdeModulePkg: PEI Core: clean up "AprioriFile" handling in
FindFileEx()
MdeModulePkg: fix UninstallMultipleProtocolInterfaces() calls
MdeModulePkg/PiSmmCore: make type punning consistent
MdeModulePkg/S3SaveState: cast Position for S3BootScriptLib explicitly
MdePkg/DxeServicesLib: remove bogus cast
NetworkPkg/DxeNetLib: fix type typo in NetLibGetMacAddress()
NetworkPkg: fix CloseProtocol & UninstallMultipleProtocolInterfaces
calls
NetworkPkg/Ip4Dxe: fix NetLibDestroyServiceChild() call
NetworkPkg/TcpDxe: fix SockFreeFoo() parameter list
OvmfPkg/XenBusDxe: fix UninstallMultipleProtocolInterfaces() call
OvmfPkg/VirtioNetDxe: fix SignalEvent() call
OvmfPkg/PlatformDxe: fix EFI_HII_HANDLE parameters of internal
functions
OvmfPkg/VideoDxe: document EFI_EDID_OVERRIDE_PROTOCOL.GetEdid() call
SecurityPkg: fix UninstallMultipleProtocolInterfaces() calls
SecurityPkg: stop abusing EFI_EVENT for protocol notify registration
ShellPkg/UefiShellDriver1CommandsLib: fix parameter list typo
ShellPkg: stop using EFI_HANDLE in place of EFI_HII_HANDLE
ShellPkg: stop taking EFI_HANDLE in place of SHELL_FILE_HANDLE
ShellPkg/UefiShellDebug1CommandsLib: fix ShellCloseFile() call
ShellPkg/UefiShellLib: clarify workaround for unfixable EdkShell bug
StandaloneMmPkg/Core: stop abusing EFI_HANDLE for FwVolHeader tracking
UefiPayloadPkg/BlSupportPei: fix MMCONFIG assignment from XSDT
UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls
EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c | 2 +-
EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c | 1 +
EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 1 +
EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h | 32 ++++++--
EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c | 8 ++
EmbeddedPkg/GdbStub/GdbStubInternal.h | 9 +++
EmbeddedPkg/MetronomeDxe/Metronome.c | 1 +
EmbeddedPkg/Universal/MmcDxe/Mmc.c | 5 +-
EmulatorPkg/EmuGopDxe/GopInput.c | 4 +-
EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c | 2 +-
MdeModulePkg/Bus/I2c/I2cDxe/I2cBus.c | 2 +-
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c | 2 +-
MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 6 +-
MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c | 2 +-
MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.c | 2 +-
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 2 +-
MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c | 2 +-
MdeModulePkg/Core/Dxe/Event/Event.c | 8 ++
MdeModulePkg/Core/Dxe/Event/Event.h | 2 +-
MdeModulePkg/Core/Dxe/Hand/Handle.h | 2 +-
MdeModulePkg/Core/Pei/FwVol/FwVol.c | 2 +-
MdeModulePkg/Core/Pei/FwVol/FwVol.h | 2 +-
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 2 +-
MdeModulePkg/Core/PiSmmCore/Smi.c | 8 +-
MdeModulePkg/Core/RuntimeDxe/Runtime.c | 10 ++-
MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c | 12 +--
MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c | 6 +-
MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c | 8 +-
MdeModulePkg/Library/UefiHiiLib/HiiString.c | 4 +-
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h | 2 +-
MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c | 4 +-
MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.c | 4 +-
MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 2 +-
MdeModulePkg/Universal/DriverSampleDxe/DriverSample.c | 4 +-
MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.c | 2 +-
MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c | 2 +-
MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h | 2 +-
MdeModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.c | 2 +-
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 2 +-
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 +-
MdePkg/Library/DxeServicesLib/DxeServicesLib.c | 2 +-
NetworkPkg/DnsDxe/DnsDriver.c | 4 +-
NetworkPkg/IScsiDxe/IScsiConfig.c | 2 +-
NetworkPkg/Ip4Dxe/Ip4Driver.c | 2 +-
NetworkPkg/Ip4Dxe/Ip4If.c | 4 +-
NetworkPkg/Ip6Dxe/Ip6Driver.c | 2 +-
NetworkPkg/Library/DxeNetLib/DxeNetLib.c | 2 +-
NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c | 2 +-
NetworkPkg/TcpDxe/SockImpl.c | 4 +-
NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c | 2 +-
OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c | 6 +-
OvmfPkg/PlatformDxe/Platform.c | 4 +-
OvmfPkg/VirtioNetDxe/Events.c | 2 +-
OvmfPkg/XenBusDxe/XenBus.c | 2 +-
SecurityPkg/HddPassword/HddPasswordDxe.c | 2 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c | 2 +-
SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c | 2 +-
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDriver.c | 2 +-
ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c | 6 +-
ShellPkg/DynamicCommand/DpDynamicCommand/Dp.h | 4 +-
ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c | 6 +-
ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.h | 4 +-
ShellPkg/Include/Library/ShellCommandLib.h | 2 +-
ShellPkg/Include/Library/ShellLib.h | 4 +-
ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 2 +-
ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 2 +-
ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c | 2 +-
ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.h | 2 +-
ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/FileBuffer.c | 2 +-
ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c | 2 +-
ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.h | 2 +-
ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c | 4 +-
ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.c | 2 +-
ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.h | 2 +-
ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.c | 2 +-
ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.h | 2 +-
ShellPkg/Library/UefiShellLevel2CommandsLib/Rm.c | 2 +-
ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.c | 2 +-
ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.h | 2 +-
ShellPkg/Library/UefiShellLevel3CommandsLib/Touch.c | 2 +-
ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.c | 2 +-
ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.h | 2 +-
ShellPkg/Library/UefiShellLib/UefiShellLib.c | 26 ++++++-
ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.c | 2 +-
ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.h | 2 +-
ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.c | 2 +-
ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.h | 2 +-
StandaloneMmPkg/Core/Dispatcher.c | 80 +++++++++++---------
StandaloneMmPkg/Core/FwVol.c | 16 ++--
StandaloneMmPkg/Core/StandaloneMmCore.h | 4 +-
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 +-
UefiPayloadPkg/BlSupportPei/BlSupportPei.c | 19 +++--
102 files changed, 294 insertions(+), 194 deletions(-)
--
2.19.1.3.g30247aa5d201
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47387): https://edk2.groups.io/g/devel/message/47387
Mute This Topic: https://groups.io/mt/34180197/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Laszlo Ersek > Sent: Wednesday, September 18, 2019 3:49 AM > To: edk2-devel-groups-io > Cc: Achin Gupta; Andrew Fish; Anthony Perard; Ard Biesheuvel; You, > Benjamin; Zhang, Chao B; Bi, Dandan; David Woodhouse; Dong, Eric; Dong, > Guo; Wu, Hao A; Carsey, Jaben; Wang, Jian J; Wu, Jiaxin; Yao, Jiewen; Justen, > Jordan L; Julien Grall; Leif Lindholm; Gao, Liming; Ma, Maurice; Kinney, > Michael D; Ni, Ray; Fu, Siyuan; Supreeth Venkatesh; Gao, Zhichao > Subject: [edk2-devel] [PATCH 00/35] edk2: clean up the usage of > standardized (VOID*) typedefs > > Repository: https://github.com/lersek/edk2.git > Branch: voidptr > > The UEFI / PI / Shell specifications define a number of standard types > as pointers to VOID. This is arguably a design mistake; those types > should have been pointers to distinct incomplete union or structure > types. Here's why: > > Roughly paraphrasing the constraints from ISO C99 "6.5.16.1 Simple > assignment" and "6.5.4 Cast operators", any pointer-to-object type > converts implicitly to, and from, pointer-to-void, provided const / > volatile qualifications are not relaxed. Such implicit conversions > prevent compilers from catching at least the following two kinds of > coding mistakes: > > - mixing up one 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). > > This series first separates these standard types from each other, in the > first patch, which is *not* being proposed for merging. This unmasks a > number of warts (semantic issues, or actual bugs) in the source code, in > the form of build breakages. The rest of the series works through those > breakages, cleaning and fixing the code. > > Every DSC file in the edk2 tree was built for at least one of the NOOPT, > DEBUG, RELEASE targets (NOOPT being preferred), with the GCC48 toolchain > (for IA32 / X64) and the GCC5 toolchain (for ARM / AARCH64). Of course, > the build arches were restricted to the SUPPORTED_ARCHITECTURES stated > in the individual DSC files. > > There were two exceptions to the above rule: DynamicTablesPkg was only > build-tested with AARCH64 (despite its SUPPORTED_ARCHITECTURES), given > that 32-bit ARM has no ACPI bindings. StandaloneMmPkg too was only > build-tested with AARCH64; it doesn't actually support IA32/X64 yet. > > Regarding boot & runtime tests, ArmVirtQemu on AARCH64 was tested with > booting to the OS (RHEL7). Furthermore, I exercised OVMF with my usual > boot and S3 tests, covering IA32, IA32X64, and X64. Finally, some other > individual tests (noted per patch) were done with OVMF. > > Cc: Achin Gupta <achin.gupta@arm.com> > Cc: Andrew Fish <afish@apple.com> > Cc: Anthony Perard <anthony.perard@citrix.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Benjamin You <benjamin.you@intel.com> > Cc: Chao Zhang <chao.b.zhang@intel.com> > Cc: Dandan Bi <dandan.bi@intel.com> > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Guo Dong <guo.dong@intel.com> > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Jaben Carsey <jaben.carsey@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Jian Wang <jian.j.wang@intel.com> > Cc: Jiaxin Wu <jiaxin.wu@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Leif Lindholm <leif.lindholm@linaro.org> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Maurice Ma <maurice.ma@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Siyuan Fu <siyuan.fu@intel.com> > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > > Thanks > Laszlo > > Laszlo Ersek (35): > DO NOT APPLY: edk2: turn standard handle types into pointers to > non-VOID > EmbeddedPkg: add missing EFIAPI calling convention specifiers > EmbeddedPkg/AndroidFastbootTransportTcpDxe: fix DestroyChild() call > EmbeddedPkg/Universal/MmcDxe: "fix" CloseProtocol() call in > BindingStop() > EmulatorPkg/DxeTimerLib: drop superfluous cast > EmulatorPkg: stop abusing EFI_HANDLE for keystroke notify registration > MdeModulePkg: fix cast in GetModuleInfoFromHandle() calls > MdeModulePkg/UefiHiiLib: stop using EFI_HANDLE in place of > EFI_HII_HANDLE > MdeModulePkg: stop abusing EFI_EVENT for protocol notify registration > MdeModulePkg/PlatformVarCleanupLib: fix HiiConstructConfigHdr() call > MdeModulePkg: document workaround for EFI_RUNTIME_EVENT_ENTRY PI > spec > bug > MdeModulePkg: stop abusing EFI_HANDLE for keystroke notify > registration > MdeModulePkg: PEI Core: clean up "AprioriFile" handling in > FindFileEx() > MdeModulePkg: fix UninstallMultipleProtocolInterfaces() calls > MdeModulePkg/PiSmmCore: make type punning consistent > MdeModulePkg/S3SaveState: cast Position for S3BootScriptLib explicitly For the patches made to MdeModulePkg (patch 7~16), Reviewed-by: Hao A Wu <hao.a.wu@intel.com> Best Regards, Hao Wu > MdePkg/DxeServicesLib: remove bogus cast > NetworkPkg/DxeNetLib: fix type typo in NetLibGetMacAddress() > NetworkPkg: fix CloseProtocol & UninstallMultipleProtocolInterfaces > calls > NetworkPkg/Ip4Dxe: fix NetLibDestroyServiceChild() call > NetworkPkg/TcpDxe: fix SockFreeFoo() parameter list > OvmfPkg/XenBusDxe: fix UninstallMultipleProtocolInterfaces() call > OvmfPkg/VirtioNetDxe: fix SignalEvent() call > OvmfPkg/PlatformDxe: fix EFI_HII_HANDLE parameters of internal > functions > OvmfPkg/VideoDxe: document EFI_EDID_OVERRIDE_PROTOCOL.GetEdid() > call > SecurityPkg: fix UninstallMultipleProtocolInterfaces() calls > SecurityPkg: stop abusing EFI_EVENT for protocol notify registration > ShellPkg/UefiShellDriver1CommandsLib: fix parameter list typo > ShellPkg: stop using EFI_HANDLE in place of EFI_HII_HANDLE > ShellPkg: stop taking EFI_HANDLE in place of SHELL_FILE_HANDLE > ShellPkg/UefiShellDebug1CommandsLib: fix ShellCloseFile() call > ShellPkg/UefiShellLib: clarify workaround for unfixable EdkShell bug > StandaloneMmPkg/Core: stop abusing EFI_HANDLE for FwVolHeader > tracking > UefiPayloadPkg/BlSupportPei: fix MMCONFIG assignment from XSDT > UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls > > > EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTranspor > tTcp.c | 2 +- > EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c | 1 > + > EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 1 + > EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h | 32 > ++++++-- > EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c | 8 > ++ > EmbeddedPkg/GdbStub/GdbStubInternal.h | 9 +++ > EmbeddedPkg/MetronomeDxe/Metronome.c | 1 + > EmbeddedPkg/Universal/MmcDxe/Mmc.c | 5 +- > EmulatorPkg/EmuGopDxe/GopInput.c | 4 +- > EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c | 2 +- > MdeModulePkg/Bus/I2c/I2cDxe/I2cBus.c | 2 +- > MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c | 2 > +- > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 6 > +- > MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c | 2 +- > MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.c | 2 +- > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 2 +- > MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c > | 2 +- > MdeModulePkg/Core/Dxe/Event/Event.c | 8 ++ > MdeModulePkg/Core/Dxe/Event/Event.h | 2 +- > MdeModulePkg/Core/Dxe/Hand/Handle.h | 2 +- > MdeModulePkg/Core/Pei/FwVol/FwVol.c | 2 +- > MdeModulePkg/Core/Pei/FwVol/FwVol.h | 2 +- > MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 2 +- > MdeModulePkg/Core/PiSmmCore/Smi.c | 8 +- > MdeModulePkg/Core/RuntimeDxe/Runtime.c | 10 ++- > > MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c > | 12 +-- > MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c > | 6 +- > > MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLi > b.c | 8 +- > MdeModulePkg/Library/UefiHiiLib/HiiString.c | 4 +- > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h | 2 > +- > MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c | > 4 +- > MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.c > | 4 +- > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c > | 2 +- > MdeModulePkg/Universal/DriverSampleDxe/DriverSample.c | > 4 +- > > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.c > | 2 +- > > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm. > c | 2 +- > MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h | 2 > +- > MdeModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.c | 2 > +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c > | 2 +- > 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 > +- > MdePkg/Library/DxeServicesLib/DxeServicesLib.c | 2 +- > NetworkPkg/DnsDxe/DnsDriver.c | 4 +- > NetworkPkg/IScsiDxe/IScsiConfig.c | 2 +- > NetworkPkg/Ip4Dxe/Ip4Driver.c | 2 +- > NetworkPkg/Ip4Dxe/Ip4If.c | 4 +- > NetworkPkg/Ip6Dxe/Ip6Driver.c | 2 +- > NetworkPkg/Library/DxeNetLib/DxeNetLib.c | 2 +- > NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c | 2 +- > NetworkPkg/TcpDxe/SockImpl.c | 4 +- > NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c | 2 +- > OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c | 6 +- > OvmfPkg/PlatformDxe/Platform.c | 4 +- > OvmfPkg/VirtioNetDxe/Events.c | 2 +- > OvmfPkg/XenBusDxe/XenBus.c | 2 +- > SecurityPkg/HddPassword/HddPasswordDxe.c | 2 +- > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c | 2 +- > SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c | 2 +- > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi > gDriver.c | 2 +- > ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c | > 6 +- > ShellPkg/DynamicCommand/DpDynamicCommand/Dp.h | > 4 +- > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c | > 6 +- > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.h | > 4 +- > ShellPkg/Include/Library/ShellCommandLib.h | 2 +- > ShellPkg/Include/Library/ShellLib.h | 4 +- > ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 2 > +- > ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > | 2 +- > ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c > | 2 +- > ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.h > | 2 +- > ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/FileBuffer.c | > 2 +- > > ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman > dsLib.c | 2 +- > > ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman > dsLib.h | 2 +- > ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c | 4 > +- > > ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1Command > sLib.c | 2 +- > > ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1Command > sLib.h | 2 +- > > ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsL > ib.c | 2 +- > > ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsL > ib.h | 2 +- > ShellPkg/Library/UefiShellLevel2CommandsLib/Rm.c | 2 +- > > ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsL > ib.c | 2 +- > > ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsL > ib.h | 2 +- > ShellPkg/Library/UefiShellLevel3CommandsLib/Touch.c | 2 +- > > ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsL > ib.c | 2 +- > > ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsL > ib.h | 2 +- > ShellPkg/Library/UefiShellLib/UefiShellLib.c | 26 ++++++- > > ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1Com > mandsLib.c | 2 +- > > ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1Com > mandsLib.h | 2 +- > > ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2Com > mandsLib.c | 2 +- > > ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2Com > mandsLib.h | 2 +- > StandaloneMmPkg/Core/Dispatcher.c | 80 > +++++++++++--------- > StandaloneMmPkg/Core/FwVol.c | 16 ++-- > StandaloneMmPkg/Core/StandaloneMmCore.h | 4 +- > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 +- > UefiPayloadPkg/BlSupportPei/BlSupportPei.c | 19 +++-- > 102 files changed, 294 insertions(+), 194 deletions(-) > > -- > 2.19.1.3.g30247aa5d201 > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this group. > > View/Reply Online (#47387): > https://edk2.groups.io/g/devel/message/47387 > Mute This Topic: https://groups.io/mt/34180197/1768737 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com] > -=-=-=-=-=-= -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47514): https://edk2.groups.io/g/devel/message/47514 Mute This Topic: https://groups.io/mt/34180197/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 09/17/19 21:49, Laszlo Ersek wrote: > Repository: https://github.com/lersek/edk2.git > Branch: voidptr > > The UEFI / PI / Shell specifications define a number of standard types > as pointers to VOID. This is arguably a design mistake; those types > should have been pointers to distinct incomplete union or structure > types. Here's why: > > Roughly paraphrasing the constraints from ISO C99 "6.5.16.1 Simple > assignment" and "6.5.4 Cast operators", any pointer-to-object type > converts implicitly to, and from, pointer-to-void, provided const / > volatile qualifications are not relaxed. Such implicit conversions > prevent compilers from catching at least the following two kinds of > coding mistakes: > > - mixing up one 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). > > This series first separates these standard types from each other, in the > first patch, which is *not* being proposed for merging. This unmasks a > number of warts (semantic issues, or actual bugs) in the source code, in > the form of build breakages. The rest of the series works through those > breakages, cleaning and fixing the code. > > Every DSC file in the edk2 tree was built for at least one of the NOOPT, > DEBUG, RELEASE targets (NOOPT being preferred), with the GCC48 toolchain > (for IA32 / X64) and the GCC5 toolchain (for ARM / AARCH64). Of course, > the build arches were restricted to the SUPPORTED_ARCHITECTURES stated > in the individual DSC files. > > There were two exceptions to the above rule: DynamicTablesPkg was only > build-tested with AARCH64 (despite its SUPPORTED_ARCHITECTURES), given > that 32-bit ARM has no ACPI bindings. StandaloneMmPkg too was only > build-tested with AARCH64; it doesn't actually support IA32/X64 yet. > > Regarding boot & runtime tests, ArmVirtQemu on AARCH64 was tested with > booting to the OS (RHEL7). Furthermore, I exercised OVMF with my usual > boot and S3 tests, covering IA32, IA32X64, and X64. Finally, some other > individual tests (noted per patch) were done with OVMF. This patch series is now ready to be pushed (it's fully reviewed), except for the following two patches: * [edk2-devel] [PATCH 01/35] DO NOT APPLY: edk2: turn standard handle types into pointers to non-VOID * [edk2-devel] [PATCH 25/35] OvmfPkg/VideoDxe: document EFI_EDID_OVERRIDE_PROTOCOL.GetEdid() call Regarding 01/35, it was never meant to be pushed (certainly not in this form); plus it would likely be too early for a number of out-of-tree platforms. We have discussed enabling "strict UEFI types" (even by default, possibly). That's a great goal. And I don't have any time for pursuing it. :( So yes, I'm aware the problems will likely reproduce over time; I'm sorry about that. Regarding 25/35, the original (unpatched) code might actually prove correct there (needing a fix in the EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID definition instead). That depends on <https://mantis.uefi.org/mantis/view.php?id=2018>, however. I've reached out to Andy Hayes here: http://mid.mail-archive.com/985de369-7880-b6cc-46e7-5a2edca6582b@redhat.com https://edk2.groups.io/g/devel/message/48487 but I've not received any feedback yet. So tomorrow I plan to re-run some sanity builds, with all patches except #01 and #25 applied. If the builds still complete, I'm going to push the other 33 patches. Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48613): https://edk2.groups.io/g/devel/message/48613 Mute This Topic: https://groups.io/mt/34180197/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/09/19 01:49, Laszlo Ersek wrote: > On 09/17/19 21:49, Laszlo Ersek wrote: >> Repository: https://github.com/lersek/edk2.git >> Branch: voidptr >> >> The UEFI / PI / Shell specifications define a number of standard types >> as pointers to VOID. This is arguably a design mistake; those types >> should have been pointers to distinct incomplete union or structure >> types. Here's why: >> >> Roughly paraphrasing the constraints from ISO C99 "6.5.16.1 Simple >> assignment" and "6.5.4 Cast operators", any pointer-to-object type >> converts implicitly to, and from, pointer-to-void, provided const / >> volatile qualifications are not relaxed. Such implicit conversions >> prevent compilers from catching at least the following two kinds of >> coding mistakes: >> >> - mixing up one 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). >> >> This series first separates these standard types from each other, in the >> first patch, which is *not* being proposed for merging. This unmasks a >> number of warts (semantic issues, or actual bugs) in the source code, in >> the form of build breakages. The rest of the series works through those >> breakages, cleaning and fixing the code. >> >> Every DSC file in the edk2 tree was built for at least one of the NOOPT, >> DEBUG, RELEASE targets (NOOPT being preferred), with the GCC48 toolchain >> (for IA32 / X64) and the GCC5 toolchain (for ARM / AARCH64). Of course, >> the build arches were restricted to the SUPPORTED_ARCHITECTURES stated >> in the individual DSC files. >> >> There were two exceptions to the above rule: DynamicTablesPkg was only >> build-tested with AARCH64 (despite its SUPPORTED_ARCHITECTURES), given >> that 32-bit ARM has no ACPI bindings. StandaloneMmPkg too was only >> build-tested with AARCH64; it doesn't actually support IA32/X64 yet. >> >> Regarding boot & runtime tests, ArmVirtQemu on AARCH64 was tested with >> booting to the OS (RHEL7). Furthermore, I exercised OVMF with my usual >> boot and S3 tests, covering IA32, IA32X64, and X64. Finally, some other >> individual tests (noted per patch) were done with OVMF. > > This patch series is now ready to be pushed (it's fully reviewed), > except for the following two patches: > > * [edk2-devel] [PATCH 01/35] > DO NOT APPLY: edk2: turn standard handle types into pointers to > non-VOID > > * [edk2-devel] [PATCH 25/35] > OvmfPkg/VideoDxe: document EFI_EDID_OVERRIDE_PROTOCOL.GetEdid() call > > Regarding 01/35, it was never meant to be pushed (certainly not in this > form); plus it would likely be too early for a number of out-of-tree > platforms. > > We have discussed enabling "strict UEFI types" (even by default, > possibly). That's a great goal. And I don't have any time for pursuing > it. :( So yes, I'm aware the problems will likely reproduce over time; > I'm sorry about that. > > Regarding 25/35, the original (unpatched) code might actually prove > correct there (needing a fix in the EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID > definition instead). That depends on > <https://mantis.uefi.org/mantis/view.php?id=2018>, however. > > I've reached out to Andy Hayes here: > > http://mid.mail-archive.com/985de369-7880-b6cc-46e7-5a2edca6582b@redhat.com > https://edk2.groups.io/g/devel/message/48487 > > but I've not received any feedback yet. > > So tomorrow I plan to re-run some sanity builds, with all patches except > #01 and #25 applied. If the builds still complete, I'm going to push the > other 33 patches. Commit range 2de1f611be06..976d0353a6ce. Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48634): https://edk2.groups.io/g/devel/message/48634 Mute This Topic: https://groups.io/mt/34180197/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Good day, Thank you, Laszlo, for your ambition to introduce stricter code style enforcements. Sorry to "hijack" the actual topic (I did not CC anyone on purpose, as this is mostly a separate topic and I'd like a quick comment first), but this seems like a good occasion to mention another few bad practices edk2 has been following. Mainly, I'd like to call *some* attention to quality problems in the code base while this has some traction, and cause a discussion on whether and how those are to be approached. Thank you for your time. Regards, Marvin "inadequate type punning": e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c#L446 This is mostly about the infamous "Strict Aliasing" rule, which is basically: "An object shall have its stored value accessed only by an lvalue expression that has one of the following types: — a type compatible with the effective type of the object, — a qualifed version of a type compatible with the effective type of the object, — a type that is the signed or unsigned type corresponding to the effective type of the object, — a type that is the signed or unsigned type corresponding to a qualifed version of the effective type of the object, — an aggregate or union type that includes one of the aforementioned types among its members (including, recursively, a member of a subaggregate or contained union), or — a character type." C18 (ISO/IEC 9899:2018), 6.5.7 (exists, though has been updated, since C90) Currently optimisations based on this are disabled. This is a bit nasty to work around if *seriously* needed when sticking to C90, I can only think of memcpy right now. However, even though there are compilers that do not fully support C99 (ahem, Microsoft :) ), type-punning by unions should be supported by them all, and has been legal as of C99, where the following part has been dropped from the standard: "With one exception, if a member of a union object is accessed after a value has been stored in a different member of the object, the behavior is implementation-defined." C90 (ISO/IEC 9899:1990), 6.3.2.3 "pointer unions": e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/IndustryStandard/SmBios.h#L2592 While the idea behind them is certainly style preference, using a union of pointers prevents two important things over a union of structs. 1) CONST declaration: When defining a variable of a union type containing pointers as CONST, speaking of its members, they are all going to be CONST pointers to arbitrary memory and not arbitrary pointers to CONST memory. With a union of structs, you can have either as required (e.g. CONST UNION_TYPE *union, or UNION_TYPE *COST union). 2) Well-defined header inspection: "if a union contains several structures that share a common initial sequence [...], it is permitted to inspect the common initial part of any of them anywhere that a declaration of the completed type of the union is visible" C18 (ISO/IEC 9899:2018), 6.5.2.3.6 (exists since at least C90) This guarantee can be used to inspect the type defined in a common header (e.g. SMBIOS_STRUCTURE) and the process the type-specific data by accessing the appropiate member (e.g. SMBIOS_TABLE_TYPE0) legally. Plain casts and "pointer union" accesses are illegal as per the "inadequate type punning" point above. "casting away CONST": e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c#L236 This should be obvious as Undefined Behaviour because memory previously guaranteed to be read-only is returned as a pointer to memory that allows writing, but for easier lookup, here's the related rule: "the left operand has atomic, qualifed, or unqualifed pointer type, and [...] the type pointed to by the left has all the qualifers of the type pointed to by the right" C18 (ISO/IEC 9899:2018), 6.5.16.1 (exists since at least C90) "structs with trailing 1-length array" e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/Guid/FileInfo.h#L51 This is undefined as per: "The behavior is undefned in the following circumstances: [...] — Addition or subtraction of a pointer into, or just beyond, an array object and an integer type produces a result that does not point into, or just beyond, the same array object (6.5.6). — Addition or subtraction of a pointer into, or just beyond, an array object and an integer type produces a result that points just beyond the array object and is used as the operand of a unary * operator that is evaluated (6.5.6). — Pointers that do not point into, or just beyond, the same array object are subtracted (6.5.6)." C18 (ISO/IEC 9899:2018), J.2 (exists since at least C90) Same as above, while not all compilers fully support C99, flexible arrays should be support by all reasonably new compilers and allow a legal declaration and usage where this hack is in place. At worst, a macro could be provided to declare a [1] vs a [] array on demand and a requirement be introduced to have a "SIZE_OF_" macro for each such struct, but my personal preference would be to just enforce flexible arrays. "Missing security checks for external data": e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L943 The given example misses an alignment verification of the resulting pointer (which technically has to be verified *before* casting), as documented here: "The behavior is undefined in the following circumstances: [...] — Conversion between two pointer types produces a result that is incorrectly aligned (6.3.2.3)." C18 (ISO/IEC 9899:2018), J.2 (exists since at least C90) There are more such issues throughout the codebase, including missing overflow and (or flawed, see before) bounds checks, however I cannot find such quickly. "signed int BIT definitions": e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/Base.h#L348 Fixing this would be prone to regressions, but I'd like to add it for tracking purposes. Related discussion can be found down the chain here: https://lists.01.org/pipermail/edk2-devel/2018-February/021919.html Am 17.09.2019 um 21:49 schrieb Laszlo Ersek: > Repository: https://github.com/lersek/edk2.git > Branch: voidptr > > The UEFI / PI / Shell specifications define a number of standard types > as pointers to VOID. This is arguably a design mistake; those types > should have been pointers to distinct incomplete union or structure > types. Here's why: > > Roughly paraphrasing the constraints from ISO C99 "6.5.16.1 Simple > assignment" and "6.5.4 Cast operators", any pointer-to-object type > converts implicitly to, and from, pointer-to-void, provided const / > volatile qualifications are not relaxed. Such implicit conversions > prevent compilers from catching at least the following two kinds of > coding mistakes: > > - mixing up one 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). > > This series first separates these standard types from each other, in the > first patch, which is *not* being proposed for merging. This unmasks a > number of warts (semantic issues, or actual bugs) in the source code, in > the form of build breakages. The rest of the series works through those > breakages, cleaning and fixing the code. > > Every DSC file in the edk2 tree was built for at least one of the NOOPT, > DEBUG, RELEASE targets (NOOPT being preferred), with the GCC48 toolchain > (for IA32 / X64) and the GCC5 toolchain (for ARM / AARCH64). Of course, > the build arches were restricted to the SUPPORTED_ARCHITECTURES stated > in the individual DSC files. > > There were two exceptions to the above rule: DynamicTablesPkg was only > build-tested with AARCH64 (despite its SUPPORTED_ARCHITECTURES), given > that 32-bit ARM has no ACPI bindings. StandaloneMmPkg too was only > build-tested with AARCH64; it doesn't actually support IA32/X64 yet. > > Regarding boot & runtime tests, ArmVirtQemu on AARCH64 was tested with > booting to the OS (RHEL7). Furthermore, I exercised OVMF with my usual > boot and S3 tests, covering IA32, IA32X64, and X64. Finally, some other > individual tests (noted per patch) were done with OVMF. > > Cc: Achin Gupta <achin.gupta@arm.com> > Cc: Andrew Fish <afish@apple.com> > Cc: Anthony Perard <anthony.perard@citrix.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Benjamin You <benjamin.you@intel.com> > Cc: Chao Zhang <chao.b.zhang@intel.com> > Cc: Dandan Bi <dandan.bi@intel.com> > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Guo Dong <guo.dong@intel.com> > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Jaben Carsey <jaben.carsey@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Jian Wang <jian.j.wang@intel.com> > Cc: Jiaxin Wu <jiaxin.wu@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Leif Lindholm <leif.lindholm@linaro.org> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Maurice Ma <maurice.ma@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Siyuan Fu <siyuan.fu@intel.com> > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com> > Cc: Zhichao Gao <zhichao.gao@intel.com> > > Thanks > Laszlo > > Laszlo Ersek (35): > DO NOT APPLY: edk2: turn standard handle types into pointers to > non-VOID > EmbeddedPkg: add missing EFIAPI calling convention specifiers > EmbeddedPkg/AndroidFastbootTransportTcpDxe: fix DestroyChild() call > EmbeddedPkg/Universal/MmcDxe: "fix" CloseProtocol() call in > BindingStop() > EmulatorPkg/DxeTimerLib: drop superfluous cast > EmulatorPkg: stop abusing EFI_HANDLE for keystroke notify registration > MdeModulePkg: fix cast in GetModuleInfoFromHandle() calls > MdeModulePkg/UefiHiiLib: stop using EFI_HANDLE in place of > EFI_HII_HANDLE > MdeModulePkg: stop abusing EFI_EVENT for protocol notify registration > MdeModulePkg/PlatformVarCleanupLib: fix HiiConstructConfigHdr() call > MdeModulePkg: document workaround for EFI_RUNTIME_EVENT_ENTRY PI spec > bug > MdeModulePkg: stop abusing EFI_HANDLE for keystroke notify > registration > MdeModulePkg: PEI Core: clean up "AprioriFile" handling in > FindFileEx() > MdeModulePkg: fix UninstallMultipleProtocolInterfaces() calls > MdeModulePkg/PiSmmCore: make type punning consistent > MdeModulePkg/S3SaveState: cast Position for S3BootScriptLib explicitly > MdePkg/DxeServicesLib: remove bogus cast > NetworkPkg/DxeNetLib: fix type typo in NetLibGetMacAddress() > NetworkPkg: fix CloseProtocol & UninstallMultipleProtocolInterfaces > calls > NetworkPkg/Ip4Dxe: fix NetLibDestroyServiceChild() call > NetworkPkg/TcpDxe: fix SockFreeFoo() parameter list > OvmfPkg/XenBusDxe: fix UninstallMultipleProtocolInterfaces() call > OvmfPkg/VirtioNetDxe: fix SignalEvent() call > OvmfPkg/PlatformDxe: fix EFI_HII_HANDLE parameters of internal > functions > OvmfPkg/VideoDxe: document EFI_EDID_OVERRIDE_PROTOCOL.GetEdid() call > SecurityPkg: fix UninstallMultipleProtocolInterfaces() calls > SecurityPkg: stop abusing EFI_EVENT for protocol notify registration > ShellPkg/UefiShellDriver1CommandsLib: fix parameter list typo > ShellPkg: stop using EFI_HANDLE in place of EFI_HII_HANDLE > ShellPkg: stop taking EFI_HANDLE in place of SHELL_FILE_HANDLE > ShellPkg/UefiShellDebug1CommandsLib: fix ShellCloseFile() call > ShellPkg/UefiShellLib: clarify workaround for unfixable EdkShell bug > StandaloneMmPkg/Core: stop abusing EFI_HANDLE for FwVolHeader tracking > UefiPayloadPkg/BlSupportPei: fix MMCONFIG assignment from XSDT > UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls > > EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c | 2 +- > EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c | 1 + > EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 1 + > EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h | 32 ++++++-- > EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c | 8 ++ > EmbeddedPkg/GdbStub/GdbStubInternal.h | 9 +++ > EmbeddedPkg/MetronomeDxe/Metronome.c | 1 + > EmbeddedPkg/Universal/MmcDxe/Mmc.c | 5 +- > EmulatorPkg/EmuGopDxe/GopInput.c | 4 +- > EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c | 2 +- > MdeModulePkg/Bus/I2c/I2cDxe/I2cBus.c | 2 +- > MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c | 2 +- > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 6 +- > MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c | 2 +- > MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.c | 2 +- > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 2 +- > MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c | 2 +- > MdeModulePkg/Core/Dxe/Event/Event.c | 8 ++ > MdeModulePkg/Core/Dxe/Event/Event.h | 2 +- > MdeModulePkg/Core/Dxe/Hand/Handle.h | 2 +- > MdeModulePkg/Core/Pei/FwVol/FwVol.c | 2 +- > MdeModulePkg/Core/Pei/FwVol/FwVol.h | 2 +- > MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 2 +- > MdeModulePkg/Core/PiSmmCore/Smi.c | 8 +- > MdeModulePkg/Core/RuntimeDxe/Runtime.c | 10 ++- > MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c | 12 +-- > MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c | 6 +- > MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c | 8 +- > MdeModulePkg/Library/UefiHiiLib/HiiString.c | 4 +- > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h | 2 +- > MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c | 4 +- > MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.c | 4 +- > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c | 2 +- > MdeModulePkg/Universal/DriverSampleDxe/DriverSample.c | 4 +- > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.c | 2 +- > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c | 2 +- > MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h | 2 +- > MdeModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.c | 2 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 2 +- > 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 +- > MdePkg/Library/DxeServicesLib/DxeServicesLib.c | 2 +- > NetworkPkg/DnsDxe/DnsDriver.c | 4 +- > NetworkPkg/IScsiDxe/IScsiConfig.c | 2 +- > NetworkPkg/Ip4Dxe/Ip4Driver.c | 2 +- > NetworkPkg/Ip4Dxe/Ip4If.c | 4 +- > NetworkPkg/Ip6Dxe/Ip6Driver.c | 2 +- > NetworkPkg/Library/DxeNetLib/DxeNetLib.c | 2 +- > NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c | 2 +- > NetworkPkg/TcpDxe/SockImpl.c | 4 +- > NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c | 2 +- > OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c | 6 +- > OvmfPkg/PlatformDxe/Platform.c | 4 +- > OvmfPkg/VirtioNetDxe/Events.c | 2 +- > OvmfPkg/XenBusDxe/XenBus.c | 2 +- > SecurityPkg/HddPassword/HddPasswordDxe.c | 2 +- > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c | 2 +- > SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c | 2 +- > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDriver.c | 2 +- > ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c | 6 +- > ShellPkg/DynamicCommand/DpDynamicCommand/Dp.h | 4 +- > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c | 6 +- > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.h | 4 +- > ShellPkg/Include/Library/ShellCommandLib.h | 2 +- > ShellPkg/Include/Library/ShellLib.h | 4 +- > ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 2 +- > ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 2 +- > ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c | 2 +- > ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.h | 2 +- > ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/FileBuffer.c | 2 +- > ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c | 2 +- > ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.h | 2 +- > ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c | 4 +- > ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.c | 2 +- > ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.h | 2 +- > ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.c | 2 +- > ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.h | 2 +- > ShellPkg/Library/UefiShellLevel2CommandsLib/Rm.c | 2 +- > ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.c | 2 +- > ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.h | 2 +- > ShellPkg/Library/UefiShellLevel3CommandsLib/Touch.c | 2 +- > ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.c | 2 +- > ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.h | 2 +- > ShellPkg/Library/UefiShellLib/UefiShellLib.c | 26 ++++++- > ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.c | 2 +- > ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.h | 2 +- > ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.c | 2 +- > ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.h | 2 +- > StandaloneMmPkg/Core/Dispatcher.c | 80 +++++++++++--------- > StandaloneMmPkg/Core/FwVol.c | 16 ++-- > StandaloneMmPkg/Core/StandaloneMmCore.h | 4 +- > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 4 +- > UefiPayloadPkg/BlSupportPei/BlSupportPei.c | 19 +++-- > 102 files changed, 294 insertions(+), 194 deletions(-) > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47861): https://edk2.groups.io/g/devel/message/47861 Mute This Topic: https://groups.io/mt/34180197/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 09/23/19 18:27, Marvin Häuser wrote:
> Good day,
>
> Thank you, Laszlo, for your ambition to introduce stricter code style
> enforcements. Sorry to "hijack" the actual topic (I did not CC anyone on
> purpose, as this is mostly a separate topic and I'd like a quick comment
> first), but this seems like a good occasion to mention another few bad
> practices edk2 has been following. Mainly, I'd like to call *some*
> attention to quality problems in the code base while this has some
> traction, and cause a discussion on whether and how those are to be
> approached.
>
> Thank you for your time.
Sure, I can offer my personal opinion on these.
> "inadequate type punning":
> e.g.
> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c#L446
>
> This is mostly about the infamous "Strict Aliasing" rule, which is
> basically:
> "An object shall have its stored value accessed only by an lvalue
> expression that has one of the
> following types:
> — a type compatible with the effective type of the object,
> — a qualifed version of a type compatible with the effective type of the
> object,
> — a type that is the signed or unsigned type corresponding to the
> effective type of the object,
> — a type that is the signed or unsigned type corresponding to a qualifed
> version of the effective
> type of the object,
> — an aggregate or union type that includes one of the aforementioned
> types among its members
> (including, recursively, a member of a subaggregate or contained union), or
> — a character type."
> C18 (ISO/IEC 9899:2018), 6.5.7 (exists, though has been updated, since C90)
>
> Currently optimisations based on this are disabled. This is a bit nasty
> to work around if *seriously* needed when sticking to C90, I can only
> think of memcpy right now. However, even though there are compilers that
> do not fully support C99 (ahem, Microsoft :) ), type-punning by unions
> should be supported by them all, and has been legal as of C99, where the
> following part has been dropped from the standard:
> "With one exception, if a member of a union object is accessed after a
> value has been stored in a different member of the object, the behavior
> is implementation-defined."
> C90 (ISO/IEC 9899:1990), 6.3.2.3
I'm opposed to enforcing the strict aliasing rules, even though in all
code that I write, I either try to conform to them, or at least I seek
to be fully conscious of breaking them.
Here's the thing: IMO, the strict aliasing rules sacrifice flexibility
for performance (optimization possibilities). Not to mention the amount
of code in edk2 that would have to be identified and updated.
BaseTools uses "-fno-strict-aliasing" everywhere, and I think that's a
good choice.
https://blog.regehr.org/archives/1180
This proposal for a "friendly dialect of C" intended to eliminate the
strict aliasing rules altogether. (Item#10; possibly also item#13.)
Also, as it points out, the Linux kernel is built with
"-fno-strict-aliasing". I've checked now, with a *long* series of "git
blame" commands, even digging into the "history" repository (which is at
<git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git>). I can
say that the current flag has been in place since *at least* Linux
v2.5.0 (2002-02-04).
QEMU has been built with "-fno-strict-aliasing" ever since commit
b932caba32c6 ("new disk image layer", 2004-08-01), known originally as
SVN rev 1030.
Consider the following example. You have a dynamically allocated buffer.
You read some data into it, from the network or the disk, using PCI DMA.
Let's assume that, from the block read via PCI DMA, the library
function(s) or protocol member(s) that you call, directly or indirectly,
there is at least one that:
- copies data from a source buffer to a target buffer, using UINT32 or
UINT64 assignments (for speed),
- and is implemented in C.
Now, according to the effective type rules, your dynamic buffer's
effective type is "array of UINT32" or "array of UINT64". That's because
of C99 6.5 Expressions, p6:
"If a value is stored into an object having no declared type through an
lvalue having a type that is not a character type, then the type of the
lvalue becomes the effective type of the object for that access and for
subsequent accesses that do not modify the stored value."
Then if you try to parse this buffer as a UEFI device path (= a packed
sequence of device path node structures), *IN PLACE*, you will break the
effective type rules no matter what. Because, you will necessarily look
at the next node in the blob as an EFI_DEVICE_PATH_PROTOCOL (because
you'll want to learn the Type and the SubType fields, and the Length
array too). Note that EFI_DEVICE_PATH_PROTOCOL is a structure with no
UINT32 or UINT64 members. Boom.
So you have to resort to one of the following things:
1. Define a union type, assign the *full union* first
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90167#c3>, and then check
the union helper variable. (First you must make sure there was enough
room in the buffer being parsed for a full union.)
2. Or, use memcpy() -- or something that the compiler is similarly
enlightened about --, to the same helper variable.
3. Or, write a manual copy loop with character access, to the helper
variable.
4. Or, use character access to read the fields.
#1 through #3 add a separate copying step, while #4 is extremely
uncomfortable to program.
In a nutshell, the effective type rules require separate
de-serialization routines for all data, and that's incredibly annoying.
It kills the utility of packed C structures.
I prefer packed C structures, size checks (!!!) against the buffers to
parse, and then type punning of pointers.
> "pointer unions":
> e.g.
> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/IndustryStandard/SmBios.h#L2592
>
> While the idea behind them is certainly style preference, using a union
> of pointers prevents two important things over a union of structs.
>
> 1) CONST declaration: When defining a variable of a union type
> containing pointers as CONST, speaking of its members, they are all
> going to be CONST pointers to arbitrary memory and not arbitrary
> pointers to CONST memory. With a union of structs, you can have either
> as required (e.g. CONST UNION_TYPE *union, or UNION_TYPE *COST union).
Formally, you are right, but I'm doubtful of the utility of
"pointer-to-union-of-structs". We cannot de-reference a pointer to the
union unless the buffer has enough data for the complete union. This
leaves the parsing of small structs unsolved.
A union of pointers is just syntactic sugar, of course, but it's
convenient. The member that is "pointer-to-smallest-header-substructure"
can be used for determining the actual structure type. Then we can
determine if there's enough data for that structure in the buffer. Then
we can use the matching pointer member, for accessing that structure.
Furthermore, CONST gets too complex really soon, and we have to start
adding explicit casts. My favorite link is:
http://c-faq.com/ansi/constmismatch.html
I had never met "pointer unions" before edk2, but I've found them quite
convenient.
> 2) Well-defined header inspection:
> "if a union contains several structures that share a common initial
> sequence [...], it is permitted to inspect the common initial part of
> any of them anywhere that a declaration of the completed type of the
> union is visible"
> C18 (ISO/IEC 9899:2018), 6.5.2.3.6 (exists since at least C90)
>
> This guarantee can be used to inspect the type defined in a common
> header (e.g. SMBIOS_STRUCTURE) and the process the type-specific data by
> accessing the appropiate member (e.g. SMBIOS_TABLE_TYPE0) legally.
This only applies *after* you have populated the union for inspection.
(Or at least enough bytes for the common header that you're going to
inspect.)
If you have a union collecting three structures: 5 bytes, 19 bytes, and
32 bytes, and you have a buffer with 24 bytes left (suggesting a 5 byte
structure and a 19 byte structure in it, or vice versa), you cannot
populate the full union -- you don't have 32 bytes left.
So let's then assume that the common header is 2 bytes long (with 3 vs.
17 vs. 30 additional bytes required in the specific structures). Fine,
then you read 2 bytes into the stand-alone union helper variable, for
inspecting the common header. Based on the header inspection, you then
decide to (attempt to) read 3 more bytes, or 17 bytes, continuing into
the union, and then parse the specific (now completed) structure through
the matching union member. Great?
Not really. Notice that, in this process, you didn't need the union *at
all*. You can simply use standalone structures, and you may not even
need more stack space.
Compare:
(i) with a union:
enum Type {
type_1,
type_2
}
struct H {
enum Type t;
...
};
struct S1 {
struct H h;
int S1_i1;
};
struct S2 {
struct H h;
char S2_c;
double S2_d;
};
union U {
S1 s1;
S2 s2;
};
Code:
union U u;
char unsigned *src = buffer;
char unsigned *dst = (void*)&u;
size_t specific;
if (room_left < sizeof(struct H)) {
return BAD;
}
memcpy(dst, src, sizeof(struct H));
dst += sizeof(struct H);
src += sizeof(struct H);
room_left -= sizeof(struct H);
switch (u.s1.h.t) {
case type_1:
specific = sizeof(struct S1) - sizeof(struct H);
if (room_left < specific) {
return BAD;
}
memcpy(dst, src, specific);
dst += specific;
src += specific;
room_left -= specific;
/* now access u.s1.S1_i1 */
return GOOD;
case type_2:
specific = sizeof(struct S2) - sizeof(struct H);
if (room_left < specific) {
return BAD;
}
memcpy(dst, src, specific);
dst += specific;
src += specific;
room_left -= specific;
/* now access u.s2.{S2_c,S2_d} */
return GOOD;
default:
return BAD;
}
(ii) without a union:
enum Type {
type_1,
type_2
}
struct H {
enum Type t;
...
};
/* note: header no longer embedded */
struct S1 {
int S1_i1;
};
/* note: header no longer embedded */
struct S2 {
char S2_c;
double S2_d;
};
Code:
struct H h; /* note: no union, just the common header */
char unsigned *src = buffer;
/* note: no dst pointer into union */
size_t specific;
if (room_left < sizeof h) {
return BAD;
}
memcpy(&h, src, sizeof h);
src += sizeof h;
room_left -= sizeof h;
switch (h.t) { /* note: no awkward reference to "u.s1" */
case type_1:
{
struct S1 s1; /* note: never co-exists with "s2" on the stack */
specific = sizeof s1; /* note: no awkward subtraction */
if (room_left < specific) {
return BAD;
}
memcpy(&s1, src, specific);
src += specific;
room_left -= specific;
/* now access s1.S1_i1 */
}
return GOOD;
case type_2:
{
struct S2 s2; /* note: never co-exists with "s1" on the stack */
specific = sizeof s2; /* note: no awkward subtraction */
if (room_left < specific) {
return BAD;
}
memcpy(&s2, src, specific);
src += specific;
room_left -= specific;
/* now access s2.S2_c, s2.S2_d */
}
return GOOD;
default:
return BAD;
}
To me, (ii) is much cleaner than (i); the union is not needed.
Of course, I find the type punning approach even better than (ii) :) See
below:
(iii) no union, yes type punning:
struct H *h;
char unsigned *src = buffer;
if (room_left < sizeof *h) {
return BAD;
}
h = (struct H *)src; /* note: no copying */
src += sizeof *h;
room_left -= sizeof *h;
switch (h->t) {
case type_1:
{
struct S1 *s1;
specific = sizeof *s1;
if (room_left < specific) {
return BAD;
}
s1 = (struct S1 *)src; /* note: no copying */
src += specific;
room_left -= specific;
/* now access s1->S1_i1 */
}
return GOOD;
and so on.
> Plain
> casts and "pointer union" accesses are illegal as per the "inadequate
> type punning" point above.
>
>
>
> "casting away CONST":
> e.g.
> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c#L236
In this case, the real problem is with the function prototype /
specification, not the implementation. The implementation follows from
the problematic function semantics -- if you take Buffer as (CONST UINT8
*), then why return the exact same buffer as (UINT8 *)?
> This should be obvious as Undefined Behaviour because memory previously
> guaranteed to be read-only is returned as a pointer to memory that
> allows writing,
Note: this is *per se* not undefined behavior. Casting away CONST
(explicitly) in itself is OK. Writing through the pointer is also OK
*if* the pointed-to object was never defined as CONST. Otherwise, the
behavior is undefined. See C99 6.7.3 Type qualifiers, p5:
"If an attempt is made to modify an object defined with a
const-qualified type through use of an lvalue with non-const-qualified
type, the behavior is undefined. If an attempt is made to refer to an
object defined with a volatile-qualified type through use of an lvalue
with non-volatile-qualified type, the behavior is undefined."
I've cited the part about "volatile" to highlight the difference between
"modify" and "refer to". When casting away const, the behavior is only
undefined if you actually try to modify an object that is actually const.
int i = 2;
int const ci = 3;
int *pi;
int const *pci;
pci = &i;
pi = (int *)pci; /* fine in itself, but now you're on your own */
*pi = 3; /* fine, as "i" is not defined const */
pci = &ci;
pi = (int *)pci; /* fine in itself, but now you're on your own */
i = *pi; /* fine, not modifying "ci" */
*pi = 3; /* undefined, as "ci" is defined const */
But, yes, the pattern seen under the link is risky practice.
> but for easier lookup, here's the related rule:
> "the left operand has atomic, qualifed, or unqualifed pointer type, and
> [...] the type pointed to by the left has all the qualifers of the type
> pointed to by the right"
> C18 (ISO/IEC 9899:2018), 6.5.16.1 (exists since at least C90)
What you quote is from the Constraints of "Simple assignment". Look at
"6.5.4 Cast operators" as well, paragraph 3:
"Conversions that involve pointers, other than where permitted by the
constraints of 6.5.16.1, shall be specified by means of an explicit cast."
In brief, casting away const is not invalid in itself; it just throws
away protections (diagnostics) that the compiler would otherwise be
required to emit. Sometimes it's unavoidable. In most cases we should
avoid it. That might require fixing a few function prototypes.
> "structs with trailing 1-length array"
> e.g.
> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/Guid/FileInfo.h#L51
>
> This is undefined as per:
> "The behavior is undefned in the following circumstances:
> [...]
> — Addition or subtraction of a pointer into, or just beyond, an array
> object and an integer type produces a result that does not point into,
> or just beyond, the same array object (6.5.6).
> — Addition or subtraction of a pointer into, or just beyond, an array
> object and an integer type produces a result that points just beyond the
> array object and is used as the operand of a unary * operator that is
> evaluated (6.5.6).
> — Pointers that do not point into, or just beyond, the same array object
> are subtracted (6.5.6)."
> C18 (ISO/IEC 9899:2018), J.2 (exists since at least C90)
>
> Same as above, while not all compilers fully support C99, flexible
> arrays should be support by all reasonably new compilers and allow a
> legal declaration and usage where this hack is in place. At worst, a
> macro could be provided to declare a [1] vs a [] array on demand and a
> requirement be introduced to have a "SIZE_OF_" macro for each such
> struct, but my personal preference would be to just enforce flexible arrays.
Yes, in C99, the flexible array member was introduced to replace the
"struct hack", which had always been undefined.
It would be nice to remove all toolchains that don't support the
flexible array member, and then to replace all struct hacks with the
flexible array member. I agree.
Unfortunately, there's one extra difficulty (beyond the "expected"
regressions in adjusting code for the fixed element at offset 0): the
struct hack is used in several places in the UEFI 2.8 spec. So that
would have to be updated too.
> "Missing security checks for external data":
> e.g.
> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L943
>
> The given example misses an alignment verification of the resulting
> pointer (which technically has to be verified *before* casting), as
> documented here:
> "The behavior is undefined in the following circumstances:
> [...]
> — Conversion between two pointer types produces a result that is
> incorrectly aligned (6.3.2.3)."
> C18 (ISO/IEC 9899:2018), J.2 (exists since at least C90)
In C99 anyway, "6.3.2.3 Pointers", paragraph 7 writes,
"A pointer to an object or incomplete type may be converted to a pointer
to a different object or incomplete type. If the resulting pointer is
not correctly aligned [...] for the pointed-to type, the behavior is
undefined. [...]"
I find this very impractical and limiting, when accessing RAM (not
MMIO). I prefer if unaligned pointers (into RAM) just work, albeit slow,
perhaps. I believe AARCH64 can be configured to trip a fault vs. work
(but more slowly). On Intel, it just works.
I think we should be given the freedom to "define" the behavior that's
left undefined in this case by the standard.
> There are more such issues throughout the codebase, including missing
> overflow and (or flawed, see before) bounds checks, however I cannot
> find such quickly.
>
>
>
> "signed int BIT definitions":
> e.g.
> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/Base.h#L348
>
> Fixing this would be prone to regressions, but I'd like to add it for
> tracking purposes. Related discussion can be found down the chain here:
> https://lists.01.org/pipermail/edk2-devel/2018-February/021919.html
I agree about this, in theory. I'm afraid it's impossible to fix in
practice, given the huge amounts of dependent code (esp. out of tree code).
More or less, I'd summarize my opinion as follows:
- we should try to write such new code that conforms to the standard,
- *except* when the standard doesn't give us enough guarantees (i.e.
leaves the behavior undefined) that we need for convenient *in-place*
parsing (from RAM). Integer range checks and buffer boundary checks are
extremely important and we should implement those meticulously, but once
we've made sure our accesses are in range, the compiler should just
follow our pointers wherever they point. We should drag our toolchains
kicking and screaming into a state where they build the source the way
we need, for *in-place* parsing of RAM buffers, through packed
structures. As long as the architectures that we target don't prevent us
from in-place parsing, the toolchains should neither.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47993): https://edk2.groups.io/g/devel/message/47993
Mute This Topic: https://groups.io/mt/34180197/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Thanks for your input.
Due to a misconception of union casts (or accesses, really), multiple
points were rendered infeasible. I suppose the main point left I'd like
some attention towards is alignment, and of course the misdesigned
prototypes (CONST).
Comments inline.
Thanks and regards,
Marvin
Am 24.09.2019 um 22:26 schrieb Laszlo Ersek:
> On 09/23/19 18:27, Marvin Häuser wrote:
>> Good day,
>>
>> Thank you, Laszlo, for your ambition to introduce stricter code style
>> enforcements. Sorry to "hijack" the actual topic (I did not CC anyone on
>> purpose, as this is mostly a separate topic and I'd like a quick comment
>> first), but this seems like a good occasion to mention another few bad
>> practices edk2 has been following. Mainly, I'd like to call *some*
>> attention to quality problems in the code base while this has some
>> traction, and cause a discussion on whether and how those are to be
>> approached.
>>
>> Thank you for your time.
>
> Sure, I can offer my personal opinion on these.
>
>
>> "inadequate type punning":
>> e.g.
>> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c#L446
>>
>> This is mostly about the infamous "Strict Aliasing" rule, which is
>> basically:
>> "An object shall have its stored value accessed only by an lvalue
>> expression that has one of the
>> following types:
>> — a type compatible with the effective type of the object,
>> — a qualifed version of a type compatible with the effective type of the
>> object,
>> — a type that is the signed or unsigned type corresponding to the
>> effective type of the object,
>> — a type that is the signed or unsigned type corresponding to a qualifed
>> version of the effective
>> type of the object,
>> — an aggregate or union type that includes one of the aforementioned
>> types among its members
>> (including, recursively, a member of a subaggregate or contained union), or
>> — a character type."
>> C18 (ISO/IEC 9899:2018), 6.5.7 (exists, though has been updated, since C90)
>>
>> Currently optimisations based on this are disabled. This is a bit nasty
>> to work around if *seriously* needed when sticking to C90, I can only
>> think of memcpy right now. However, even though there are compilers that
>> do not fully support C99 (ahem, Microsoft :) ), type-punning by unions
>> should be supported by them all, and has been legal as of C99, where the
>> following part has been dropped from the standard:
>> "With one exception, if a member of a union object is accessed after a
>> value has been stored in a different member of the object, the behavior
>> is implementation-defined."
>> C90 (ISO/IEC 9899:1990), 6.3.2.3
>
> I'm opposed to enforcing the strict aliasing rules, even though in all
> code that I write, I either try to conform to them, or at least I seek
> to be fully conscious of breaking them.
I agree with that, but I often see completely needless violations of
them. In my opinion, all intentional violations should be documented to
signal the conscious breakage of the standard, as should be any "abuse"
of known implementation-defined behaviour. I suppose for the amount of
in-place parsing done, the Strict Aliasing rule should be an exception
for these cases, as per the assumptions below.
>
> Here's the thing: IMO, the strict aliasing rules sacrifice flexibility
> for performance (optimization possibilities). Not to mention the amount
> of code in edk2 that would have to be identified and updated.
>
> BaseTools uses "-fno-strict-aliasing" everywhere, and I think that's a
> good choice.
>
> https://blog.regehr.org/archives/1180
>
> This proposal for a "friendly dialect of C" intended to eliminate the
> strict aliasing rules altogether. (Item#10; possibly also item#13.)
>
> Also, as it points out, the Linux kernel is built with
> "-fno-strict-aliasing". I've checked now, with a *long* series of "git
> blame" commands, even digging into the "history" repository (which is at
> <git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git>). I can
> say that the current flag has been in place since *at least* Linux
> v2.5.0 (2002-02-04).
>
> QEMU has been built with "-fno-strict-aliasing" ever since commit
> b932caba32c6 ("new disk image layer", 2004-08-01), known originally as
> SVN rev 1030.
My point here is invalidated two comments below.
>
>
> Consider the following example. You have a dynamically allocated buffer.
> You read some data into it, from the network or the disk, using PCI DMA.
> Let's assume that, from the block read via PCI DMA, the library
> function(s) or protocol member(s) that you call, directly or indirectly,
> there is at least one that:
> - copies data from a source buffer to a target buffer, using UINT32 or
> UINT64 assignments (for speed),
Honestly, I did not consider this and only had memcpy/memmove in mind.
However, if we "virtually" treat CopyMem as memmove, the compiler
compatibility verification would be reduced from all callers to just it,
i.e. CopyMem must be implemented in a way that, for all supported
compilers, we can assume the original effective type is "carried over",
such with at worst (which should not be required with any sane compiler)
char-copying.
I'm not looking to have absolute C compliance enforced, but to reduce
pointless violations and possibly "concentrate" violations for easier
compatibility verification.
> - and is implemented in C.
>
> Now, according to the effective type rules, your dynamic buffer's
> effective type is "array of UINT32" or "array of UINT64". That's because
> of C99 6.5 Expressions, p6:
>
> "If a value is stored into an object having no declared type through an
> lvalue having a type that is not a character type, then the type of the
> lvalue becomes the effective type of the object for that access and for
> subsequent accesses that do not modify the stored value."
>
> Then if you try to parse this buffer as a UEFI device path (= a packed
> sequence of device path node structures), *IN PLACE*, you will break the
> effective type rules no matter what. Because, you will necessarily look
> at the next node in the blob as an EFI_DEVICE_PATH_PROTOCOL (because
> you'll want to learn the Type and the SubType fields, and the Length
> array too). Note that EFI_DEVICE_PATH_PROTOCOL is a structure with no
> UINT32 or UINT64 members. Boom.
>
> So you have to resort to one of the following things:
>
> 1. Define a union type, assign the *full union* first
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90167#c3>, and then check
> the union helper variable. (First you must make sure there was enough
> room in the buffer being parsed for a full union.)
I was going to argue the exact same way you did in that bug, I was not
aware of GCC treating this rule, in my opinion, inappropriately (the
reasoning there makes sense, but I'd hope the committee was actually
seeking to allow this). As long as no writes are performed through the
union (because that yields unspecified values for all bytes between the
end of the written-to member and the end of the full union size), I
would not have thought there'd be problems with such a cast, for the
exact part you quoted. This makes the situation ridiculous to handle, I
agree.
I hope there will be some sort of adaption that will allow exactly this
in the future, then this should be enforced (it cannot break more than
the current solution, and for future standards and complying compilers,
would avoid Undefined Behaviour).
>
> 2. Or, use memcpy() -- or something that the compiler is similarly
> enlightened about --, to the same helper variable.
>
> 3. Or, write a manual copy loop with character access, to the helper
> variable.
>
> 4. Or, use character access to read the fields.
>
> #1 through #3 add a separate copying step, while #4 is extremely
> uncomfortable to program.
>
> In a nutshell, the effective type rules require separate
> de-serialization routines for all data, and that's incredibly annoying.
> It kills the utility of packed C structures.
>
> I prefer packed C structures, size checks (!!!) against the buffers to
> parse, and then type punning of pointers.
>
>
>> "pointer unions":
>> e.g.
>> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/IndustryStandard/SmBios.h#L2592
>>
>> While the idea behind them is certainly style preference, using a union
>> of pointers prevents two important things over a union of structs.
>>
>> 1) CONST declaration: When defining a variable of a union type
>> containing pointers as CONST, speaking of its members, they are all
>> going to be CONST pointers to arbitrary memory and not arbitrary
>> pointers to CONST memory. With a union of structs, you can have either
>> as required (e.g. CONST UNION_TYPE *union, or UNION_TYPE *COST union).
>
> Formally, you are right, but I'm doubtful of the utility of
> "pointer-to-union-of-structs". We cannot de-reference a pointer to the
> union unless the buffer has enough data for the complete union. This
> leaves the parsing of small structs unsolved.
>
> A union of pointers is just syntactic sugar, of course, but it's
> convenient. The member that is "pointer-to-smallest-header-substructure"
> can be used for determining the actual structure type. Then we can
> determine if there's enough data for that structure in the buffer. Then
> we can use the matching pointer member, for accessing that structure.
>
> Furthermore, CONST gets too complex really soon, and we have to start
> adding explicit casts. My favorite link is:
>
> http://c-faq.com/ansi/constmismatch.html
>
> I had never met "pointer unions" before edk2, but I've found them quite
> convenient.
>
>
>> 2) Well-defined header inspection:
>> "if a union contains several structures that share a common initial
>> sequence [...], it is permitted to inspect the common initial part of
>> any of them anywhere that a declaration of the completed type of the
>> union is visible"
>> C18 (ISO/IEC 9899:2018), 6.5.2.3.6 (exists since at least C90)
>>
>> This guarantee can be used to inspect the type defined in a common
>> header (e.g. SMBIOS_STRUCTURE) and the process the type-specific data by
>> accessing the appropiate member (e.g. SMBIOS_TABLE_TYPE0) legally.
>
> This only applies *after* you have populated the union for inspection.
> (Or at least enough bytes for the common header that you're going to
> inspect.)
>
> If you have a union collecting three structures: 5 bytes, 19 bytes, and
> 32 bytes, and you have a buffer with 24 bytes left (suggesting a 5 byte
> structure and a 19 byte structure in it, or vice versa), you cannot
> populate the full union -- you don't have 32 bytes left.
>
> So let's then assume that the common header is 2 bytes long (with 3 vs.
> 17 vs. 30 additional bytes required in the specific structures). Fine,
> then you read 2 bytes into the stand-alone union helper variable, for
> inspecting the common header. Based on the header inspection, you then
> decide to (attempt to) read 3 more bytes, or 17 bytes, continuing into
> the union, and then parse the specific (now completed) structure through
> the matching union member. Great?
>
> Not really. Notice that, in this process, you didn't need the union *at
> all*. You can simply use standalone structures, and you may not even
> need more stack space.
>
> Compare:
>
> (i) with a union:
>
> enum Type {
> type_1,
> type_2
> }
>
> struct H {
> enum Type t;
> ...
> };
>
> struct S1 {
> struct H h;
> int S1_i1;
> };
>
> struct S2 {
> struct H h;
> char S2_c;
> double S2_d;
> };
>
> union U {
> S1 s1;
> S2 s2;
> };
>
> Code:
>
> union U u;
> char unsigned *src = buffer;
> char unsigned *dst = (void*)&u;
> size_t specific;
>
> if (room_left < sizeof(struct H)) {
> return BAD;
> }
>
> memcpy(dst, src, sizeof(struct H));
> dst += sizeof(struct H);
> src += sizeof(struct H);
> room_left -= sizeof(struct H);
>
> switch (u.s1.h.t) {
> case type_1:
> specific = sizeof(struct S1) - sizeof(struct H);
> if (room_left < specific) {
> return BAD;
> }
> memcpy(dst, src, specific);
> dst += specific;
> src += specific;
> room_left -= specific;
> /* now access u.s1.S1_i1 */
> return GOOD;
>
> case type_2:
> specific = sizeof(struct S2) - sizeof(struct H);
> if (room_left < specific) {
> return BAD;
> }
> memcpy(dst, src, specific);
> dst += specific;
> src += specific;
> room_left -= specific;
> /* now access u.s2.{S2_c,S2_d} */
> return GOOD;
>
> default:
> return BAD;
> }
>
>
> (ii) without a union:
>
> enum Type {
> type_1,
> type_2
> }
>
> struct H {
> enum Type t;
> ...
> };
>
> /* note: header no longer embedded */
> struct S1 {
> int S1_i1;
> };
>
> /* note: header no longer embedded */
> struct S2 {
> char S2_c;
> double S2_d;
> };
>
> Code:
>
> struct H h; /* note: no union, just the common header */
> char unsigned *src = buffer;
> /* note: no dst pointer into union */
> size_t specific;
>
> if (room_left < sizeof h) {
> return BAD;
> }
>
> memcpy(&h, src, sizeof h);
> src += sizeof h;
> room_left -= sizeof h;
>
> switch (h.t) { /* note: no awkward reference to "u.s1" */
> case type_1:
> {
> struct S1 s1; /* note: never co-exists with "s2" on the stack */
>
> specific = sizeof s1; /* note: no awkward subtraction */
> if (room_left < specific) {
> return BAD;
> }
> memcpy(&s1, src, specific);
> src += specific;
> room_left -= specific;
> /* now access s1.S1_i1 */
> }
> return GOOD;
>
> case type_2:
> {
> struct S2 s2; /* note: never co-exists with "s1" on the stack */
>
> specific = sizeof s2; /* note: no awkward subtraction */
> if (room_left < specific) {
> return BAD;
> }
> memcpy(&s2, src, specific);
> src += specific;
> room_left -= specific;
> /* now access s2.S2_c, s2.S2_d */
> }
> return GOOD;
>
> default:
> return BAD;
> }
>
>
> To me, (ii) is much cleaner than (i); the union is not needed.
>
> Of course, I find the type punning approach even better than (ii) :) See
> below:
>
> (iii) no union, yes type punning:
>
> struct H *h;
> char unsigned *src = buffer;
>
> if (room_left < sizeof *h) {
> return BAD;
> }
> h = (struct H *)src; /* note: no copying */
> src += sizeof *h;
> room_left -= sizeof *h;
>
> switch (h->t) {
> case type_1:
> {
> struct S1 *s1;
>
> specific = sizeof *s1;
> if (room_left < specific) {
> return BAD;
> }
> s1 = (struct S1 *)src; /* note: no copying */
> src += specific;
> room_left -= specific;
> /* now access s1->S1_i1 */
> }
> return GOOD;
>
> and so on.
>
Thanks for your comprehensive examples, but that part was written with
the idea of casting to a union pointer as you mentioned in the previous
point in mind. Without such casts and subsequent accesses being legal,
this is not of a lot of use for us, I agree.
>
>> Plain
>> casts and "pointer union" accesses are illegal as per the "inadequate
>> type punning" point above.
>>
>>
>>
>> "casting away CONST":
>> e.g.
>> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c#L236
>
> In this case, the real problem is with the function prototype /
> specification, not the implementation. The implementation follows from
> the problematic function semantics -- if you take Buffer as (CONST UINT8
> *), then why return the exact same buffer as (UINT8 *)?
I agree, the prototype has been misdesigned. I was just referring to
such "patterns" being a problem, not from where they actually originate.
>
>
>> This should be obvious as Undefined Behaviour because memory previously
>> guaranteed to be read-only is returned as a pointer to memory that
>> allows writing,
>
> Note: this is *per se* not undefined behavior. Casting away CONST
> (explicitly) in itself is OK. Writing through the pointer is also OK
> *if* the pointed-to object was never defined as CONST. Otherwise, the
> behavior is undefined. See C99 6.7.3 Type qualifiers, p5:
>
> "If an attempt is made to modify an object defined with a
> const-qualified type through use of an lvalue with non-const-qualified
> type, the behavior is undefined. If an attempt is made to refer to an
> object defined with a volatile-qualified type through use of an lvalue
> with non-volatile-qualified type, the behavior is undefined."
>
> I've cited the part about "volatile" to highlight the difference between
> "modify" and "refer to". When casting away const, the behavior is only
> undefined if you actually try to modify an object that is actually const.
>
> int i = 2;
> int const ci = 3;
> int *pi;
> int const *pci;
>
> pci = &i;
> pi = (int *)pci; /* fine in itself, but now you're on your own */
> *pi = 3; /* fine, as "i" is not defined const */
>
> pci = &ci;
> pi = (int *)pci; /* fine in itself, but now you're on your own */
> i = *pi; /* fine, not modifying "ci" */
> *pi = 3; /* undefined, as "ci" is defined const */
>
>
> But, yes, the pattern seen under the link is risky practice.
>
>
>> but for easier lookup, here's the related rule:
>> "the left operand has atomic, qualifed, or unqualifed pointer type, and
>> [...] the type pointed to by the left has all the qualifers of the type
>> pointed to by the right"
>> C18 (ISO/IEC 9899:2018), 6.5.16.1 (exists since at least C90)
>
> What you quote is from the Constraints of "Simple assignment". Look at
> "6.5.4 Cast operators" as well, paragraph 3:
>
> "Conversions that involve pointers, other than where permitted by the
> constraints of 6.5.16.1, shall be specified by means of an explicit cast."
>
> In brief, casting away const is not invalid in itself; it just throws
> away protections (diagnostics) that the compiler would otherwise be
> required to emit. Sometimes it's unavoidable. In most cases we should
> avoid it. That might require fixing a few function prototypes.
Sorry, that was a bit rushed, but the actual problem persists. If
nothing else, casting away CONST drastically increases the likeliness of
misuse happening, as the only indicator for const-ness has been dropped.
>
>
>> "structs with trailing 1-length array"
>> e.g.
>> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/Guid/FileInfo.h#L51
>>
>> This is undefined as per:
>> "The behavior is undefned in the following circumstances:
>> [...]
>> — Addition or subtraction of a pointer into, or just beyond, an array
>> object and an integer type produces a result that does not point into,
>> or just beyond, the same array object (6.5.6).
>> — Addition or subtraction of a pointer into, or just beyond, an array
>> object and an integer type produces a result that points just beyond the
>> array object and is used as the operand of a unary * operator that is
>> evaluated (6.5.6).
>> — Pointers that do not point into, or just beyond, the same array object
>> are subtracted (6.5.6)."
>> C18 (ISO/IEC 9899:2018), J.2 (exists since at least C90)
>>
>> Same as above, while not all compilers fully support C99, flexible
>> arrays should be support by all reasonably new compilers and allow a
>> legal declaration and usage where this hack is in place. At worst, a
>> macro could be provided to declare a [1] vs a [] array on demand and a
>> requirement be introduced to have a "SIZE_OF_" macro for each such
>> struct, but my personal preference would be to just enforce flexible arrays.
>
> Yes, in C99, the flexible array member was introduced to replace the
> "struct hack", which had always been undefined.
>
> It would be nice to remove all toolchains that don't support the
> flexible array member, and then to replace all struct hacks with the
> flexible array member. I agree.
>
> Unfortunately, there's one extra difficulty (beyond the "expected"
> regressions in adjusting code for the fixed element at offset 0): the
> struct hack is used in several places in the UEFI 2.8 spec. So that
> would have to be updated too.
Agreed. However, I see value in updating the UEFI specification, as it
should mandate the abstract-ish concept (trailing array of a length not
known at compile time), not the implementation (struct hack), which in
this case even is a language standard violation.
>
>
>> "Missing security checks for external data":
>> e.g.
>> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L943
>>
>> The given example misses an alignment verification of the resulting
>> pointer (which technically has to be verified *before* casting), as
>> documented here:
>> "The behavior is undefined in the following circumstances:
>> [...]
>> — Conversion between two pointer types produces a result that is
>> incorrectly aligned (6.3.2.3)."
>> C18 (ISO/IEC 9899:2018), J.2 (exists since at least C90)
>
> In C99 anyway, "6.3.2.3 Pointers", paragraph 7 writes,
>
> "A pointer to an object or incomplete type may be converted to a pointer
> to a different object or incomplete type. If the resulting pointer is
> not correctly aligned [...] for the pointed-to type, the behavior is
> undefined. [...]"
>
> I find this very impractical and limiting, when accessing RAM (not
> MMIO). I prefer if unaligned pointers (into RAM) just work, albeit slow,
> perhaps. I believe AARCH64 can be configured to trip a fault vs. work
> (but more slowly). On Intel, it just works.
On Intel it depends actually, e.g. SSE often (always?) mandates aligned
pointers. If, in C code, you ignore the alignment requirements, the
compiler is technically allowed to perform optimizations with
instructions that do require aligned addresses. I think for Intel,
SSE-based optimizations are disabled (for this reason? I'm not sure),
but this only makes life tougher when adding support for new compilers
and architectures honestly.
Also, in case of file parsing, if the file format mandates alignment for
certain offsets (segments, sections, sub-headers) and the offset is
unalgined, an alignment verification aids as an additional layer of
input sanity verification. As I see it, you either have an aligned
struct and make sure the pointer is aligned, or you have an unaligned
struct and the compiler takes care of accessing the data unaligned.
>
> I think we should be given the freedom to "define" the behavior that's
> left undefined in this case by the standard.
What when dealing with an architecture that just does not support
unaligned accesses? I suppose that's going to be unlikely.
>
>
>> There are more such issues throughout the codebase, including missing
>> overflow and (or flawed, see before) bounds checks, however I cannot
>> find such quickly.
>>
>>
>>
>> "signed int BIT definitions":
>> e.g.
>> https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/Base.h#L348
>>
>> Fixing this would be prone to regressions, but I'd like to add it for
>> tracking purposes. Related discussion can be found down the chain here:
>> https://lists.01.org/pipermail/edk2-devel/2018-February/021919.html
>
> I agree about this, in theory. I'm afraid it's impossible to fix in
> practice, given the huge amounts of dependent code (esp. out of tree code).
Definitely.
>
>
> More or less, I'd summarize my opinion as follows:
>
> - we should try to write such new code that conforms to the standard,
>
> - *except* when the standard doesn't give us enough guarantees (i.e.
> leaves the behavior undefined) that we need for convenient *in-place*
> parsing (from RAM). Integer range checks and buffer boundary checks are
> extremely important and we should implement those meticulously, but once
> we've made sure our accesses are in range, the compiler should just
> follow our pointers wherever they point. We should drag our toolchains
> kicking and screaming into a state where they build the source the way
> we need, for *in-place* parsing of RAM buffers, through packed
> structures. As long as the architectures that we target don't prevent us
> from in-place parsing, the toolchains should neither.
Agreed.
>
> Thanks
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48012): https://edk2.groups.io/g/devel/message/48012
Mute This Topic: https://groups.io/mt/34180197/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
(snipping liberally)
On 09/25/19 10:13, Marvin Häuser wrote:
> Am 24.09.2019 um 22:26 schrieb Laszlo Ersek:
>> I'm opposed to enforcing the strict aliasing rules, even though in
>> all code that I write, I either try to conform to them, or at least I
>> seek to be fully conscious of breaking them.
>
> I agree with that, but I often see completely needless violations of
> them. In my opinion, all intentional violations should be documented
> to signal the conscious breakage of the standard, as should be any
> "abuse" of known implementation-defined behaviour.
Agreed!
> I suppose for the amount of in-place parsing done, the Strict Aliasing
> rule should be an exception for these cases, as per the assumptions
> below.
Probably so; from personal experience, I'm basically only drawn to type
punning when there's some binary data to parse into (or via)
structure(s); and in firmware, there are *surprisingly* many sequences /
tables to parse.
>> Consider the following example. You have a dynamically allocated
>> buffer. You read some data into it, from the network or the disk,
>> using PCI DMA. Let's assume that, from the block read via PCI DMA,
>> the library function(s) or protocol member(s) that you call, directly
>> or indirectly, there is at least one that:
>> - copies data from a source buffer to a target buffer, using UINT32
>> or UINT64 assignments (for speed),
>
> Honestly, I did not consider this and only had memcpy/memmove in mind.
It may not be an intuitive example.
There are other (similarly practical) instances of this pattern. See
commit 6e2543b01d0c ("ArmVirtualizationPkg: introduce QemuFwCfgLib
instance for DXE drivers", 2015-01-02), for example:
> [...]
>
> Because MMIO accesses are costly on KVM/ARM, InternalQemuFwCfgReadBytes()
> accesses the fw_cfg data register in full words. This speeds up transfers
> almost linearly.
>
> [...]
>
> +/**
> + Reads firmware configuration bytes into a buffer
> +
> + @param[in] Size Size in bytes to read
> + @param[in] Buffer Buffer to store data into (OPTIONAL if Size is 0)
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +InternalQemuFwCfgReadBytes (
> + IN UINTN Size,
> + IN VOID *Buffer OPTIONAL
> + )
> +{
> + UINTN Left;
> + UINT8 *Ptr;
> + UINT8 *End;
> +
> +#ifdef MDE_CPU_AARCH64
> + Left = Size & 7;
> +#else
> + Left = Size & 3;
> +#endif
> +
> + Size -= Left;
> + Ptr = Buffer;
> + End = Ptr + Size;
> +
> +#ifdef MDE_CPU_AARCH64
> + while (Ptr < End) {
> + *(UINT64 *)Ptr = MmioRead64 (mFwCfgDataAddress);
> + Ptr += 8;
> + }
> + if (Left & 4) {
> + *(UINT32 *)Ptr = MmioRead32 (mFwCfgDataAddress);
> + Ptr += 4;
> + }
> +#else
> + while (Ptr < End) {
> + *(UINT32 *)Ptr = MmioRead32 (mFwCfgDataAddress);
> + Ptr += 4;
> + }
> +#endif
> +
> + if (Left & 2) {
> + *(UINT16 *)Ptr = MmioRead16 (mFwCfgDataAddress);
> + Ptr += 2;
> + }
> + if (Left & 1) {
> + *Ptr = MmioRead8 (mFwCfgDataAddress);
> + }
> +}
And then the data read like this from the hypervisor may contain
arbitrary structures for the firmware to parse.
Back to the discussion:
On 09/25/19 10:13, Marvin Häuser wrote:
> However, if we "virtually" treat CopyMem as memmove, the compiler
> compatibility verification would be reduced from all callers to just
> it, i.e. CopyMem must be implemented in a way that, for all supported
> compilers, we can assume the original effective type is "carried
> over", such with at worst (which should not be required with any sane
> compiler) char-copying.
As far as I understand, you're saying that, if we ensure that compilers
recognize CopyMem() as similar to memmove(), then we can apply the C
standard (~ the effective type rules) to edk2 too, only replacing
memcpy() / memmove() references in the std language with CopyMem()
references.
Then we could call edk2 conformant once all such data manipulation
boiled down to correct use of CopyMem().
If that's your point, then I agree with it.
> I'm not looking to have absolute C compliance enforced, but to reduce
> pointless violations and possibly "concentrate" violations for easier
> compatibility verification.
These are very good goals.
(As a digression, consider the following -- very frequent -- pattern:
EFI_PCI_IO_PROTOCOL *PciIo;
Status = gBS->OpenProtocol (
DeviceHandle,
&gEfiPciIoProtocolGuid,
(VOID **)&PciIo,
This->DriverBindingHandle,
DeviceHandle,
EFI_OPEN_PROTOCOL_BY_DRIVER
);
Technically, the third argument
(VOID **)&PciIo
is wrong, as pointer-to-structure types (which have identical
representation to each other) need not have the same representation as
pointer-to-void. See C99 "6.2.5 Types", p27:
> A pointer to void shall have the same representation and alignment
> requirements as a pointer to a character type. [...] Similarly,
> pointers to qualified or unqualified versions of compatible types
> shall have the same representation and alignment requirements. All
> pointers to structure types shall have the same representation and
> alignment requirements as each other. All pointers to union types
> shall have the same representation and alignment requirements as each
> other. Pointers to other types need not have the same representation
> or alignment requirements.
The proper way to do it would be:
VOID *Interface;
EFI_PCI_IO_PROTOCOL *PciIo;
Status = gBS->OpenProtocol (
DeviceHandle,
&gEfiPciIoProtocolGuid,
&Interface,
This->DriverBindingHandle,
DeviceHandle,
EFI_OPEN_PROTOCOL_BY_DRIVER
);
if (!EFI_ERROR (Status)) {
PciIo = Interface;
}
Because, in the assignment, the pointer-to-void is *converted* to
pointer-to-structure (not just re-interpreted as), with any necessary
updates to the internal representation.
IIRC, POSIX ultimately added a requirement (beyond the C standard) for
implementations, for said pointer representations to be identical.
So, this is a very frequent violation of the standard; I think it's even
visible in the UEFI spec, in various example code.
Is this violation pointless? It wouldn't be difficult to write all new
code following the second (proper) pattern. Updating all existent sites
would be a nightmare though.
)
Continuing:
On 09/25/19 10:13, Marvin Häuser wrote:
> If nothing else, casting away CONST drastically increases the
> likeliness of misuse happening, as the only indicator for const-ness
> has been dropped.
I agree; before casting away an existing const-qualification, we should
think thrice.
(OTOH, trying to design all function prototypes as tightly as possible,
const-qualifying as many as possible pointed-to objects under the
pointer-typed parameters, tends to become unwieldy really quick. In my
experience anyway.)
>> It would be nice to remove all toolchains that don't support the
>> flexible array member, and then to replace all struct hacks with the
>> flexible array member. I agree.
>>
>> Unfortunately, there's one extra difficulty (beyond the "expected"
>> regressions in adjusting code for the fixed element at offset 0): the
>> struct hack is used in several places in the UEFI 2.8 spec. So that
>> would have to be updated too.
>
> Agreed. However, I see value in updating the UEFI specification, as it
> should mandate the abstract-ish concept (trailing array of a length not
> known at compile time), not the implementation (struct hack), which in
> this case even is a language standard violation.
It's not that I don't see value in updating the spec -- I generally
don't have time for working on the spec (or for reviewing ECRs)! :)
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48039): https://edk2.groups.io/g/devel/message/48039
Mute This Topic: https://groups.io/mt/34180197/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.