svn commit: r278634 - head/lib/libc/gen

Bruce Evans brde at optusnet.com.au
Fri Feb 13 07:19:09 UTC 2015


On Thu, 12 Feb 2015, Bjoern A. Zeeb wrote:

>> On 12 Feb 2015, at 22:34 , Pedro Giffuni <pfg at FreeBSD.org> wrote:
>>
>>
>> On 02/12/15 17:27, Bjoern A. Zeeb wrote:
>>>> On 12 Feb 2015, at 21:07 , Pedro F. Giffuni <pfg at FreeBSD.org> wrote:
>>>> ...
>>>> Log:
>>>>  ulimit(3): Fix broken check.
>>>> ...
>>> Did this compile?
>>
>> Yes! Any log message to share?
>
> Now I do again; had lost them due to buildworld starting over again:
>
> ===> lib/libc_nonshared (obj,depend,all,install)
> cc1: warnings being treated as errors
> /scratch/tmp/bz/head.svn/lib/libc/gen/ulimit.c: In function 'ulimit':
> /scratch/tmp/bz/head.svn/lib/libc/gen/ulimit.c:56: warning: comparison is always false due to limited range of data type
> /scratch/tmp/bz/head.svn/lib/libc/gen/ulimit.c:57: warning: overflow in implicit constant conversion
> --- ulimit.So ---
> *** [ulimit.So] Error code 1

Grump.  The first warning inhibits doing clean range checking.  Considered
the following simplified case:

 	long arg;

 	/*
 	 * We want to know if arg is representable as an rlim_t.  This is
 	 * remarkably difficult.  The following doesn't work due to
 	 * compiler zeal and other bugs:
 	 */
 	if (arg > RLIM_INFINITY)
 		err(...);

The check is false on all arches since RLIM_INFINITY happens to be >=
LONG_MAX, and excessively zealous compilers warn.

This omits complications for negative args (see below).  There are also
system bugs for negative args.  POSIX specifies that rlim_t shall be an
unsigned type, but in FreeBSD it is still a signed type like it always
was.  FreeBSD is also missing POSIX's magic values RLIM_SAVED_{MAX,CUR}
for handling unrepresentable values.  After fixing the type, these
could be used for the historical mishandling of historical negative
values and might even allow binary compatibility for those values.

The second warning is about a bug.  A last-minute change made the error
handling the same for all negative values (representable or not) as for
unrepresentable positive values.  But that doesn't work, since it give
an overflow in live code as well as in dead code.  We also depend on
the compiler understanding the range checking and not warning for overflows
in dead code.  The unsimplified code for this is:

 	if (arg > RLIM_INFINITY / 512)
 		arg = RLIM_INFINITY / 512;

or equivalently:

 	arg = MIN(arg, RLIM_INFINITY / 512);

The check is vacuously false on 64-bit arches only.  The assignment overflows
on 32-bit arches only.  So zealous compilers have something to warn about
in both lines.  We want a warning for only the second line iff it is live
and overflows.  This requires the compiler to understand when the first
line is vacuously false so that the second line is dead, but not warn
about the first line.

Other common forms of the range check shouldn't work any better:

 	if ((rlim_t)arg > RLIM_INFINITY)

If rlim_t were unsigned as specified by POSIX, then this cast would probably
just be a bug.  Otherwise, it should have no effect in the vacuously false
case.  We depend on it having no effect so that the compiler knows when
the following code is dead.

 	rlim_t tmp;

 	tmp = arg;
 	if (tmp != arg)
 		err(1, "arg not representable by rlim_t");

The compiler should still be able to see when the comparison is vacuously
false.  This form of the comparison is also more difficult to write when
the limit is not simply the maximum for the type.  Here we want to multiply
by 512, but there may be no type that can hold the result.

This shows that the MIN() macro is hard to used with mixed types and
zealous compilers.  Using doesn't avoid many of the problems in the
imin() family.  With this family, you have to translate all typedefed
types to basic types and promote to a larger common type, if any.  This
tends to hide the original values, so the zealous compilers are not
smart enough to warn.  However, smart ones should see inside:

 	arg = llmin(arg, RLIM_INFINITY / 512);

and warn for it too.

Add complications for portability to BSD and POSIX for the full mess.
Translation of typedefed types for the imin() family becomes impossible
because there is no suitable larger common type in some cases; using
MIN() gives unwanted (un)sign extension in these cases.

Bruce


More information about the svn-src-all mailing list