[Crash-utility] [PATCH] Support module memory layout change on Linux 6.4
lijiang
lijiang at redhat.com
Tue Jun 20 11:14:27 UTC 2023
On Tue, Jun 20, 2023 at 3:47 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab at nec.com>
wrote:
> Hi Lianbo,
>
> thank you for the review.
>
> On 2023/06/20 10:46, lijiang wrote:
>
> >>> +#define for_each_mod_mem_type(type) \
> >>> + for (int (type) = MOD_TEXT; (type) < MOD_MEM_NUM_TYPES;
> (type)++)
>
> I found that this cannot build with an old gcc, e.g. on RHEL7.
> Please note that -std=gnu99 or later is required for such a gcc.
>
> $ gcc --version
> gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44)
> ...
> $ make -j 16 warn CFLAGS='-std=gnu99'
> ...
> ar: creating crashlib.a
> CXXLD gdb
> $ make clean
> ...
> $ make -j 16 warn
> ...
> In file included from symbols.c:18:0:
> symbols.c: In function 'module_symbol_dump':
> defs.h:3007:2: error: 'for' loop initial declarations are only allowed
> in C99 mode
> for (int (type) = MOD_TEXT; (type) < MOD_MEM_NUM_TYPES; (type)++)
> ^
> symbols.c:1352:3: note: in expansion of macro 'for_each_mod_mem_type'
> for_each_mod_mem_type(t) {
> ^
> defs.h:3007:2: note: use option -std=c99 or -std=gnu99 to compile your code
> for (int (type) = MOD_TEXT; (type) < MOD_MEM_NUM_TYPES; (type)++)
> ^
> symbols.c:1352:3: note: in expansion of macro 'for_each_mod_mem_type'
> for_each_mod_mem_type(t) {
> ^
>
It should be good to define the above macro like this:
+#define for_each_mod_mem_type(type) \
+ for (type = MOD_TEXT; type < MOD_MEM_NUM_TYPES; type++)
+
And also define a variable ' int t' before using the for-loop macro. For
example:
+ int t;
......
+ for_each_mod_mem_type(t) {
+ ...
+ }
That can fix the current error.
In addition, I can see many similar definitions and usages in kernel code:
#define list_for_each_safe(pos, n, head) \
for (pos = (head)->next, n = pos->next; \
!list_is_head(pos, (head)); \
pos = n, n = pos->next)
static void free_devices(struct list_head *devices, struct mapped_device
*md)
{
struct list_head *tmp, *next;
list_for_each_safe(tmp, next, devices) {
...
}
...
>
> >>> +--- gdb-10.2/gdb/symtab.c.orig
> >>> ++++ gdb-10.2/gdb/symtab.c
> >>> +@@ -7515,8 +7515,11 @@ gdb_add_symbol_file(struct gnu_request *
> >>> + secname = lm->mod_section_data[i].name;
> >>> + if ((lm->mod_section_data[i].flags &
> >>> SEC_FOUND) &&
> >>> + !STREQ(secname, ".text")) {
> >>> +- sprintf(buf, " -s %s 0x%lx",
> >>> secname,
> >>> +-
> lm->mod_section_data[i].offset
> >>> + lm->mod_base);
> >>> ++ if (lm->mod_section_data[i].addr)
> >>> ++ sprintf(buf, " -s %s 0x%lx",
> >>> secname, lm->mod_section_data[i].addr);
> >>> ++ else
> >>> ++ sprintf(buf, " -s %s 0x%lx",
> >>> secname,
> >>> ++
> >>> lm->mod_section_data[i].offset + lm->mod_base);
> >>> + strcat(req->buf, buf);
> >>> + }
> >>> + }
> >>>
> >>
> > BTW: I can still get the following warnings:
> > ...
> > CXX symtab.o
> > symtab.c: In function ‘void gdb_command_funnel_1(gnu_request*)’:
> > symtab.c:7519:64: warning: ‘%lx’ directive writing between 1 and 16 bytes
> > into a region of size between 10 and 73 [-Wformat-overflow=]
> > 7519 | sprintf(buf, " -s %s
> > 0x%lx", secname, lm->mod_section_data[i].addr);
> > |
> ^~~
> > symtab.c:7519:54: note: directive argument in the range [1,
> > 18446744073709551615]
> > 7519 | sprintf(buf, " -s %s
> > 0x%lx", secname, lm->mod_section_data[i].addr);
> > |
> ^~~~~~~~~~~~~~
>
> Oops, thanks, I missed this. will fix.
>
> >>> +static int module_mem_type(ulong, struct load_module *);
> >>> +static ulong module_mem_end(ulong, struct load_module *);
> >>> +static int in_module_range(ulong, struct load_module *, int, int);
> >>> +struct syment *value_search_module_6_4(ulong, ulong *);
> >>> +struct syment *next_symbol_by_symname(char *);
> >>> +struct syment *prev_symbol_by_symname(char *);
> >>> +struct syment *next_module_symbol_by_value(ulong);
> >>> +struct syment *prev_module_symbol_by_value(ulong);
> >>> +struct syment *next_module_symbol_by_syment(struct syment *);
> >>> +struct syment *prev_module_symbol_by_syment(struct syment *);
> >>> +
> >>>
> >>
> >> The above functions are only used in the symbols.c, It should be good to
> >> add a 'static' keyword to them.
>
> Agreed.
>
> >>> +/* val_in should be a pseudo module end symbol. */
> >>> +struct syment *
> >>> +next_module_symbol_by_value(ulong val_in)
> >>> +{
> >>> + struct load_module *lm;
> >>> + struct syment *sp, *sp_end;
> >>> + ulong start, min;
> >>> + int i;
> >>> +
> >>> +retry:
> >>> + sp = sp_end = NULL;
> >>> + min = (ulong)-1;
> >>> + for (i = 0; i < st->mods_installed; i++) {
> >>> + lm = &st->load_modules[i];
> >>> +
> >>> + /* Search for the next lowest symtable. */
> >>> + for_each_mod_mem_type(t) {
> >>> + if (!lm->symtable[t])
> >>> + continue;
> >>> +
> >>> + start = lm->symtable[t]->value;
> >>> + if (start > val_in && start < min) {
> >>> + min = start;
> >>> + sp = lm->symtable[t];
> >>> + sp_end = lm->symend[t];
> >>> + }
> >>> + }
> >>> + }
> >>> +
> >>> + if (!sp)
> >>> + return NULL;
> >>> +
> >>> + for ( ; sp < sp_end; sp++) {
> >>> + if (MODULE_PSEUDO_SYMBOL(sp))
> >>> + continue;
> >>> + if (sp->value > val_in)
> >>> + return sp;
> >>> + }
> >>> +
> >>> + /* Found a table that has only pseudo symbols. */
> >>> + val_in = sp_end->value;
> >>> + goto retry;
> >>>
> >>
> >> Is it possible for 'retry' to become an infinite loop? And there is
> also a
> >> similar 'retry' in the prev_module_symbol_by_value().
>
> No, it should return NULL if (!sp) as above, i.e. there is no
> higher/lower module symbol.
>
>
Got it, thank you for the explanation, Kazu.
Thanks.
Lianbo
> For example, it tested ok with the highest module symbol:
>
> crash-6.4> sym -M | sort | tail
> ffffffffc2d242c0 (r) rd_ptr.16
> ffffffffc2d24300 (r) suffixes.15
> ffffffffc2d24340 (r) tjmax_model_table
> ffffffffc2d24380 (r) tjmax_pci_table
> ffffffffc2d243a0 (r) __param_str_tjmax
> ffffffffc2d243a8 (r) __param
> ffffffffc2d243a8 (r) __param_tjmax
> ffffffffc2d25000 MODULE RODATA END: coretemp
> ffffffffc2d26000 MODULE RO_AFTER_INIT START: coretemp
> ffffffffc2d27000 MODULE RO_AFTER_INIT END: coretemp
> crash-6.4> sym -n __param_tjmax
> ffffffffc2d243a8 (r) __param_tjmax [coretemp]
> crash-6.4>
>
> Thanks,
> Kazu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20230620/1bb5ac3d/attachment-0001.htm>
More information about the Crash-utility
mailing list