On Tue, Jun 20, 2023 at 3:47 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> 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:
+ 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) {
+ 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)
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) {
{
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
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki
