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:[email protected]] On Behalf Of > Burigo, Arthur Crippa > Sent: Tuesday, January 19, 2016 10:11 AM > To: Long, Qin <[email protected]>; Laszlo Ersek <[email protected]>; > [email protected] > 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:[email protected]] > Sent: terça-feira, 19 de janeiro de 2016 15:38 > To: Laszlo Ersek; Burigo, Arthur Crippa; [email protected] > 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:[email protected]] > > Sent: Wednesday, January 20, 2016 1:10 AM > > To: Burigo, Arthur Crippa; [email protected] > > 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 <[email protected]> > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > Acked-by: Paulo Alcantara Cavalcanti <[email protected]> > > > Signed-off-by: Karyne Mayer <[email protected]> > > > Signed-off-by: Rodrigo Dias Correa <[email protected]> > > > Signed-off-by: Arthur Crippa Burigo <[email protected]> > > > --- > > > 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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
