Hi, Jaben! The function PerformQuickSort (in MdeModulePkg\Include\Library\SortLib.h) is correct, since both PerformQuickSort and QuickSortWorker use the same function type for their comparison function pointer.
Kind regards, Arthur. -----Original Message----- From: Carsey, Jaben [mailto:jaben.car...@intel.com] Sent: terça-feira, 19 de janeiro de 2016 16:53 To: Burigo, Arthur Crippa; Long, Qin; Laszlo Ersek; edk2-de...@ml01.01.org Cc: Carsey, Jaben Subject: RE: [edk2] [PATCH] CryptoPkg: Fix function qsort for non 32-bit machines Shouldn't we also update the MdeModulePkg header and implementation where that is copied from if we are changing the way it works? Sidenote: why does CryptoPkg carry a separate quick sort implementation? Is it worth just using the MdeModulePkg BaseSortLib? That library used to be in ShellPkg so that may have been the reason... -Jaben > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Burigo, Arthur Crippa > Sent: Tuesday, January 19, 2016 10:11 AM > To: Long, Qin <qin.l...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > edk2-de...@ml01.01.org > Subject: Re: [edk2] [PATCH] CryptoPkg: Fix function qsort for non > 32-bit machines > > Hello! > > Unless it is guaranteed an "int" will always have exactly 32-bits, using > "INT32" > instead of "int" does not guarantee the algorithm will work for > machines where an "int" is not a 32-bit integer. > > Thanks for your attention! > > Kind regards, > Arthur. > > -----Original Message----- > From: Long, Qin [mailto:qin.l...@intel.com] > Sent: terça-feira, 19 de janeiro de 2016 15:38 > To: Laszlo Ersek; Burigo, Arthur Crippa; edk2-de...@ml01.01.org > Subject: RE: [edk2] [PATCH] CryptoPkg: Fix function qsort for non > 32-bit machines > > Thanks for the detailed analysis. The fix makes sense to me. > And yes, I also prefer to use INT32 as Laszlo's comment (since we > don't consider ILP64 data model), to keep the consistent style in this > function declaration. > > > Best Regards & Thanks, > LONG, Qin > > > -----Original Message----- > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > Sent: Wednesday, January 20, 2016 1:10 AM > > To: Burigo, Arthur Crippa; edk2-de...@ml01.01.org > > Cc: Long, Qin > > Subject: Re: [edk2] [PATCH] CryptoPkg: Fix function qsort for non > > 32-bit machines > > > > On 01/19/16 16:13, Burigo, Arthur Crippa wrote: > > > Although the function qsort receives as an argument a "compare" > > > function which returns an "int", QuickSortWorker (the function > > > used internally by qsort to do its job) receives as an argument a > > > "CompareFunction" which returns an "INTN". In a 32-bit machine, > > > "INTN" is defined as "INT32", which is defined as "int" and > > > everything works well. However, when qsort is compiled for a > > > 64-bit machine, "INTN" is defined as "INT64" and the return values > > > of the compare functions become incompatible ("int" for qsort and > > > "INT64" for > QuickSortWorker), causing malfunction. > > > > > > For example, let's assume qsort is being compiled for a 64-bit machine. > > > As stated before, the "compare" function will be returning an > > > "int", and "CompareFunction" will be returning an "INT64". When, > > > for example, the "compare" function (which was passed as an > > > argument to qsort and, then, re-passed as an argument to > > > QuickSortWorker) returns -1 (or 0xffffffff, in a 32-bit integer, > > > its original return > > > type) from inside a call to QuickSortWorker, its return value is > > > interpreted > as being an "INT64" > > > value - which turns out to be 4294967295 (or 0x00000000ffffffff, > > > in a 64-bit integer) -, making the function QuickSortWorker to > > > behave unexpectedly. > > > > > > Note that this unexpected (or incorrect) conversion does not > > > happen when casting an "INT32" to an "INT64" directly, but does > > > happen when casting function types. > > > > > > The issue is fixed by changing the return type of SORT_COMPARE > > > (the type of "CompareFunction", used by QuickSortWorker) from > > > "INTN" to > "int". > > > This way, both qsort and QuickSortWorker use compatible > > > definitions for their compare functions. > > > > > > Cc: Qin Long <qin.l...@intel.com> > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > Acked-by: Paulo Alcantara Cavalcanti <paulo.alc.cavalca...@hp.com> > > > Signed-off-by: Karyne Mayer <kma...@hp.com> > > > Signed-off-by: Rodrigo Dias Correa <rodrigo.dia.cor...@hp.com> > > > Signed-off-by: Arthur Crippa Burigo <a...@hp.com> > > > --- > > > CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c > > > b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c > > > index fb446b6..f2c8987 100644 > > > --- a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c > > > +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c > > > @@ -22,7 +22,7 @@ FILE *stdin = NULL; FILE *stdout = NULL; > > > > > > typedef > > > -INTN > > > +int > > > (*SORT_COMPARE)( > > > IN VOID *Buffer1, > > > IN VOID *Buffer2 > > > > > > > INT32 is more idiomatic for edk2, I think. > > > > Thanks > > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
