From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by sourceware.org (Postfix) with ESMTPS id 69AB23858435 for ; Wed, 24 Nov 2021 16:36:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 69AB23858435 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 relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 95C5F20005; Wed, 24 Nov 2021 16:36:03 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id D00D9581C2F; Wed, 24 Nov 2021 17:36:02 +0100 (CET) From: Dodji Seketeli To: "Guillermo E. Martinez via Libabigail" Subject: Re: [PATCH v2] Add regression tests for ctf reading Organization: Me, myself and I References: <20211118041625.622972-1-guillermo.e.martinez@oracle.com> <20211122213353.2456208-1-guillermo.e.martinez@oracle.com> X-Operating-System: Fedora 36 X-URL: http://www.seketeli.net/~dodji Date: Wed, 24 Nov 2021 17:36:02 +0100 In-Reply-To: <20211122213353.2456208-1-guillermo.e.martinez@oracle.com> (Guillermo E. Martinez via Libabigail's message of "Mon, 22 Nov 2021 15:33:53 -0600") Message-ID: <87ee75tsst.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 24 Nov 2021 16:36:12 -0000 Hello, [...] Thanks for working on this. Nice patch, by the way! I like its direction. I have a few comments and I believe that when they are addressed, we'll be able to apply the patch. [...] > Dependencies/limitations: > > * It was worked on the top of the following patches: > https://sourceware.org/pipermail/libabigail/2021q4/003853.html > > * Some CTF tests were *disabled* because it generates the XML ABI > corpus with *same information* but the XML nodes *are not* always > in the *same order*, so using diff command fails. Details here: > https://sourceware.org/pipermail/libabigail/2021q4/003824.html In those cases where the abixml file generated from CTF is different from the one generated from DWARF, I think we should have two different reference abixml files to diff against. No CTF test should be disabled, I think. So: This: { "data/test-read-dwarf/test0", "", "", SEQUENCE_TYPE_ID_STYLE, "data/test-read-dwarf/test0.abi", "output/test-read-dwarf/test0.abi" }, would be changed into: { "data/test-read-common/test0", "", "", SEQUENCE_TYPE_ID_STYLE, "data/test-read-dwarf/test0.abi", "output/test-read-dwarf/test0.abi" }, For the DWARF test entry in test-read-dwarf.cc, and it would be changed into: { "data/test-read-common/test0", "", "", SEQUENCE_TYPE_ID_STYLE, "data/test-read-ctf/test0.abi", "output/test-read-ctf/test0.abi" }, for the CTF test netry in test-read-ctf.cc. By the way, I am seeing entries like this in test-read-ctf.cc: > + "data/test-read-common/test3.so", > + "", > + "", > + SEQUENCE_TYPE_ID_STYLE, > + "data/test-read-common/test3-ctf.so.abi", > + "output/test-read-common/test3-ctf.so.abi" Here this entry does exactly what I am suggesting, even if test3-ctf.so.abi is stored in data/test-read-common. So where exactly is the CTF test disabled? [...] > diff --git a/tests/test-read-common.cc b/tests/test-read-common.cc [...] > + > +namespace abigail > +{ > +namespace tests > +{ > +namespace read_common > +{ Because this file now contains an API definition that is to be used be some tests, every single function of the file should be documented using doxygen comments, just like we have in src/abg-*.cc files. For instance: > + > +test_task::test_task(const InOutSpec &s, > + string& a_out_abi_base, > + string& a_in_elf_base, > + string& a_in_abi_base) This function should be fully doxygen-documented. > + : is_ok(true), > + spec(s), > + out_abi_base(a_out_abi_base), > + in_elf_base(a_in_elf_base), > + in_abi_base(a_in_abi_base) > + {} > + > +bool > +test_task::serialize_corpus(const string& out_abi_path, > + corpus_sptr corp) Likewise. > +{ > + ofstream of(out_abi_path.c_str(), std::ios_base::trunc); > + if (!of.is_open()) > + { > + error_message = string("failed to read ") + out_abi_path + "\n"; > + return false; > + } > + > + write_context_sptr write_ctxt > + = create_write_context(corp->get_environment(), of); > + set_type_id_style(*write_ctxt, spec.type_id_style); > + is_ok = write_corpus(*write_ctxt, corp, /*indent=*/0); > + of.close(); > + > + return true; > +} > + > +bool > +test_task::run_abidw(const string& extargs) Likewise. > +{ > + string abidw = string(get_build_dir()) + "/tools/abidw"; > + string drop_private_types; > + if (!in_public_headers_path.empty()) > + drop_private_types += "--headers-dir " + in_public_headers_path + > + " --drop-private-types"; > + string cmd = abidw + " " + drop_private_types + " --abidiff " + extargs + > + in_elf_path; > + if (system(cmd.c_str())) > + { > + error_message = string("ABIs differ:\n") > + + in_elf_path > + + "\nand:\n" > + + out_abi_path > + + "\n"; > + > + return false; > + } > + > + return true; > +} > + > +bool > +test_task::run_diff() Likewise. > +{ > + set_in_abi_path(); > + string cmd = "diff -u " + in_abi_path + " " + out_abi_path; > + if (system(cmd.c_str())) > + { > + error_message = string("ABIs differ:\n") > + + in_abi_path > + + "\nand:\n" > + + out_abi_path > + + "\n"; > + > + return false; > + } > + > + return true; > +} > + > +void > +display_usage(const string& prog_name, ostream& out) Likewise. > +{ > + emit_prefix(prog_name, out) > + << "usage: " << prog_name << " [options]\n" > + << " where options can be: \n" > + << " --help|-h display this message\n" > + << " --no-parallel execute testsuite is a sigle thread\n" > + ; > +} > + > +bool > +parse_command_line(int argc, char* argv[], options& opts) Likewise. > +{ > + for (int i = 1; i < argc; ++i) > + { > + if (!strcmp(argv[i], "--no-parallel")) > + opts.parallel = false; > + else if (!strcmp(argv[i], "--help") > + || !strcmp(argv[i], "--h")) > + return false; > + else > + { > + if (strlen(argv[i]) >= 2 && argv[i][0] == '-' && argv[i][1] == '-') > + opts.wrong_option = argv[i]; > + return false; > + } > + } > + > + return true; > +} > + > +bool > +run_tests(const size_t num_tests, const InOutSpec* specs, > + const options& opts, create_new_test new_test) Likewise. > +{ [...] > diff --git a/tests/test-read-common.h b/tests/test-read-common.h [...] > +/// This is an aggregate that specifies where a test shall get its > +/// input from, and where it shall write its ouput to. > +struct InOutSpec > +{ > + const char* in_elf_path; > + const char* in_suppr_spec_path; > + const char* in_public_headers_path; > + type_id_style_kind type_id_style; > + const char* in_abi_path; > + const char* out_abi_path; > +};// end struct InOutSpec > + > +/// The task that peforms the tests. > +struct test_task : public abigail::workers::task > +{ > + bool is_ok; > + InOutSpec spec; > + string error_message; > + string out_abi_base; > + string in_elf_base; > + string in_abi_base; > + > + string in_elf_path; > + string in_abi_path; > + string in_suppr_spec_path; > + string in_public_headers_path; > + string out_abi_path; > + > + void > + set_in_elf_path() Please doxygen-document this function. > + { > + in_elf_path = in_elf_base + spec.in_elf_path; > + } > + > + void > + set_in_suppr_spec_path() Likewise. > + { > + if (spec.in_suppr_spec_path) > + in_suppr_spec_path = in_elf_base + spec.in_suppr_spec_path; > + else > + in_suppr_spec_path.clear(); > + } > + > + void > + set_in_public_headers_path() Likewise. > + { > + if (spec.in_public_headers_path) > + in_public_headers_path = spec.in_public_headers_path; > + if (!in_public_headers_path.empty()) > + in_public_headers_path = in_elf_base + spec.in_public_headers_path; > + } > + > + bool > + set_out_abi_path() Likewise. > + { > + out_abi_path = out_abi_base + spec.out_abi_path; > + if (!abigail::tools_utils::ensure_parent_dir_created(out_abi_path)) > + { > + error_message = > + string("Could not create parent directory for ") + out_abi_path; > + return false; > + } > + return true; > + } > + > + void > + set_in_abi_path() Likewise. > + { > + in_abi_path = in_abi_base + spec.in_abi_path; > + } > + [...] > + test_task(const InOutSpec &s, > + string& a_out_abi_base, > + string& a_in_elf_base, > + string& a_in_abi_base); > + bool > + serialize_corpus(const string& out_abi_path, > + corpus_sptr corp); > + bool > + run_abidw(const string& extargs = ""); > + > + bool > + run_diff(); > + > + virtual > + ~test_task() > + {} > + > +}; // end struct test_task > + > +typedef shared_ptr test_task_sptr; > + > +struct options > +{ Please doxygen-document this struct. > + string wrong_option; > + bool parallel; > + > + options() > + : parallel(true) > + {} > + > + ~options() > + { > + } > +}; [...] > diff --git a/tests/test-read-ctf.cc b/tests/test-read-ctf.cc [...] > +test_task_ctf::test_task_ctf(const InOutSpec &s, > + string& a_out_abi_base, > + string& a_in_elf_base, > + string& a_in_abi_base) Please doxygen-document this function. > + : test_task(s, a_out_abi_base, a_in_elf_base, a_in_abi_base) > + {} > + [...] > +static test_task* > +new_task(const InOutSpec* s, string& a_out_abi_base, > + string& a_in_elf_base, string& a_in_abi_base) Please doxygen-document this function. > +{ > + return new test_task_ctf(*s, a_in_abi_base, This 'a_in_abi_base' should be a_out_abi_base. > + a_in_elf_base, a_in_abi_base); > +} [...] > diff --git a/tests/test-read-dwarf.cc b/tests/test-read-dwarf.cc [...] > +static test_task* > +new_task(const InOutSpec* s, string& a_out_abi_base, > + string& a_in_elf_base, string& a_in_abi_base) Please doxygen-document this function. > +{ > + return new test_task_dwarf(*s, a_in_abi_base, This 'a_in_abi_base' should be a_out_abi_base. > + a_in_elf_base, a_in_abi_base); > +} > > int > main(int argc, char *argv[]) > { > bool no_parallel = false; This variable is not unused. Thanks for working on this. It's really appreciated! [...] Cheers, -- Dodji