From owner-svn-src-all@freebsd.org Fri Nov 24 07:45:36 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E62AEDBD3B4; Fri, 24 Nov 2017 07:45:36 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 6D7C0695B9; Fri, 24 Nov 2017 07:45:35 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id D3A8D3C60F9; Fri, 24 Nov 2017 18:45:28 +1100 (AEDT) Date: Fri, 24 Nov 2017 18:45:28 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326139 - head/usr.bin/vmstat In-Reply-To: <201711231910.vANJA9gl060002@repo.freebsd.org> Message-ID: <20171124170952.Y980@besplex.bde.org> References: <201711231910.vANJA9gl060002@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=ve-Pef5DYMJq4Nw43_sA:9 a=KtRL4uPeHvNv6vRM:21 a=DE4My3xT9DWS8wlG:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Nov 2017 07:45:37 -0000 On Thu, 23 Nov 2017, Konstantin Belousov wrote: > Log: > vmstat: use 64-bit counters from struct vmtotal. > > Consistently print counters using unsigned intmax type. Not very consistently. After fixing 0.01% of libxo (just add __printflike() to xo_emit(), the following errors are detected: X vmstat.c:817:23: error: format specifies type 'long' but the argument has type 'int' [-Werror,-Wformat] X total.t_rq - 1, total.t_dw + total.t_pw, total.t_sw); X ^~~~~~~~~~~~~~~~~~~~~~~ X vmstat.c:817:48: error: format specifies type 'long' but the argument has type 'int16_t' (aka 'short') [-Werror,-Wformat] X total.t_rq - 1, total.t_dw + total.t_pw, total.t_sw); X ^~~~~~~~~~ Broken in r291090 (the first libxo commit to this file). All the arg types here are still int since all the fields in 'total' still have type int16_t. X vmstat.c:865:7: error: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Werror,-Wformat] X (unsigned long)rate(sum.v_swtch - osum.v_swtch)); X ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Broken in r291090. Everything in the same printf is broken, but the other bugs are newer and older: - in 4.4BSD-Lite2, the the format was %lu for all 3 values to be printed, but the type was MD. It was apparently long. The struct fields were all u_int IIRC, but the rate() expression involves time_t. time_t was _BSD_TIME_T_, which was apparently long on all arches. - FreeBSD fixed time_t to be int on at least i386. The type error was still non-fatal on 32-bit arches with 32-bit time_t. - I fixed this in r37453. The fix wasn't so good. It retained the %lu format and added casts to u_long to match the format. But the casts are unportable and became wrong when v_swtch etc. were expanded. - r123407 broke the style by changing all u_* to unsigned *. - the casts became wrong, but fairly harmlessly so, on 32-bit arches when v_swtch, etc. was changed to counter_u64_t in r317061 when v_swtch was expanded to counter_u64_t. The problem is limit because the values are rays of differences. Even %u format is probably enough for this. X vmstat.c:1035:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_swtch); X ^~~~~~~~~~~ X vmstat.c:1037:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_intr); X ^~~~~~~~~~ X vmstat.c:1039:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_soft); X ^~~~~~~~~~ X vmstat.c:1040:38: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X xo_emit("{:traps/%9u} {N:traps}\n", sum.v_trap); X ~~~ ^~~~~~~~~~ X %9lu X vmstat.c:1042:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_syscall); X ^~~~~~~~~~~~~ X vmstat.c:1044:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_kthreads); X ^~~~~~~~~~~~~~ X vmstat.c:1045:46: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X xo_emit("{:forks/%9u} {N: fork() calls}\n", sum.v_forks); X ~~~ ^~~~~~~~~~~ X %9lu X vmstat.c:1047:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_vforks); X ^~~~~~~~~~~~ X vmstat.c:1049:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_rforks); X ^~~~~~~~~~~~ X vmstat.c:1051:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_swapin); X ^~~~~~~~~~~~ X vmstat.c:1053:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_swappgsin); X ^~~~~~~~~~~~~~~ X vmstat.c:1055:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_swapout); X ^~~~~~~~~~~~~ X vmstat.c:1057:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_swappgsout); X ^~~~~~~~~~~~~~~~ X vmstat.c:1059:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_vnodein); X ^~~~~~~~~~~~~ X vmstat.c:1061:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_vnodepgsin); X ^~~~~~~~~~~~~~~~ X vmstat.c:1063:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_vnodeout); X ^~~~~~~~~~~~~~ X vmstat.c:1065:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_vnodepgsout); X ^~~~~~~~~~~~~~~~~ X vmstat.c:1067:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_pdwakeups); X ^~~~~~~~~~~~~~~ X vmstat.c:1069:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_pdpages); X ^~~~~~~~~~~~~ X vmstat.c:1071:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_pdshortfalls); X ^~~~~~~~~~~~~~~~~~ X vmstat.c:1073:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_reactivated); X ^~~~~~~~~~~~~~~~~ X vmstat.c:1075:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_cow_faults); X ^~~~~~~~~~~~~~~~ X vmstat.c:1077:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_cow_optim); X ^~~~~~~~~~~~~~~ X vmstat.c:1079:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_zfod); X ^~~~~~~~~~ X vmstat.c:1081:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_ozfod); X ^~~~~~~~~~~ X vmstat.c:1083:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_intrans); X ^~~~~~~~~~~~~ X vmstat.c:1085:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_vm_faults); X ^~~~~~~~~~~~~~~ X vmstat.c:1087:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_io_faults); X ^~~~~~~~~~~~~~~ X vmstat.c:1089:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_kthreadpages); X ^~~~~~~~~~~~~~~~~~ X vmstat.c:1091:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_forkpages); X ^~~~~~~~~~~~~~~ X vmstat.c:1093:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_vforkpages); X ^~~~~~~~~~~~~~~~ X vmstat.c:1095:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_rforkpages); X ^~~~~~~~~~~~~~~~ X vmstat.c:1097:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_tfree); X ^~~~~~~~~~~ X vmstat.c:1099:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_dfree); X ^~~~~~~~~~~ X vmstat.c:1101:3: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_pfree); X ^~~~~~~~~~~ Breakage of printing of the raw v_foo fields has a shorter history. I think they always had type u_int and were honestly printed with %u before counter_u64_t. But libxo prepared to break them by not using __printflike(), and counter_u64_t broke them by not updating the format. clang reports that uint64_t is aka u_long, but this is only for 64-bit arches. X vmstat.c:1127:39: error: flag ' ' results in undefined behavior with 'p' conversion specifier [-Werror,-Wformat] X "({:positive-cache-hits/%ld}% pos + " X ~^~ X vmstat.c:1131:6: error: format specifies type 'void *' but the argument has type 'long' [-Werror,-Wformat] X PCT(lnchstats.ncs_neghits, nchtotal), X ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Maybe xo_emit() is not actually printf() compatible. For printf(), the percent sign must be printed using %%, but xo_emit() seems to print unquoted % correct. This one is "% ". The format checker seems to have a bug too. It treats "% p" as %p, which gives a cascade of errors. Broken code like this probably just wants literal %. X vmstat.c:1024:23: note: expanded from macro 'PCT' X #define PCT(top, bot) pct((long)(top), (long)(bot)) X ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ X vmstat.c:1128:39: error: invalid conversion specifier '{' [-Werror,-Wformat-invalid-specifier] X "{:negative-cache-hits/%ld}% {N:neg}) " X ~~^ X vmstat.c:1129:40: error: more '%' conversions than data arguments [-Werror,-Wformat] X "system {:cache-hit-percent/%ld}% per-directory\n", X ~~^ X vmstat.c:1133:51: error: invalid conversion specifier ',' [-Werror,-Wformat-invalid-specifier] X xo_emit("{P:/%9s} {L:deletions} {:deletions/%ld}%, " X ~^ This one is "%,". This ends the error cascade for the xo_emit() on line 1126 (it has 3 quoting errors for %). X vmstat.c:1134:43: error: invalid conversion specifier ',' [-Werror,-Wformat-invalid-specifier] X "{L:falsehits} {:false-hits/%ld}%, " X ~^ X vmstat.c:1135:39: error: invalid conversion specifier '\x0a' [-Werror,-Wformat-invalid-specifier] X "{L:toolong} {:too-long/%ld}%\n", "", X ~^ Similarly. 3 more quoting errors for the xo_emit() on line 1133. X vmstat.c:1149:6: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_forks, sum.v_forkpages, X ^~~~~~~~~~~ X vmstat.c:1149:19: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_forks, sum.v_forkpages, X ^~~~~~~~~~~~~~~ X vmstat.c:1154:6: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_vforks, sum.v_vforkpages, X ^~~~~~~~~~~~ X vmstat.c:1154:20: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_vforks, sum.v_vforkpages, X ^~~~~~~~~~~~~~~~ X vmstat.c:1159:6: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_rforks, sum.v_rforkpages, X ^~~~~~~~~~~~ X vmstat.c:1159:20: error: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X sum.v_rforks, sum.v_rforkpages, X ^~~~~~~~~~~~~~~~ Back to misprinting counter_u64_t's. X vmstat.c:1473:30: error: format specifies type 'char *' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X memstat_get_name(mtp), memstat_get_count(mtp), X ^~~~~~~~~~~~~~~~~~~~~~ X vmstat.c:1474:47: error: format specifies type 'unsigned long' but the argument has type 'char *' [-Werror,-Wformat] A very large error. The count is misprinted using %s format... X (memstat_get_bytes(mtp) + 1023) / 1024, "-", X ^~~ X vmstat.c:1475:7: error: format specifies type 'char *' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] X memstat_get_numallocs(mtp)); X ^~~~~~~~~~~~~~~~~~~~~~~~~~ X vmstat.c:1472:20: error: more '%' conversions than data arguments [-Werror,-Wformat] X "{:requests/%8" PRIu64 "} ", X ~~~~^ X /usr/include/x86/_inttypes.h:100:25: note: expanded from macro 'PRIu64' X #define PRIu64 __PRI64"u" /* uint64_t */ X ^ There are 6 format specifiers but only 5 args. Apparently the second specifier (%s) is missing a string arg or shouldn't be there. After removing this, no errors are detected for this xo_emit(). The __PRI64 mistake is used in a few places including this. It helps make the format almost unreadable. X vmstat.c:1673:20: error: more '%' conversions than data arguments [-Werror,-Wformat] X xo_emit("{T:RES/%5s} {T:ACT/%5s} {T:INACT/%5s} {T:REF/%3s} {T:SHD/%3s} " X ~~^ Here it seems clear that xo_emit() is not actually printf() compatible. It passes no args to this call. This prints nothing for vmstat -o. I don't want libxo and don't know how to use it. The documented syntax "vmstat --libxo followed by normal args" is a syntax error. X 56 errors generated. vmstat.c now has no printf()s except for 2 snprintf()s, so has essentially no printf() format checking. A few things are printed in dehumanized format by prthuman() = dehumanize_number() + xo_attr() with %ju format. These get upcast enough to work, and the format of the xo_attr() for this is easily checked manually. xo_attr() is also not declared as __printflike(). Bruce