Messages in this thread | | | Date | Tue, 15 Mar 2022 12:30:46 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled |
| |
On Tue, Mar 15, 2022 at 03:21:12PM +0530, Maninder Singh wrote: > print module information when KALLSYMS is disabled. > > No change for %pB, as it needs to know symbol name to adjust address > value which can't be done without KALLSYMS.
...
> +int sprint_kallsym_common(char *buffer, unsigned long address, int build_id, > + int backtrace, int symbol) > +{ > + if (backtrace) > + return __sprint_symbol(buffer, address, -1, 1, build_id);
> + else if (symbol) > + return __sprint_symbol(buffer, address, 0, 1, build_id); > + else > + return __sprint_symbol(buffer, address, 0, 0, 0);
Redundant 'else' in both cases.
> +}
...
> +static int sprint_module_info(char *buf, char *end, unsigned long value, > + const char *fmt, int modbuildid, int backtrace, int symbol)
fmt is not used.
> +{ > + struct module *mod; > + unsigned long offset = 0;
> + unsigned long base;
Can it be the same type as core_layout.base? Why not?
> + char *modname; > + int len; > + const unsigned char *buildid = NULL; > + > + if (is_ksym_addr(value)) > + return 0; > + > + if (backtrace || symbol) > + offset = 1;
I would expect here to have
else offset = 0;
But see below.
> + preempt_disable(); > + mod = __module_address(value); > + if (mod) { > + modname = mod->name; > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > + if (modbuildid) > + buildid = mod->build_id; > +#endif
> + if (offset) {
This seems quite confusing because semantically you use offset as a boolean flag and offset. Why not add a boolean variable with a clear name?
> + base = (unsigned long)mod->core_layout.base; > + offset = value - base; > + } > + }
> +
Probably you can drop this blank line to group entire critical section, or add a blank line above.
> + preempt_enable(); > + if (!mod) > + return 0; > + > + /* address belongs to module */ > + if (offset) > + len = sprintf(buf, "0x%p+0x%lx", (void *)base, offset); > + else
> + len = sprintf(buf, "0x%lx", (void *)value);
What this casting is for? Don't you have a compilation warning?
> + len += fill_name_build_id(buf, modname, modbuildid, buildid, len); > + return len;
return len + ...;
?
> +}
-- With Best Regards, Andy Shevchenko
| |