Re: [Kgdb-bugreport] [PATCH] i386/x86_64 h/w breakpoints: coding style cleanups
Status: Beta
Brought to you by:
jwessel
Menu
▾
▴
Re: [Kgdb-bugreport] [PATCH] i386/x86_64 h/w breakpoints: coding style cleanups
|
From: Piet D. <pde...@bl...> - 2007-07-31 00:31:49
|
Sergei Shtylyov wrote:
> Perform the following coding style cleanups in i386/x86_64 KGDB ports:
>
> - make the 'breakinfo' array 'static' (x86_64 only);
>
> - replace the references to 'breakno' variable with constants in the 'switch'
> statement of kgdb_correct_hw_break();
>
> - fixed the parameter list indentation and got rid of the 'idx' variable in
> kgdb_{set|remove}_hw_break();
>
> - get rid of the useless curly braces in kgdb_{set|remove}_hw_break() and
> kgdb_remove_all_hw_break();
>
Oh my, it looks so much better.
> - use the more fitting 'switch' statement when filling the 'breakinfo' array
> slot's fields based on a passed breakpoint type, grouping all the common code
> together at the end of kgdb_set_hw_break();
>
> - in kgdb_arch_handle_exception(): format the function call better, get rid of
> the useless parens in the 'return' statment; insert new line after 'breakno'
> variable declaration (i386), move the 'breakno' declaration into the least
> enclosing block, fold two 'if' statements (x86_64) in the code checking if
> EFLAGS.RF needs to be set, and otherwise make the i386 and x86_64 versions
> of the function alike;
>
> - insert new line after the declaration block, pre-initialize 'regs' variable
> to NULL to get rid of the curly braces in in_interrupt_stack() (x86_64 only);
>
> - insert new line after the declaration block, move 'regs' variable to the top
> level in in_exception_stack() (x86_64 only);
>
> - format the long 'if' statements better in kgdb_notify();
>
> - fix a comment to 'kgdb_notifier' variable declaration and insert a new line
> after kgdb_arch_init() (x86_64 only)...
>
> ---
> This patch is against the linux_2_6_21_uprev branch, it doesn't influence the
> issue with i386 h/w breakpoints not being hit...
>
> Signed-off-by: Sergei Shtylyov <ssh...@ru...>
>
> arch/i386/kernel/kgdb.c | 109 ++++++++++++++++++-----------------
> arch/x86_64/kernel/kgdb.c | 143 +++++++++++++++++++++++-----------------------
> 2 files changed, 129 insertions(+), 123 deletions(-)
>
> Index: linux-2.6/arch/i386/kernel/kgdb.c
> ===================================================================
> --- linux-2.6.orig/arch/i386/kernel/kgdb.c
> +++ linux-2.6/arch/i386/kernel/kgdb.c
> @@ -148,19 +148,19 @@ static void kgdb_correct_hw_break(void)
> ((breakno << 2) + 16);
> switch (breakno) {
> case 0:
> - set_debugreg(breakinfo[breakno].addr, 0);
> + set_debugreg(breakinfo[0].addr, 0);
> break;
>
> case 1:
> - set_debugreg(breakinfo[breakno].addr, 1);
> + set_debugreg(breakinfo[1].addr, 1);
> break;
>
> case 2:
> - set_debugreg(breakinfo[breakno].addr, 2);
> + set_debugreg(breakinfo[2].addr, 2);
> break;
>
> case 3:
> - set_debugreg(breakinfo[breakno].addr, 3);
> + set_debugreg(breakinfo[3].addr, 3);
> break;
> }
> } else if ((dr7 & breakbit) && !breakinfo[breakno].enabled) {
> @@ -174,19 +174,17 @@ static void kgdb_correct_hw_break(void)
> }
>
> static int kgdb_remove_hw_break(unsigned long addr, int len,
> - enum kgdb_bptype bptype)
> + enum kgdb_bptype bptype)
> {
> - int i, idx = -1;
> - for (i = 0; i < 4; i++) {
> - if (breakinfo[i].addr == addr && breakinfo[i].enabled) {
> - idx = i;
> + int i;
> +
> + for (i = 0; i < 4; i++)
> + if (breakinfo[i].addr == addr && breakinfo[i].enabled)
> break;
> - }
> - }
> - if (idx == -1)
> + if (i == 4)
> return -1;
>
> - breakinfo[idx].enabled = 0;
> + breakinfo[i].enabled = 0;
> return 0;
> }
>
> @@ -194,42 +192,45 @@ static void kgdb_remove_all_hw_break(voi
> {
> int i;
>
> - for (i = 0; i < 4; i++) {
> + for (i = 0; i < 4; i++)
> memset(&breakinfo[i], 0, sizeof(struct hw_breakpoint));
> - }
> }
>
> static int kgdb_set_hw_break(unsigned long addr, int len,
> - enum kgdb_bptype bptype)
> + enum kgdb_bptype bptype)
> {
> - int i, idx = -1;
> - for (i = 0; i < 4; i++) {
> - if (!breakinfo[i].enabled) {
> - idx = i;
> + int i;
> + unsigned type;
> +
> + for (i = 0; i < 4; i++)
> + if (!breakinfo[i].enabled)
> break;
> - }
> - }
> - if (idx == -1)
> + if (i == 4)
> + return -1;
> +
> + switch (bptype) {
> + case bp_hardware_breakpoint:
> + type = 0;
> + len = 1;
> + break;
> + case bp_write_watchpoint:
> + type = 1;
> + break;
> + case bp_access_watchpoint:
> + type = 3;
> + break;
> + default:
> return -1;
> - if (bptype == bp_hardware_breakpoint) {
> - breakinfo[idx].type = 0;
> - breakinfo[idx].len = 0;
> - } else if (bptype == bp_write_watchpoint) {
> - breakinfo[idx].type = 1;
> - if (len == 1 || len == 2 || len == 4)
> - breakinfo[idx].len = len - 1;
> - else
> - return -1;
> - } else if (bptype == bp_access_watchpoint) {
> - breakinfo[idx].type = 3;
> - if (len == 1 || len == 2 || len == 4)
> - breakinfo[idx].len = len - 1;
> - else
> - return -1;
> - } else
> + }
> +
> + if (len == 1 || len == 2 || len == 4)
> + breakinfo[i].len = len - 1;
> + else
> return -1;
> - breakinfo[idx].enabled = 1;
> - breakinfo[idx].addr = addr;
> +
> + breakinfo[i].enabled = 1;
> + breakinfo[i].addr = addr;
> + breakinfo[i].type = type;
> return 0;
> }
>
> @@ -279,12 +280,14 @@ int kgdb_arch_handle_exception(int e_vec
> if (remcom_in_buffer[0] == 's') {
> linux_regs->eflags |= TF_MASK;
> debugger_step = 1;
> - atomic_set(&cpu_doing_single_step,raw_smp_processor_id());
> + atomic_set(&cpu_doing_single_step,
> + raw_smp_processor_id());
> }
>
> get_debugreg(dr6, 6);
> if (!(dr6 & 0x4000)) {
> - long breakno;
> + int breakno;
> +
> for (breakno = 0; breakno < 4; ++breakno) {
> if (dr6 & (1 << breakno) &&
> breakinfo[breakno].type == 0) {
> @@ -297,8 +300,8 @@ int kgdb_arch_handle_exception(int e_vec
> set_debugreg(0, 6);
> kgdb_correct_hw_break();
>
> - return (0);
> - } /* switch */
> + return 0;
> + }
> /* this means that we do not want to exit from the handler */
> return -1;
> }
> @@ -313,28 +316,28 @@ static int kgdb_notify(struct notifier_b
>
> /* Bad memory access? */
> if (cmd == DIE_PAGE_FAULT_NO_CONTEXT && atomic_read(&debugger_active)
> - && kgdb_may_fault) {
> + && kgdb_may_fault) {
> kgdb_fault_longjmp(kgdb_fault_jmp_regs);
> return NOTIFY_STOP;
> } else if (cmd == DIE_PAGE_FAULT)
> /* A normal page fault, ignore. */
> return NOTIFY_DONE;
> - else if ((cmd == DIE_NMI || cmd == DIE_NMI_IPI ||
> - cmd == DIE_NMIWATCHDOG) && atomic_read(&debugger_active)) {
> + else if ((cmd == DIE_NMI || cmd == DIE_NMI_IPI || cmd == DIE_NMIWATCHDOG)
> + && atomic_read(&debugger_active)) {
> /* CPU roundup */
> kgdb_nmihook(raw_smp_processor_id(), regs);
> return NOTIFY_STOP;
> - } else if (cmd == DIE_DEBUG
> - && atomic_read(&cpu_doing_single_step) == raw_smp_processor_id()
> - && user_mode(regs)) {
> + } else if (cmd == DIE_DEBUG &&
> + atomic_read(&cpu_doing_single_step) == raw_smp_processor_id()
> + && user_mode(regs)) {
> /* single step exception from kernel space to user space so
> * eat the exception and continue the process
> */
> printk(KERN_ERR "KGDB: trap/step from kernel to user space, resuming...\n");
> kgdb_arch_handle_exception(args->trapnr, args->signr, args->err, "c","",regs);
> return NOTIFY_STOP;
> - } else if (cmd == DIE_NMI_IPI || cmd == DIE_NMI || user_mode(regs) ||
> - (cmd == DIE_DEBUG && atomic_read(&debugger_active)))
> + } else if (cmd == DIE_NMI_IPI || cmd == DIE_NMI || user_mode(regs)
> + || (cmd == DIE_DEBUG && atomic_read(&debugger_active)))
> /* Normal watchdog event or userspace debugging, or spurious
> * debug exception, ignore. */
> return NOTIFY_DONE;
> Index: linux-2.6/arch/x86_64/kernel/kgdb.c
> ===================================================================
> --- linux-2.6.orig/arch/x86_64/kernel/kgdb.c
> +++ linux-2.6/arch/x86_64/kernel/kgdb.c
> @@ -127,7 +127,7 @@ void gdb_regs_to_regs(unsigned long *gdb
>
> } /* gdb_regs_to_regs */
>
> -struct hw_breakpoint {
> +static struct hw_breakpoint {
> unsigned enabled;
> unsigned type;
> unsigned len;
> @@ -158,19 +158,19 @@ static void kgdb_correct_hw_break(void)
> ((breakno << 2) + 16);
> switch (breakno) {
> case 0:
> - set_debugreg(breakinfo[breakno].addr, 0);
> + set_debugreg(breakinfo[0].addr, 0);
> break;
>
> case 1:
> - set_debugreg(breakinfo[breakno].addr, 1);
> + set_debugreg(breakinfo[1].addr, 1);
> break;
>
> case 2:
> - set_debugreg(breakinfo[breakno].addr, 2);
> + set_debugreg(breakinfo[2].addr, 2);
> break;
>
> case 3:
> - set_debugreg(breakinfo[breakno].addr, 3);
> + set_debugreg(breakinfo[3].addr, 3);
> break;
> }
> } else if ((dr7 & breakbit) && !breakinfo[breakno].enabled) {
> @@ -184,19 +184,17 @@ static void kgdb_correct_hw_break(void)
> }
>
> static int kgdb_remove_hw_break(unsigned long addr, int len,
> - enum kgdb_bptype bptype)
> + enum kgdb_bptype bptype)
> {
> - int i, idx = -1;
> - for (i = 0; i < 4; i++) {
> - if (breakinfo[i].addr == addr && breakinfo[i].enabled) {
> - idx = i;
> + int i;
> +
> + for (i = 0; i < 4; i++)
> + if (breakinfo[i].addr == addr && breakinfo[i].enabled)
> break;
> - }
> - }
> - if (idx == -1)
> + if (i == 4)
> return -1;
>
> - breakinfo[idx].enabled = 0;
> + breakinfo[i].enabled = 0;
> return 0;
> }
>
> @@ -204,42 +202,45 @@ static void kgdb_remove_all_hw_break(voi
> {
> int i;
>
> - for (i = 0; i < 4; i++) {
> + for (i = 0; i < 4; i++)
> memset(&breakinfo[i], 0, sizeof(struct hw_breakpoint));
> - }
> }
>
> static int kgdb_set_hw_break(unsigned long addr, int len,
> - enum kgdb_bptype bptype)
> + enum kgdb_bptype bptype)
> {
> - int i, idx = -1;
> - for (i = 0; i < 4; i++) {
> - if (!breakinfo[i].enabled) {
> - idx = i;
> + int i;
> + unsigned type;
> +
> + for (i = 0; i < 4; i++)
> + if (!breakinfo[i].enabled)
> break;
> - }
> - }
> - if (idx == -1)
> + if (i == 4)
> return -1;
> - if (bptype == bp_hardware_breakpoint) {
> - breakinfo[idx].type = 0;
> - breakinfo[idx].len = 0;
> - } else if (bptype == bp_write_watchpoint) {
> - breakinfo[idx].type = 1;
> - if (len == 1 || len == 2 || len == 4)
> - breakinfo[idx].len = len - 1;
> - else
> - return -1;
> - } else if (bptype == bp_access_watchpoint) {
> - breakinfo[idx].type = 3;
> - if (len == 1 || len == 2 || len == 4)
> - breakinfo[idx].len = len - 1;
> - else
> - return -1;
> - } else
> +
> + switch (bptype) {
> + case bp_hardware_breakpoint:
> + type = 0;
> + len = 1;
> + break;
> + case bp_write_watchpoint:
> + type = 1;
> + break;
> + case bp_access_watchpoint:
> + type = 3;
> + break;
> + default:
> return -1;
> - breakinfo[idx].enabled = 1;
> - breakinfo[idx].addr = addr;
> + }
> +
> + if (len == 1 || len == 2 || len == 4)
> + breakinfo[i].len = len - 1;
> + else
> + return -1;
> +
> + breakinfo[i].enabled = 1;
> + breakinfo[i].addr = addr;
> + breakinfo[i].type = type;
> return 0;
> }
>
> @@ -266,7 +267,6 @@ int kgdb_arch_handle_exception(int e_vec
> struct pt_regs *linux_regs)
> {
> unsigned long addr;
> - unsigned long breakno;
> char *ptr;
> int newPC;
> unsigned long dr6;
> @@ -282,8 +282,8 @@ int kgdb_arch_handle_exception(int e_vec
>
> /* clear the trace bit */
> linux_regs->eflags &= ~TF_MASK;
> -
> atomic_set(&cpu_doing_single_step, -1);
> +
> /* set the trace bit if we're stepping */
> if (remcomInBuffer[0] == 's') {
> linux_regs->eflags |= TF_MASK;
> @@ -296,47 +296,50 @@ int kgdb_arch_handle_exception(int e_vec
>
> get_debugreg(dr6, 6);
> if (!(dr6 & 0x4000)) {
> + int breakno;
> +
> for (breakno = 0; breakno < 4; ++breakno) {
> - if (dr6 & (1 << breakno)) {
> - if (breakinfo[breakno].type == 0) {
> - /* Set restore flag */
> - linux_regs->eflags |=
> - X86_EFLAGS_RF;
> - break;
> - }
> + if (dr6 & (1 << breakno) &&
> + breakinfo[breakno].type == 0) {
> + /* Set restore flag */
> + linux_regs->eflags |= X86_EFLAGS_RF;
> + break;
> }
> }
> }
> set_debugreg(0UL, 6);
> kgdb_correct_hw_break();
>
> - return (0);
> - } /* switch */
> + return 0;
> + }
> + /* this means that we do not want to exit from the handler */
> return -1;
> }
>
> static struct pt_regs *in_interrupt_stack(unsigned long rsp, int cpu)
> {
> - struct pt_regs *regs;
> + struct pt_regs *regs = NULL;
> unsigned long end = (unsigned long)cpu_pda(cpu)->irqstackptr;
> - if (rsp <= end && rsp >= end - IRQSTACKSIZE + 8) {
> +
> + if (rsp <= end && rsp >= end - IRQSTACKSIZE + 8)
> regs = *(((struct pt_regs **)end) - 1);
> - return regs;
> - }
> - return NULL;
> +
> + return regs;
> }
>
> static struct pt_regs *in_exception_stack(unsigned long rsp, int cpu)
> {
> - int i;
> struct tss_struct *init_tss = &__get_cpu_var(init_tss);
> + struct pt_regs *regs;
> + int i;
> +
> for (i = 0; i < N_EXCEPTION_STACKS; i++)
> if (rsp >= init_tss[cpu].ist[i] &&
> rsp <= init_tss[cpu].ist[i] + EXCEPTION_STKSZ) {
> - struct pt_regs *r =
> - (void *)init_tss[cpu].ist[i] + EXCEPTION_STKSZ;
> - return r - 1;
> + regs = (void *) init_tss[cpu].ist[i] + EXCEPTION_STKSZ;
> + return regs - 1;
> }
> +
> return NULL;
> }
>
> @@ -388,7 +391,7 @@ static int kgdb_notify(struct notifier_b
> struct pt_regs *regs = args->regs;
>
> if (cmd == DIE_PAGE_FAULT_NO_CONTEXT && atomic_read(&debugger_active)
> - && kgdb_may_fault) {
> + && kgdb_may_fault) {
> kgdb_fault_longjmp(kgdb_fault_jmp_regs);
> return NOTIFY_STOP;
> /* CPU roundup? */
> @@ -396,18 +399,17 @@ static int kgdb_notify(struct notifier_b
> kgdb_nmihook(raw_smp_processor_id(), regs);
> return NOTIFY_STOP;
> /* See if KGDB is interested. */
> - } else if (cmd == DIE_DEBUG
> - && atomic_read(&cpu_doing_single_step) == raw_smp_processor_id()
> - && user_mode(regs)) {
> + } else if (cmd == DIE_DEBUG &&
> + atomic_read(&cpu_doing_single_step) == raw_smp_processor_id()
> + && user_mode(regs)) {
> /* single step exception from kernel space to user space so
> * eat the exception and continue the process
> */
> printk(KERN_ERR "KGDB: trap/step from kernel to user space, resuming...\n");
> kgdb_arch_handle_exception(args->trapnr, args->signr, args->err, "c","",regs);
> return NOTIFY_STOP;
> - } else if (cmd == DIE_PAGE_FAULT || user_mode(regs) ||
> - cmd == DIE_NMI_IPI || (cmd == DIE_DEBUG &&
> - atomic_read(&debugger_active)))
> + } else if (cmd == DIE_PAGE_FAULT || user_mode(regs) || cmd == DIE_NMI_IPI
> + || (cmd == DIE_DEBUG && atomic_read(&debugger_active)))
> /* Userpace events, normal watchdog event, or spurious
> * debug exception. Ignore. */
> return NOTIFY_DONE;
> @@ -419,7 +421,7 @@ static int kgdb_notify(struct notifier_b
>
> static struct notifier_block kgdb_notifier = {
> .notifier_call = kgdb_notify,
> - .priority = 0x7fffffff, /* we need to notified first */
> + .priority = 0x7fffffff, /* we need to be notified first */
> };
>
> int kgdb_arch_init(void)
> @@ -427,6 +429,7 @@ int kgdb_arch_init(void)
> atomic_notifier_chain_register(&die_chain, &kgdb_notifier);
> return 0;
> }
> +
> /*
> * Skip an int3 exception when it occurs after a breakpoint has been
> * removed. Backtrack eip by 1 since the int3 would have caused it to
>
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems? Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >> http://get.splunk.com/
> _______________________________________________
> Kgdb-bugreport mailing list
> Kgd...@li...
> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
>
|