diff mbox series

[Bug,default/26135] New: Possible issue with type equality

Message ID bug-26135-9487@http.sourceware.org/bugzilla/
State Dropped
Headers
Series [Bug,default/26135] New: Possible issue with type equality |

Commit Message

fche at redhat dot com June 18, 2020, 4:48 p.m. UTC 
  https://sourceware.org/bugzilla/show_bug.cgi?id=26135

            Bug ID: 26135
           Summary: Possible issue with type equality
           Product: libabigail
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: gprocida+abigail at google dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

The patch below may indicate a potential issue with type equality in libabigail
(note that this builds on the patch in
https://sourceware.org/pipermail/libabigail/2020q2/002355.html).

runtestreaddwarf reveals some instances where
types_defined_same_linux_kernel_corpus_public returns true for types that
compare as unequal. I think they are all anonymous structs with naming
typedefs.

Regards,
Giuliano

From dd249b7dc83df3609c23ec237a7ddb94e29abf44 Mon Sep 17 00:00:00 2001
From: Giuliano Procida <gprocida@google.com>
Date: Thu, 18 Jun 2020 13:50:59 +0100
Subject: [PATCH] check linux optimisation

---
 src/abg-ir.cc | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

          // flags.
--
2.27.0.290.gba653c62da-goog

Comments

fche at redhat dot com June 24, 2020, 4:18 p.m. UTC | #1 
https://sourceware.org/bugzilla/show_bug.cgi?id=26135

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-06-24
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED
fche at redhat dot com June 24, 2020, 5:17 p.m. UTC | #2 
https://sourceware.org/bugzilla/show_bug.cgi?id=26135

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Possible issue with type    |Wrong linkage name causes
                   |equality                    |anonymous classes
                   |                            |miscomparison
fche at redhat dot com June 24, 2020, 5:22 p.m. UTC | #3 
https://sourceware.org/bugzilla/show_bug.cgi?id=26135

--- Comment #1 from dodji at redhat dot com ---
I can reproduce the issue.  Thank you for raising it.

Based on my current understanding of the problem, I have put together the patch
that you can get from commit
https://sourceware.org/git/?p=libabigail.git;a=commit;h=7da86264ff2c0aff7ecf1c62dee9c0d64ed11798,
from the branch dodji/PR26135.

The branch can be browed at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/dodji/PR26135.

Maybe you can test it in your environment and see if it addresses the problem
you were looking at?

For the curious reader, here is an explanation of the issue.

When comparing decls, the overload of the 'equals' function for
instances of decl_base compares their linkage names.  If they are
different, then the decls are generally considered different.

Class declarations (and definitions) also use the 'equals' function
referred to above.  So when two classes have different linkage names,
they are always considered different.

Now let's consider the case of an anonymous class.  It doesn't have
any user-provided name, by definition.  Libabigail does, however,
assigns it an internal name for various (internal) purposes.  That
internal name is generally ignored for the purpose of (anonymous) type
comparison.  So by design, two anonymous classes can have different
internal anonymous names and yet still happen to be equal.

The root issue in this problem report is that by default, the linkage
name of a class is set to its name.  And when that class is anonymous,
its internal name is used as its linkage name.  Oops.  That leads to
anonymous classes being wrongly considered different.

This patch fixes the issue by providing additional constructors for a
class type to avoid using the internal anonymous name as its linkage
name.

Note that the same issue is present for unions so the patch does the
a similar thing for union types.

Enums are properly handled so we don't need to do anything in that
regard.
fche at redhat dot com June 24, 2020, 5:22 p.m. UTC | #4 
https://sourceware.org/bugzilla/show_bug.cgi?id=26135

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |WAITING
fche at redhat dot com June 25, 2020, 2:56 p.m. UTC | #5 
https://sourceware.org/bugzilla/show_bug.cgi?id=26135

--- Comment #2 from Giuliano Procida <gprocida+abigail at google dot com> ---
Hi.

Your change fixes the discrepancies my patch picked up.

It makes no difference to extracting kernel ABIs though.

It looks like it's safe to apply.

Thank you.

Regards,
Giuliano.
fche at redhat dot com July 6, 2020, 5:14 p.m. UTC | #6 
https://sourceware.org/bugzilla/show_bug.cgi?id=26135

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|WAITING                     |RESOLVED

--- Comment #3 from dodji at redhat dot com ---
The patch was applied to upstream to the master branch at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/master.

Thank you for taking the time to report this problem!
fche at redhat dot com July 16, 2020, 11:41 a.m. UTC | #7 
https://sourceware.org/bugzilla/show_bug.cgi?id=26135

Giuliano Procida <gprocida+abigail at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |---

--- Comment #4 from Giuliano Procida <gprocida+abigail at google dot com> ---
Hi Dodji.

I was looking at type ids in another context and noticed the following.

$ git grep -c " id='type-id-24'"
346e88dd~1:tests/data/test-read-dwarf/PR22122-libftdc.so.abi
346e88dd~1:tests/data/test-read-dwarf/PR22122-libftdc.so.abi:138
$ git grep -c " id='type-id-24'"
346e88dd:tests/data/test-read-dwarf/PR22122-libftdc.so.abi
346e88dd:tests/data/test-read-dwarf/PR22122-libftdc.so.abi:708

While 138 declaration-only anonymous types sharing the same id (and so I
imagine the same canonical type) looks suspicious, 708 doing so is presumably
worse.

I'm sorry I didn't check the test files more carefully sooner.

Please take a look. Thanks.
fche at redhat dot com Sept. 17, 2020, 2:17 p.m. UTC | #8 
https://sourceware.org/bugzilla/show_bug.cgi?id=26135

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|REOPENED                    |RESOLVED

--- Comment #6 from dodji at redhat dot com ---
(In reply to Giuliano Procida from comment #4)
> Hi Dodji.
> 
> I was looking at type ids in another context and noticed the following.
> 
> $ git grep -c " id='type-id-24'"
> 346e88dd~1:tests/data/test-read-dwarf/PR22122-libftdc.so.abi
> 346e88dd~1:tests/data/test-read-dwarf/PR22122-libftdc.so.abi:138
> $ git grep -c " id='type-id-24'"
> 346e88dd:tests/data/test-read-dwarf/PR22122-libftdc.so.abi
> 346e88dd:tests/data/test-read-dwarf/PR22122-libftdc.so.abi:708
> 
> While 138 declaration-only anonymous types sharing the same id (and so I
> imagine the same canonical type) looks suspicious, 708 doing so is
> presumably worse.

I looked into this and it appears that the type with the ID "type-id-24" is an
anonymous class with no data member, no base classes,  and which only contains
non-virtual member functions and/or some member type.

For classes containing no virtual member functions, only base classes and data
member are taken into account for identity.

So this seems normal to me.  Furthermore, as we've been increasing the
likelihood of recognizing two anonymous types as equivalent, I rather find this
particular increase in duplicated ids as positive.

> I'm sorry I didn't check the test files more carefully sooner.

Nah, I think you've done great spotting this.  This could have been a real
issue.

Incidentally, I have filed an enhancement request to detect pathological cases
of duplicated IDs at https://sourceware.org/bugzilla/show_bug.cgi?id=26591.

Thanks!
diff mbox series

Patch

diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 41a8e3f9..a8096690 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -11904,8 +11904,18 @@  type_base::get_canonical_type_for(type_base_sptr t)
          // Compare types by considering that decl-only classes don't
          // equal their definition.
          env->decl_only_class_equals_definition(false);
-         bool equal = types_defined_same_linux_kernel_corpus_public(**it, *t)
-                      || *it == t;
+         bool linux_eq =
types_defined_same_linux_kernel_corpus_public(**it, *t);
+         bool plain_eq = *it == t;
+         // is it really just an optimisation?
+         if (linux_eq > plain_eq)
+           {
+             std::cerr << "type equality discrepancy with "
+                       << t->get_pretty_representation()
+                       << " and "
+                       << (*it)->get_pretty_representation()
+                       << std::endl;
+           }
+         bool equal = linux_eq || plain_eq;
          // Restore the state of the on-the-fly-canonicalization and
          // the decl-only-class-being-equal-to-a-matching-definition