>> +#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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
