From owner-svn-src-all@freebsd.org Mon Nov 27 08:27:43 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 8EBB9DFC0A5; Mon, 27 Nov 2017 08:27:43 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 4A2B468C54; Mon, 27 Nov 2017 08:27:42 +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 mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 593DB10440E8; Mon, 27 Nov 2017 19:27:34 +1100 (AEDT) Date: Mon, 27 Nov 2017 19:27:34 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Nathan Whitehorn cc: John Baldwin , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326218 - head/sys/kern In-Reply-To: Message-ID: <20171127183109.C1790@besplex.bde.org> References: <201711252341.vAPNf5Qx001464@repo.freebsd.org> <3170692.kvv90QqB0X@ralph.baldwin.cx> 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=1QgWAXTlbbf8hjtPRX8A:9 a=yrE8NTKN8dZdtXkm:21 a=Fgy4hgLG1BemluUb: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: Mon, 27 Nov 2017 08:27:43 -0000 On Sun, 26 Nov 2017, Nathan Whitehorn wrote: > On 11/26/17 20:50, John Baldwin wrote: >> On Saturday, November 25, 2017 11:41:05 PM Nathan Whitehorn wrote: >>> ... >>> Modified: head/sys/kern/kern_clock.c >>> ============================================================================== >>> --- head/sys/kern/kern_clock.c Sat Nov 25 23:23:24 2017 >>> (r326217) >>> +++ head/sys/kern/kern_clock.c Sat Nov 25 23:41:05 2017 >>> (r326218) >>> @@ -573,7 +573,9 @@ hardclock_cnt(int cnt, int usermode) >>> void >>> hardclock_sync(int cpu) >>> { >>> - int *t = DPCPU_ID_PTR(cpu, pcputicks); >>> + int *t; >>> + KASSERT(!CPU_ABSENT(cpu), ("Absent CPU %d", cpu)); >> Blank line before the KASSERT() perhaps? >> >>> + t = DPCPU_ID_PTR(cpu, pcputicks); >>> *t = ticks; >> Probably don't need this blank line though? > > Those are both good ideas. Preserving the existing normal style is an even better idea. The change does fix 2 style bugs (indentation of t and initializing it in its declaration) as well as adding 2. >>> Modified: head/sys/kern/subr_pcpu.c >>> ============================================================================== >>> --- head/sys/kern/subr_pcpu.c Sat Nov 25 23:23:24 2017 >>> (r326217) >>> +++ head/sys/kern/subr_pcpu.c Sat Nov 25 23:41:05 2017 >>> (r326218) >>> @@ -279,6 +279,8 @@ pcpu_destroy(struct pcpu *pcpu) >>> struct pcpu * >>> pcpu_find(u_int cpuid) >>> { >>> + KASSERT(cpuid_to_pcpu[cpuid] != NULL, >>> + ("Getting uninitialized PCPU %d", cpuid)); >> This KASSERT seems unnecessary? If the caller uses an invalid one, it will >> just fault anyway. This also removes the blank line after the declarations and adds a bogus one after the new statement. > It won't necessarily fault. For example, on PowerPC, NULL is a valid address > that does not trigger faults. It's unfortunately quite complicated to fix > this in a general way. Even if it did fault, this makes the fault more > informative (and has found at least one bug on arm64 already). On arches that trap null pointer accesses, the KASSERT() itself will fault. This makes the fault less informative. Trapping of null pointers (with offset < PAGE_SIZE) is currently broken on i386 by a hack to support acpi wakeup. Bruce