[0/3,V2] For WIP branch check-uapi-support: Fix comparing non-reachable anonymous enums

Message ID 87r0mapspt.fsf@redhat.com
Headers
Series For WIP branch check-uapi-support: Fix comparing non-reachable anonymous enums |

Message

Dodji Seketeli Oct. 4, 2023, 10:50 a.m. UTC
  Hello,

The version 2 of this patch set is intended for the check-uapi-support
branch dedicated to supporting the check-uapi.sh script [1].

Note that the difference with the first version is that the first
patch of the previous version got merged into the master branch
because it was independant from the task at hand.  You can see it at
https://inbox.sourceware.org/libabigail/87cyxur879.fsf@redhat.com.

Please find below a thorough description of this patch set.

The description of the task of supporting the comparison of
non-reachable anonymous enums can be read on the email
https://inbox.sourceware.org/libabigail/340b33bd-2b43-9f99-58e1-f1b77a51b48a@quicinc.com/:

    Another issue that comes up when comparing ABI across wide swaths
    of kernel commit history are changes to anonymous enums. From what
    I can tell, there's not a great way to handle this. If someone
    adds a new anonymous enum, the tag abidiff gives (e.g. enum
    __anonymous_enum__6) can change, so abidiff reports a new
    anonymous enum with all the same fields even though it was
    basically just a "rename".

    For reference, this file is full of anonymous enums:
    https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool_netlink.h#L15.
    Basically any change in there is triggering a failure from the script.

So, this patch set allows changes to anonymous enums that are not used
by any function or global to be represented as we would expect them.

For instance, suppose that a translation unit has the anonymous enums below:

    enum
    {
      E1_0,
      E1_1
    };

    enum
    {
      E2_0,
      E2_1
    };

Suppose also that a subsequent version of that translation unit had
some changes to that anonymous enum, making it become:

    enum
    {
      E1_0,
      E1_1,
      E1_2
    };

    enum
    {
      E2_0,
      E2_1,
      E2_2
    };

Before this patch, compiling the files with the gcc option
-fno-eliminate-unused-debug-types and using abidiff --non-reachable-types on the output
would yield something like:

    Unreachable types summary: 0 removed, 1 changed, 1 added types

    1 changed type unreachable from any public interface:

      [C] 'enum __anonymous_enum__1' changed:
	type name changed from '__anonymous_enum__1' to '__anonymous_enum__'
	type size hasn't changed
	2 enumerator deletions:
	  '__anonymous_enum__1::E2_0' value '0'
	  '__anonymous_enum__1::E2_1' value '1'
	3 enumerator insertions:
	  '__anonymous_enum__::E1_O' value '0'
	  '__anonymous_enum__::E1_1' value '1'
	  '__anonymous_enum__::E1_2' value '2'

    1 added type unreachable from any public interface:

      [A] 'enum __anonymous_enum__1' at test1-v1.c:9:1

What happens here is that:

1/ the internal name used to designate anonymous enums for the purpose
of type canonicalization is used for user-facing reporting, which
arguably makes very little sense from the point of view of the users.

2/ The generation of the internal names for anonymous enums is not
meant to be "stable" across translation units.  So the internal name
of the first anonymous enum in the first version of the translation
unit doesn't necessary match the one in the second version of the
translation unit.

And a number of downstream issues originates from 1/ and 2/.

The second patch of the set designates anonymous enums by their "flat
textual representation".  Said otherwise, the name of the first enum is
going to be: "enum {E1_0, E1_1,}".  This should solve 1/ and 2/.

In the reporting engine, the first patch uses that "flat
representation" to name anonymous types in general (and enums in
particular), instead of trying to use their qualified name (and thus
their internal meaningless names).

The third patch teaches the comparison engine that if an anonymous enum is
deleted and added again (with the two version of the anonymous enum
differing by a few enumerators), then we are probably looking at a
change of anonymous enum.

In any case, with this patch set applied, abidiff says the following
about the example given earlier:

    Unreachable types summary: 0 removed, 2 changed, 0 added types

    2 changed types unreachable from any public interface:

      [C] 'enum {E2_0=0, E2_1=1, }' changed:
	type size hasn't changed
	1 enumerator insertion:
	  'E2_2' value '2'

      [C] 'enum {E1_1=1, E1_O=0, }' changed:
	type size hasn't changed
	1 enumerator insertion:
	  'E1_2' value '2'

This is much closer to what we'd intuitively expect.

This patch set is to be merging into the branch "check-uapi-support"
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/check-uapi-support,
so that it stays somewhat testable.

For now, the patches are in the branch "better-anon-enums" which can
be browsed at https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/better-anon-enums.

[1]: The bug that tracks the work to support the check-uapi.sh script
is https://sourceware.org/bugzilla/show_bug.cgi?id=30931.

Dodji Seketeli (3):
  default-reporter,reporter-priv: Do not report names of anonymous enums
  ir,comparison,corpus: Better support anonymous enums comparison
  ir,comparison: Represent changed anonymous enums

 include/abg-fwd.h                             |    27 +
 include/abg-ir.h                              |     7 +
 src/abg-comparison.cc                         |    86 +-
 src/abg-corpus.cc                             |     4 +-
 src/abg-default-reporter.cc                   |    12 +-
 src/abg-ir.cc                                 |   205 +-
 src/abg-reporter-priv.cc                      |     3 +-
 tests/data/Makefile.am                        |     6 +
 .../test-anonymous-enums-change-report-v0.txt |    16 +
 .../test-anonymous-enums-change-report-v1.txt |    21 +
 .../test-anonymous-enums-change-v0.c          |    36 +
 .../test-anonymous-enums-change-v0.o          |   Bin 0 -> 3296 bytes
 .../test-anonymous-enums-change-v1.c          |    41 +
 .../test-anonymous-enums-change-v1.o          |   Bin 0 -> 3336 bytes
 tests/data/test-annotate/libtest23.so.abi     |   224 +-
 .../test-anonymous-members-0.o.abi            |    22 +-
 .../data/test-annotate/test15-pr18892.so.abi  |     8 +-
 ...st18-pr19037-libvtkRenderingLIC-6.1.so.abi |    54 +-
 ...19-pr19023-libtcmalloc_and_profiler.so.abi |    10 +-
 ...st20-pr19025-libvtkParallelCore-6.1.so.abi |     2 +-
 .../test43-PR22913-report-0.txt               |     4 +-
 .../test-read-dwarf/PR22122-libftdc.so.abi    | 12416 ++++++++--------
 tests/data/test-read-dwarf/libtest23.so.abi   |   216 +-
 .../test-read-dwarf/test-libandroid.so.abi    |    44 +-
 .../test-read-dwarf/test16-pr18904.so.abi     |  1816 +--
 ...st18-pr19037-libvtkRenderingLIC-6.1.so.abi |    40 +-
 .../test22-pr19097-libstdc++.so.6.0.17.so.abi |    60 +-
 tests/test-abidiff-exit.cc                    |    32 +
 28 files changed, 7881 insertions(+), 7531 deletions(-)
 create mode 100644 tests/data/test-abidiff-exit/test-anonymous-enums-change-report-v0.txt
 create mode 100644 tests/data/test-abidiff-exit/test-anonymous-enums-change-report-v1.txt
 create mode 100644 tests/data/test-abidiff-exit/test-anonymous-enums-change-v0.c
 create mode 100644 tests/data/test-abidiff-exit/test-anonymous-enums-change-v0.o
 create mode 100644 tests/data/test-abidiff-exit/test-anonymous-enums-change-v1.c
 create mode 100644 tests/data/test-abidiff-exit/test-anonymous-enums-change-v1.o