Date: Mon, 1 Jun 2015 19:48:45 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dimitry Andric <dim@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r283870 - head/sys/amd64/amd64 Message-ID: <20150601185112.V1517@besplex.bde.org> In-Reply-To: <201506010650.t516oeNY080402@svn.freebsd.org> References: <201506010650.t516oeNY080402@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 1 Jun 2015, Dimitry Andric wrote:
> Log:
> Remove unneeded NULL checks in amd64's trap_fatal().
>
> Since td_name is an array member of struct thread, it can never be NULL,
> so the check can be removed. In addition, curproc can never be NULL,
> so remove the if statement, and splice the two printfs() together.
I fixed the first part of this in my version (for p_comm) for i386 long ago.
> While here, remove the u_long cast, and use the correct printf format
> specifier curproc->p_pid.
This was correct, modulo a harmless sign error. Now it assumes that pid_t
is int.
Careful code elsewhere, and in my i386 version here, casts pid_t to signed
long for printing. More careful code would cast pid_t to intmax_t, but
that would be excessive. The less careful code here was copied from i386.
amd64 and i386 are MD, so can hard-code assumptions about type sizes, but
this is still fragile. In FreeBSD-1 (and presumably Net/2), pid_t was
signed short and PID_MAX was 30000. Signed short is just barely enough
to hold 30000, and 30000 is just small enough to fit in signed short.
In 4.4BSD-Lite1, pid_t was expanded to signed long, although PID_MAX
was not expanded. pid_t was apparently expanded for portability. If
PID_MAX is expanded above 32767 then it is too large for a portable
signed short, and also too large for a portable signed int, so before
<stdint.h> existed, then only portable type for pid_t was signed long
and 4.4BSD used this. But in practice, signed long is the most
unportable type, since is usually actually longer than int on 64-bit
systems, so using it mainly gives binary compatibility problems. These
problem for pid_t and most of the other long types were fixed in NetBSD
and merged into 4.4BSD-Lite2, or vice versa, and merged into FreeBSD a
few years later (1997). So pid_t now has type int32_t in FreeBSD.
Code that was careful about printing pid_t's always cast to long for
printing and never noticed the 2 previous type changes and wouldn't
notice any further ones, except C99 broke the promise that long is the
longest signed integer type, so the careful code might be broken by
future expansion of pid_t.
> Modified: head/sys/amd64/amd64/trap.c
> ==============================================================================
> --- head/sys/amd64/amd64/trap.c Mon Jun 1 06:14:17 2015 (r283869)
> +++ head/sys/amd64/amd64/trap.c Mon Jun 1 06:50:39 2015 (r283870)
> @@ -840,14 +840,8 @@ trap_fatal(frame, eva)
> if (frame->tf_rflags & PSL_RF)
> printf("resume, ");
> printf("IOPL = %ld\n", (frame->tf_rflags & PSL_IOPL) >> 12);
> - printf("current process = ");
> - if (curproc) {
> - printf("%lu (%s)\n",
> - (u_long)curproc->p_pid, curthread->td_name ?
> - curthread->td_name : "");
> - } else {
> - printf("Idle\n");
> - }
> + printf("current process = %d (%s)\n",
> + curproc->p_pid, curthread->td_name);
Technically, it is still an error to print int32_t's using %d format.
This assumes that ints are at least 32 bits. The amd64 and i386 can
safely assume this. Also, POSIX started requiring at least 32-bit ints
in about 2007. But it is easier not to assume this.
In FreeBSD-1, the i386 code correspoding to the above was:
X if (curproc) {
X printf("%d (%s)\n",
X curproc->p_pid, curproc->p_comm ?
X curproc->p_comm : "");
X } else {
X printf("Idle\n");
X }
In this, the curproc check was correct (curproc was sometimes NULL),
the %d format was correct (pid_t was signed short), and the p_comm
check was garbage as now (p_comm was an array, and even if it was
a null pointer it is feature to let printf() print it as "(null)"
since it is unexpected for it to be either empty or null).
The %d format broke in FreeBSD-2 when FreeBSD was forced to merge with
4.4BSD-Lite-1, and the (wrong but adequate) fix for this was to cast
to unsigned long and change the format to %lu. The sign mismatches
from this are harmless, but it is easier to not have any mismatches
than to verify that mismatches are harmless. Casting to int and keeping
the %d format would also have worked, but again it was easier to not
change the type, or to upcast the type (casting to long would have become
an upcast when pid_t was reduced to int32_t a few years later). C99's
breakage of long, and impending problems with intmax_t, made it not so
clear that upcasting is right or what the upcast should be to.
Upcasting to intmax_t is now only to 64 bits, but even that is excessive
for pid_t on 32-bit arches. 128-bit integers will require intmax_t to
be 128 bits, and that will be excessive for pid_t on on 64 bit arches.
C99 also has the PRI* formats, but those are worse than what they fix.
They are harder to use than upcasts, and just fix runtime space+time
bloat from larger than necessary types.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150601185112.V1517>