Date: Mon, 27 Nov 2017 19:27:34 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Nathan Whitehorn <nwhitehorn@freebsd.org> Cc: John Baldwin <jhb@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326218 - head/sys/kern Message-ID: <20171127183109.C1790@besplex.bde.org> In-Reply-To: <abfdf2b1-bb2f-4eb1-a448-bfee68158577@freebsd.org> References: <201711252341.vAPNf5Qx001464@repo.freebsd.org> <3170692.kvv90QqB0X@ralph.baldwin.cx> <abfdf2b1-bb2f-4eb1-a448-bfee68158577@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171127183109.C1790>