From: "Guillermo E. Martinez" <guillermo.e.martinez@oracle.com>
To: Dodji Seketeli <dodji@seketeli.org>,
"Guillermo E. Martinez via Libabigail"
<libabigail@sourceware.org>
Subject: Re: [PATCH] ctf-reader: Lookup debug info for symbols in a non default archive member
Date: Wed, 7 Sep 2022 13:40:34 -0500 [thread overview]
Message-ID: <b9a6a56d-6624-a55a-a064-3f294c5659f6@oracle.com> (raw)
In-Reply-To: <87k06gis6h.fsf@seketeli.org>
On 9/6/22 07:49, Dodji Seketeli wrote:
> Hello Guillermo,
Hello Dodji,
> 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?
That's correct, `sym_name' is the symbol name.
>> +
>> + /* 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?
Correct, libctf `ctf_lookup_*' functions return CTF_ERR when fails,
so I'm goinf to changed it for clarity.
>> + && !(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.
OK. The linker (ld) in the Kenel build mechanism uses: `--ctf-variables',
then it emits the symbols type definitions using just the CTF Variable
ection:
$ objdump --ctf foo
...
Labels:
Data objects:
Function objects:
Variables:
main -> 0x2: (kind 5) int (*) () (aligned at 0x8)
main_func -> 0x4: (kind 5) void (*) () (aligned at 0x8)
okkk -> 0x1: (kind 1) int (format 0x1) (size 0x4) (aligned at 0x4)
Otherwise, it must be splitted across CTF Data, Function and Variable
sections:
$ objdump --ctf foo.o
Data objects:
okkk -> 0x1: (kind 1) int (format 0x1) (size 0x4) (aligned at 0x4)
Function objects:
main -> 0x2: (kind 5) int (*) () (aligned at 0x8)
main_func -> 0x4: (kind 5) void (*) () (aligned at 0x8)
Variables:
okkk -> 0x1: (kind 1) int (format 0x1) (size 0x4) (aligned at 0x4)
Since, vmlinux + *.ko, is *big* binary, I arranged the order of CTF
lookup functions invoking at first: 'ctf_lookup_variable` and then,
if it fails `ctf_lookup_by_symbol_name' by performance reasons.
But I'm agree to remove `!(corp->get_origin() & corpus::LINUX_KERNEL_BINARY_ORIGIN))'
changing the invocation order for those functions, the penalty performance
was less than 10s building the ABI representation for the kernel,
I consider it as acceptable.
>> + 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?
True.
> 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?
Exactly. Performance when we are processing a Linux tree directory.
> 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.
I'm afraid that the documentation is just in the source code.
> 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,
>
> [...]
>
>
Really thanks for your comments!,
I will prepare the v2
Kind regards,
guillermo
next prev parent reply other threads:[~2022-09-07 18:40 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-31 15:16 Guillermo E. Martinez
2022-08-31 15:16 ` Guillermo E. Martinez
2022-09-06 12:49 ` Dodji Seketeli
2022-09-07 18:40 ` Guillermo E. Martinez [this message]
2022-09-07 23:40 ` [PATCHv v2] " Guillermo E. Martinez
2022-09-13 9:26 ` Dodji Seketeli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b9a6a56d-6624-a55a-a064-3f294c5659f6@oracle.com \
--to=guillermo.e.martinez@oracle.com \
--cc=dodji@seketeli.org \
--cc=libabigail@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).