[PATCH v2] Add regression tests for ctf reading

[Dodji Seketeli]

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> 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