From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::223]) by sourceware.org (Postfix) with ESMTPS id 5605F3854834 for ; Wed, 8 Jun 2022 15:31:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5605F3854834 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 74C1160007; Wed, 8 Jun 2022 15:31:14 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id 6122C5800FC; Wed, 8 Jun 2022 17:31:13 +0200 (CEST) From: Dodji Seketeli To: Ben Woodard via Libabigail Subject: Re: [PATCH] Give abicompat the ability to read CTF and abixml Organization: Me, myself and I References: <20220603154750.318114-1-woodard@redhat.com> X-Operating-System: Fedora 37 X-URL: http://www.seketeli.net/~dodji Date: Wed, 08 Jun 2022 17:31:13 +0200 In-Reply-To: <20220603154750.318114-1-woodard@redhat.com> (Ben Woodard via Libabigail's message of "Fri, 3 Jun 2022 08:47:51 -0700") Message-ID: <87wndrkw8u.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=-9.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, 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 X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Jun 2022 15:31:20 -0000 Hello Ben, Ben Woodard via Libabigail a =C3=A9crit: > While abidw generates abidw and abidiff can consume that abidw > abicompat could only read from ELF objects with DWARF. To bring it in > line with the capabilities of abidiff, this patch adds the ability for > abicompat to read both abixml and CTF. The reason why being able to > handle abixml as well as an ELF file with DWARF is that it allows > someone to archive the abixml for an object and use it in place of > having to keep the entire object when doing abi compatibility studies. > > The ability to handle CTF was also in the code that I copied over from > abidiff and so I brought it over at the same time. This should be > tested more extensively. The make check works and a trivial test using > the same object seems to work but I do not have any CTF other binaries > around to test against. > > Also a feature that I asked for a long time ago was to fail if the > binaries don't have debug infomation which is needed for proper > comparison. This was added to abidiff but it didn't get added to > abicompat. Since the code was in the same area, I copied it over from > abidiff at the same time. > > I do not think it makes sense to try to split these three features > apart because they are just replicating pre-existing code in a > different tool. Thanks for the patch! I have applied it to the master branch. I have slightly amended it for some nits. Please see below for my comments about what I changed. > * tools/abicompat.cc - add abixml and ctf handling, add fail on > no debug info option. > * doc/manuals/abicompat.rst - add documentation for new options I have amended this part to better comply with the ChangeLog standards. [...] > index f4bc384d..8cbce3a1 100644 > --- a/tools/abicompat.cc > +++ b/tools/abicompat.cc [...] > +static corpus_sptr read_corpus(char *exename, bool use_ctf, bool weak_mo= de, > + status &status, const vector di_roots, > + const environment_sptr &env, const string &path){ I made the function name start on column number 0, just like all function definitions in the code base. Also, I made the openenig '{' start on the following line, at column 0 as well. Furthermore, rather than passing the three parameters "exename, use_ctf and weak_mode" to this function, I just passed 'options opts' as the value of these parameters can all be retrieve from the instance of 'options' which carries all the program options. Also, I have added a doxygen comment for that function, as per what's done throughout the code. So, that start of the function definition becomes: +/// Read an ABI corpus, be it from ELF or abixml. +/// +/// @param opts the options passed from the user to the program. +/// +/// @param status the resulting elf_reader::status to send back to the +/// caller. +/// +/// @param di_roots the directories from where to look for debug info. +/// +/// @param env the environment used for libabigail. +/// +/// @param path the path to the ABI corpus to read from. +static corpus_sptr +read_corpus(options opts, status &status, + const vector di_roots, + const environment_sptr &env, + const string &path) +{ [...] I have adjusted the body of the function definition as well as the callers of this function accordingly. [...] Below is the patch that was applied. Thanks! >From 57a5ce73cfa3136a673f661aaba3de48f51898b2 Mon Sep 17 00:00:00 2001 From: Ben Woodard via Libabigail Date: Fri, 3 Jun 2022 08:47:51 -0700 Subject: [PATCH] abicompat: Support reading CTF and abixml While abidw generates abidw and abidiff can consume that abidw abicompat could only read from ELF objects with DWARF. To bring it in line with the capabilities of abidiff, this patch adds the ability for abicompat to read both abixml and CTF. The reason why being able to handle abixml as well as an ELF file with DWARF is that it allows someone to archive the abixml for an object and use it in place of having to keep the entire object when doing abi compatibility studies. The ability to handle CTF was also in the code that I copied over from abidiff and so I brought it over at the same time. This should be tested more extensively. The make check works and a trivial test using the same object seems to work but I do not have any CTF other binaries around to test against. Also a feature that I asked for a long time ago was to fail if the binaries don't have debug infomation which is needed for proper comparison. This was added to abidiff but it didn't get added to abicompat. Since the code was in the same area, I copied it over from abidiff at the same time. I do not think it makes sense to try to split these three features apart because they are just replicating pre-existing code in a different tool. * tools/abicompat.cc (options::{fail_no_debug_info, use_ctf}): New data members. (options::options): Initialize them. (display_usage): Add a help string for the new options --fail-no-debug-info and --ctf. (parse_command_line): Support parsing the new options --fail-no-debug-info and --ctf. (read_corpus): New static function. (main): Use the new read_corpus in lieu of using dwarf_reader::read_corpus_from_elf and/or ctf_reader::read_corpus and/or xml::reader::read_corpus_from_input. * doc/manuals/abicompat.rst: add documentation for new options. Signed-off-by: Ben Woodard Signed-off-by: Dodji Seketeli --- doc/manuals/abicompat.rst | 12 +++ tools/abicompat.cc | 172 ++++++++++++++++++++++++++++++-------- 2 files changed, 151 insertions(+), 33 deletions(-) diff --git a/doc/manuals/abicompat.rst b/doc/manuals/abicompat.rst index 6bc56388..e88a05a8 100644 --- a/doc/manuals/abicompat.rst +++ b/doc/manuals/abicompat.rst @@ -77,6 +77,18 @@ Options Do not show information about where in the *second shared library* the respective type was changed. =20 + * ``--ctf`` + + When comparing binaries, extract ABI information from CTF debug + information, if present. + + * ``--fail-no-debug-info`` + + If no debug info was found, then this option makes the program to + fail. Otherwise, without this option, the program will attempt to + compare properties of the binaries that are not related to debug + info, like pure ELF properties. + * ``--ignore-soname`` =20 Ignore differences in the SONAME when doing a comparison diff --git a/tools/abicompat.cc b/tools/abicompat.cc index 913724c7..9348f55c 100644 --- a/tools/abicompat.cc +++ b/tools/abicompat.cc @@ -37,7 +37,9 @@ #include "abg-config.h" #include "abg-tools-utils.h" #include "abg-corpus.h" +#include "abg-reader.h" #include "abg-dwarf-reader.h" +#include "abg-ctf-reader.h" #include "abg-comparison.h" #include "abg-suppression.h" =20 @@ -74,7 +76,11 @@ public: bool redundant_opt_set; bool no_redundant_opt_set; bool show_locs; + bool fail_no_debug_info; bool ignore_soname; +#ifdef WITH_CTF + bool use_ctf; +#endif =20 options(const char* program_name) :prog_name(program_name), @@ -87,7 +93,12 @@ public: redundant_opt_set(), no_redundant_opt_set(), show_locs(true), + fail_no_debug_info(), ignore_soname(false) +#ifdef WITH_CTF + , + use_ctf() +#endif {} }; // end struct options =20 @@ -115,9 +126,13 @@ display_usage(const string& prog_name, ostream& out) << " --no-redundant do not display redundant changes\n" << " --no-show-locs do now show location information\n" << " --ignore-soname do not take the SONAMEs into account\n" + << " --fail-no-debug-info bail out if no debug info was found\n" << " --redundant display redundant changes (this is the default)\n" << " --weak-mode check compatibility between the application and " "just one version of the library.\n" +#ifdef WITH_CTF + << " --ctf use CTF instead of DWARF in ELF files\n" +#endif ; } =20 @@ -211,6 +226,8 @@ parse_command_line(int argc, char* argv[], options& opt= s) opts.show_locs =3D false; else if (!strcmp(argv[i], "--ignore-soname")) opts.ignore_soname=3Dtrue; + else if (!strcmp(argv[i], "--fail-no-debug-info")) + opts.fail_no_debug_info =3D true; else if (!strcmp(argv[i], "--help") || !strcmp(argv[i], "-h")) { @@ -219,6 +236,10 @@ parse_command_line(int argc, char* argv[], options& op= ts) } else if (!strcmp(argv[i], "--weak-mode")) opts.weak_mode =3D true; +#ifdef WITH_CTF + else if (!strcmp(argv[i], "--ctf")) + opts.use_ctf =3D true; +#endif else { opts.unknow_option =3D argv[i]; @@ -252,6 +273,8 @@ using abigail::ir::function_type_sptr; using abigail::ir::function_decl; using abigail::ir::var_decl; using abigail::elf_reader::status; +using abigail::elf_reader::STATUS_ALT_DEBUG_INFO_NOT_FOUND; +using abigail::elf_reader::STATUS_DEBUG_INFO_NOT_FOUND; using abigail::dwarf_reader::read_corpus_from_elf; using abigail::comparison::diff_context_sptr; using abigail::comparison::diff_context; @@ -617,6 +640,75 @@ perform_compat_check_in_weak_mode(options& opts, return status; } =20 +/// Read an ABI corpus, be it from ELF or abixml. +/// +/// @param opts the options passed from the user to the program. +/// +/// @param status the resulting elf_reader::status to send back to the +/// caller. +/// +/// @param di_roots the directories from where to look for debug info. +/// +/// @param env the environment used for libabigail. +/// +/// @param path the path to the ABI corpus to read from. +static corpus_sptr +read_corpus(options opts, status &status, + const vector di_roots, + const environment_sptr &env, + const string &path) +{ + corpus_sptr retval =3D NULL; + abigail::tools_utils::file_type type =3D + abigail::tools_utils::guess_file_type(path); + + switch (type) + { + case abigail::tools_utils::FILE_TYPE_UNKNOWN: + emit_prefix(opts.prog_name, cerr) + << "Unknown content type for file " << path << "\n"; + break; + case abigail::tools_utils::FILE_TYPE_ELF: + { +#ifdef WITH_CTF + if (opts.use_ctf) + { + abigail::ctf_reader::read_context_sptr r_ctxt + =3D abigail::ctf_reader::create_read_context(path, + env.get()); + ABG_ASSERT(r_ctxt); + + retval =3D abigail::ctf_reader::read_corpus(r_ctxt.get(), status); + } + else +#endif + retval =3D read_corpus_from_elf(path, di_roots, env.get(), + /*load_all_types=3D*/opts.weak_mode, + status); + } + break; + case abigail::tools_utils::FILE_TYPE_XML_CORPUS: + { + abigail::xml_reader::read_context_sptr r_ctxt =3D + abigail::xml_reader::create_native_xml_read_context(path, env.get()); + assert(r_ctxt); + retval =3D abigail::xml_reader::read_corpus_from_input(*r_ctxt); + } + break; + case abigail::tools_utils::FILE_TYPE_AR: + case abigail::tools_utils::FILE_TYPE_XML_CORPUS_GROUP: + case abigail::tools_utils::FILE_TYPE_RPM: + case abigail::tools_utils::FILE_TYPE_SRPM: + case abigail::tools_utils::FILE_TYPE_DEB: + case abigail::tools_utils::FILE_TYPE_DIR: + case abigail::tools_utils::FILE_TYPE_TAR: + case abigail::tools_utils::FILE_TYPE_NATIVE_BI: + break; + } + + return retval; +} + int main(int argc, char* argv[]) { @@ -674,15 +766,6 @@ main(int argc, char* argv[]) if (!abigail::tools_utils::check_file(opts.app_path, cerr, opts.prog_nam= e)) return abigail::tools_utils::ABIDIFF_ERROR; =20 - abigail::tools_utils::file_type type =3D - abigail::tools_utils::guess_file_type(opts.app_path); - if (type !=3D abigail::tools_utils::FILE_TYPE_ELF) - { - emit_prefix(argv[0], cerr) - << opts.app_path << " is not an ELF file\n"; - return abigail::tools_utils::ABIDIFF_ERROR; - } - // Create the context of the diff diff_context_sptr ctxt =3D create_diff_context(opts); =20 @@ -705,12 +788,24 @@ main(int argc, char* argv[]) app_di_roots.push_back(&app_di_root); status status =3D abigail::elf_reader::STATUS_UNKNOWN; environment_sptr env(new environment); - corpus_sptr app_corpus=3D - read_corpus_from_elf(opts.app_path, - app_di_roots, env.get(), - /*load_all_types=3D*/opts.weak_mode, - status); =20 + corpus_sptr app_corpus =3D read_corpus(opts, status, + app_di_roots, env, + opts.app_path); + if (!app_corpus) + { + emit_prefix(argv[0], cerr) << opts.app_path + << " is not a supported file\n"; + return abigail::tools_utils::ABIDIFF_ERROR; + } + + if (opts.fail_no_debug_info && (status & STATUS_ALT_DEBUG_INFO_NOT_FOUND) + && (status & STATUS_DEBUG_INFO_NOT_FOUND)) + { + emit_prefix(argv[0], cerr) << opts.app_path + << " does not have debug symbols\n"; + return abigail::tools_utils::ABIDIFF_ERROR; + } if (status & abigail::elf_reader::STATUS_NO_SYMBOLS_FOUND) { emit_prefix(argv[0], cerr) @@ -746,26 +841,27 @@ main(int argc, char* argv[]) ABG_ASSERT(!opts.lib1_path.empty()); if (!abigail::tools_utils::check_file(opts.lib1_path, cerr, opts.prog_na= me)) return abigail::tools_utils::ABIDIFF_ERROR; - type =3D abigail::tools_utils::guess_file_type(opts.lib1_path); - if (type !=3D abigail::tools_utils::FILE_TYPE_ELF) - { - emit_prefix(argv[0], cerr) << opts.lib1_path << " is not an ELF file= \n"; - return abigail::tools_utils::ABIDIFF_ERROR; - } =20 char * lib1_di_root =3D opts.lib1_di_root_path.get(); vector lib1_di_roots; lib1_di_roots.push_back(&lib1_di_root); - corpus_sptr lib1_corpus =3D read_corpus_from_elf(opts.lib1_path, - lib1_di_roots, env.get(), - /*load_all_types=3D*/false, - status); - if (status & abigail::elf_reader::STATUS_DEBUG_INFO_NOT_FOUND) + corpus_sptr lib1_corpus =3D read_corpus(opts, status, + lib1_di_roots, + env, opts.lib1_path); + if (!lib1_corpus) + { + emit_prefix(argv[0], cerr) << opts.lib1_path + << " is not a supported file\n"; + return abigail::tools_utils::ABIDIFF_ERROR; + } + if (opts.fail_no_debug_info && (status & STATUS_ALT_DEBUG_INFO_NOT_FOUND) + && (status & STATUS_DEBUG_INFO_NOT_FOUND)) emit_prefix(argv[0], cerr) << "could not read debug info for " << opts.lib1_path << "\n"; if (status & abigail::elf_reader::STATUS_NO_SYMBOLS_FOUND) { - cerr << "could not read symbols from " << opts.lib1_path << "\n"; + emit_prefix(argv[0], cerr) << "could not read symbols from " + << opts.lib1_path << "\n"; return abigail::tools_utils::ABIDIFF_ERROR; } if (!(status & abigail::elf_reader::STATUS_OK)) @@ -783,13 +879,23 @@ main(int argc, char* argv[]) char * lib2_di_root =3D opts.lib2_di_root_path.get(); vector lib2_di_roots; lib2_di_roots.push_back(&lib2_di_root); - lib2_corpus =3D read_corpus_from_elf(opts.lib2_path, - lib2_di_roots, env.get(), - /*load_all_types=3D*/false, - status); - if (status & abigail::elf_reader::STATUS_DEBUG_INFO_NOT_FOUND) - emit_prefix(argv[0], cerr) - << "could not read debug info for " << opts.lib2_path << "\n"; + lib2_corpus =3D read_corpus(opts, status, + lib2_di_roots, env, + opts.lib2_path); + if (!lib2_corpus) + { + emit_prefix(argv[0], cerr) << opts.lib2_path + << " is not a supported file\n"; + return abigail::tools_utils::ABIDIFF_ERROR; + } + + if (opts.fail_no_debug_info && (status & STATUS_ALT_DEBUG_INFO_NOT_F= OUND) + && (status & STATUS_DEBUG_INFO_NOT_FOUND)) + { + emit_prefix(argv[0], cerr) + << "could not read debug info for " << opts.lib2_path << "\n"; + return abigail::tools_utils::ABIDIFF_ERROR; + } if (status & abigail::elf_reader::STATUS_NO_SYMBOLS_FOUND) { emit_prefix(argv[0], cerr) --=20 2.36.1 Cheers, --=20 Dodji