Re: [PATCH 2/5] ctf-front-end: Fix size and name for underlying types

[Dodji Seketeli <dodji@seketeli.org>]

Hello Guillermo,

"Guillermo E. Martinez via Libabigail" <libabigail@sourceware.org> a
écrit:

> It fixed an incorrect representation in size and name for underlying
> enum and struct types when it has a bitfield members:
>
>     struct foo
>     {
>       unsigned bar : 2;
>       unsigned baz : 1;
>     };
>
>   <type-decl name='' is-anonymous='yes' size-in-bits='2' id='type-id-1'/>
>
> So, `name' empty property, no showing the right enumerator's type and size
> in bits:
>
>   <type-decl name='unsigned char' size-in-bits='8' id='type-id-4'/>

Hmmh, I think here, the name should be "unsigned int", because "unsigned
bar" implicitly means 'unsigned int bar".  And the size-in-bits should
be set to 32.

I have thus changed the introductory paragraphs of this commit log to
the following:

    This patch fixes an incorrect representation in size and name of the
    underlying type of enums as well as underlying types of bitfield data
    members types.

    For instance, consider this struct.

        struct foo
        {
          unsigned bar : 2;
          unsigned baz : 1;
        };

    The data members bar and baz have an underlying type that is "unsigned
    int".  Yet, the CTF front-end represents the underlying type of these
    data members as:

      <type-decl name='' is-anonymous='yes' size-in-bits='2' id='type-id-1'/>

    The name property is empty, and it should be "unsigned int".

    The size in bit is '2', but it should be the size of the underlying
    "unsigned int", in bits, which is 32.

    In other words, the underlying type of bar and baz should be:

      <type-decl name='unsigned int' size-in-bits='32' id='type-id-4'/>

    Note that today, libabigail doesn't represent the bitfield properties
    of the data member.  Those bitfield properties are properties of the
    data member, not of their type.  This is a known "Not Yet Implemented"
    feature request that has been filed upstream at
    https://sourceware.org/bugzilla/show_bug.cgi?id=27334.

    Similarly, the underlying type of enums is not properly represented by
    the CTF front-end.

    Fixed thus.

It's a little bit more verbose, but hopefully, that should give a little
bit more context for future code maintenance.

>
> 	* src/abg-ctf-reader.cc (process_ctf_{base_type,enum_type}):
> 	Look at ctf refence type to build the underlying type if present.
> 	* tests/data/Makefile.am: New test cases.
> 	* tests/data/test-read-ctf/PR27700/test-PR27700.abi: New test input.
> 	* tests/data/test-read-ctf/test-bitfield-enum.abi: Likewise.
> 	* tests/data/test-read-ctf/test-bitfield-enum.c: Likewise.
> 	* tests/data/test-read-ctf/test-bitfield-enum.o: Likewise.
> 	* tests/data/test-read-ctf/test-bitfield.abi: Likewise.
> 	* tests/data/test-read-ctf/test-bitfield.c: Likewise.
> 	* tests/data/test-read-ctf/test-bitfield.o: Likewise.
> 	* tests/data/test-read-ctf/test-enum-many.o.hash.abi: Adjust.
> 	* tests/data/test-read-ctf/test-enum-symbol.o.hash.abi: Likewise.
> 	* tests/data/test-read-ctf/test-enum.o.abi: Likewise:
> 	* tests/data/test-read-ctf/test0.abi: Likewise.
> 	* tests/data/test-read-ctf/test0.hash.abi: Likewise.
> 	* tests/data/test-read-ctf/test1.so.abi: Likewise.
> 	* tests/data/test-read-ctf/test1.so.hash.abi: Likewise.
> 	* tests/data/test-read-ctf/test5.o.abi: Likewise.
> 	* tests/test-read-ctf.cc: Update test suite.
>
> Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>

Applied to the master branch with the change above.  Please find below
the actual patch that got applied.

Many thanks!

[...]

From 217f579bf788a11643c0066a7dc8aa76faa5a05d Mon Sep 17 00:00:00 2001
From: "Guillermo E. Martinez" <guillermo.e.martinez@oracle.com>
Date: Wed, 16 Nov 2022 21:43:02 -0600
Subject: [PATCH] ctf-reader: Fix size and name for underlying types

This patch fixes an incorrect representation in size and name of the
underlying type of enums as well as underlying types of bitfield data
members types.

For instance, consider this struct.

    struct foo
    {
      unsigned bar : 2;
      unsigned baz : 1;
    };

The data members bar and baz have an underlying type that is "unsigned
int".  Yet, the CTF front-end represents the underlying type of these
data members as:

  <type-decl name='' is-anonymous='yes' size-in-bits='2' id='type-id-1'/>

The name property is empty, and it should be "unsigned int".

The size in bit is '2', but it should be the size of the underlying
"unsigned int", in bits, which is 32.

In other words, the underlying type of bar and baz should be:

  <type-decl name='unsigned int' size-in-bits='32' id='type-id-4'/>

Note that today, libabigail doesn't represent the bitfield properties
of the data member.  Those bitfield properties are properties of the
data member, not of their type.  This is a known "Not Yet Implemented"
feature request that has been filed upstream at
https://sourceware.org/bugzilla/show_bug.cgi?id=27334.

Similarly, the underlying type of enums is not properly represented by
the CTF front-end.

Fixed thus.

	* src/abg-ctf-reader.cc (process_ctf_{base_type,enum_type}):
	Look at ctf refence type to build the underlying type if present.
	* tests/data/Makefile.am: New test cases.
	* tests/data/test-read-ctf/PR27700/test-PR27700.abi: New test input.
	* tests/data/test-read-ctf/test-bitfield-enum.abi: Likewise.
	* tests/data/test-read-ctf/test-bitfield-enum.c: Likewise.
	* tests/data/test-read-ctf/test-bitfield-enum.o: Likewise.
	* tests/data/test-read-ctf/test-bitfield.abi: Likewise.
	* tests/data/test-read-ctf/test-bitfield.c: Likewise.
	* tests/data/test-read-ctf/test-bitfield.o: Likewise.
	* tests/data/test-read-ctf/test-enum-many.o.hash.abi: Adjust.
	* tests/data/test-read-ctf/test-enum-symbol.o.hash.abi: Likewise.
	* tests/data/test-read-ctf/test-enum.o.abi: Likewise:
	* tests/data/test-read-ctf/test0.abi: Likewise.
	* tests/data/test-read-ctf/test0.hash.abi: Likewise.
	* tests/data/test-read-ctf/test1.so.abi: Likewise.
	* tests/data/test-read-ctf/test1.so.hash.abi: Likewise.
	* tests/data/test-read-ctf/test5.o.abi: Likewise.
	* tests/test-read-ctf.cc: Update test suite.

Signed-off-by: Guillermo E. Martinez <guillermo.e.martinez@oracle.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-ctf-reader.cc | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
index cbc5cbca..d000556b 100644
--- a/src/abg-ctf-reader.cc
+++ b/src/abg-ctf-reader.cc
@@ -783,15 +783,17 @@ process_ctf_base_type(reader *rdr,
   translation_unit_sptr tunit = rdr->cur_transl_unit();
   type_decl_sptr result;
 
-  const char *type_name = ctf_type_name_raw(ctf_dictionary, ctf_type);
+  ctf_id_t ctf_ref = ctf_type_reference(ctf_dictionary, ctf_type);
+  const char *type_name = ctf_type_name_raw(ctf_dictionary,
+                                            (ctf_ref != CTF_ERR) ? ctf_ref : ctf_type);
 
   /* Get the type encoding and extract some useful properties of
      the type from it.  In case of any error, just ignore the
      type.  */
   ctf_encoding_t type_encoding;
   if (ctf_type_encoding(ctf_dictionary,
-                         ctf_type,
-                         &type_encoding))
+                        (ctf_ref != CTF_ERR) ? ctf_ref : ctf_type,
+                        &type_encoding))
     return result;
 
   /* Create the IR type corresponding to the CTF type.  */
@@ -1357,7 +1359,10 @@ process_ctf_enum_type(reader *rdr,
 {
   translation_unit_sptr tunit = rdr->cur_transl_unit();
   enum_type_decl_sptr result;
-  std::string enum_name = ctf_type_name_raw(ctf_dictionary, ctf_type);
+  ctf_id_t ctf_ref = ctf_type_reference(ctf_dictionary, ctf_type);
+  std::string enum_name = ctf_type_name_raw(ctf_dictionary,
+                                            (ctf_ref != CTF_ERR)
+                                              ? ctf_ref : ctf_type);
 
   if (!enum_name.empty())
     if (corpus_sptr corp = rdr->should_reuse_type_from_corpus_group())
@@ -1367,18 +1372,24 @@ process_ctf_enum_type(reader *rdr,
   /* Build a signed integral type for the type of the enumerators, aka
      the underlying type.  The size of the enumerators in bytes is
      specified in the CTF enumeration type.  */
-  size_t utype_size_in_bits = ctf_type_size(ctf_dictionary, ctf_type) * 8;
-  type_decl_sptr utype;
+  size_t utype_size_in_bits = ctf_type_size(ctf_dictionary,
+                                            (ctf_ref != CTF_ERR)
+                                              ? ctf_ref : ctf_type) * 8;
+  string underlying_type_name =
+        build_internal_underlying_enum_type_name(enum_name, true,
+                                                 utype_size_in_bits);
 
+  type_decl_sptr utype;
   utype.reset(new type_decl(rdr->env(),
-                              "",
-                              utype_size_in_bits,
-                              utype_size_in_bits,
-                              location()));
+                            underlying_type_name,
+                            utype_size_in_bits,
+                            utype_size_in_bits,
+                            location()));
   utype->set_is_anonymous(true);
   utype->set_is_artificial(true);
   if (!utype)
     return result;
+
   add_decl_to_scope(utype, tunit->get_global_scope());
   canonicalize(utype);
 
-- 
2.38.1


-- 
		Dodji