[PATCH v3] kdb: kdb_support: Fix debugging information problem

[PATCH v3] kdb: kdb_support: Fix debugging information problem

[Stephen Zhang]

There are several common patterns.

0:
        kdb_printf("...",...);

which is the normal one.

1:
        kdb_printf("%s: "...,__func__,...)

We could improve '1' to this :

        #define kdb_func_printf(format, args...) \
                   kdb_printf("%s: " format, __func__, ## args)

2:
        if(KDB_DEBUG(AR))
                kdb_printf("%s "...,__func__,...);

We could improve '2' to this :
        #define kdb_dbg_printf(mask, format, args...) \
                           do { \
                                        if (KDB_DEBUG(mask)) \
                                                kdb_func_printf(format, ## args); \
                           } while (0)

In additon, we changed the format code of size_t to %zu.

Signed-off-by: Stephen Zhang <stephenzhangzsd@gmail.com>
---
v1->v2 Changelog:
- Add 'mask' parameter in kdb_dbg_printf()

Thanks to Daniel and Doug's suggestions and review.

v2->v3 Changelog:
- Adjust alignment for some lines.

Suggested by Douglas Anderson.

 kernel/debug/kdb/kdb_private.h | 10 ++++++++
 kernel/debug/kdb/kdb_support.c | 53 ++++++++++++++++++------------------------
 2 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index a4281fb..0a56d35 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -254,4 +254,14 @@ extern unsigned long kdb_task_state(const struct task_struct *p,
 #define	KDB_WORD_SIZE	((int)sizeof(unsigned long))
 
 #endif /* CONFIG_KGDB_KDB */
+
+#define kdb_func_printf(format, args...) \
+	kdb_printf("%s: " format, __func__, ## args)
+
+#define kdb_dbg_printf(mask, format, args...) \
+	do { \
+		if (KDB_DEBUG(mask)) \
+			kdb_func_printf(format, ## args); \
+	} while (0)
+
 #endif	/* !_KDBPRIVATE_H */
diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index 6226502..99a6232 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -39,20 +39,15 @@
  */
 int kdbgetsymval(const char *symname, kdb_symtab_t *symtab)
 {
-	if (KDB_DEBUG(AR))
-		kdb_printf("kdbgetsymval: symname=%s, symtab=%px\n", symname,
-			   symtab);
+	kdb_dbg_printf(AR, "symname=%s, symtab=%px\n", symname, symtab);
 	memset(symtab, 0, sizeof(*symtab));
 	symtab->sym_start = kallsyms_lookup_name(symname);
 	if (symtab->sym_start) {
-		if (KDB_DEBUG(AR))
-			kdb_printf("kdbgetsymval: returns 1, "
-				   "symtab->sym_start=0x%lx\n",
-				   symtab->sym_start);
+		kdb_dbg_printf(AR, "returns 1,symtab->sym_start=0x%lx\n",
+			      symtab->sym_start);
 		return 1;
 	}
-	if (KDB_DEBUG(AR))
-		kdb_printf("kdbgetsymval: returns 0\n");
+	kdb_dbg_printf(AR, "returns 0\n");
 	return 0;
 }
 EXPORT_SYMBOL(kdbgetsymval);
@@ -87,15 +82,14 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
 #define knt1_size 128		/* must be >= kallsyms table size */
 	char *knt1 = NULL;
 
-	if (KDB_DEBUG(AR))
-		kdb_printf("kdbnearsym: addr=0x%lx, symtab=%px\n", addr, symtab);
+	kdb_dbg_printf(AR, "addr=0x%lx, symtab=%px\n", addr, symtab);
 	memset(symtab, 0, sizeof(*symtab));
 
 	if (addr < 4096)
 		goto out;
 	knt1 = debug_kmalloc(knt1_size, GFP_ATOMIC);
 	if (!knt1) {
-		kdb_printf("kdbnearsym: addr=0x%lx cannot kmalloc knt1\n",
+		kdb_func_printf("addr=0x%lx cannot kmalloc knt1\n",
 			   addr);
 		goto out;
 	}
@@ -147,11 +141,10 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
 
 	if (symtab->mod_name == NULL)
 		symtab->mod_name = "kernel";
-	if (KDB_DEBUG(AR))
-		kdb_printf("kdbnearsym: returns %d symtab->sym_start=0x%lx, "
-		   "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
-		   symtab->sym_start, symtab->mod_name, symtab->sym_name,
-		   symtab->sym_name);
+	kdb_dbg_printf(AR, "returns %d symtab->sym_start=0x%lx, "
+		"symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
+		symtab->sym_start, symtab->mod_name, symtab->sym_name,
+		symtab->sym_name);
 
 out:
 	debug_kfree(knt1);
@@ -328,7 +321,7 @@ int kdb_getarea_size(void *res, unsigned long addr, size_t size)
 	int ret = copy_from_kernel_nofault((char *)res, (char *)addr, size);
 	if (ret) {
 		if (!KDB_STATE(SUPPRESS)) {
-			kdb_printf("kdb_getarea: Bad address 0x%lx\n", addr);
+			kdb_func_printf("Bad address 0x%lx\n", addr);
 			KDB_STATE_SET(SUPPRESS);
 		}
 		ret = KDB_BADADDR;
@@ -353,7 +346,7 @@ int kdb_putarea_size(unsigned long addr, void *res, size_t size)
 	int ret = copy_from_kernel_nofault((char *)addr, (char *)res, size);
 	if (ret) {
 		if (!KDB_STATE(SUPPRESS)) {
-			kdb_printf("kdb_putarea: Bad address 0x%lx\n", addr);
+			kdb_func_printf("Bad address 0x%lx\n", addr);
 			KDB_STATE_SET(SUPPRESS);
 		}
 		ret = KDB_BADADDR;
@@ -435,7 +428,7 @@ int kdb_getphysword(unsigned long *word, unsigned long addr, size_t size)
 		fallthrough;
 	default:
 		diag = KDB_BADWIDTH;
-		kdb_printf("kdb_getphysword: bad width %ld\n", (long) size);
+		kdb_func_printf("bad width %zu\n", size);
 	}
 	return diag;
 }
@@ -484,7 +477,7 @@ int kdb_getword(unsigned long *word, unsigned long addr, size_t size)
 		fallthrough;
 	default:
 		diag = KDB_BADWIDTH;
-		kdb_printf("kdb_getword: bad width %ld\n", (long) size);
+		kdb_func_printf("bad width %zu\n", size);
 	}
 	return diag;
 }
@@ -528,7 +521,7 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
 		fallthrough;
 	default:
 		diag = KDB_BADWIDTH;
-		kdb_printf("kdb_putword: bad width %ld\n", (long) size);
+		kdb_func_printf("bad width %zu\n", size);
 	}
 	return diag;
 }
@@ -602,8 +595,7 @@ unsigned long kdb_task_state_string(const char *s)
 			res = ~0UL;
 			break;
 		default:
-			  kdb_printf("%s: unknown flag '%c' ignored\n",
-				     __func__, *s);
+			  kdb_func_printf("unknown flag '%c' ignored\n", *s);
 			  break;
 		}
 		++s;
@@ -884,18 +876,17 @@ void debug_kusage(void)
 	if (!debug_kusage_one_time)
 		goto out;
 	debug_kusage_one_time = 0;
-	kdb_printf("%s: debug_kmalloc memory leak dah_first %d\n",
-		   __func__, dah_first);
+	kdb_func_printf("debug_kmalloc memory leak dah_first %d\n", dah_first);
 	if (dah_first) {
 		h_used = (struct debug_alloc_header *)debug_alloc_pool;
-		kdb_printf("%s: h_used %px size %d\n", __func__, h_used,
+		kdb_func_printf("h_used %px size %d\n", h_used,
 			   h_used->size);
 	}
 	do {
 		h_used = (struct debug_alloc_header *)
 			  ((char *)h_free + dah_overhead + h_free->size);
-		kdb_printf("%s: h_used %px size %d caller %px\n",
-			   __func__, h_used, h_used->size, h_used->caller);
+		kdb_func_printf("h_used %px size %d caller %px\n",
+			  h_used, h_used->size, h_used->caller);
 		h_free = (struct debug_alloc_header *)
 			  (debug_alloc_pool + h_free->next);
 	} while (h_free->next);
@@ -903,8 +894,8 @@ void debug_kusage(void)
 		  ((char *)h_free + dah_overhead + h_free->size);
 	if ((char *)h_used - debug_alloc_pool !=
 	    sizeof(debug_alloc_pool_aligned))
-		kdb_printf("%s: h_used %px size %d caller %px\n",
-			   __func__, h_used, h_used->size, h_used->caller);
+		kdb_func_printf("h_used %px size %d caller %px\n",
+			   h_used, h_used->size, h_used->caller);
 out:
 	spin_unlock(&dap_lock);
 }
-- 
1.8.3.1

Re: [PATCH v3] kdb: kdb_support: Fix debugging information problem

[Doug Anderson]

Hi,

On Sat, Jan 30, 2021 at 3:24 AM Stephen Zhang <stephenzhangzsd@gmail.com> wrote:
>
>  int kdbgetsymval(const char *symname, kdb_symtab_t *symtab)
>  {
> -       if (KDB_DEBUG(AR))
> -               kdb_printf("kdbgetsymval: symname=%s, symtab=%px\n", symname,
> -                          symtab);
> +       kdb_dbg_printf(AR, "symname=%s, symtab=%px\n", symname, symtab);
>         memset(symtab, 0, sizeof(*symtab));
>         symtab->sym_start = kallsyms_lookup_name(symname);
>         if (symtab->sym_start) {
> -               if (KDB_DEBUG(AR))
> -                       kdb_printf("kdbgetsymval: returns 1, "
> -                                  "symtab->sym_start=0x%lx\n",
> -                                  symtab->sym_start);
> +               kdb_dbg_printf(AR, "returns 1,symtab->sym_start=0x%lx\n",

nit: There used to be a space after the "," and there no longer is.


> +                             symtab->sym_start);

nit: I think it was supposed to be indented 1 more space so it's under "AR".


>  EXPORT_SYMBOL(kdbgetsymval);
> @@ -87,15 +82,14 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
>  #define knt1_size 128          /* must be >= kallsyms table size */
>         char *knt1 = NULL;
>
> -       if (KDB_DEBUG(AR))
> -               kdb_printf("kdbnearsym: addr=0x%lx, symtab=%px\n", addr, symtab);
> +       kdb_dbg_printf(AR, "addr=0x%lx, symtab=%px\n", addr, symtab);
>         memset(symtab, 0, sizeof(*symtab));
>
>         if (addr < 4096)
>                 goto out;
>         knt1 = debug_kmalloc(knt1_size, GFP_ATOMIC);
>         if (!knt1) {
> -               kdb_printf("kdbnearsym: addr=0x%lx cannot kmalloc knt1\n",
> +               kdb_func_printf("addr=0x%lx cannot kmalloc knt1\n",
>                            addr);

nit: "addr" used to be indented properly before your change and now
it's not.  It could also move up to the previous line.


> @@ -147,11 +141,10 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
>
>         if (symtab->mod_name == NULL)
>                 symtab->mod_name = "kernel";
> -       if (KDB_DEBUG(AR))
> -               kdb_printf("kdbnearsym: returns %d symtab->sym_start=0x%lx, "
> -                  "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
> -                  symtab->sym_start, symtab->mod_name, symtab->sym_name,
> -                  symtab->sym_name);
> +       kdb_dbg_printf(AR, "returns %d symtab->sym_start=0x%lx, "
> +               "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
> +               symtab->sym_start, symtab->mod_name, symtab->sym_name,
> +               symtab->sym_name);

This indention doesn't line up, but it didn't before.  I guess I'd let
Daniel say if he likes this or would prefer different wrapping /
indentation.


> @@ -884,18 +876,17 @@ void debug_kusage(void)
>         if (!debug_kusage_one_time)
>                 goto out;
>         debug_kusage_one_time = 0;
> -       kdb_printf("%s: debug_kmalloc memory leak dah_first %d\n",
> -                  __func__, dah_first);
> +       kdb_func_printf("debug_kmalloc memory leak dah_first %d\n", dah_first);
>         if (dah_first) {
>                 h_used = (struct debug_alloc_header *)debug_alloc_pool;
> -               kdb_printf("%s: h_used %px size %d\n", __func__, h_used,
> +               kdb_func_printf("h_used %px size %d\n", h_used,
>                            h_used->size);

nit: "h_used->size" used to be indented properly before your change
and now it's not.  It could also move up to the previous line.

>         }
>         do {
>                 h_used = (struct debug_alloc_header *)
>                           ((char *)h_free + dah_overhead + h_free->size);
> -               kdb_printf("%s: h_used %px size %d caller %px\n",
> -                          __func__, h_used, h_used->size, h_used->caller);
> +               kdb_func_printf("h_used %px size %d caller %px\n",
> +                         h_used, h_used->size, h_used->caller);

nit: "h_used" used to be indented properly before your change and now it's not.


> @@ -903,8 +894,8 @@ void debug_kusage(void)
>                   ((char *)h_free + dah_overhead + h_free->size);
>         if ((char *)h_used - debug_alloc_pool !=
>             sizeof(debug_alloc_pool_aligned))
> -               kdb_printf("%s: h_used %px size %d caller %px\n",
> -                          __func__, h_used, h_used->size, h_used->caller);
> +               kdb_func_printf("h_used %px size %d caller %px\n",
> +                          h_used, h_used->size, h_used->caller);

nit: "h_used" used to be indented properly before your change and now it's not.


It's possible that Daniel would prefer to fix these word-wrapping and
indention things when he applies your change?

I know the above is all pretty nitty, but given that the whole point
of the change is to clean up the code it seems like it's fair game to
make sure the code touched is fully clean...

-Doug

Re: [PATCH v3] kdb: kdb_support: Fix debugging information problem

[Doug Anderson]

Hi,

On Tue, Feb 2, 2021 at 3:15 AM Stephen Zhang <stephenzhangzsd@gmail.com> wrote:
>>
>>
>> > @@ -147,11 +141,10 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
>> >
>> >         if (symtab->mod_name == NULL)
>> >                 symtab->mod_name = "kernel";
>> > -       if (KDB_DEBUG(AR))
>> > -               kdb_printf("kdbnearsym: returns %d symtab->sym_start=0x%lx, "
>> > -                  "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
>> > -                  symtab->sym_start, symtab->mod_name, symtab->sym_name,
>> > -                  symtab->sym_name);
>> > +       kdb_dbg_printf(AR, "returns %d symtab->sym_start=0x%lx, "
>> > +               "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
>> > +               symtab->sym_start, symtab->mod_name, symtab->sym_name,
>> > +               symtab->sym_name);
>>
>> This indention doesn't line up, but it didn't before.  I guess I'd let
>> Daniel say if he likes this or would prefer different wrapping /
>> indentation.
>
>
> Thanks for your patience. You told me that  the alignment for continued arguments is to
> match up with the first argument, so I was confused that the first line and the second line
> here don't follow the rule.  There are many  cases like this in  this file.

There's a saying: all rules are made to be broken.

I think the general rule is that the 2nd line of arguments should line
up with the first.  However, sometimes that forces way too much
wrapping.  If it's "too ugly" to use the general rule, then you can
fall back to some alternate rule.  Usually this alternate rule is
something like indending all subsequent lines by one tab.  The
definition of "too ugly" is left to the judgement of the person
writing / reviewing the code.  In this case maybe the general rule
would make your call need to take up 3 lines?

If I had to make a judgement call on this code, I'd say:

1. It seems to have been written before the "don't split strings" rule
was in full force.  See
<https://www.kernel.org/doc/html/v5.10/process/coding-style.html#breaking-long-lines-and-strings>
where it says "never break user-visible strings such as printk
messages because that breaks the ability to grep for them".

2. If we fix #1, we're already blowing over the 80 columns limit for
this string anyway.

3. Once we blow over the 80 columns, might as well do it for the
second line.  So then you'd end up with:

<tab>kdb_dbg_printf(AR, "returns [...] (%s)\n",
<tab><tab>       ret, symtab->sym_start, [...], symtab->sym_name);

Re: [PATCH v3] kdb: kdb_support: Fix debugging information problem

[Doug Anderson]

Hi,

On Wed, Feb 3, 2021 at 3:31 AM Stephen Zhang <stephenzhangzsd@gmail.com> wrote:
>
> Doug Anderson <dianders@chromium.org> 于2021年2月3日周三 上午1:40写道：
>>
>> <tab>kdb_dbg_printf(AR, "returns [...] (%s)\n",
>> <tab><tab>       ret, symtab->sym_start, [...], symtab->sym_name);
>
>
> Thank you for the detailed explanation. In this case, Shouldn't the "ret"  be under
> the "(AR"? like below:
>
> <tab>kdb_dbg_printf(AR, "returns [...] (%s)\n",
> <tab><tab>               ret, symtab->sym_start, [...], symtab->sym_name);
>
>  See
> <https://www.kernel.org/doc/html/v5.10/process/coding-style.html#breaking-long-lines-and-strings>
> where it says "A very commonly used style is to align descendants to a function open parenthesis".
> The "descendants" here means the next line, right?

Yes, "descendants" means the next line.

I have a guess about what your problem is.  In kernel land, tabs are 8
spaces, not 4.  Also make sure your editor is using a fixed-width
font.  In gmail my example might not look like it's lining up, but
when you go into the editor it should.

-Doug