On Tue, 10 Nov 2015, Serguey Parkhomovsky wrote:
> Some of the comparator functions in gprof(1) have incompatible pointer
> types and generate compiler warnings. The following diff fixes the
> problem.
Your diff is a step forward, yes. I think it's slightly improved to add
more 'const's so that the casts in topcmp(), timecmp(), and namecmp() can
be eliminated, ala:
+ const nltype * const *npp1 = v1;
+ const nltype * const *npp2 = v2;
Beyond that, looking at those compare functions I see at least two
additional problems.
The underlying requirement is that the comparison function provided to
qsort() must be _consistent_. This means that compare(A,B) and
compare(B,A) must either both be zero or have opposite signs. If that's
not true, then qsort()'s behavior isn't defined: some versions have
segfaulted or accessed beyond the ends of the provided array when that
happens!
So, given that understanding:
1) topcmp() assumes the subtraction can't overflow; if it does then it's
straight up undefined behavior (signed overflow). If your system
defined that as 2's-complement wraparound, then it's still an
inconsistent comparison!
2) totalcmp(A,B) and totalcmp(B,A) both return <0 if both A and B have
name==0 and cycleno!=0, and they both return >0 if both A and B have
naem==0 and cycleno==0, violating the consistency requirement.
Diff below extends yours to fix those as well. Does this still work for
you?
Anyone care to review my changes here to verify I got the directions on
the comparisons to match the original code?
Philip
Index: arcs.c
===================================================================
RCS file: /data/src/openbsd/src/usr.bin/gprof/arcs.c,v
retrieving revision 1.13
diff -u -p -r1.13 arcs.c
--- arcs.c 20 Aug 2015 22:32:41 -0000 1.13
+++ arcs.c 15 Nov 2015 06:51:44 -0000
@@ -95,9 +95,14 @@ addarc(nltype *parentp, nltype *childp,
nltype **topsortnlp;
int
-topcmp(nltype **npp1, nltype **npp2)
+topcmp(const void *v1, const void *v2)
{
- return (*npp1) -> toporder - (*npp2) -> toporder;
+ const nltype * const *npp1 = v1;
+ const nltype * const *npp2 = v2;
+
+ if ((*npp1) -> toporder < (*npp2) -> toporder)
+ return -1;
+ return (*npp1) -> toporder > (*npp2) -> toporder;
}
nltype **
Index: gprof.h
===================================================================
RCS file: /data/src/openbsd/src/usr.bin/gprof/gprof.h,v
retrieving revision 1.14
diff -u -p -r1.14 gprof.h
--- gprof.h 19 Oct 2013 13:51:40 -0000 1.14
+++ gprof.h 15 Nov 2015 06:25:33 -0000
@@ -281,10 +281,10 @@ void sortchildren(nltype *);
void sortmembers(nltype *);
void sortparents(nltype *);
void tally(struct rawarc *);
-int timecmp(nltype **, nltype **);
+int timecmp(const void *, const void *);
void timepropagate(nltype *);
-int topcmp(nltype **, nltype **);
-int totalcmp(nltype **, nltype **);
+int topcmp(const void *, const void *);
+int totalcmp(const void *, const void *);
#define LESSTHAN -1
#define EQUALTO 0
Index: printgprof.c
===================================================================
RCS file: /data/src/openbsd/src/usr.bin/gprof/printgprof.c,v
retrieving revision 1.13
diff -u -p -r1.13 printgprof.c
--- printgprof.c 20 Aug 2015 22:32:41 -0000 1.13
+++ printgprof.c 15 Nov 2015 06:52:09 -0000
@@ -35,7 +35,7 @@
#include "gprof.h"
#include "pathnames.h"
-int namecmp(nltype **, nltype **);
+int namecmp(const void *, const void *);
void
printprof()
@@ -66,21 +66,19 @@ printprof()
}
int
-timecmp(nltype **npp1, nltype **npp2)
+timecmp(const void *v1, const void *v2)
{
- double timediff;
- long calldiff;
+ const nltype * const *npp1 = v1;
+ const nltype * const *npp2 = v2;
- timediff = (*npp2) -> time - (*npp1) -> time;
- if ( timediff > 0.0 )
+ if ((*npp2) -> time < (*npp1) -> time)
+ return -1;
+ if ((*npp2) -> time > (*npp1) -> time)
return 1 ;
- if ( timediff < 0.0 )
+ if ((*npp2) -> ncall < (*npp1) -> ncall)
return -1;
- calldiff = (*npp2) -> ncall - (*npp1) -> ncall;
- if ( calldiff > 0 )
+ if ((*npp2) -> ncall > (*npp1) -> ncall)
return 1;
- if ( calldiff < 0 )
- return -1;
return( strcmp( (*npp1) -> name , (*npp2) -> name ) );
}
@@ -233,26 +231,32 @@ printgprof(nltype **timesortnlp)
* all else being equal, sort by names.
*/
int
-totalcmp(nltype **npp1, nltype **npp2)
+totalcmp(const void *v1, const void *v2)
{
- nltype *np1 = *npp1;
- nltype *np2 = *npp2;
- double diff;
-
- diff = ( np1 -> propself + np1 -> propchild )
- - ( np2 -> propself + np2 -> propchild );
- if ( diff < 0.0 )
+ const nltype *np1 = *(const nltype **)v1;
+ const nltype *np2 = *(const nltype **)v2;
+ double t1, t2;
+
+ t1 = np1 -> propself + np1 -> propchild;
+ t2 = np2 -> propself + np2 -> propchild;
+ if (t2 > t1)
return 1;
- if ( diff > 0.0 )
+ if (t2 < t1)
return -1;
- if ( np1 -> name == 0 && np1 -> cycleno != 0 )
- return -1;
- if ( np2 -> name == 0 && np2 -> cycleno != 0 )
- return 1;
- if ( np1 -> name == 0 )
+
+ /* have to be careful to keep the sort consistent */
+ if ( np1 -> name == 0 && np1 -> cycleno != 0 ) {
+ if ( np2 -> name != 0 || np2 -> cycleno == 0 )
+ return 1;
+ } else if ( np2 -> name != 0 || np2 -> cycleno == 0 )
return -1;
- if ( np2 -> name == 0 )
+
+ if ( np1 -> name == 0 ) {
+ if ( np2 -> name != 0 )
+ return -1;
+ } else if ( np2 -> name == 0 )
return 1;
+
if ( *(np1 -> name) != '_' && *(np2 -> name) == '_' )
return -1;
if ( *(np1 -> name) == '_' && *(np2 -> name) != '_' )
@@ -642,8 +646,11 @@ printblurb(const char *blurbname)
}
int
-namecmp(nltype **npp1, nltype **npp2)
+namecmp(const void *v1, const void *v2)
{
+ const nltype * const *npp1 = v1;
+ const nltype * const *npp2 = v2;
+
return( strcmp( (*npp1) -> name , (*npp2) -> name ) );
}
