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

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

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