Search code examples
cgccclangwarningssigned

Qualification of code - comparisons between signed and unsigned values in libc's qsort


While doing a verification pass over code that is safety-critical - and is supposed to be executed in an embedded device - I saw some warnings emitted by both GCC and Clang. The warnings are easy to reproduce, since the code in question comes from the open-source BSD libc:

$ wget https://raw.githubusercontent.com/freebsd/\
freebsd/af3e10e5a78d3af8cef6088748978c6c612757f0/lib/libc/stdlib/qsort.c

$ gcc -c -Wall -Wsign-compare -DI_AM_QSORT_R -Wall qsort.c

qsort.c: In function 'qsort_r':
qsort.c:45:24: warning: comparison between signed and unsigned 
               integer expressions [-Wsign-compare]

#define MIN(a, b) ((a) < (b) ? a : b)
                        ^
qsort.c:186:6: note: in expansion of macro 'MIN'
  r = MIN(pd - pc, pn - pd - es);
      ^
qsort.c:45:34: warning: signed and unsigned type in conditional 
               expression [-Wsign-compare]
 #define MIN(a, b) ((a) < (b) ? a : b)
                                  ^
qsort.c:186:6: note: in expansion of macro 'MIN'
  r = MIN(pd - pc, pn - pd - es);

To understand these warnings (emitted from both GCC and CLang)...

  • pd, pc and pn are pointers
  • es is size_t (that is, unsigned)

It could be argued that C's rules for handling comparisons between signed and unsigned entities can be adequately expressed as DON'T, FOR THE LOVE OF GOD [1].

But in this case, the BSD implementation of qsort compares...

  • the result of subtracting two pointers (which has a signed type, ptrdiff_t)
  • to the result of subtracting two pointers, reduced by es - which is unsigned.

Why does subtracting an unsigned value from a ptrdiff_t one (that is, a signed one) result in an unsigned one? That's something you can read about in the post referenced above. Suffice to say, that for word-sized entities, any expression of signed OPERATOR unsigned results in an unsigned type.

So, to cut a long story short, for GCC and CLang to stop complaining, the lines would have to be changed from...

r = MIN(pd - pc, pn - pd - es);

to either:

r = MIN((unsigned)(pd - pc), pn - pd - es)

or to:

r = MIN(pd - pc, (signed)(pn - pd - es));

The question is... what is the right patch?

[1] "Why Not Mix Signed and Unsigned Values in C/C++?", http://blog.regehr.org/archives/268


Solution

  • Clearly I am on my own here :-)

    My answer to the question comes from a very simplistic way of looking at things...

    If you apply the unsigned cast on the left, and re-compile, the object file does not change (verified via MD5 sums). If you instead cast the right side (to signed), the object files DOES change - which means this change deviates (at assembly level, at least - for both amd64 and sparcs) from what the original code was doing.

    Short answer: I proceed with casting to unsigned (size_t). That's also what Apple seems to have done (thanks for the pointer, @alk).