Re: [edk2-devel] [PATCH 00/35] edk2: clean up the usage of standardized (VOID*) typedefs

["Laszlo Ersek" <lersek@redhat.com>]

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