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.web12.11766.1634396266376581896 for ; Sat, 16 Oct 2021 07:57:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=D9s/YaFJ; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 30181240104 for ; Sat, 16 Oct 2021 16:57:42 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1634396263; bh=lvqel3QkexqvmrNxe610/3KUQKts0AOdXQq3fmI0m8w=; h=Date:Subject:To:From:Cc:From; b=D9s/YaFJxy38Xq650joj81PHyCKghDavhHytQF0GQtl06wK2ATKZofXDsTcLjZMFn daJ5fMLxXFn0Whs+iLn8bujouh7jwMuYJPVQZuh7HV46n57hnbWXhLsokK9jlyAq/6 PfER3B2pVYVU3yaHZpsCj0tBCIY1LMrIUbAjGXIqv85IQUfl3B2yMJBHsxMrG79JKn pfcZZ8/fbbHLB/f546F4Gg2W1Xyq7nM6cAGvXhG55T5ZItOyLXRxdAAbuZEbnGJCCD s3kdGKbeeYWZpmRL8exAx17vU2Mi8FUdIWRWh1pk42u156kEkv+X5w1NxRfxLCDJDX r2vQ8ARQjTflQ== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4HWmTL0pWbz9rxL; Sat, 16 Oct 2021 16:57:41 +0200 (CEST) Message-ID: <87808d66-c8fb-46a2-4ee0-01253fca01b7@posteo.de> Date: Sat, 16 Oct 2021 14:57:41 +0000 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/CpuCacheInfoLib: Add QuickSort function on BaseLib To: devel@edk2.groups.io, ianx.kuo@intel.com References: <20211015232526.1839-1-ianx.kuo@intel.com> <20211015232526.1839-4-ianx.kuo@intel.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Cc: amy.chan@intel.com, ray.ni@intel.com, Eric Dong , Rahul Kumar In-Reply-To: <20211015232526.1839-4-ianx.kuo@intel.com> Content-Language: en-GB Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hey IanX, On 16.10.21 01:25, IanX Kuo wrote: > From: IanX Kuo > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3675 > > Remove MdeModulePkg dependency > > Cc: Eric Dong > Cc: Ray Ni > Cc: Rahul Kumar > Signed-off-by: IanX Kuo > --- > UefiCpuPkg/Library/CpuCacheInfoLib/CpuCacheInfoLib.c | 9 ++++++++- > .../Library/CpuCacheInfoLib/DxeCpuCacheInfoLib.inf | 2 -- > .../Library/CpuCacheInfoLib/InternalCpuCacheInfoLib.h | 1 - > .../Library/CpuCacheInfoLib/PeiCpuCacheInfoLib.inf | 2 -- > 4 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/UefiCpuPkg/Library/CpuCacheInfoLib/CpuCacheInfoLib.c b/UefiCpuPkg/Library/CpuCacheInfoLib/CpuCacheInfoLib.c > index c0077d6770..45c2ca13dc 100644 > --- a/UefiCpuPkg/Library/CpuCacheInfoLib/CpuCacheInfoLib.c > +++ b/UefiCpuPkg/Library/CpuCacheInfoLib/CpuCacheInfoLib.c > @@ -282,6 +282,7 @@ CpuCacheInfoCollectCpuCacheInfoData ( > UINTN LocalCacheInfoCount; > UINTN Index; > UINTN NextIndex; > + VOID *QuickSortBuffer; > > // > // Get number of Packages and Package ID. > @@ -369,7 +370,13 @@ CpuCacheInfoCollectCpuCacheInfoData ( > // > // Sort LocalCacheInfo array by CPU package ID, core type, cache level and cache type. > // > - PerformQuickSort (LocalCacheInfo, LocalCacheInfoCount, sizeof (*LocalCacheInfo), (SORT_COMPARE) CpuCacheInfoCompare); > + QuickSortBuffer = AllocateZeroPool (sizeof (*LocalCacheInfo)); > + ASSERT (QuickSortBuffer != NULL); Sorry, maybe I should have been more specific. Asserting on dynamic memory allocations is a bad idea (independently of whether it's otherwise handled or not) as this makes all kinds of analyses harder, for example fuzz-testing would just die with ASSERTs enabled, and static analysis may draw incorrect conclusions from this incorrect assertion. Disabling ASSERTs for analyses can work, but it may decrease their efficacy (e.g. fuzz-testing can no longer verify invariants this way, and static analysis may start emitting more False Positives). This is an edk2-wide problem that I hope to address with a new macro in the near future. For now, if it does not interrupt your testing much, I'd prefer dropping it. I still believe this data could just live on the stack, avoiding this issue entirely. > + if (QuickSortBuffer == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } Thanks! Best regards, Marvin > + > + QuickSort (LocalCacheInfo, LocalCacheInfoCount, sizeof (*LocalCacheInfo), CpuCacheInfoCompare, QuickSortBuffer); > CopyMem (CacheInfo, LocalCacheInfo, sizeof (*CacheInfo) * LocalCacheInfoCount); > DEBUG_CODE ( > CpuCacheInfoPrintCpuCacheInfoTable (CacheInfo, LocalCacheInfoCount); > diff --git a/UefiCpuPkg/Library/CpuCacheInfoLib/DxeCpuCacheInfoLib.inf b/UefiCpuPkg/Library/CpuCacheInfoLib/DxeCpuCacheInfoLib.inf > index c3d3f1e799..fdd79970f9 100644 > --- a/UefiCpuPkg/Library/CpuCacheInfoLib/DxeCpuCacheInfoLib.inf > +++ b/UefiCpuPkg/Library/CpuCacheInfoLib/DxeCpuCacheInfoLib.inf > @@ -25,7 +25,6 @@ > > [Packages] > MdePkg/MdePkg.dec > - MdeModulePkg/MdeModulePkg.dec > UefiCpuPkg/UefiCpuPkg.dec > > [LibraryClasses] > @@ -34,7 +33,6 @@ > BaseMemoryLib > MemoryAllocationLib > UefiBootServicesTableLib > - SortLib > > [Protocols] > gEfiMpServiceProtocolGuid > diff --git a/UefiCpuPkg/Library/CpuCacheInfoLib/InternalCpuCacheInfoLib.h b/UefiCpuPkg/Library/CpuCacheInfoLib/InternalCpuCacheInfoLib.h > index 26e1f46516..829a9f43ce 100644 > --- a/UefiCpuPkg/Library/CpuCacheInfoLib/InternalCpuCacheInfoLib.h > +++ b/UefiCpuPkg/Library/CpuCacheInfoLib/InternalCpuCacheInfoLib.h > @@ -17,7 +17,6 @@ > #include > #include > #include > -#include > #include > > typedef union { > diff --git a/UefiCpuPkg/Library/CpuCacheInfoLib/PeiCpuCacheInfoLib.inf b/UefiCpuPkg/Library/CpuCacheInfoLib/PeiCpuCacheInfoLib.inf > index 0864497849..c643fc89be 100644 > --- a/UefiCpuPkg/Library/CpuCacheInfoLib/PeiCpuCacheInfoLib.inf > +++ b/UefiCpuPkg/Library/CpuCacheInfoLib/PeiCpuCacheInfoLib.inf > @@ -25,7 +25,6 @@ > > [Packages] > MdePkg/MdePkg.dec > - MdeModulePkg/MdeModulePkg.dec > UefiCpuPkg/UefiCpuPkg.dec > > [LibraryClasses] > @@ -34,7 +33,6 @@ > BaseMemoryLib > MemoryAllocationLib > PeiServicesTablePointerLib > - SortLib > > [Ppis] > gEdkiiPeiMpServices2PpiGuid