Re: [PATCH] ctf-reader: Lookup debug info for symbols in a non default archive member

[Dodji Seketeli <dodji@seketeli.org>]

Hello Guillermo,

Thanks for the patch.  I have tested and it seems to pass regression
testing on my system.  However, there are some things that I don't
understand so I have some questions below.  The questions are just for
my own understanding.  I don't have anything major against the patch,
obviously.

[...]

"Guillermo E. Martinez via Libabigail" <libabigail@sourceware.org> a
écrit:


[...]

> +/// Given a symbol name, lookup the corresponding CTF information in
> +/// the default dictionary (CTF archive member provided by the caller)
> +/// If the search is not success, the  looks for the symbol name
> +/// in _all_ archive members.
> +///
> +/// @param ctfa the CTF archive.
> +/// @param dict the default dictionary to looks for.
> +/// @param sym_name the symbol name.
> +/// @param corp the IR corpus.
> +///
> +/// Note that if @ref sym_name is found in other than default dictionary
> +/// @ref ctf_dict will be updated and it must be explicate closed by its
> +/// caller.
> +///
> +/// @return a valid CTF type id, if @ref sym_name was found, -1 otherwise.
> +
> +static ctf_id_t
> +lookup_symbol_in_ctf_archive(ctf_archive_t *ctfa, ctf_dict_t **ctf_dict,
> +                             const char *sym_name, corpus_sptr corp)
> +{
> +  int ctf_err;
> +  ctf_dict_t *dict = *ctf_dict;
> +  ctf_id_t ctf_type = ctf_lookup_variable(dict, sym_name);

So, here, we begin by looking for a variable (using ctf_lookup_variable)
which ELF symbol is sym_name, is that correct?

> +
> +  /* lookup CTF type for a given symbol in its default
> +     dictionary */
> +  if (ctf_type == (ctf_id_t) -1

So, I guess the variable lookup failed, right?

> +      && !(corp->get_origin() & corpus::LINUX_KERNEL_BINARY_ORIGIN))

Why this condition?  Why only considering cases where we are not looking
at a Linux Kernel binary?  I would think that we would want to consider
the case where the variable lookup failed, even in the case of a Linux
Kernel binary, wouldn't we? If not why?  Maybe we should add a comment
to explain this.

> +    ctf_type = ctf_lookup_by_symbol_name(dict, sym_name);

So I am guessing that ctf_lookup_by_symbol_name looks up both variable
and function symbols from the same dictionary, is that correct?
Also, I don't understand why we don't just use ctf_lookup_by_symbol_name
rather than starting with ctf_lookup_variable first.  Is it a
performance things?


Incidentally, I haven't found documentation for the lookup functions
other than by looking at the code, in say:
https://sourceware.org/git/?p=binutils-gdb.git;a=blob_plain;f=libctf/ctf-lookup.c;hb=refs/heads/master.

If there is documentation for it somewhere else, maybe we can link that
place in the code here in a comment somewhere, or we can just point to
that link above.  Both would be fine by me.

> +
> +  /* Not lucky, then, search in whole archive */
> +  if (ctf_type == (ctf_id_t) -1)
> +    {
> +      ctf_dict_t *fp;
> +      ctf_next_t *i = NULL;
> +      const char *arcname;
> +
> +      while ((fp = ctf_archive_next(ctfa, &i, &arcname, 1, &ctf_err)) != NULL)
> +        {
> +          ctf_type = ctf_lookup_variable (fp, sym_name);
> +          if (ctf_type == (ctf_id_t) -1
> +              && !(corp->get_origin() & corpus::LINUX_KERNEL_BINARY_ORIGIN))

The same questions as above.

> +            ctf_type = ctf_lookup_by_symbol_name(fp, sym_name);
> +
> +          if (ctf_type != (ctf_id_t) -1)
> +            {
> +              *ctf_dict = fp;
> +              break;
> +            }
> +          ctf_dict_close(fp);
> +        }
> +    }
> +
> +  return ctf_type;
> +}
> +

Cheers,

[...]


-- 
		Dodji