From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web10.103418.1674830229104864226 for ; Fri, 27 Jan 2023 06:37:09 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=fvJTg+wx; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 52A4C2407DB for ; Fri, 27 Jan 2023 15:37:07 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1674830227; bh=tQ/sgug4IlGVzFSYpUyvXR0ceUfRK12eIQUCxKcFmLg=; h=From:Subject:Date:Cc:To:From; b=fvJTg+wxnKv6nq+6owcC1CS4o3iomRwUvHLyC+ZQ01XREN/w8x1CRlMdCGWYp8x0/ mOjWrQopzGVcsNbgSBzEXXsK5RX8qMP+wzQF71myQcpswna3pZowCunnDn2XUVWyrx 4ysRaYLAZGbXYX40Ykk1sH/PyBkZ0XIhH9CMF6tIhjYn+PYnTic3C7xX3HZMFC86ik rpCvpFmCpkH8zlFVim7syAvdyLLLyT35yP4GW2/t9hci4c/HqRXfvTs4K36ZFspdcz 4W6NwazpBvGr6cT12GmwU/MbJ3lUTGIr1uDBU/c5cJMV/r4SJG09qS6w9Rs+5nllZ5 hVXNW+B7GucEw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4P3KsZ437Mz6tmh; Fri, 27 Jan 2023 15:37:06 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-Id: Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\)) Subject: Re: [edk2-platforms][PATCH v3 10/11] Ext4Pkg: Fixes build on MSVC Date: Fri, 27 Jan 2023 14:36:56 +0000 In-Reply-To: Cc: Savva Mitrofanov , devel@edk2.groups.io, Vitaly Cheptsov To: Pedro Falcato References: <20230127092945.94389-1-savvamtr@gmail.com> <20230127092945.94389-11-savvamtr@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_C7AD87E4-472F-4BE1-BA78-A0A8BC3419C7" --Apple-Mail=_C7AD87E4-472F-4BE1-BA78-A0A8BC3419C7 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 On 27. Jan 2023, at 15:33, Pedro Falcato = wrote: >=20 > On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov > wrote: >>=20 >> Accessing array using index of uint64 type makes MSVC compiler to >> include `__allmul` function in NOOPT which is not referenced in IA32. >> So we null-terminates string using ReadSize, which should be equal to >> SymlinkSizeTmp after correct reading. Also adds missing MultU64x32 >> in Ext4Read. >>=20 >> Cc: Marvin H=C3=A4user >> Cc: Pedro Falcato >> Cc: Vitaly Cheptsov >> Fixes: 7c46116b0e18 ("Ext4Pkg: Add ext2/3 support") >> Fixes: e81432fbacb7 ("Ext4Pkg: Add symbolic links support") >> Signed-off-by: Savva Mitrofanov >> --- >> Features/Ext4Pkg/Ext4Dxe/Inode.c | 4 ++-- >> Features/Ext4Pkg/Ext4Dxe/Symlink.c | 12 ++++++------ >> 2 files changed, 8 insertions(+), 8 deletions(-) >>=20 >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c = b/Features/Ext4Pkg/Ext4Dxe/Inode.c >> index 90e3eb88f523..8db051d3c444 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c >> @@ -152,7 +152,7 @@ Ext4Read ( >> } else { >> // Uninitialized extents behave exactly the same as file = holes, except they have >> // blocks already allocated to them. >> - HoleLen =3D (Ext4GetExtentLength (&Extent) * = Partition->BlockSize) - HoleOff; >> + HoleLen =3D MultU64x32 (Ext4GetExtentLength (&Extent), = Partition->BlockSize) - HoleOff; >> } >>=20 >> WasRead =3D HoleLen > RemainingRead ? RemainingRead : = (UINTN)HoleLen; >> @@ -166,7 +166,7 @@ Ext4Read ( >> Partition->BlockSize >> ); >> ExtentLengthBytes =3D Extent.ee_len * Partition->BlockSize; >> - ExtentLogicalBytes =3D (UINT64)Extent.ee_block * = Partition->BlockSize; >> + ExtentLogicalBytes =3D MultU64x32 ((UINT64)Extent.ee_block, = Partition->BlockSize); >> ExtentOffset =3D CurrentSeek - ExtentLogicalBytes; >> ExtentMayRead =3D (UINTN)(ExtentLengthBytes - = ExtentOffset); >>=20 >> diff --git a/Features/Ext4Pkg/Ext4Dxe/Symlink.c = b/Features/Ext4Pkg/Ext4Dxe/Symlink.c >> index 19b357ac6ba0..8b1511a38b55 100644 >> --- a/Features/Ext4Pkg/Ext4Dxe/Symlink.c >> +++ b/Features/Ext4Pkg/Ext4Dxe/Symlink.c >> @@ -1,7 +1,7 @@ >> /** @file >> Symbolic links routines >>=20 >> - Copyright (c) 2022 Savva Mitrofanov All rights reserved. >> + Copyright (c) 2022-2023 Savva Mitrofanov All rights reserved. >> SPDX-License-Identifier: BSD-2-Clause-Patent >> **/ >>=20 >> @@ -155,11 +155,6 @@ Ext4ReadSlowSymlink ( >> return Status; >> } >>=20 >> - // >> - // Add null-terminator >> - // >> - SymlinkTmp[SymlinkSizeTmp] =3D '\0'; >> - >> if (SymlinkSizeTmp !=3D ReadSize) { >> DEBUG (( >> DEBUG_FS, >> @@ -168,6 +163,11 @@ Ext4ReadSlowSymlink ( >> return EFI_VOLUME_CORRUPTED; >> } >>=20 >> + // >> + // Add null-terminator >> + // > Sidenote: please don't use this comment style, I know it's prevalent > in EDK2 and EDK2 platforms but Ext4Pkg just does: > // Comment >=20 > Also, why are you bringing this null-termination down here? I don't think there's a strong functional reason, but it adheres to the = principle of not touching malformed data if at all possible. Not sure it = needs to be part of this particular commit, but why not, it causes less = noise this way. >> + SymlinkTmp[ReadSize] =3D '\0'; >> + >> *AsciiSymlinkSize =3D SymlinkAllocateSize; >> *AsciiSymlink =3D SymlinkTmp; >>=20 >> -- >> 2.39.0 >>=20 >=20 >=20 > --=20 > Pedro --Apple-Mail=_C7AD87E4-472F-4BE1-BA78-A0A8BC3419C7 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 On 27. Jan = 2023, at 15:33, Pedro Falcato <pedro.falcato@gmail.com> = wrote:

On Fri, Jan 27, 2023 at 9:29 AM Savva = Mitrofanov <savvamtr@gmail.com> = wrote:

Accessing = array using index of uint64 type makes MSVC compiler to
include = `__allmul` function in NOOPT which is not referenced in IA32.
So we = null-terminates string using ReadSize, which should be equal = to
SymlinkSizeTmp after correct reading. Also adds missing = MultU64x32
in Ext4Read.

Cc: Marvin H=C3=A4user = <mhaeuser@posteo.de>
Cc: Pedro Falcato = <pedro.falcato@gmail.com>
Cc: Vitaly Cheptsov = <vit9696@protonmail.com>
Fixes: 7c46116b0e18 ("Ext4Pkg: Add = ext2/3 support")
Fixes: e81432fbacb7 ("Ext4Pkg: Add symbolic links = support")
Signed-off-by: Savva Mitrofanov = <savvamtr@gmail.com>
---
Features/Ext4Pkg/Ext4Dxe/Inode.c =   |  4 ++--
Features/Ext4Pkg/Ext4Dxe/Symlink.c | 12 = ++++++------
2 files changed, 8 insertions(+), 8 = deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c = b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 90e3eb88f523..8db051d3c444 = 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ = b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -152,7 +152,7 @@ Ext4Read = (
      } else = {
        // Uninitialized = extents behave exactly the same as file holes, except they = have
        // blocks = already allocated to them.
- =        HoleLen =3D = (Ext4GetExtentLength (&Extent) * Partition->BlockSize) - = HoleOff;
+        HoleLen =3D = MultU64x32 (Ext4GetExtentLength (&Extent), Partition->BlockSize) = - = HoleOff;
      }

   = ;   WasRead =3D HoleLen > RemainingRead ? = RemainingRead : (UINTN)HoleLen;
@@ -166,7 +166,7 @@ Ext4Read = (
           &nb= sp;            = ;   Partition->BlockSize
    &nbs= p;            =           );
 &n= bsp;    ExtentLengthBytes  =3D Extent.ee_len * = Partition->BlockSize;
- =      ExtentLogicalBytes =3D = (UINT64)Extent.ee_block * Partition->BlockSize;
+ =      ExtentLogicalBytes =3D MultU64x32 = ((UINT64)Extent.ee_block, = Partition->BlockSize);
      ExtentOff= set       =3D CurrentSeek - = ExtentLogicalBytes;
      ExtentMayRead =      =3D (UINTN)(ExtentLengthBytes - = ExtentOffset);

diff --git a/Features/Ext4Pkg/Ext4Dxe/Symlink.c = b/Features/Ext4Pkg/Ext4Dxe/Symlink.c
index 19b357ac6ba0..8b1511a38b55 = 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Symlink.c
+++ = b/Features/Ext4Pkg/Ext4Dxe/Symlink.c
@@ -1,7 +1,7 @@
/** = @file
  Symbolic links routines

-  Copyright = (c) 2022 Savva Mitrofanov All rights reserved.
+  Copyright (c) = 2022-2023 Savva Mitrofanov All rights = reserved.
  SPDX-License-Identifier: = BSD-2-Clause-Patent
**/

@@ -155,11 +155,6 @@ = Ext4ReadSlowSymlink (
    return = Status;
  }

-  //
-  // Add = null-terminator
-  //
-  SymlinkTmp[SymlinkSizeTmp] =3D = '\0';
-
  if (SymlinkSizeTmp !=3D ReadSize) = {
    DEBUG = ((
      DEBUG_FS,
@@ -168,6 +163,11 = @@ Ext4ReadSlowSymlink (
    return = EFI_VOLUME_CORRUPTED;
  }

+  //
+  // = Add null-terminator
+  //
Sidenote: please don't use this comment = style, I know it's prevalent
in EDK2 = and EDK2 platforms but Ext4Pkg just does:
// = Comment

Also, why are you = bringing this null-termination down here?

I don't = think there's a strong functional reason, but it adheres to the = principle of not touching malformed data if at all possible. Not sure it = needs to be part of this particular commit, but why not, it causes less = noise this way.

+ =  SymlinkTmp[ReadSize] =3D = '\0';
+
  *AsciiSymlinkSize =3D = SymlinkAllocateSize;
  *AsciiSymlink =     =3D = SymlinkTmp;

--
2.39.0



-- 
Pedro

= --Apple-Mail=_C7AD87E4-472F-4BE1-BA78-A0A8BC3419C7--