lkml.org 
[lkml]   [2006]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] reliable stack trace support
>> +#ifdef CONFIG_STACK_UNWIND
>> +#include <asm/unwind.h>
>> +#else
>> +#include <asm-generic/unwind.h>
>> +#endif
>
>Normally the other archs should get stub includes that include asm-generic/unwind.h

This would make them appear kind of supporting unwind, even though they really don't, wouldn't it?

>> + unwind_init();
>
>Stupid q. but what happens when we get a crash before unwind_init?
>Is the failure benign?

You'll get old fashioned stack traces, which I consider benign.

>> +#ifdef MODULE_UNWIND_INFO
>> +#include <asm/unwind.h>
>> +#endif
>
>It should be possible to include this without the ifdef, no?

Only if all arch-s get an asm/unwind.h, which I consider ill (see above).

>> +#ifdef MODULE_UNWIND_INFO
>> + unwind_remove_table(mod->unwind_info, 0);
>> +#endif
>
>Might be better to stub it in the include

It is stubbed, but as above - the include isn't always there.

>> +static const struct {
>> + unsigned offs:BITS_PER_LONG / 2;
>> + unsigned width:BITS_PER_LONG / 2;
>> +} reg_info[] = {
>> + UNW_REGISTER_INFO
>> +};
>> +
>> +#undef PTREGS_INFO
>> +#undef EXTRA_INFO
>
>Where are they actually used? I can't find UNW_REGISTER_INFO
>in the patch.

These are defined per architecture.

>In general it looks a bit overcomplicated. Can you just
>use the values directly in the unwinder code?

Which values? The offsets into pt_regs are clearly architecture specific, so I don't think it'd be nice to put them
into generic code.

>> +#ifndef REG_INVALID
>
>Who would set it?

Again, an architecture if the default definition isn't sufficient.

>> +#define DW_CFA_nop 0x00
>
>I guess it would be useful to have them in some include.
>Maybe linux/dwarf2.h ?

Do you think they might be re-used by anyone else? I generally prefer keeping stuff used only in a single place out of
sight for anyone else.

>In general please replace all uintN_t with uN

Why that? What are these types for then? After all, they're standard mandated, and one more of my preferences is to use
standard types where-ever possible.

>> +
>> +static struct unwind_table *
>> +find_table(unsigned long pc)
>
>Should be on one line. More further down.

Make the code uglier in my opinion, especially when the parameter declarations are quite long.

>> + atomic_inc(&table->users);
>> + break;
>> + }
>> + atomic_dec(&lookups);
>> + } while (atomic_read(&removals) != old_removals);
>
>This looks like a seq lock? Use the real thing?

The code here should get away without taking *any* locks, otherwise you may end up not seeing any backtrace at all when
the system dies.

>> + table = kmalloc(sizeof(*table), GFP_USER);
>
>GFP_KERNEL.

Not sure about the significance - I took this from respective ia64 code.

>> + if (table) {
>> + while (atomic_read(&table->users) || atomic_read(&lookups))
>> + msleep(1);
>
>Can't this livelock?
>
>I suspect it isn't needed anyways because module unload uses stop_machine()
>already and that should be enough to stop the lockups which don't block.

That is what I wasn't sure of - if these functions are indeed meant to be called only from the module loader (which I
think they are), then table_lock isn't needed (serialized by module_mutex).

>> +#ifdef UNW_FP
>
>This should be CONFIG_FRAME_POINTER

No - there are arch-s (ia64, while clearly not going to be getting here, immediately comes to mind) where you cannot
define what register the frame pointer is in. I rather think x86 is special in that you actually can.

> + unsigned long top, bottom;
> +#endif
> +
> + drop_table(table);
> +#ifdef UNW_FP
> + top = STACK_TOP(frame->task);
> + bottom = STACK_BOTTOM(frame->task);
> +# if FRAME_RETADDR_OFFSET < 0

Nasty ifdefs. Can you perhaps isolate that < 0 case in a separate function. Also
when does it happen anyways? A little bit cleanup here would be good.

>> +EXPORT_SYMBOL_GPL(unwind_init_frame_info);
>
>I would actually use EXPORT_SYMBOL(). Would be unfair to not give an unwinder
>to any modules.

Fine with me - I just followed other people's demands (on other occasions) to only use GPL exports for new symbols.

>> config UNWIND_INFO
>> bool "Compile the kernel with frame unwind information"
>> - depends on !IA64
>> + depends on !IA64 && !PARISC
>
>Why PARISC?

Because it, like ia64, has its own unwinding logic.

Jan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-05-18 15:43    [W:0.093 / U:0.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site