On Tue, Jun 20, 2023 at 3:47 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@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