From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 7C4053858C60 for ; Wed, 24 Nov 2021 19:10:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7C4053858C60 Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-289-YhQg5TEdOJS_b9-py6OWWA-1; Wed, 24 Nov 2021 14:10:03 -0500 X-MC-Unique: YhQg5TEdOJS_b9-py6OWWA-1 Received: by mail-qk1-f198.google.com with SMTP id p18-20020a05620a057200b00467bc32b45aso3010553qkp.12 for ; Wed, 24 Nov 2021 11:10:03 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=iqHSIFkJl3cdsHUHfEfqoxIdYukXrKkdk8ws24qNjfM=; b=XfDFVqOlcp/o8mNRGGa4SUgCrD3WdG6JO8i/1Kh+Uv49LAr8wMZtInkIgRobrZFgLD 3njn0/h7YcXHwEwhi27KoCjfZ9+kG0JlrfGMRCdEQaKYkl0XGYvJbcem3WFWW7q9Hdie xj9/xXS5rrHK3J8iihKF/16u6BPSeYY90mtHzlwUEzvz9/BbynVfK6FFKbMA7qozDbr+ yg37Sd0pKzy26D5krgqWrxqjT0K9e5iiQKWLZEQGkIN0u0nUDs++uAKAGViW21gCkuhW RGfOUJNS2y7TRvYdaHYV6Cy1m5xpl2Th/mE4HWW1zSWpq20eLcenvOMVGo3NX37P1FbQ Vw+w== X-Gm-Message-State: AOAM530EZo266zf0vfUPL5O5Uwarf7sVtjvZpQI8Lhl22F4bVj4CC5XC +B/QpkUqPPS6n/rpooorXdZrmT1ox3F1N0tMTL2wWn2xpiIAn+L5ey0CW49jmfRzMRpBpK4Y266 hmuOaLZvn/4x6QeJYV+fe X-Received: by 2002:a05:6214:e8b:: with SMTP id hf11mr11005737qvb.40.1637781002526; Wed, 24 Nov 2021 11:10:02 -0800 (PST) X-Google-Smtp-Source: ABdhPJxJJn9G5vLxVCicQVji2vLJSpueRLyevt4ZGldLD2c5NlV22WDJwwH2iT76/0csjQO8G2Um6A== X-Received: by 2002:a05:6214:e8b:: with SMTP id hf11mr11005624qvb.40.1637781001904; Wed, 24 Nov 2021 11:10:01 -0800 (PST) Received: from smtpclient.apple (47-208-193-143.trckcmtc01.res.dyn.suddenlink.net. [47.208.193.143]) by smtp.gmail.com with ESMTPSA id g19sm326901qtg.82.2021.11.24.11.10.01 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 24 Nov 2021 11:10:01 -0800 (PST) Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.20.0.1.32\)) Subject: Re: [PATCH v2] Add regression tests for ctf reading From: Ben Woodard In-Reply-To: <87ee75tsst.fsf@seketeli.org> Date: Wed, 24 Nov 2021 11:09:59 -0800 Cc: "Guillermo E. Martinez via Libabigail" Message-Id: References: <20211118041625.622972-1-guillermo.e.martinez@oracle.com> <20211122213353.2456208-1-guillermo.e.martinez@oracle.com> <87ee75tsst.fsf@seketeli.org> To: Dodji Seketeli X-Mailer: Apple Mail (2.3693.20.0.1.32) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, 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 19:10:11 -0000 > On Nov 24, 2021, at 8:36 AM, Dodji Seketeli wrote: >=20 > Hello, >=20 > [...] >=20 > Thanks for working on this. Nice patch, by the way! I like its > direction. >=20 > I have a few comments and I believe that when they are addressed, we'll > be able to apply the patch. >=20 > [...] >=20 >> Dependencies/limitations: >>=20 >> * It was worked on the top of the following patches: >> https://sourceware.org/pipermail/libabigail/2021q4/003853.html >>=20 >> * 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 >=20 > 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. Here is a somewhat deeper question that I think needs to be considered. I= =E2=80=99ve generally referred to it as =E2=80=9CDWARF Idioms=E2=80=9D but = this email makes me think that it is even larger than that.=20 For my work, I need libabigail to generate an abstract notion of the ABI co= rpus. How it constructs that abstract notion of the ABI needs to be indepen= dent of the producer. Think of it this way, say we take the same compiler a= nd have it compile the same library producing both CTF and DWARF, the ABI o= f the library doesn=E2=80=99t change. Since it is literally the same object= , the program text is same. Therefore the ABI is unquestionably the the sam= e. Any difference reported by libabigail therefore is a problem with libabi= gail. It is not taking the source material and abstracting it enough into t= he ABI artifacts to separate the artifacts from the implementation.=20 So I kind of believe that we need to look more deeply into WHY the CTF and = DWARF are not comparing as equivalent and begin the process of filing the c= ompiler bugs when we need to, and doing what is necessary to abstract the A= BI from the source material that libabigail used to construct its IR of the= ABI corpus from. So, I must say that I disagree with both dodji=E2=80=99s approach here and = to a lesser extent Guillermo=E2=80=99s approach of disabling the tests. I t= hink that the tests where the CTF doesn=E2=80=99t match the DWARF should be= investigated and when necessary marked =E2=80=9Cxfail=E2=80=9D with a note= citing their individual cause.=20 I think that what we need to work towards is: abidw produces the same output (or more precisely libabigail produces the s= ame IR) whether you compile with -gdwarf-3 -gdwarf-4 -gdwarf-5 -gsplit-dwar= f -gctf also for the most part the ABI should not change between compiler versions.= There may be a few cases that we need to look into where the compiler actu= ally breaks ABI and of course libabigail should flag those but I would asse= rt that libabigail needs to abstract its IR of the ABI enough that compiler= version changes that don=E2=80=99t actually change the ABI of the ELF obje= ct are not reported as ABI breaks. It currently does pretty well at this at= the moment. Then once that foundation is built, being able to abstract the ABI IR enoug= h that differences in toolchains e.g. LLVM vs GCC are not flagged as change= s in the object=E2=80=99s ABI. This is important to provide people with a t= ool that will allow them to mix toolchains within a project to achieve opti= mal code. -ben >=20 > So: >=20 > This: >=20 > { > "data/test-read-dwarf/test0", > "", > "", > SEQUENCE_TYPE_ID_STYLE, > "data/test-read-dwarf/test0.abi", > "output/test-read-dwarf/test0.abi" > }, >=20 > would be changed into: >=20 > { > "data/test-read-common/test0", > "", > "", > SEQUENCE_TYPE_ID_STYLE, > "data/test-read-dwarf/test0.abi", > "output/test-read-dwarf/test0.abi" > }, >=20 > For the DWARF test entry in test-read-dwarf.cc, and it would be > changed into: >=20 > { > "data/test-read-common/test0", > "", > "", > SEQUENCE_TYPE_ID_STYLE, > "data/test-read-ctf/test0.abi", > "output/test-read-ctf/test0.abi" > }, >=20 > for the CTF test netry in test-read-ctf.cc. >=20 > By the way, I am seeing entries like this in test-read-ctf.cc: >=20 >> + "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" >=20 > Here this entry does exactly what I am suggesting, even if > test3-ctf.so.abi is stored in data/test-read-common. >=20 > So where exactly is the CTF test disabled? >=20 > [...] >=20 >=20 >> diff --git a/tests/test-read-common.cc b/tests/test-read-common.cc >=20 > [...] >=20 >> + >> +namespace abigail >> +{ >> +namespace tests >> +{ >> +namespace read_common >> +{ >=20 > 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. >=20 > For instance: >=20 >> + >> +test_task::test_task(const InOutSpec &s, >> + string& a_out_abi_base, >> + string& a_in_elf_base, >> + string& a_in_abi_base) >=20 > This function should be fully doxygen-documented. >=20 >> + : 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) >=20 > Likewise. >=20 >> +{ >> + ofstream of(out_abi_path.c_str(), std::ios_base::trunc); >> + if (!of.is_open()) >> + { >> + error_message =3D string("failed to read ") + out_abi_path + "\n= "; >> + return false; >> + } >> + >> + write_context_sptr write_ctxt >> + =3D create_write_context(corp->get_environment(), of); >> + set_type_id_style(*write_ctxt, spec.type_id_style); >> + is_ok =3D write_corpus(*write_ctxt, corp, /*indent=3D*/0); >> + of.close(); >> + >> + return true; >> +} >> + >> +bool >> +test_task::run_abidw(const string& extargs) >=20 > Likewise. >=20 >> +{ >> + string abidw =3D string(get_build_dir()) + "/tools/abidw"; >> + string drop_private_types; >> + if (!in_public_headers_path.empty()) >> + drop_private_types +=3D "--headers-dir " + in_public_headers_path + >> + " --drop-private-types"; >> + string cmd =3D abidw + " " + drop_private_types + " --abidiff " + ext= args + >> + in_elf_path; >> + if (system(cmd.c_str())) >> + { >> + error_message =3D string("ABIs differ:\n") >> + + in_elf_path >> + + "\nand:\n" >> + + out_abi_path >> + + "\n"; >> + >> + return false; >> + } >> + >> + return true; >> +} >> + >> +bool >> +test_task::run_diff() >=20 > Likewise. >=20 >> +{ >> + set_in_abi_path(); >> + string cmd =3D "diff -u " + in_abi_path + " " + out_abi_path; >> + if (system(cmd.c_str())) >> + { >> + error_message =3D 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) >=20 > Likewise. >=20 >> +{ >> + 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) >=20 > Likewise. >=20 >> +{ >> + for (int i =3D 1; i < argc; ++i) >> + { >> + if (!strcmp(argv[i], "--no-parallel")) >> + opts.parallel =3D false; >> + else if (!strcmp(argv[i], "--help") >> + || !strcmp(argv[i], "--h")) >> + return false; >> + else >> + { >> + if (strlen(argv[i]) >=3D 2 && argv[i][0] =3D=3D '-' && argv[i= ][1] =3D=3D '-') >> + opts.wrong_option =3D 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) >=20 > Likewise. >=20 >> +{ >=20 > [...] >=20 >=20 >> diff --git a/tests/test-read-common.h b/tests/test-read-common.h >=20 > [...] >=20 >> +/// 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() >=20 > Please doxygen-document this function. >=20 >> + { >> + in_elf_path =3D in_elf_base + spec.in_elf_path; >> + } >> + >> + void >> + set_in_suppr_spec_path() >=20 > Likewise. >=20 >> + { >> + if (spec.in_suppr_spec_path) >> + in_suppr_spec_path =3D in_elf_base + spec.in_suppr_spec_path; >> + else >> + in_suppr_spec_path.clear(); >> + } >> + >> + void >> + set_in_public_headers_path() >=20 > Likewise. >=20 >> + { >> + if (spec.in_public_headers_path) >> + in_public_headers_path =3D spec.in_public_headers_path; >> + if (!in_public_headers_path.empty()) >> + in_public_headers_path =3D in_elf_base + spec.in_public_headers_p= ath; >> + } >> + >> + bool >> + set_out_abi_path() >=20 > Likewise. >=20 >> + { >> + out_abi_path =3D out_abi_base + spec.out_abi_path; >> + if (!abigail::tools_utils::ensure_parent_dir_created(out_abi_path)) >> + { >> + error_message =3D >> + string("Could not create parent directory for ") + out_abi_= path; >> + return false; >> + } >> + return true; >> + } >> + >> + void >> + set_in_abi_path() >=20 > Likewise. >=20 >> + { >> + in_abi_path =3D in_abi_base + spec.in_abi_path; >> + } >> + >=20 > [...] >=20 >> + 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 =3D ""); >> + >> + bool >> + run_diff(); >> + >> + virtual >> + ~test_task() >> + {} >> + >> +}; // end struct test_task >> + >> +typedef shared_ptr test_task_sptr; >> + >> +struct options >> +{ >=20 > Please doxygen-document this struct. >=20 >> + string wrong_option; >> + bool parallel; >> + >> + options() >> + : parallel(true) >> + {} >> + >> + ~options() >> + { >> + } >> +}; >=20 > [...] >=20 >> diff --git a/tests/test-read-ctf.cc b/tests/test-read-ctf.cc >=20 > [...] >=20 >=20 >> +test_task_ctf::test_task_ctf(const InOutSpec &s, >> + string& a_out_abi_base, >> + string& a_in_elf_base, >> + string& a_in_abi_base) >=20 > Please doxygen-document this function. >=20 >> + : test_task(s, a_out_abi_base, a_in_elf_base, a_in_abi_base) >> + {} >> + >=20 > [...] >=20 >> +static test_task* >> +new_task(const InOutSpec* s, string& a_out_abi_base, >> + string& a_in_elf_base, string& a_in_abi_base) >=20 > Please doxygen-document this function. >=20 >> +{ >> + return new test_task_ctf(*s, a_in_abi_base, >=20 > This 'a_in_abi_base' should be a_out_abi_base. >=20 >> + a_in_elf_base, a_in_abi_base); >> +} >=20 > [...] >=20 >=20 >> diff --git a/tests/test-read-dwarf.cc b/tests/test-read-dwarf.cc >=20 > [...] >=20 >=20 >> +static test_task* >> +new_task(const InOutSpec* s, string& a_out_abi_base, >> + string& a_in_elf_base, string& a_in_abi_base) >=20 > Please doxygen-document this function. >=20 >> +{ >> + return new test_task_dwarf(*s, a_in_abi_base, >=20 > This 'a_in_abi_base' should be a_out_abi_base. >=20 >> + a_in_elf_base, a_in_abi_base); >> +} >>=20 >> int >> main(int argc, char *argv[]) >> { >> bool no_parallel =3D false; >=20 > This variable is not unused. >=20 >=20 > Thanks for working on this. It's really appreciated! >=20 > [...] >=20 > Cheers, >=20 > --=20 > =09=09Dodji >=20