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: 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?

> 
> 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