Changeset View
Standalone View
sys/kern/subr_stack.c
Show First 20 Lines • Show All 160 Lines • ▼ Show 20 Lines | else | ||||
printf("%p", (void *)st->pcs[i]); | printf("%p", (void *)st->pcs[i]); | ||||
} | } | ||||
printf("\n"); | printf("\n"); | ||||
} | } | ||||
#endif | #endif | ||||
/* | /* | ||||
* Format stack into sbuf from live kernel. | * Format stack into sbuf from live kernel. | ||||
* | * | ||||
* flags - M_WAITOK or M_NOWAIT (EWOULDBLOCK). | * flags - M_WAITOK or M_NOWAIT (EWOULDBLOCK). | ||||
*/ | */ | ||||
int | int | ||||
stack_sbuf_print_flags(struct sbuf *sb, const struct stack *st, int flags) | stack_sbuf_print_flags(struct sbuf *sb, const struct stack *st, int flags, | ||||
enum stack_sbuf_fmt format) | |||||
{ | { | ||||
cem: I'd encourage adding this to the existing flags parameter. Only need one bit for `FMT_COMPACT`. | |||||
Done Inline ActionsUntil one day someone decide to use that one bit as a malloc(9) flag that is passed in flags :-( kaktus: Until one day someone decide to use that one bit as a malloc(9) flag that is passed in `flags`… | |||||
Not Done Inline ActionsYou can _Static_assert that it stays true if you are concerned. cem: You can `_Static_assert` that it stays true if you are concerned. | |||||
char namebuf[64]; | char namebuf[64]; | ||||
long offset; | long offset; | ||||
int i, error; | int i, error; | ||||
KASSERT(st->depth <= STACK_MAX, ("bogus stack")); | KASSERT(st->depth <= STACK_MAX, ("bogus stack")); | ||||
for (i = 0; i < st->depth; i++) { | for (i = 0; i < st->depth; i++) { | ||||
error = stack_symbol(st->pcs[i], namebuf, sizeof(namebuf), | error = stack_symbol(st->pcs[i], namebuf, sizeof(namebuf), | ||||
&offset, flags); | &offset, flags); | ||||
if (error == EWOULDBLOCK) | if (error == EWOULDBLOCK) | ||||
return (error); | return (error); | ||||
sbuf_printf(sb, "#%d %p at %s+%#lx\n", i, (void *)st->pcs[i], | switch (format) { | ||||
namebuf, offset); | case STACK_SBUF_FMT_LONG: | ||||
sbuf_printf(sb, "#%d %p at %s+%#lx\n", i, | |||||
(void *)st->pcs[i], namebuf, offset); | |||||
break; | |||||
case STACK_SBUF_FMT_COMPACT: | |||||
sbuf_printf(sb, "%s+%#lx ", namebuf, offset); | |||||
break; | |||||
default: | |||||
__assert_unreachable(); | |||||
Not Done Inline ActionsThis is still wrong -- kernel consumers should pass a correct value. cem: This is still wrong -- kernel consumers should pass a correct value. | |||||
Done Inline ActionsThis is enum, -Werror,-Wswitch (correctly) doesn't like when I don't check all variables. kaktus: This is enum, -Werror,-Wswitch (correctly) doesn't like when I don't check all variables. | |||||
Not Done Inline ActionsSo perhaps switch is an inappropriate construct here when if/else would be more accurate, in addition to thwarting false-positive compiler warnings? cem: So perhaps switch is an inappropriate construct here when `if`/else would be more accurate, in… | |||||
Done Inline ActionsThis isn't the only place in kernel that does that. kaktus: This isn't the only place in kernel that does that. | |||||
Not Done Inline ActionsThere is also __assert_unreachable() now. cem: There is also `__assert_unreachable()` now. | |||||
Not Done Inline Actionsyou can __assert_unreachable in the default case mjg: you can __assert_unreachable in the default case | |||||
} | } | ||||
Done Inline ActionsThis routine is effectively used by witness and other stack-printers, meaning the tty-related change alters other output. Instead you should add an argument specifying the format (can be an enum) -- at least "long" and "short". Then the tty-related caller can pass the right value depending on tty_info_kstacks. This routine can be renamed to add _format to it and the current name can become a macro which expands to stack_sbuf_print_flags_format(..., STACK_SBUF_LONG) or similar. mjg: This routine is effectively used by witness and other stack-printers, meaning the tty-related… | |||||
Done Inline ActionsI assume you meant stack_sbuf_print(). I adjusted it to always use long format. I find stack_sbuf_print_flags() a perfect name for a function that takes flags as an arguments though. kaktus: I assume you meant stack_sbuf_print(). I adjusted it to always use long format. I find… | |||||
} | |||||
Not Done Inline ActionsKernel consumers should have correct API usage. Just assert API correctness. cem: Kernel consumers should have correct API usage. Just assert API correctness. | |||||
sbuf_nl_terminate(sb); | |||||
Not Done Inline ActionsCalling stack_sbuf_print with FMT_NONE is nonsensical. If anything, throw a KASSERT(format != FMT_NONE) at top of function. It does not make sense to handle it in every loop iteration. cem: Calling `stack_sbuf_print` with `FMT_NONE` is nonsensical. If anything, throw a `KASSERT… | |||||
return (0); | return (0); | ||||
} | } | ||||
void | void | ||||
stack_sbuf_print(struct sbuf *sb, const struct stack *st) | stack_sbuf_print(struct sbuf *sb, const struct stack *st) | ||||
{ | { | ||||
(void)stack_sbuf_print_flags(sb, st, M_WAITOK); | (void)stack_sbuf_print_flags(sb, st, M_WAITOK, STACK_SBUF_FMT_LONG); | ||||
} | } | ||||
#if defined(DDB) || defined(WITNESS) | #if defined(DDB) || defined(WITNESS) | ||||
void | void | ||||
stack_sbuf_print_ddb(struct sbuf *sb, const struct stack *st) | stack_sbuf_print_ddb(struct sbuf *sb, const struct stack *st) | ||||
{ | { | ||||
const char *name; | const char *name; | ||||
long offset; | long offset; | ||||
▲ Show 20 Lines • Show All 74 Lines • Show Last 20 Lines |
I'd encourage adding this to the existing flags parameter. Only need one bit for FMT_COMPACT.