Page MenuHomeFreeBSD

libc qsort(3): Eliminate ambiguous sign comparison
ClosedPublic

Authored by cem on Jul 23 2021, 7:56 PM.

Details

Summary

The left side of the MIN() expression is the (signed) result of pointer
subtraction (ptrdiff_t). The right hand side is the also the (signed)
result of pointer subtraction, additionally subtracting the element size
('es'), which is unsigned size_t. This coerces the right-hand
expression into an unsigned value. MIN(signed, unsigned) triggers
-Wsign-compare.

Sorting elements of size greater than SSIZE_MAX is nonsensical, so we
can instead treat the element size as ssize_t, leaving the right-hand
result the same signedness as the left.

Test Plan

buildworld

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 40653
Build 37542: arc lint + arc unit

Event Timeline

cem requested review of this revision.Jul 23 2021, 7:56 PM

ptrdiff_t sounds arbitrary. Why did not you used e.g. ssize_t? Because it is not defined by C STD?

Also the cast needs a comment explaining it purpose.

In D31292#704741, @kib wrote:

ptrdiff_t sounds arbitrary. Why did not you used e.g. ssize_t? Because it is not defined by C STD?

It is arbitrary. I agree ssize_t would be reasonable.

Also the cast needs a comment explaining it purpose.

Ok.

Looks good to me

lib/libc/stdlib/Makefile.inc
21

Could we add this to all C files in this directory or are we not ready for that?

Unrelated, but do you know how close we are to bring able to increasing WARNS for libc?

lib/libc/stdlib/qsort.c
104

Can we change the type of es here instead of adding the cast?

lib/libc/stdlib/Makefile.inc
21

Could we add this to all C files in this directory or are we not ready for that?

lib/libc/gen/getgrent.c:1146:28: error: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
                            (unsigned long)gid) >= bufsize)
                            ~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~

Looks like no.

Unrelated, but do you know how close we are to bring able to increasing WARNS for libc?

I don't.

lib/libc/stdlib/qsort.c
104

It's worse:

lib/libc/stdlib/qsort.c:181:10: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'ssize_t' (aka 'long') [-Werror,-Wsign-compare]
                if (d1 > es) {
                    ~~ ^ ~~
lib/libc/stdlib/qsort.c:184:10: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'ssize_t' (aka 'long') [-Werror,-Wsign-compare]
                if (d2 > es) {
                    ~~ ^ ~~
lib/libc/stdlib/qsort.c:193:10: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'ssize_t' (aka 'long') [-Werror,-Wsign-compare]
                if (d2 > es) {
                    ~~ ^ ~~
lib/libc/stdlib/qsort.c:196:10: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'ssize_t' (aka 'long') [-Werror,-Wsign-compare]
                if (d1 > es) {
                    ~~ ^ ~~
lib/libc/stdlib/qsort.c
104

In that case, the current patch looks fine. Thanks

Add comment explaining the cast.

lib/libc/stdlib/qsort.c
175

Please use C89 comments, in style.

Oops. Use C89-style comments per style(9).

This revision is now accepted and ready to land.Jul 28 2021, 10:56 PM