From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 92F6B81F40 for ; Sun, 26 Feb 2017 23:06:20 -0800 (PST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga105.fm.intel.com with ESMTP; 26 Feb 2017 23:06:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,213,1484035200"; d="scan'208";a="69929698" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga005.fm.intel.com with ESMTP; 26 Feb 2017 23:06:20 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 26 Feb 2017 23:06:20 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.59]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.177]) with mapi id 14.03.0248.002; Mon, 27 Feb 2017 15:06:16 +0800 From: "Ni, Ruiyu" To: "Wu, Hao A" , "edk2-devel@lists.01.org" CC: "Carsey, Jaben" Thread-Topic: [PATCH v3 6/6] ShellPkg: Refine type cast for pointer subtraction Thread-Index: AQHSjxxsGjX9XqMH1kCwnFNz4CfgA6F8ULoA//+IQACAAJiK8A== Date: Mon, 27 Feb 2017 07:06:15 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5B8B4D5A@SHSMSX104.ccr.corp.intel.com> References: <1487995514-7628-1-git-send-email-hao.a.wu@intel.com> <1487995514-7628-7-git-send-email-hao.a.wu@intel.com> <734D49CCEBEEF84792F5B80ED585239D5B8B4808@SHSMSX104.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v3 6/6] ShellPkg: Refine type cast for pointer subtraction 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, 27 Feb 2017 07:06:20 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks/Ray > -----Original Message----- > From: Wu, Hao A > Sent: Monday, February 27, 2017 1:59 PM > To: Ni, Ruiyu ; edk2-devel@lists.01.org > Cc: Carsey, Jaben > Subject: RE: [PATCH v3 6/6] ShellPkg: Refine type cast for pointer subtra= ction >=20 > > -----Original Message----- > > From: Ni, Ruiyu > > Sent: Monday, February 27, 2017 1:10 PM > > To: Wu, Hao A; edk2-devel@lists.01.org > > Cc: Carsey, Jaben > > Subject: RE: [PATCH v3 6/6] ShellPkg: Refine type cast for pointer > > subtraction > > > > > > > > Thanks/Ray > > > > > -----Original Message----- > > > From: Wu, Hao A > > > Sent: Saturday, February 25, 2017 12:05 PM > > > To: edk2-devel@lists.01.org > > > Cc: Wu, Hao A ; Carsey, Jaben > > > ; Ni, Ruiyu > > > Subject: [PATCH v3 6/6] ShellPkg: Refine type cast for pointer > > > subtraction > > > > > > For pointer subtraction, the result is of type "ptrdiff_t". > > > According to the C11 standard (Committee Draft - April 12, 2011): > > > > > > "When two pointers are subtracted, both shall point to elements of > > > the same array object, or one past the last element of the array > > > object; the result is the difference of the subscripts of the two > > > array elements. The size of the result is implementation-defined, > > > and its type (a signed integer type) is ptrdiff_t defined in the > > > header. If the result is not representable in an object > > > of that type, the behavior is undefined." > > > > > > In our codes, there are cases that the pointer subtraction is not > > > performed by pointers to elements of the same array object. This > > > might lead to potential issues, since the behavior is undefined > > > according to C11 standard. > > > > > > Also, since the size of type "ptrdiff_t" is implementation-defined. > > > Some static code checkers may warn that the pointer subtraction > > > might underflow first and then being cast to a bigger size. For examp= le: > > > > > > UINT8 *Ptr1, *Ptr2; > > > UINTN PtrDiff; > > > ... > > > PtrDiff =3D (UINTN) (Ptr1 - Ptr2); > > > > > > The commit will refine the pointer subtraction expressions by > > > casting each pointer to UINTN first and then perform the subtraction: > > > > > > PtrDiff =3D (UINTN) Ptr1 - (UINTN) Ptr2; > > > > Is this the new coding rule? I think the below code is not very readabl= e: > > Diff =3D ((UINTN) Ptr1 - (UINTN) Ptr2) / sizeof (STRUCTURE)) Do we have > > better fix? > > >=20 > Hi Ray, >=20 > Most of the pointer subtraction expressions in the code base are not > performed between two pointers to elements of the same array object, or > one past the last element of the array object (according to C11 standard)= . > Although the refined code is less readable, but I think the refine stick = with > the standard better. >=20 > The number of pointer subtractions used in the edk2 code base is not very > large. So the series does not touch lots of files. Also, we may take this= as part > of the coding style when dealing with pointer subtractions. Thanks for the explanation. If the number of cases is small, I am fine! >=20 > Best Regards, > Hao Wu >=20 > > > > > > Cc: Jaben Carsey > > > Cc: Ruiyu Ni > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > Signed-off-by: Hao Wu > > > Acked-by: Laszlo Ersek > > > --- > > > ShellPkg/Application/Shell/ShellParametersProtocol.c = | 6 +++--- > > > ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c = | 4 > ++-- > > > > ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosView.c > > > | 4 ++-- > > > ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c = | 4 ++-- > > > ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c = | 4 ++-- > > > 5 files changed, 11 insertions(+), 11 deletions(-) > > > > > > diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c > > > b/ShellPkg/Application/Shell/ShellParametersProtocol.c > > > index 4999155..8d76fb4 100644 > > > --- a/ShellPkg/Application/Shell/ShellParametersProtocol.c > > > +++ b/ShellPkg/Application/Shell/ShellParametersProtocol.c > > > @@ -5,7 +5,7 @@ > > > (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> > > Copyright (C) 2014, Red Hat, Inc. > > > (C) Copyright 2013 Hewlett-Packard Development Company, L.P.
> > > - Copyright (c) 2009 - 2016, Intel Corporation. All rights > > > reserved.
> > > + Copyright (c) 2009 - 2017, Intel Corporation. All rights > > > + reserved.
> > > This program and the accompanying materials > > > are licensed and made available under the terms and conditions of > > > the BSD License > > > which accompanies this distribution. The full text of the > > > license may be found at @@ -1034,9 +1034,9 @@ > > > UpdateStdInStdOutStdErr( > > > StrnCpyS(CommandLineCopy, > > > StrSize(CommandLineCopy)/sizeof(CHAR16), > > > NewCommandLine, StrLen(NewCommandLine)); > > > > > > if (FirstLocation !=3D CommandLineCopy + StrLen(CommandLineCopy) > > > - && ((UINTN)(FirstLocation - CommandLineCopy) < > > > StrLen(NewCommandLine)) > > > + && (((UINTN)FirstLocation - > > > + (UINTN)CommandLineCopy)/sizeof(CHAR16) > > > < StrLen(NewCommandLine)) > > > ){ > > > - *(NewCommandLine + (UINTN)(FirstLocation - CommandLineCopy)) =3D > > > CHAR_NULL; > > > + *(NewCommandLine + ((UINTN)FirstLocation - > > > (UINTN)CommandLineCopy)/sizeof(CHAR16)) =3D CHAR_NULL; > > > } > > > > > > if (!EFI_ERROR(Status)) { > > > diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c > > > b/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c > > > index bb2c0b9..18f15fa 100644 > > > --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c > > > +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c > > > @@ -2,7 +2,7 @@ > > > Main file for DmpStore shell Debug1 function. > > > > > > (C) Copyright 2013-2015 Hewlett-Packard Development Company, > > > L.P.
> > > - Copyright (c) 2005 - 2016, Intel Corporation. All rights > > > reserved.
> > > + Copyright (c) 2005 - 2017, Intel Corporation. All rights > > > + reserved.
> > > This program and the accompanying materials > > > are licensed and made available under the terms and conditions of > > > the BSD License > > > which accompanies this distribution. The full text of the > > > license may be found at @@ -364,7 +364,7 @@ > > > AppendSingleVariableToFile ( > > > // > > > // Crc32 > > > // > > > - gBS->CalculateCrc32 (Buffer, (UINTN) (Ptr - Buffer), (UINT32 *) > > > Ptr); > > > + gBS->CalculateCrc32 (Buffer, (UINTN) Ptr - (UINTN) Buffer, > > > + (UINT32 *) Ptr); > > > > > > Status =3D ShellWriteFile (FileHandle, &BufferSize, Buffer); > > > FreePool (Buffer); > > > diff --git > > > > a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosView > > > .c > > > > b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosView > > > .c > > > index 56b682a..a5b16fe 100644 > > > --- > > > > a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosView > > > .c > > > +++ > > > > b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosView > > > .c > > > @@ -2,7 +2,7 @@ > > > Tools of clarify the content of the smbios table. > > > > > > (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
> > > - Copyright (c) 2005 - 2012, Intel Corporation. All rights > > > reserved.
> > > + Copyright (c) 2005 - 2017, Intel Corporation. All rights > > > + reserved.
> > > This program and the accompanying materials > > > are licensed and made available under the terms and conditions of > > > the BSD License > > > which accompanies this distribution. The full text of the > > > license may be found at @@ -700,7 +700,7 @@ > > > CalculateSmbios64BitStructureCountAndLength ( > > > // > > > // Length =3D Next structure head - this structure head > > > // > > > - (*Smbios64TableLength) +=3D (UINTN) (Smbios.Raw - Raw); > > > + (*Smbios64TableLength) +=3D ((UINTN) Smbios.Raw - (UINTN) Raw); > > > if ((*Smbios64TableLength) > Smbios64EntryPoint- > >TableMaximumSize) { > > > // > > > // The actual table length exceeds maximum table size, diff > > > --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c > > > b/ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c > > > index d17a29d..0bcee92 100644 > > > --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c > > > +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c > > > @@ -75,7 +75,7 @@ IsValidGuidString( > > > ) { > > > Walker++; > > > } else { > > > - if (*Walker =3D=3D L'-' && (UINTN)(Walker - PrevWalker) =3D=3D > > > mGuidDataLen[Index]) { > > > + if (*Walker =3D=3D L'-' && (((UINTN)Walker - (UINTN)PrevWalker= ) / > > > + sizeof > > > (CHAR16)) =3D=3D mGuidDataLen[Index]) { > > > Walker++; > > > PrevWalker =3D Walker; > > > Index++; > > > @@ -85,7 +85,7 @@ IsValidGuidString( > > > } > > > } > > > > > > - if ((UINTN)(Walker - PrevWalker) =3D=3D mGuidDataLen[Index]) { > > > + if ((((UINTN)Walker - (UINTN)PrevWalker) / sizeof (CHAR16)) =3D=3D > > > mGuidDataLen[Index]) { > > > return TRUE; > > > } else { > > > return FALSE; > > > diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c > > > b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c > > > index dd4a740..9ae8176 100644 > > > --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c > > > +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c > > > @@ -99,10 +99,10 @@ IsCurrentFileSystem ( > > > > > > Splitter2 =3D StrStr (Cwd, L":"); > > > > > > - if ((UINTN) (Splitter1 - FullPath) !=3D (UINTN) (Splitter2 - Cwd)) > > > { > > > + if (((UINTN) Splitter1 - (UINTN) FullPath) !=3D ((UINTN) Splitter2 > > > + - (UINTN) > > > Cwd)) { > > > return FALSE; > > > } else { > > > - if (StrniCmp (FullPath, Cwd, (UINTN) (Splitter1 - FullPath)) =3D= =3D NULL) { > > > + if (StrniCmp (FullPath, Cwd, ((UINTN) Splitter1 - (UINTN) > > > + FullPath) / sizeof > > > (CHAR16)) =3D=3D NULL) { > > > return TRUE; > > > } else { > > > return FALSE; > > > -- > > > 1.9.5.msysgit.0