From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::228]) by sourceware.org (Postfix) with ESMTPS id 6C7A0385354A for ; Tue, 6 Sep 2022 12:49:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6C7A0385354A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=seketeli.org Received: (Authenticated sender: dodji@seketeli.org) by mail.gandi.net (Postfix) with ESMTPSA id 3B43E1BF20D; Tue, 6 Sep 2022 12:49:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=seketeli.org; s=gm1; t=1662468568; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8lFJ5LO6Kgz5MftAZbJukFCxRLndV7H5bxr8DTYn/S0=; b=V4sox+L4DK+bQtDTsG1noudthEwn2QsrcRTyuZ+WIij/IwpQJxmuDYumXw8155fWMh55dm kdnmi91bB2cVRb2ip54OB7IAhOsfxC64i0rWxuO89QcXHYJumNFGtqUrEiOY5qYTo025mK NuNGqMeNbXdGcvmB/26dA3MmspMZ1WaggifMZ/HrpD8n/l8oQelz7xd9wzm9PbtxaUln/D HTLHxh6kZ5+eKDdOXg4p+sojF/Uo+rBr9FAmo84nvlOF0+fdU/7ZNidij8hCsM16Dnf56A yYFezG8OP14FhvR2gajxx5Im1Lwi2+gPcD8OGEtYP/fBVRTbEpKl38NfJIX+oQ== Received: by localhost (Postfix, from userid 1000) id 2C63D5802BD; Tue, 6 Sep 2022 14:49:27 +0200 (CEST) From: Dodji Seketeli To: "Guillermo E. Martinez via Libabigail" Cc: "Guillermo E. Martinez" Subject: Re: [PATCH] ctf-reader: Lookup debug info for symbols in a non default archive member Organization: Me, myself and I References: <20220831151603.915945-1-guillermo.e.martinez@oracle.com> X-Operating-System: Fedora 38 X-URL: http://www.seketeli.net/~dodji Date: Tue, 06 Sep 2022 14:49:26 +0200 In-Reply-To: <20220831151603.915945-1-guillermo.e.martinez@oracle.com> (Guillermo E. Martinez via Libabigail's message of "Wed, 31 Aug 2022 10:16:03 -0500") Message-ID: <87k06gis6h.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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" a =C3=A9crit: [...] > +/// 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 otherwis= e. > + > +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 =3D *ctf_dict; > + ctf_id_t ctf_type =3D 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 =3D=3D (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 =3D 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=3Dbinutils-gdb.git;a=3Dblob_plain;f=3Dlibctf/= ctf-lookup.c;hb=3Drefs/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 =3D=3D (ctf_id_t) -1) > + { > + ctf_dict_t *fp; > + ctf_next_t *i =3D NULL; > + const char *arcname; > + > + while ((fp =3D ctf_archive_next(ctfa, &i, &arcname, 1, &ctf_err)) = !=3D NULL) > + { > + ctf_type =3D ctf_lookup_variable (fp, sym_name); > + if (ctf_type =3D=3D (ctf_id_t) -1 > + && !(corp->get_origin() & corpus::LINUX_KERNEL_BINARY_ORIG= IN)) The same questions as above. > + ctf_type =3D ctf_lookup_by_symbol_name(fp, sym_name); > + > + if (ctf_type !=3D (ctf_id_t) -1) > + { > + *ctf_dict =3D fp; > + break; > + } > + ctf_dict_close(fp); > + } > + } > + > + return ctf_type; > +} > + Cheers, [...] --=20 Dodji