From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web11.29043.1595452312392533782 for ; Wed, 22 Jul 2020 14:11:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RKGTT+yt; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1595452311; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=J8tgtZT7jo8Gw3GmldcTz1A2vJqXRmzhdGVLJN60fOw=; b=RKGTT+ytEFRvs0pxx8Pd07Q8+Jh4pEK0ZqQIi5fnMVYe74casJR2eSDXcW1ZDeJsEsY1NR cJCYTK6bIv7fiuQ6EAJJ/fMs+A1b0LVCAGYVMpO9oCVYsegzdC9f5l/rGCGK6EnpBd71HX j3KFGX3QIpaPeq7Yy8956GDBgHoEWME= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-47-i4EtujlxN0ucBdhAcKZ8zQ-1; Wed, 22 Jul 2020 17:11:44 -0400 X-MC-Unique: i4EtujlxN0ucBdhAcKZ8zQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B1F751005510; Wed, 22 Jul 2020 21:11:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-129.ams2.redhat.com [10.36.113.129]) by smtp.corp.redhat.com (Postfix) with ESMTP id 547535D9CD; Wed, 22 Jul 2020 21:11:41 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 2/2] EmbeddedPkg: Add cast from (void*) for VS2017 build To: devel@edk2.groups.io, leif@nuviainc.com, PierreGondois Cc: ard.biesheuvel@arm.com, nd@arm.com References: <20200630104901.11648-1-pierre.gondois@arm.com> <20200630104901.11648-3-pierre.gondois@arm.com> <20200722153524.GE1337@vanye> From: "Laszlo Ersek" Message-ID: Date: Wed, 22 Jul 2020 23:11:40 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200722153524.GE1337@vanye> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 07/22/20 17:35, Leif Lindholm wrote: > On Tue, Jun 30, 2020 at 11:49:01 +0100, PierreGondois wrote: >> From: Pierre Gondois >> >> The following build configrations: >> build -b DEBUG -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc >> build -b NOOPT -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc >> build -b RELEASE -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc >> >> are generating the following build errors: >> edk2\EmbeddedPkg\Library\AndroidBootImgLib\AndroidBootImgLib.c(100): >> error C2036: 'void *': unknown size >> edk2\EmbeddedPkg\Library\AndroidBootImgLib\AndroidBootImgLib.c(347): >> error C2036: 'void *': unknown size >> >> Since the size of void* depends on the architecture, it can be >> dangerous to use void* pointer arithmetic. Plus the C99 doesn't state >> that void* pointer arithmetic is allowed. >> This patch adds a cast to fix the Visual Studio errors reported. >> >> Signed-off-by: Pierre Gondois >> --- >> >> The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Fix_VS2017_build_error_v1 >> >> Notes: >> v1: >> - Fix VS2017 build errors. [Pierre] >> >> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c >> index e1036954ee586dfc30266eec2897d71bfc949038..bbe0d41018b3d5665c72ee61efe737ae57b1b2eb 100644 >> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c >> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c >> @@ -1,6 +1,6 @@ >> /** @file >> >> - Copyright (c) 2013-2014, ARM Ltd. All rights reserved.
>> + Copyright (c) 2013-2020, ARM Ltd. All rights reserved.
>> Copyright (c) 2017, Linaro. All rights reserved. >> >> SPDX-License-Identifier: BSD-2-Clause-Patent >> @@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo ( >> ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize)); >> >> *KernelSize = Header->KernelSize; >> - *Kernel = BootImg + Header->PageSize; >> + *Kernel = (UINT8*)BootImg + Header->PageSize; > > The reason I prefer my version, although this one would also solve the > compilation error, is that I really don't like casts to char * (which > this effectively is) as a workaround. > > The problem I have with it is that a cast is a signal of intent (this > thing that we have been viewing as an X should now be seen as a Y) - > and the intent here is simply to get the side effect that a char has a > known size of 1 (whereas a void doesn't). > I will admit it is the first time I have seen it used for this purpose > :) Personally I'd be OK with either fix (yours or Pierre's); to me both express the exact same thing -- move Header->PageSize bytes past BootImg. Viewed from a portability perspective, Pierre's patch is actually more portable (per C standard); as UINT8 stands for "unsigned char", and that is called "object representation" by the C standard: > 6.2.6 Representations of types > 6.2.6.1 General > > 3 Values stored in unsigned bit-fields and objects of type unsigned > char shall be represented using a pure binary notation. (Footnote > 40.) > > 4 Values stored in non-bit-field objects of any other object type > consist of n * CHAR_BIT bits, where n is the size of an object of > that type, in bytes. The value may be copied into an object of type > unsigned char [n] (e.g., by memcpy); the resulting set of bytes is > called the object representation of the value. > > Footnote 40: [...] A byte contains CHAR_BIT bits, and the values of > type unsigned char range from 0 to (2^CHAR_BIT)-1. Further references: > 6.2.5 Types > > 27 A pointer to void shall have the same representation and alignment > requirements as a pointer to a character type. Footnote 39 [...] > > Footnote 39: The same representation and alignment requirements are > meant to imply interchangeability as arguments to > functions, return values from functions, and members of > unions. > 6.3.2.3 Pointers > > 7 [...] When a pointer to an object is converted to a pointer to a > character type, the result points to the lowest addressed byte of > the object. Successive increments of the result, up to the size of > the object, yield pointers to the remaining bytes of the object. > 6.5 Expressions > > 6 The effective type of an object for an access to its stored value is > the declared type of the object, if any. (Footnote 75.) [...] If a > value is copied into an object having no declared type using memcpy > or memmove, or is copied as an array of character type, then the > effective type of the modified object for that access and for > subsequent accesses that do not modify the value is the effective > type of the object from which the value is copied, if it has one. > Footnote 75: Allocated objects have no declared type. (This is one of the most exciting parts of the standard BTW: it means that, if you malloc() 4 bytes, and copy a local uint32_t variable into it, with an open-coded (unsigned char*) loop, then the effective type of the dynamically allocated object becomes uint32_t for subsequent reads! Which means for example, considering the effective type rules strictly, that reading the dynamically allocated object post-copy, even via *correctly aligned* (uint16_t*) pointers, is undefined behavior.) > 7 An object shall have its stored value accessed only by an lvalue > expression that has one of the following types (Footnote 76): > - [...] > - a character type. > > Footnote 76: The intent of this list is to specify those circumstances > in which an object may or may not be aliased. Whereas converting a (VOID*) to UINTN (and maybe back) is only covered here (if I remember correctly): > 6.3 Conversions > 6.3.2 Other operands > 6.3.2.3 Pointers > > 5 An integer may be converted to any pointer type. Except as > previously specified, the result is implementation-defined, might > not be correctly aligned, might not point to an entity of the > referenced type, and might be a trap representation. (Footnote 56) > > 6 Any pointer type may be converted to an integer type. Except as > previously specified, the result is implementation-defined. If the > result cannot be represented in the integer type, the behavior is > undefined. The result need not be in the range of values of any > integer type. > > Footnote 56: The mapping functions for converting a pointer to an > integer or an integer to a pointer are intended to be > consistent with the addressing structure of the execution > environment. So my only point is that (void*) --> (unsigned char *) has no "implementation-defined" dependencies, while (VOID*) --> (UINTN) is implementation-defined. (The dependency exploited in edk2 is that UINTN is always just as wide as (VOID*).) But, given both patches are for edk2, where UINTN <-> (VOID*) interchangeability is guaranteed, I'm equally fine with both patches. Thanks Laszlo > > The same problem occure when casting to char * in order to not have to > deal with pointer arithmetic at all or to avoid thinking about > alignment requirements. And both of these are frequently indicators of > buggy code. > > / > Leif > >> return EFI_SUCCESS; >> } >> >> @@ -339,9 +339,12 @@ AndroidBootImgUpdateFdt ( >> goto Fdt_Exit; >> } >> >> - Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode, >> - "linux,initrd-end", >> - (UINTN)(RamdiskData + RamdiskSize)); >> + Status = AndroidBootImgSetProperty64 ( >> + UpdatedFdtBase, >> + ChosenNode, >> + "linux,initrd-end", >> + (UINTN)((UINT8*)RamdiskData + RamdiskSize) >> + ); >> if (EFI_ERROR (Status)) { >> goto Fdt_Exit; >> } >> -- >> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' >> > > >