cvs commit: src/sys/net if_vlan.c

[Bruce Evans]

On Fri, 4 Aug 2006, John Baldwin wrote:

> On Friday 04 August 2006 16:01, Marcel Moolenaar wrote:
>
>> The point is that kdb_backtrace() is there if you want a backtrace and
>> you call it based on whatever option that makes sense at the call-site
>> or even unconditionally if that's the right thing.
>> Whether there's actually a backend that can make a backtrace is really
>> a seperate issue. We just happen to implement backtracing and unwinding
>> by debuggers, but with an unwinder in the kernel on ia64, we really
>> don't need a debugger in order to make a backtrace and it's not that
>> unrealistic that I create a backend that can only do backtraces...
>
> To be honest, as someone who works with bug reports, I'd actually like
> backtraces up front w/o requiring the user to compile a custom kernel, etc.
> Having a simple backend in place and kdb_backtrace()'s where relevant would
> be very handy. :)

This was non-broken in the original implementation of backtrace():

% RCS file: /home/ncvs/src/sys/kern/kern_shutdown.c,v
% Working file: kern_shutdown.c
% ...
% ----------------------------
% revision 1.138
% date: 2003/01/04 20:54:58;  author: phk;  state: Exp;  lines: +14 -0
% Introduce the
% 	void backtrace(void);
% function which will print a backtrace if DDB is in the kernel and an
% explanation if not.
% 
% This is useful for recording backtraces in non-fatal circumstances and
% does not require pollution with DDB #includes in the files where it
% is used.
% 
% It would of course be nice to have a non-DDB dependent version too,
% but since the meat of a backtrace is MD it is probably not worth it.
% ----------------------------

Debugger() has been broken similarly.  It was designed to be called
independently of the configuration of a debugger, and print a message
if it is called when no debugger is configured (not quite right since
Debugger() was often used as a quick replacement for panic()).  Now
Debugger() is misspelled kdb_enter() and is silent if there is no
low-level debugger to enter.

>>> Places that call kdb_enter() aren't all #ifdef KDB IIRC.  It's
>>> just a feature that kdb_foo() functions become NOPs when the kernel isn't
>>> configured for debugging, so I think the #ifdef KDB's would be redundant.

Just a bug, for kdb_enter() at least.

>> None of the kdb_*() functions in src/sys/kern/subr_kdb.c turn into
>> NOPs when option KDB is not present. They are all unconditionally
>> functional by design and should therefore be called conditionally
>> by consequence.

Some of them need to at least print a message if they are called when
no low-level debugger is present, since most calls in that case are
errors -- debugger functions shouldn't be called in production kernels.
Panicing would be too much for most calls.

> Well, given that separation, I'm not sure KDB is the right option to make
> calls conditional.  Rather, some specific is-debugging-enabled? option (like
> INARIANTS or FOO_DEBUG) should be used instead.  i.e.:
>
> #ifdef FOO_DEBUG
> 	if (foo_bad) {
> 		printf("foo is bad\n");
> 		kdb_backtrace();
> 	}
> #endif
>
> I don't think that warrants an extra #ifdef KDB.

Yes, most calls that belong in production kernels are near kern_shutdown.c
and already have control variables.  For backtrace in panics, it is harmless
for kdb to print a message saying that backtrace is unavailable.  For calls
from other places, where you want some debugging info but don't want to
enter a debugger even if one is available, it should be possible to just
call backtrace() and then a message that backtrace is unavailable is good
as a reminder that the call should be conditional in production kernels.

Bruce