From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 71D1781EDF for ; Mon, 23 Jan 2017 03:01:08 -0800 (PST) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 765E2C05683F; Mon, 23 Jan 2017 11:01:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-14.phx2.redhat.com [10.3.116.14]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v0NB17DG027417; Mon, 23 Jan 2017 06:01:07 -0500 To: Hao Wu , edk2-devel@ml01.01.org References: <1485067434-12064-1-git-send-email-hao.a.wu@intel.com> From: Laszlo Ersek Message-ID: <08dcec9c-48bf-1878-dcb9-60d28441064f@redhat.com> Date: Mon, 23 Jan 2017 12:01:05 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <1485067434-12064-1-git-send-email-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Mon, 23 Jan 2017 11:01:08 +0000 (UTC) Subject: Re: [PATCH 0/1] Refine casting expression result to bigger size X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Jan 2017 11:01:08 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 01/22/17 07:43, Hao Wu wrote: > Please note that this patch is maily for feedback collection and the patch > only covers MdePkg. We are working on patches for other packages. > > > There are cases that the operands of an expression are all with rank less > than UINT64/INT64 and the result of the expression is casted to > UINT64/INT64 to fit the target size. > > An example will be: > UINT32 a,b; > // a and b can be any unsigned int type with rank less than UINT64, like > // UINT8, UINT16, etc. > UINT64 c; > c = (UINT64) (a + b); > > Some static code checkers may warn that the expression result might > overflow within the rank of int (integer promotions) and the result is > then cast to a bigger size. > > For the consideration of generated binaries size, the commit will keep the > size of the operands as the size of int, and explitly add a type cast > before converting the result to UINT64/INT64. > > 1). When there is no operand with type UINTN > (UINTN) (a + b) -> (UINTN)(UINT32) (a + b) or > (UINT64) (a + b) -> (UINT64)(UINT32) (a + b) > > 2). Otherwise > (UINT64) (a + b) -> (UINT64)(UINTN) (a + b) > > Hao Wu (1): > MdePkg: Refine casting expression result to bigger size > > MdePkg/Library/BaseLib/String.c | 4 ++-- > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 4 ++-- > MdePkg/Library/BaseS3PciLib/S3PciLib.c | 4 ++-- > MdePkg/Library/SmmMemoryAllocationLib/MemoryAllocationLib.c | 4 ++-- > MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c | 4 ++-- > 5 files changed, 10 insertions(+), 10 deletions(-) > - If it is justified to do the addition in 64-bits (or in UINTN), then the addition should be modified accordingly. This might incur a code size increase, but if that's necessary for correctness, then it should be done. - However, if the addition is just fine as-is, because we know for sure that the sum will never overflow "int" or "unsigned int" (as selected by the integer promotions and the usual arithmetic conversions), then we're addressing the issue in the wrong direction. Namely, in this case, the solution is to simply drop the outermost cast, which is already useless. (Because, it would automatically happen as part of the assignment or the "return" statement.) I mean... Those (useless) outermost casts were probably introduced to appease the Visual Studio compiler. Apparently, they cause various static code checkers to complain, so now we introduce yet more casts to keep them quiet as well. When will it end? For example, the 2nd return statement of the InternalHexCharToUintn() function is proposed as return (UINTN)(UINT32) (10 + InternalCharToUpper (Char) - L'A'); in reality it should be just return 10 + InternalCharToUpper (Char) - L'A'; Thanks Laszlo