svn commit: r326218 - head/sys/kern

[Bruce Evans]

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