From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 25 Sep 2019 08:54:43 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 924F68A1C95; Wed, 25 Sep 2019 15:54:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-25.rdu2.redhat.com [10.10.120.25]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9790A600C8; Wed, 25 Sep 2019 15:54:41 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 00/35] edk2: clean up the usage of standardized (VOID*) typedefs To: =?UTF-8?Q?Marvin_H=c3=a4user?= , "devel@edk2.groups.io" References: <20190917194935.24322-1-lersek@redhat.com> <06b8b728-e38e-cc64-4499-9733fc993e5d@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 25 Sep 2019 17:54:40 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.69]); Wed, 25 Sep 2019 15:54:42 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable (snipping liberally) On 09/25/19 10:13, Marvin H=C3=A4user 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, InternalQemuFwCfgReadB= ytes() > accesses the fw_cfg data register in full words. This speeds up tra= nsfers > 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 =3D Size & 7; > +#else > + Left =3D Size & 3; > +#endif > + > + Size -=3D Left; > + Ptr =3D Buffer; > + End =3D Ptr + Size; > + > +#ifdef MDE_CPU_AARCH64 > + while (Ptr < End) { > + *(UINT64 *)Ptr =3D MmioRead64 (mFwCfgDataAddress); > + Ptr +=3D 8; > + } > + if (Left & 4) { > + *(UINT32 *)Ptr =3D MmioRead32 (mFwCfgDataAddress); > + Ptr +=3D 4; > + } > +#else > + while (Ptr < End) { > + *(UINT32 *)Ptr =3D MmioRead32 (mFwCfgDataAddress); > + Ptr +=3D 4; > + } > +#endif > + > + if (Left & 2) { > + *(UINT16 *)Ptr =3D MmioRead16 (mFwCfgDataAddress); > + Ptr +=3D 2; > + } > + if (Left & 1) { > + *Ptr =3D 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=C3=A4user 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 =3D 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 =3D gBS->OpenProtocol ( DeviceHandle, &gEfiPciIoProtocolGuid, &Interface, This->DriverBindingHandle, DeviceHandle, EFI_OPEN_PROTOCOL_BY_DRIVER ); if (!EFI_ERROR (Status)) { PciIo =3D 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=C3=A4user 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