Patch review request for Bug 2664 - unexpected declaration-only types

Patch review request for Bug 2664 - unexpected declaration-only types

[Dodji Seketeli]

Hello Giuliano, Matthias and whoever would be interested in reviewing
some patches :-)

These two small patches that follow this message are meant to fix the
problem reported at
https://sourceware.org/bugzilla/show_bug.cgi?id=26646.

The first one is a pre-requisite to fix a crash caused by the fact
that we hit an assert in the symtab reader.

The second one should hopefully fix the actual problem reported.

Could you please review the patches and test them in your environment
to see if they are useful at all?

Thanks!

-- 
		Dodji

[PATCH 1/2] symtab-reader: Remove an over-agressive assertion

[Dodji Seketeli]

In symtab::load, the symtab reader walks the symbol table and records
each relation "symbol <-> address".
So, the relation "foo <-> address-of-foo" is going to be recorded.
The relation "foo.cfi <-> address-of-foo.cfi" is going to be recorded
as well.

But then, because the symbol foo.cfi has a special meaning, in the
realm of "control flow integrity", the relation "foo.cfi <->
address-of-foo.cfi" (as well as all the *.cfi <-> address-of*.cfi
relations) is going to be recorded (again but) in a particular way by
calling symtab::add_alternative_address_lookups.

The problem is that in, symtab::add_alternative_address_lookups there
is an assert that (wrongly) assumes that the relation foo.cfi <->
address-of-foo.cfi is being seen for the first time.  This is wrong
because the loop in symtab::load that records all the "symbol <->
address" relations has seen and recorded this foo.cfi <->
address-of-foo.cfi relation once already.

This patch removes that assert so that the kernel referred to in the bug
report of PR26646, as mentioned in
https://sourceware.org/bugzilla/show_bug.cgi?id=26646#c5, can be
processed by abidw without crashing.

	* src/abg-symtab-reader.cc
	(symtab::add_alternative_address_lookups): Remove over-aggressive
	assert.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-symtab-reader.cc | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
index 78dec36d..b42ce87d 100644
--- a/src/abg-symtab-reader.cc
+++ b/src/abg-symtab-reader.cc
@@ -651,9 +651,7 @@ symtab::add_alternative_address_lookups(Elf* elf_handle)
 							     symbol_sptr);
 		}
 
-	      const auto result =
-		  addr_symbol_map_.emplace(symbol_value, symbol_sptr);
-	      ABG_ASSERT(result.second);
+	      addr_symbol_map_.emplace(symbol_value, symbol_sptr);
 	    }
 	}
     }
-- 
2.35.0.rc2



-- 
		Dodji

Re: [PATCH 1/2] symtab-reader: Remove an over-agressive assertion

[Giuliano Procida]

Hi.

On Mon, 7 Feb 2022 at 11:10, Dodji Seketeli <dodji@redhat.com> wrote:
>
>
> In symtab::load, the symtab reader walks the symbol table and records
> each relation "symbol <-> address".
> So, the relation "foo <-> address-of-foo" is going to be recorded.
> The relation "foo.cfi <-> address-of-foo.cfi" is going to be recorded
> as well.
>
> But then, because the symbol foo.cfi has a special meaning, in the
> realm of "control flow integrity", the relation "foo.cfi <->
> address-of-foo.cfi" (as well as all the *.cfi <-> address-of*.cfi
> relations) is going to be recorded (again but) in a particular way by
> calling symtab::add_alternative_address_lookups.
>
> The problem is that in, symtab::add_alternative_address_lookups there
> is an assert that (wrongly) assumes that the relation foo.cfi <->
> address-of-foo.cfi is being seen for the first time.  This is wrong
> because the loop in symtab::load that records all the "symbol <->
> address" relations has seen and recorded this foo.cfi <->
> address-of-foo.cfi relation once already.
>
> This patch removes that assert so that the kernel referred to in the bug
> report of PR26646, as mentioned in
> https://sourceware.org/bugzilla/show_bug.cgi?id=26646#c5, can be
> processed by abidw without crashing.
>
>         * src/abg-symtab-reader.cc
>         (symtab::add_alternative_address_lookups): Remove over-aggressive
>         assert.
>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>

Reviewed-by: Giuliano Procida <gprocida@google.com>

I didn't see a better solution and you've written up a proper analysis
in the commit message. Thanks!

Giuliano.

> ---
>  src/abg-symtab-reader.cc | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
> index 78dec36d..b42ce87d 100644
> --- a/src/abg-symtab-reader.cc
> +++ b/src/abg-symtab-reader.cc
> @@ -651,9 +651,7 @@ symtab::add_alternative_address_lookups(Elf* elf_handle)
>                                                              symbol_sptr);
>                 }
>
> -             const auto result =
> -                 addr_symbol_map_.emplace(symbol_value, symbol_sptr);
> -             ABG_ASSERT(result.second);
> +             addr_symbol_map_.emplace(symbol_value, symbol_sptr);
>             }
>         }
>      }
> --
> 2.35.0.rc2
>
>
>
> --
>                 Dodji
>

Re: [PATCH 1/2] symtab-reader: Remove an over-agressive assertion

[Dodji Seketeli]

Giuliano Procida via Libabigail <libabigail@sourceware.org> a écrit:

> Hi.
>
> On Mon, 7 Feb 2022 at 11:10, Dodji Seketeli <dodji@redhat.com> wrote:
>>
>>
>> In symtab::load, the symtab reader walks the symbol table and records
>> each relation "symbol <-> address".
>> So, the relation "foo <-> address-of-foo" is going to be recorded.
>> The relation "foo.cfi <-> address-of-foo.cfi" is going to be recorded
>> as well.
>>
>> But then, because the symbol foo.cfi has a special meaning, in the
>> realm of "control flow integrity", the relation "foo.cfi <->
>> address-of-foo.cfi" (as well as all the *.cfi <-> address-of*.cfi
>> relations) is going to be recorded (again but) in a particular way by
>> calling symtab::add_alternative_address_lookups.
>>
>> The problem is that in, symtab::add_alternative_address_lookups there
>> is an assert that (wrongly) assumes that the relation foo.cfi <->
>> address-of-foo.cfi is being seen for the first time.  This is wrong
>> because the loop in symtab::load that records all the "symbol <->
>> address" relations has seen and recorded this foo.cfi <->
>> address-of-foo.cfi relation once already.
>>
>> This patch removes that assert so that the kernel referred to in the bug
>> report of PR26646, as mentioned in
>> https://sourceware.org/bugzilla/show_bug.cgi?id=26646#c5, can be
>> processed by abidw without crashing.
>>
>>         * src/abg-symtab-reader.cc
>>         (symtab::add_alternative_address_lookups): Remove over-aggressive
>>         assert.
>>
>> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
>
> Reviewed-by: Giuliano Procida <gprocida@google.com>
>
> I didn't see a better solution and you've written up a proper analysis
> in the commit message. Thanks!

Thanks!

Applied to master.

[...]

Cheers,

-- 
		Dodji

[PATCH 2/2] Bug 26646 - unexpected declaration-only types

[Dodji Seketeli]

In a version of the kernel binary referred to in this problem report,
the parameter 'skb' of the udp4_hwcsum function, which is of type
"pointer to struct sk_buff", indirectly refers to a pointer to a
declaration-only struct ip_mc_list.

In another version of that kernel binary, the same parameter skb of
the udp4_hwcsum function is still of type "pointer to struct sk_buff",
but in that case, the sk_buff indirectly refers to a pointer to a
fully defined struct ip_mc_list.

The first kernel only contains a decl-only struct ip_mc_list whereas
the second one contains a fully defined struct ip_mc_list.

This problem comes from the fact that in add_or_update_class_type, we
"reuse" the "struct sk_buff" that we've already seen in the same
binary, if any.  Depending on the order in which types are defined in
the debug information, if the DIE for struct sk_buff that refers to a
decl-only struct ip_mc_list has already been "seen" by the
DWARF-reader, then add_or_update_class_type re-uses the IR of that DIE
that's been constructed already; otherwise, the IR for the struct
sk_buff represented by the current DIE is constructed.

This patch fixes the problem by always constructing an IR for the
DIE that is being seen, in add_or_update_class_type.

	* src/abg-dwarf-reader.cc (add_or_update_class_type): Do not reuse
	the IR for a DIE with the same textual representation as the one
	we are seeing now.
	* tests/data/test-annotate/test21-pr19092.so.abi: Adjust.
	* tests/data/test-read-dwarf/test21-pr19092.so.abi: Likewise.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-dwarf-reader.cc                          | 13 -------------
 tests/data/test-annotate/test21-pr19092.so.abi   |  2 +-
 tests/data/test-read-dwarf/test21-pr19092.so.abi |  2 +-
 3 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index d8545b4c..53b5845d 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -12160,19 +12160,6 @@ add_or_update_class_type(read_context&	 ctxt,
       }
   }
 
-  if (!ctxt.die_is_in_cplus_plus(die))
-    // In c++, a given class might be put together "piecewise".  That
-    // is, in a translation unit, some data members of that class
-    // might be defined; then in another later, some member types
-    // might be defined.  So we can't just re-use a class "verbatim"
-    // just because we've seen previously.  So in c++, re-using the
-    // class is a much clever process.  In the other languages however
-    // (like in C) we can re-use a class definition verbatim.
-    if (class_decl_sptr class_type =
-	is_class_type(ctxt.lookup_type_from_die(die)))
-      if (!class_type->get_is_declaration_only())
-	return class_type;
-
   string name, linkage_name;
   location loc;
   die_loc_and_name(ctxt, die, loc, name, linkage_name);
diff --git a/tests/data/test-annotate/test21-pr19092.so.abi b/tests/data/test-annotate/test21-pr19092.so.abi
index e009a191..bb87fc54 100644
--- a/tests/data/test-annotate/test21-pr19092.so.abi
+++ b/tests/data/test-annotate/test21-pr19092.so.abi
@@ -4395,7 +4395,7 @@
       </data-member>
     </class-decl>
     <!-- struct htab -->
-    <class-decl name='htab' size-in-bits='896' is-struct='yes' visibility='default' filepath='../.././libcpp/../include/hashtab.h' line='100' column='1' id='type-id-208'>
+    <class-decl name='htab' size-in-bits='896' is-struct='yes' visibility='default' filepath='../.././libiberty/../include/hashtab.h' line='100' column='1' id='type-id-208'>
       <data-member access='public' layout-offset-in-bits='0'>
         <!-- htab_hash htab::hash_f -->
         <var-decl name='hash_f' type-id='type-id-183' visibility='default' filepath='../.././gcc/../include/hashtab.h' line='102' column='1'/>
diff --git a/tests/data/test-read-dwarf/test21-pr19092.so.abi b/tests/data/test-read-dwarf/test21-pr19092.so.abi
index c10916fa..90ecd590 100644
--- a/tests/data/test-read-dwarf/test21-pr19092.so.abi
+++ b/tests/data/test-read-dwarf/test21-pr19092.so.abi
@@ -2915,7 +2915,7 @@
         <var-decl name='next' type-id='type-id-207' visibility='default' filepath='../.././gcc/tlink.c' line='199' column='1'/>
       </data-member>
     </class-decl>
-    <class-decl name='htab' size-in-bits='896' is-struct='yes' visibility='default' filepath='../.././libcpp/../include/hashtab.h' line='100' column='1' id='type-id-208'>
+    <class-decl name='htab' size-in-bits='896' is-struct='yes' visibility='default' filepath='../.././libiberty/../include/hashtab.h' line='100' column='1' id='type-id-208'>
       <data-member access='public' layout-offset-in-bits='0'>
         <var-decl name='hash_f' type-id='type-id-183' visibility='default' filepath='../.././gcc/../include/hashtab.h' line='102' column='1'/>
       </data-member>
-- 
2.35.0.rc2



-- 
		Dodji

Re: [PATCH 2/2] Bug 26646 - unexpected declaration-only types

[Dodji Seketeli]

Giuliano Procida <gprocida@google.com> writes:

> This looks good to me.

Thanks!

>
> In the case of the two kernel ABIs, it eliminates one of the two
> differences in declaration-only status. I'll follow up on the other
> difference in the bug when I have a bit more information to share.

Good to know, thank you.

[...]

>>
>>         * src/abg-dwarf-reader.cc (add_or_update_class_type): Do not reuse
>>         the IR for a DIE with the same textual representation as the one
>>         we are seeing now.
>>         * tests/data/test-annotate/test21-pr19092.so.abi: Adjust.
>>         * tests/data/test-read-dwarf/test21-pr19092.so.abi: Likewise.
>>
>> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
>
> Reviewed-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks!

[...]

>
> Cheers!
>

Cheers!

-- 
		Dodji