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