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 A4DC221E87979 for ; Tue, 12 Sep 2017 02:52:58 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CB73780F6D; Tue, 12 Sep 2017 09:55:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CB73780F6D Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-50.rdu2.redhat.com [10.10.120.50]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4F59D65E9E; Tue, 12 Sep 2017 09:55:53 +0000 (UTC) To: "Zeng, Star" , edk2-devel-01 Cc: Ard Biesheuvel , "Dong, Eric" , Paulo Alcantara , "Ni, Ruiyu" References: <20170910001304.8628-1-lersek@redhat.com> <20170910001304.8628-4-lersek@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9400FA@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Tue, 12 Sep 2017 11:55:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B9400FA@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 12 Sep 2017 09:55:54 +0000 (UTC) Subject: Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Sep 2017 09:52:58 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/12/17 10:55, Zeng, Star wrote: > Reviewed-by: Star Zeng > > BTW: How about to use "sizeof ()" instead of "sizeof"? "sizeof" is a unary operator. The parentheses are mandatory when sizeof is used with a type name, but when the operand of sizeof is an expression, the parentheses frequently make the wrong impression. Consider for example UINTN Var1, Var2, Var3; STRUCTURE_TYPE VarStruct; Var1 = 5; Var2 = sizeof Var3 + Var1; // [1] Most people dislike the assignment to Var2, and will rewrite it as: Var2 = sizeof (Var3) + Var1; // [2] However, this is totally irrelevant, and does not reflect the binding strengths between the "sizeof" and "+" operators. In order to reflect that relationship correctly, the following parentheses should be added: Var2 = (sizeof Var3) + Var1; // [3] None of [2] or [3] make any difference relative to [1], of course, in behavior. I just dislike [2] because it *suggests* that putting Var3 in parens "protects" Var1 from falling under "sizeof". That's not the case. "sizeof" is not a function, it is a unary operator like "*", "&", "!", etc. If we want to add parens for emphasizing the actual operator binding, then [3] is the way to go. For example, if "Var3" was a pointer, you wouldn't write *(Var3) + Var1 in order to emphasize that "*" takes precedence over "+"; you would write (*Var3) + Var1 TL;DR: "sizeof" is a unary, prefix operator, not a function designator. ... In order not to delay this any longer, I will add the parens before I push. But, we should all know that those parens don't mean what most people think they mean :) Laszlo > > > Thanks, > Star > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Sunday, September 10, 2017 8:13 AM > To: edk2-devel-01 > Cc: Ard Biesheuvel ; Dong, Eric ; Paulo Alcantara ; Ni, Ruiyu ; Zeng, Star > Subject: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() > > In edk2, initialization of local variables is forbidden, both for stylistic reasons and because such initialization may generate calls to compiler intrinsics. > > For the following initialization in UdfRead(): > > CHAR16 FileName[UDF_FILENAME_LENGTH] = { 0 }; > > clang-3.8 generates a memset() call, when building UdfDxe for IA32, which then fails to link. > > Replace the initialization with ZeroMem(). > > Do the same to "FilePath" in UdfOpen(). > > Cc: Ard Biesheuvel > Cc: Eric Dong > Cc: Paulo Alcantara > Cc: Ruiyu Ni > Cc: Star Zeng > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c > index 8b9339567f8e..e7159ff861f7 100644 > --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c > +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c > @@ -174,15 +174,16 @@ UdfOpen ( > { > EFI_TPL OldTpl; > EFI_STATUS Status; > PRIVATE_UDF_FILE_DATA *PrivFileData; > PRIVATE_UDF_SIMPLE_FS_DATA *PrivFsData; > - CHAR16 FilePath[UDF_PATH_LENGTH] = { 0 }; > + CHAR16 FilePath[UDF_PATH_LENGTH]; > UDF_FILE_INFO File; > PRIVATE_UDF_FILE_DATA *NewPrivFileData; > CHAR16 *TempFileName; > > + ZeroMem (FilePath, sizeof FilePath); > OldTpl = gBS->RaiseTPL (TPL_CALLBACK); > > if (This == NULL || NewHandle == NULL || FileName == NULL) { > Status = EFI_INVALID_PARAMETER; > goto Error_Invalid_Params; > @@ -322,14 +323,15 @@ UdfRead ( > EFI_BLOCK_IO_PROTOCOL *BlockIo; > EFI_DISK_IO_PROTOCOL *DiskIo; > UDF_FILE_INFO FoundFile; > UDF_FILE_IDENTIFIER_DESCRIPTOR *NewFileIdentifierDesc; > VOID *NewFileEntryData; > - CHAR16 FileName[UDF_FILENAME_LENGTH] = { 0 }; > + CHAR16 FileName[UDF_FILENAME_LENGTH]; > UINT64 FileSize; > UINT64 BufferSizeUint64; > > + ZeroMem (FileName, sizeof FileName); > OldTpl = gBS->RaiseTPL (TPL_CALLBACK); > > if (This == NULL || BufferSize == NULL || (*BufferSize != 0 && > Buffer == NULL)) { > Status = EFI_INVALID_PARAMETER; > -- > 2.14.1.3.gb7cf6e02401b > >