[PATCH] Fix bug that suppressed DWARF read tests.

[PATCH] Fix bug that suppressed DWARF read tests.

[Giuliano Procida]

An earlier commit added code to allow the ABI write/reread and compare
phases of the tests to skipped in the case that no ABI files are given
for comparison.

Unfortunately, the new code skipped those phases unconditionally.

This patch changes the in_abi_path and out_abi_path values used in
in_out_specs used to trigger the early termination from "" to nullptr
and updates the conditional logic checking them. Several later commits
also need adjustment to test specification or test data.

The changes below are listed against the commit they fix.

4252dfd6 dwarf-reader: handle symtab.section_header.sh_entsize == 0

	* tests/test-read-dwarf.cc (in_out_specs): Use nullptr instead
	of "" for test25 case. (perform): Check members of spec,
	rather than locals with same name, when deciding to terminate
	testing early.

4457c10e dwarf-reader: handle binaries with missing symtab

	* tests/test-read-dwarf.cc (in_out_specs): Use nullptr to
	signal early termination of test26.

34e867e7 dwarf-reader: remove superfluous ABG_ASSERT

	* tests/test-read-dwarf.cc (in_out_specs): Use nullptr to
	signal early termination of test27.

2d5389f2 Fix size calculations for multidimensional arrays.

	* tests/data/test-read-dwarf/test14-pr18893.so.abi: Update
	array sizes.
	* tests/data/test-read-dwarf/test17-pr19027.so.abi: Ditto.
	* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test7.so.abi: Ditto.

246ca200 corpus/writer: sort emitted translation units by path name

	* tests/data/test-read-dwarf/PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi:
	Update following translation unit ordering change.
	* tests/data/test-read-dwarf/test13-pr18894.so.abi: Ditto.
	* tests/data/test-read-dwarf/test14-pr18893.so.abi: Ditto.
	* tests/data/test-read-dwarf/test15-pr18892.so.abi: Ditto.
	* tests/data/test-read-dwarf/test16-pr18904.so.abi: Ditto.
	* tests/data/test-read-dwarf/test17-pr19027.so.abi: Ditto.
	* tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test21-pr19092.so.abi: Ditto.
	* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
	Ditto.

e8bf5b80 Bug 25989 - type_topo_comp doesn't meet irreflexive requirements

	* tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Update
	following code changes affecting ordering of some ABI
	elements.
	* tests/data/test-read-dwarf/test16-pr18904.so.abi

Finally, this commit also corrects some bad code formatting.

	* tests/test-read-dwarf.cc (perform): Fix some whitespace.

Reviewed-by: Matthias Maennich <maennich@google.com>
Tested-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 .../data/test-read-dwarf/PR25007-sdhci.ko.abi |  3442 +-
 .../PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi |  1072 +-
 .../test-read-dwarf/test13-pr18894.so.abi     |   454 +-
 .../test-read-dwarf/test14-pr18893.so.abi     | 10274 +--
 .../test-read-dwarf/test15-pr18892.so.abi     | 38226 ++++----
 .../test-read-dwarf/test16-pr18904.so.abi     | 44259 ++++-----
 .../test-read-dwarf/test17-pr19027.so.abi     | 11110 +--
 ...st18-pr19037-libvtkRenderingLIC-6.1.so.abi |    42 +-
 ...19-pr19023-libtcmalloc_and_profiler.so.abi | 37213 ++++----
 ...st20-pr19025-libvtkParallelCore-6.1.so.abi |  8540 +-
 .../test-read-dwarf/test21-pr19092.so.abi     |  7734 +-
 .../test22-pr19097-libstdc++.so.6.0.17.so.abi | 75710 ++++++++--------
 tests/data/test-read-dwarf/test7.so.abi       |     2 +-
 tests/test-read-dwarf.cc                      |    19 +-
 14 files changed, 119199 insertions(+), 118898 deletions(-)

Patch omitted as too large to send by email.

See:
https://github.com/myxoid/libabigail/commit/6d52caf932a39267c4ee0e073a8f5010d566feae
-- 
2.27.0.290.gba653c62da-goog

Re: [PATCH] Fix bug that suppressed DWARF read tests.

[Dodji Seketeli]

Hello,

Giuliano Procida <gprocida@google.com> a écrit:

[...]

> Patch omitted as too large to send by email.
>
> See:
> https://github.com/myxoid/libabigail/commit/6d52caf932a39267c4ee0e073a8f5010d566feae

Noted, thanks.  I found it in the branch fix-test-read-dwarf in that
repository.

[...]

diff --git a/tests/test-read-dwarf.cc b/tests/test-read-dwarf.cc
index 7c99d21..ecc70cd 100644
--- a/tests/test-read-dwarf.cc
+++ b/tests/test-read-dwarf.cc
@@ -256,20 +256,20 @@ InOutSpec in_out_specs[] =
   {
     "data/test-read-dwarf/test25-bogus-binary.elf",
     "",
-    "",
-    "",
+    nullptr,
+    nullptr,
   },
   {
     "data/test-read-dwarf/test26-bogus-binary.elf",
     "",
-    "",
-    "",
+    nullptr,
+    nullptr,
   },
   {
     "data/test-read-dwarf/test27-bogus-binary.elf",
     "",
-    "",
-    "",
+    nullptr,
+    nullptr,

The nullptr there is valid only in a c++11 context.  It won't compile
pre-c++11.  So please use NULL, or 0 instead.

[...]

-    if (in_abi_path.empty() && out_abi_path.empty()) {
+    if (!spec.in_abi_path && !spec.out_abi_path)
+      {

Whoah, great catch!

[...]

> An earlier commit added code to allow the ABI write/reread and compare
> phases of the tests to skipped in the case that no ABI files are given
> for comparison.

I would explicitely mention the commit (by hash and title) that you
refer to as "earlier commit", e.g:

    This commit:
	4252dfd6c5e0a dwarf-reader: handle symtab.section_header.sh_entsize == 0
    added code to allow the ABI write/reread etc ...

[...]


> The changes below are listed against the commit they fix.
>
> 4252dfd6 dwarf-reader: handle symtab.section_header.sh_entsize == 0
>
> 	* tests/test-read-dwarf.cc (in_out_specs): Use nullptr instead
> 	of "" for test25 case. (perform): Check members of spec,
> 	rather than locals with same name, when deciding to terminate
> 	testing early.

I understand that you are explicitly referring to the commits that
are modified by yours; that possibly eases the review and I thank
you for that.  However, for the sake of consistency, this ChangeLog
part of the commit log should just keep the same structure as all the
rest.  So it'd look like this, rather:

        * tests/test-read-dwarf.cc (in_out_specs): Use nullptr instead of
	"" for test25 case. (perform): Check members of spec, rather than
	locals with same name, when deciding to terminate testing early.
	* tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Update as it
	should have been by commit "e8bf5b80 Bug 25989 - type_topo_comp
	doesn't meet irreflexive requirements".
	* tests/data/test-read-dwarf/PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi:
	Update as it should have been by commit "246ca200 corpus/writer:
	sort emitted translation units by path name".
	* etc ...


Thanks,

Cheers.

-- 
		Dodji

[PATCH v2] Fix bug that suppressed DWARF read tests.

[Giuliano Procida]

Commit 4252dfd6 added code to allow the ABI write/reread and compare
phases of the tests to skipped in the case that no ABI files are given
for comparison.

Unfortunately, the new code skipped those phases unconditionally.

This patch changes the in_abi_path and out_abi_path values used in
in_out_specs used to trigger the early termination from "" to NULL and
updates the conditional logic checking them. Several subsequent
commits which affect ABI output were missing these changes to the test
data files.

This change fixes the following list of commits.

4252dfd6 dwarf-reader: handle symtab.section_header.sh_entsize == 0
4457c10e dwarf-reader: handle binaries with missing symtab
34e867e7 dwarf-reader: remove superfluous ABG_ASSERT
2d5389f2 Fix size calculations for multidimensional arrays.
246ca200 corpus/writer: sort emitted translation units by path name
e8bf5b80 Bug 25989 - type_topo_comp doesn't meet irreflexive requirements

Finally, this commit also corrects some bad code formatting.

	* tests/test-read-dwarf.cc (in_out_specs): Use NULL instead of
	empty ABI paths for test25, test26 and test27. (perform):
	Check members of spec, rather than locals with same name, when
	deciding to terminate testing early; fix some code whitespace.
	* tests/data/test-read-dwarf/test14-pr18893.so.abi: Update
	multidimensional array sizes.
	* tests/data/test-read-dwarf/test17-pr19027.so.abi: Ditto.
	* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test7.so.abi: Ditto.
	* tests/data/test-read-dwarf/PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi:
	Update following translation unit ordering change.
	* tests/data/test-read-dwarf/test13-pr18894.so.abi: Ditto.
	* tests/data/test-read-dwarf/test14-pr18893.so.abi: Ditto.
	* tests/data/test-read-dwarf/test15-pr18892.so.abi: Ditto.
	* tests/data/test-read-dwarf/test16-pr18904.so.abi: Ditto.
	* tests/data/test-read-dwarf/test17-pr19027.so.abi: Ditto.
	* tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test21-pr19092.so.abi: Ditto.
	* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Update
	following code changes affecting ordering of some ABI
	elements.
	* tests/data/test-read-dwarf/test16-pr18904.so.abi

Reviewed-by: Matthias Maennich <maennich@google.com>
Tested-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 .../data/test-read-dwarf/PR25007-sdhci.ko.abi |  3442 +-
 .../PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi |  1072 +-
 .../test-read-dwarf/test13-pr18894.so.abi     |   454 +-
 .../test-read-dwarf/test14-pr18893.so.abi     | 10274 +--
 .../test-read-dwarf/test15-pr18892.so.abi     | 38226 ++++----
 .../test-read-dwarf/test16-pr18904.so.abi     | 44259 ++++-----
 .../test-read-dwarf/test17-pr19027.so.abi     | 11110 +--
 ...st18-pr19037-libvtkRenderingLIC-6.1.so.abi |    42 +-
 ...19-pr19023-libtcmalloc_and_profiler.so.abi | 37213 ++++----
 ...st20-pr19025-libvtkParallelCore-6.1.so.abi |  8540 +-
 .../test-read-dwarf/test21-pr19092.so.abi     |  7734 +-
 .../test22-pr19097-libstdc++.so.6.0.17.so.abi | 75710 ++++++++--------
 tests/data/test-read-dwarf/test7.so.abi       |     2 +-
 tests/test-read-dwarf.cc                      |    19 +-
 14 files changed, 119199 insertions(+), 118898 deletions(-)

Patch omitted as too large to send by email.

See:
https://github.com/myxoid/libabigail/commit/0a6bed1d37935172188102351a8cba95eb05b7ff

-- 
2.27.0.290.gba653c62da-goog

Re: [PATCH] Fix bug that suppressed DWARF read tests.

[Giuliano Procida]

Thanks! v2 should be with you.

The one-line commit message is quoted below, I just used the short
hash in the opening line.

Regards,
Giuliano.

On Tue, 16 Jun 2020 at 14:57, Dodji Seketeli <dodji@seketeli.org> wrote:
>
> Hello,
>
> Giuliano Procida <gprocida@google.com> a écrit:
>
> [...]
>
> > Patch omitted as too large to send by email.
> >
> > See:
> > https://github.com/myxoid/libabigail/commit/6d52caf932a39267c4ee0e073a8f5010d566feae
>
> Noted, thanks.  I found it in the branch fix-test-read-dwarf in that
> repository.
>
> [...]
>
> diff --git a/tests/test-read-dwarf.cc b/tests/test-read-dwarf.cc
> index 7c99d21..ecc70cd 100644
> --- a/tests/test-read-dwarf.cc
> +++ b/tests/test-read-dwarf.cc
> @@ -256,20 +256,20 @@ InOutSpec in_out_specs[] =
>    {
>      "data/test-read-dwarf/test25-bogus-binary.elf",
>      "",
> -    "",
> -    "",
> +    nullptr,
> +    nullptr,
>    },
>    {
>      "data/test-read-dwarf/test26-bogus-binary.elf",
>      "",
> -    "",
> -    "",
> +    nullptr,
> +    nullptr,
>    },
>    {
>      "data/test-read-dwarf/test27-bogus-binary.elf",
>      "",
> -    "",
> -    "",
> +    nullptr,
> +    nullptr,
>
> The nullptr there is valid only in a c++11 context.  It won't compile
> pre-c++11.  So please use NULL, or 0 instead.
>
> [...]
>
> -    if (in_abi_path.empty() && out_abi_path.empty()) {
> +    if (!spec.in_abi_path && !spec.out_abi_path)
> +      {
>
> Whoah, great catch!
>
> [...]
>
> > An earlier commit added code to allow the ABI write/reread and compare
> > phases of the tests to skipped in the case that no ABI files are given
> > for comparison.
>
> I would explicitely mention the commit (by hash and title) that you
> refer to as "earlier commit", e.g:
>
>     This commit:
>         4252dfd6c5e0a dwarf-reader: handle symtab.section_header.sh_entsize == 0
>     added code to allow the ABI write/reread etc ...
>
> [...]
>
>
> > The changes below are listed against the commit they fix.
> >
> > 4252dfd6 dwarf-reader: handle symtab.section_header.sh_entsize == 0
> >
> >       * tests/test-read-dwarf.cc (in_out_specs): Use nullptr instead
> >       of "" for test25 case. (perform): Check members of spec,
> >       rather than locals with same name, when deciding to terminate
> >       testing early.
>
> I understand that you are explicitly referring to the commits that
> are modified by yours; that possibly eases the review and I thank
> you for that.  However, for the sake of consistency, this ChangeLog
> part of the commit log should just keep the same structure as all the
> rest.  So it'd look like this, rather:
>
>         * tests/test-read-dwarf.cc (in_out_specs): Use nullptr instead of
>         "" for test25 case. (perform): Check members of spec, rather than
>         locals with same name, when deciding to terminate testing early.
>         * tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Update as it
>         should have been by commit "e8bf5b80 Bug 25989 - type_topo_comp
>         doesn't meet irreflexive requirements".
>         * tests/data/test-read-dwarf/PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi:
>         Update as it should have been by commit "246ca200 corpus/writer:
>         sort emitted translation units by path name".
>         * etc ...
>
>
> Thanks,
>
> Cheers.
>
> --
>                 Dodji

[PATCH v3] Fix bug that suppressed DWARF read tests.

[Giuliano Procida]

Commit 4252dfd6 added code to allow the ABI write/reread and compare
phases of the tests to be skipped in the case that no ABI files are
given for comparison.

Unfortunately, the new code skipped those phases unconditionally.

This patch changes the in_abi_path and out_abi_path values used in
in_out_specs used to trigger the early termination from "" to NULL and
updates the conditional logic checking them. Several subsequent
commits which affect ABI output were missing these changes to the test
data files.

This change fixes the following list of commits.

4252dfd6 dwarf-reader: handle symtab.section_header.sh_entsize == 0
4457c10e dwarf-reader: handle binaries with missing symtab
34e867e7 dwarf-reader: remove superfluous ABG_ASSERT
2d5389f2 Fix size calculations for multidimensional arrays.
246ca200 corpus/writer: sort emitted translation units by path name
e8bf5b80 Bug 25989 - type_topo_comp doesn't meet irreflexive requirements

Finally, this commit also corrects some bad code formatting.

	* tests/test-read-dwarf.cc (in_out_specs): Use NULL instead of
	empty ABI paths for test25, test26 and test27. (perform):
	Check members of spec, rather than locals with same name, when
	deciding to terminate testing early; fix some code whitespace.
	* tests/data/test-read-dwarf/test14-pr18893.so.abi: Update
	multidimensional array sizes.
	* tests/data/test-read-dwarf/test17-pr19027.so.abi: Ditto.
	* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test7.so.abi: Ditto.
	* tests/data/test-read-dwarf/PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi:
	Update following translation unit ordering change.
	* tests/data/test-read-dwarf/test13-pr18894.so.abi: Ditto.
	* tests/data/test-read-dwarf/test14-pr18893.so.abi: Ditto.
	* tests/data/test-read-dwarf/test15-pr18892.so.abi: Ditto.
	* tests/data/test-read-dwarf/test16-pr18904.so.abi: Ditto.
	* tests/data/test-read-dwarf/test17-pr19027.so.abi: Ditto.
	* tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/test21-pr19092.so.abi: Ditto.
	* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
	Ditto.
	* tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Update
	following code changes affecting ordering of some ABI
	elements.
	* tests/data/test-read-dwarf/test16-pr18904.so.abi

Reviewed-by: Matthias Maennich <maennich@google.com>
Tested-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 .../data/test-read-dwarf/PR25007-sdhci.ko.abi |  3442 +-
 .../PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi |  1072 +-
 .../test-read-dwarf/test13-pr18894.so.abi     |   454 +-
 .../test-read-dwarf/test14-pr18893.so.abi     | 10274 +--
 .../test-read-dwarf/test15-pr18892.so.abi     | 38226 ++++----
 .../test-read-dwarf/test16-pr18904.so.abi     | 44259 ++++-----
 .../test-read-dwarf/test17-pr19027.so.abi     | 11110 +--
 ...st18-pr19037-libvtkRenderingLIC-6.1.so.abi |    42 +-
 ...19-pr19023-libtcmalloc_and_profiler.so.abi | 37213 ++++----
 ...st20-pr19025-libvtkParallelCore-6.1.so.abi |  8540 +-
 .../test-read-dwarf/test21-pr19092.so.abi     |  7734 +-
 .../test22-pr19097-libstdc++.so.6.0.17.so.abi | 75710 ++++++++--------
 tests/data/test-read-dwarf/test7.so.abi       |     2 +-
 tests/test-read-dwarf.cc                      |    19 +-
 14 files changed, 119199 insertions(+), 118898 deletions(-)

Patch omitted as too large to send by email.

See:
https://github.com/myxoid/libabigail/commit/15579d778efe0b79a35d7c8b1efa0867cc328956

-- 
2.27.0.290.gba653c62da-goog

Re: [PATCH v3] Fix bug that suppressed DWARF read tests.

[Dodji Seketeli]

Giuliano Procida <gprocida@google.com> a écrit:

[...]

> 	* tests/test-read-dwarf.cc (in_out_specs): Use NULL instead of
> 	empty ABI paths for test25, test26 and test27. (perform):
> 	Check members of spec, rather than locals with same name, when
> 	deciding to terminate testing early; fix some code whitespace.
> 	* tests/data/test-read-dwarf/test14-pr18893.so.abi: Update
> 	multidimensional array sizes.
> 	* tests/data/test-read-dwarf/test17-pr19027.so.abi: Ditto.
> 	* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
> 	Ditto.
> 	* tests/data/test-read-dwarf/test7.so.abi: Ditto.
> 	* tests/data/test-read-dwarf/PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi:
> 	Update following translation unit ordering change.
> 	* tests/data/test-read-dwarf/test13-pr18894.so.abi: Ditto.
> 	* tests/data/test-read-dwarf/test14-pr18893.so.abi: Ditto.
> 	* tests/data/test-read-dwarf/test15-pr18892.so.abi: Ditto.
> 	* tests/data/test-read-dwarf/test16-pr18904.so.abi: Ditto.
> 	* tests/data/test-read-dwarf/test17-pr19027.so.abi: Ditto.
> 	* tests/data/test-read-dwarf/test18-pr19037-libvtkRenderingLIC-6.1.so.abi:
> 	Ditto.
> 	* tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
> 	Ditto.
> 	* tests/data/test-read-dwarf/test20-pr19025-libvtkParallelCore-6.1.so.abi:
> 	Ditto.
> 	* tests/data/test-read-dwarf/test21-pr19092.so.abi: Ditto.
> 	* tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
> 	Ditto.
> 	* tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Update
> 	following code changes affecting ordering of some ABI
> 	elements.
> 	* tests/data/test-read-dwarf/test16-pr18904.so.abi
>
> Reviewed-by: Matthias Maennich <maennich@google.com>
> Tested-by: Matthias Maennich <maennich@google.com>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks!

Cheers,

-- 
		Dodji