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

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

Message

Dodji Seketeli Oct. 3, 2023, 12:21 p.m. UTC
  Hello,

This patch set is intended for the check-uapi-support branch dedicated
to supporting the check-uapi.sh script [1].

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 third patch of the set designates anonymous enums by their "flat
textual representation".  Said otherwise, the name of the firs enum is
going to be: "enum {E1_0, E1_1,}".  This should solve 1/ and 2/.

In the reporting engine, the second 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 fourth 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.

The first patch fixes a subtle issue where
corpus_diff::has_incompatible_changes() fails to accurately say if an
unreachable enum has an incompatible change.  That function was
invariability saying that any change to an unreachable type was an
incompatible change.  Which is false.  Note that the fourth patch has
a test case that covers this fix.  Now that I am writing this, I am
thinking that I should probably come up with a separate test case for
this patch alone, and maybe this patch should independently go into
the master branch.  Oh well.

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.

I am thus asking a for a review for merging this work-in-progress set
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 becomes somewhat testable.

[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 (4):
  abg-comparison[-priv]: Detect incompatible unreachable type changes
  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-priv.h                     |     3 +
 src/abg-comparison.cc                         |   140 +-
 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 +
 29 files changed, 7927 insertions(+), 7542 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