Re: [PATCH v3 6/6] ShellPkg: Refine type cast for pointer subtraction

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Thanks/Ray

> -----Original Message-----
> From: Wu, Hao A
> Sent: Monday, February 27, 2017 1:59 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>
> Subject: RE: [PATCH v3 6/6] ShellPkg: Refine type cast for pointer subtraction
> 
> > -----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 <hao.a.wu@intel.com>; Carsey, Jaben
> > > <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > > 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
> > > <stddef.h> 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 example:
> > >
> > > UINT8  *Ptr1, *Ptr2;
> > > UINTN  PtrDiff;
> > > ...
> > > PtrDiff = (UINTN) (Ptr1 - Ptr2);
> > >
> > > The commit will refine the pointer subtraction expressions by
> > > casting each pointer to UINTN first and then perform the subtraction:
> > >
> > > PtrDiff = (UINTN) Ptr1 - (UINTN) Ptr2;
> >
> > Is this the new coding rule? I think the below code is not very readable:
> > Diff = ((UINTN) Ptr1 - (UINTN) Ptr2) / sizeof (STRUCTURE)) Do we have
> > better fix?
> >
> 
> Hi Ray,
> 
> 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.
> 
> 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!

> 
> Best Regards,
> Hao Wu
> 
> > >
> > > Cc: Jaben Carsey <jaben.carsey@intel.com>
> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> > > Acked-by: Laszlo Ersek <lersek@redhat.com>
> > > ---
> > >  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<BR>
> > >    Copyright (C) 2014, Red Hat, Inc.
> > >    (C) Copyright 2013 Hewlett-Packard Development Company, L.P.<BR>
> > > -  Copyright (c) 2009 - 2016, Intel Corporation. All rights
> > > reserved.<BR>
> > > +  Copyright (c) 2009 - 2017, Intel Corporation. All rights
> > > + reserved.<BR>
> > >    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 != CommandLineCopy + StrLen(CommandLineCopy)
> > > -    && ((UINTN)(FirstLocation - CommandLineCopy) <
> > > StrLen(NewCommandLine))
> > > +    && (((UINTN)FirstLocation -
> > > + (UINTN)CommandLineCopy)/sizeof(CHAR16)
> > > < StrLen(NewCommandLine))
> > >      ){
> > > -    *(NewCommandLine + (UINTN)(FirstLocation - CommandLineCopy)) =
> > > CHAR_NULL;
> > > +    *(NewCommandLine + ((UINTN)FirstLocation -
> > > (UINTN)CommandLineCopy)/sizeof(CHAR16)) = 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.<BR>
> > > -  Copyright (c) 2005 - 2016, Intel Corporation. All rights
> > > reserved.<BR>
> > > +  Copyright (c) 2005 - 2017, Intel Corporation. All rights
> > > + reserved.<BR>
> > >    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 = 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.<BR>
> > > -  Copyright (c) 2005 - 2012, Intel Corporation. All rights
> > > reserved.<BR>
> > > +  Copyright (c) 2005 - 2017, Intel Corporation. All rights
> > > + reserved.<BR>
> > >    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 = Next structure head - this structure head
> > >      //
> > > -    (*Smbios64TableLength) += (UINTN) (Smbios.Raw - Raw);
> > > +    (*Smbios64TableLength) += ((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 == L'-' && (UINTN)(Walker - PrevWalker) ==
> > > mGuidDataLen[Index]) {
> > > +      if (*Walker == L'-' && (((UINTN)Walker - (UINTN)PrevWalker) /
> > > + sizeof
> > > (CHAR16)) == mGuidDataLen[Index]) {
> > >          Walker++;
> > >          PrevWalker = Walker;
> > >          Index++;
> > > @@ -85,7 +85,7 @@ IsValidGuidString(
> > >      }
> > >    }
> > >
> > > -  if ((UINTN)(Walker - PrevWalker) == mGuidDataLen[Index]) {
> > > +  if ((((UINTN)Walker - (UINTN)PrevWalker) / sizeof (CHAR16)) ==
> > > 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 = StrStr (Cwd, L":");
> > >
> > > -  if ((UINTN) (Splitter1 - FullPath) != (UINTN) (Splitter2 - Cwd))
> > > {
> > > +  if (((UINTN) Splitter1 - (UINTN) FullPath) != ((UINTN) Splitter2
> > > + - (UINTN)
> > > Cwd)) {
> > >      return FALSE;
> > >    } else {
> > > -    if (StrniCmp (FullPath, Cwd, (UINTN) (Splitter1 - FullPath)) == NULL) {
> > > +    if (StrniCmp (FullPath, Cwd, ((UINTN) Splitter1 - (UINTN)
> > > + FullPath) / sizeof
> > > (CHAR16)) == NULL) {
> > >        return TRUE;
> > >      } else {
> > >        return FALSE;
> > > --
> > > 1.9.5.msysgit.0