Re: [PATCH v2] Add regression tests for ctf reading

[Guillermo Martinez <guillermo.e.martinez@oracle.com>]

Hello Dodji,

Thanks for your comments!!

> On Wednesday, November 24, 2021 10:36:02 AM CST Dodji Seketeli wrote:
> Hello,
> 
> [...]
> 
> Thanks for working on this.  Nice patch, by the way!  I like its
> direction.
good to know it :-)
 
> 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.
perfect, then I'll move the expected abixml file to accurate directory
and I'll remove the suffix depending of which reader we are testing,
instead of store it in common directory.
> So where exactly is the CTF test disabled?
I remove those test entries, so the possible problem detected using external diff
command in CTF is, for instance, in: test-read-common/test-PR26568-1-ctf.o.abi,
the first time that runtestreadctf is executed an abixml file  is generated:

...
    <class-decl name='' size-in-bits='32' alignment-in-bits='32' is-struct='yes' is-anonymous='yes' visibility='default' id='type-id-3'>
      <data-member access='public' layout-offset-in-bits='0'>
        <var-decl name='x' type-id='type-id-4' visibility='default'/>
      </data-member>
    </class-decl>
    <class-decl name='' size-in-bits='64' alignment-in-bits='64' is-struct='yes' is-anonymous='yes' visibility='default' id='type-id-5'>
      <data-member access='public' layout-offset-in-bits='0'>
        <var-decl name='y' type-id='type-id-6' visibility='default'/>
      </data-member>
    </class-decl>

and this is second execution:
...
    <class-decl name='' size-in-bits='64' alignment-in-bits='64' is-struct='yes' is-anonymous='yes' visibility='default' id='type-id-5'>
      <data-member access='public' layout-offset-in-bits='0'>
        <var-decl name='y' type-id='type-id-6' visibility='default'/>
      </data-member>
    </class-decl>
    <class-decl name='' size-in-bits='32' alignment-in-bits='32' is-struct='yes' is-anonymous='yes' visibility='default' id='type-id-3'>
      <data-member access='public' layout-offset-in-bits='0'>
        <var-decl name='x' type-id='type-id-4' visibility='default'/>
      </data-member>
    </class-decl>

As we can see both abixml files  have *same nodes information* but the nodes
are in a different order, so the test should be marked as passed, however using
the external diff command the test fails. so my question is should we rely in diff
command to compare the corpus of the ELF input files (abixml files)?

https://sourceware.org/pipermail/libabigail/2021q4/003885.html

> [...]
> 
> 
> > 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.
Ok, will wok on in in the next patch version.
> 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.
Ok,
> > +{
> > +  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.
Ok, 
> > +{
> > +  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.
Ok,
> > +{
> > +  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.
Ok,
> > +{
> > +  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.
Ok,
> > +{
> > +  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.
Ok,
> > +{
> 
> [...]
> 
> 
> > 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.
Ok,
> > +  {
> > +    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.
Ok, 
> > +  {
> > +    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.
Ok, 
> > +  {
> > +    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.
Ok,
> > +  {
> > +    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> test_task_sptr;
> > +
> > +struct options
> > +{
> 
> Please doxygen-document this struct.
Ok,
> > +  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.
Ok,
> > +        : 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.
Ok,
> > +{
> > +  return new test_task_ctf(*s, a_in_abi_base,
>
> This 'a_in_abi_base' should be a_out_abi_base.
Ok, sorry .. thanks! .. will be fixed in the next patch.
> > +                           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.
Ok,
> > +{
> > +  return new test_task_dwarf(*s, a_in_abi_base,
> 
> This 'a_in_abi_base' should be a_out_abi_base.
agree, it will be fixed in the next patch.
> > +                             a_in_elf_base, a_in_abi_base);
> > +}
> >  
> >  int
> >  main(int argc, char *argv[])
> >  {
> >    bool no_parallel = false;
will be removed in the next patch.
> This variable is not unused.
> 
> 
> Thanks for working on this.  It's really appreciated!
> 
> [...]
> 
> Cheers,
Thanks again,
Guillermo