From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x229.google.com (mail-it0-x229.google.com [IPv6:2607:f8b0:4001:c0b::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 50E8B81EEA for ; Mon, 23 Jan 2017 12:58:10 -0800 (PST) Received: by mail-it0-x229.google.com with SMTP id r185so76151862ita.0 for ; Mon, 23 Jan 2017 12:58:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=XZFpeyhW8FUn4Qz321UynNxkW1PHY7ecSstaoIMoIp0=; b=WRgedmHA98kfxmX22NNbcdkwwUkRDPMbjGlwmFV+B6eRWYZ7fNE5BUrhLZ6CnBNRpJ Zx8wHKVwkJCWaWcOUh8eO5329OJt/biv78zIEGRFpvjFst06+UUEoAYxtN9B+vW25s7F nMcl1EI0x1p8tbMc7mTN6A94XZAIq4uWTgugM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=XZFpeyhW8FUn4Qz321UynNxkW1PHY7ecSstaoIMoIp0=; b=bAStb3T12m1itfhpdUNJE5+TbCazE1WtLeQX8qYYVtT5XAuKSc76mDzYu2eYCuHRT2 Gbfh8eV4Psa3p/hBQAWA5do0ExENKkXvdpHBH+3Exdk6VDncm/Fo5pGAYIMowiJR3l6/ kWPSdjNw8O44f/+aZYHQauUnZMjSbxioUPb/4QCr9LXY4aQ87jxXfIKylcDCfNRcQpcb 2poOQlPq/kEXL6o/Qga4GnyEjTYdCXtBZ9WUBjIah9qAGvKmXtoAK4RqlqqUtxiv3Wru Yto4NMZ7jHeoZiiIQCraCYbgjzIApx/BM4IF27yn1BU4G2kU4VPWIueK9vhdG/3eHBWC uAuw== X-Gm-Message-State: AIkVDXInELk0EIBkpGLPhlTfIWQYpTIaUnHUwQQiciRjxKtFUXECG5mT/H9cxSjPYcDE/FWfxEaqYzXRy111ccVq X-Received: by 10.36.74.67 with SMTP id k64mr16097905itb.37.1485205089567; Mon, 23 Jan 2017 12:58:09 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.144.135 with HTTP; Mon, 23 Jan 2017 12:58:09 -0800 (PST) In-Reply-To: <08dcec9c-48bf-1878-dcb9-60d28441064f@redhat.com> References: <1485067434-12064-1-git-send-email-hao.a.wu@intel.com> <08dcec9c-48bf-1878-dcb9-60d28441064f@redhat.com> From: Ard Biesheuvel Date: Mon, 23 Jan 2017 20:58:09 +0000 Message-ID: To: Laszlo Ersek Cc: Hao Wu , "edk2-devel@lists.01.org" 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 20:58:10 -0000 Content-Type: text/plain; charset=UTF-8 On 23 January 2017 at 11:01, Laszlo Ersek wrote: > 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. > Indeed. The problem with c = (UINT64) (a + b); is the parens: this should be written as (UINT64)a + b if the sum may overflow a UINT32 > - 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.) > Agreed. > 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'; > Indeed. IMO this is related to our warnings-as-errors policy, while in reality, some warnings are simply warnings, and should be ignored if it can be confirmed by visual inspection that the expression can never overflow.