CTF: incorrect underlying type setting for enumeration types

Message ID 1634081977-19508-1-git-send-email-weimin.pan@oracle.com
State New
Headers show
Series
  • CTF: incorrect underlying type setting for enumeration types
Related show

Commit Message

Lancelot SIX via Gdb-patches Oct. 12, 2021, 11:39 p.m.
A bug was reported - incorrect underlying type setting for an
enumeration type, which was caused by a copy and paste error.
This patch fixes the problem by setting it to signed int of
size ctf_type_size bits, similar to what libctf and libabigail 
do. Also add error checking on the ctf_func_type_info call.
---
 gdb/ctfread.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
1.8.3.1

Comments

Lancelot SIX via Gdb-patches Oct. 15, 2021, 10:41 a.m. | #1
Hi Weimin.

> A bug was reported - incorrect underlying type setting for an

> enumeration type, which was caused by a copy and paste error.

> This patch fixes the problem by setting it to signed int of

> size ctf_type_size bits, similar to what libctf and libabigail 

> do. Also add error checking on the ctf_func_type_info call.


FWIW this looks good to me.

> ---

>  gdb/ctfread.c | 10 ++++++----

>  1 file changed, 6 insertions(+), 4 deletions(-)

>

> diff --git a/gdb/ctfread.c b/gdb/ctfread.c

> index 712683b..80e20c9 100644

> --- a/gdb/ctfread.c

> +++ b/gdb/ctfread.c

> @@ -691,7 +691,8 @@ struct ctf_tid_and_type

>    type = alloc_type (of);

>  

>    type->set_code (TYPE_CODE_FUNC);

> -  ctf_func_type_info (fp, tid, &cfi);

> +  if (ctf_func_type_info (fp, tid, &cfi) < 0)

> +    error(_("Error getting function type info"));

>    rettype = fetch_tid_type (ccp, cfi.ctc_return);

>    TYPE_TARGET_TYPE (type) = rettype;

>    set_type_align (type, ctf_type_align (fp, tid));

> @@ -734,7 +735,6 @@ struct ctf_tid_and_type

>    struct objfile *of = ccp->of;

>    ctf_dict_t *fp = ccp->fp;

>    struct type *type, *target_type;

> -  ctf_funcinfo_t fi;

>  

>    type = alloc_type (of);

>  

> @@ -744,8 +744,10 @@ struct ctf_tid_and_type

>  

>    type->set_code (TYPE_CODE_ENUM);

>    TYPE_LENGTH (type) = ctf_type_size (fp, tid);

> -  ctf_func_type_info (fp, tid, &fi);

> -  target_type = get_tid_type (of, fi.ctc_return);

> +  /* Create a signed integer type of size ctf_type_size bits for

> +     the underlying type.  */

> +  target_type = arch_integer_type (of->arch (), TYPE_LENGTH (type) * 8,

> +				   0, "int");

>    TYPE_TARGET_TYPE (type) = target_type;

>    set_type_align (type, ctf_type_align (fp, tid));
Simon Marchi Oct. 15, 2021, 3:41 p.m. | #2
On 2021-10-12 7:39 p.m., Weimin Pan via Gdb-patches wrote:
> A bug was reported - incorrect underlying type setting for an

> enumeration type, which was caused by a copy and paste error.

> This patch fixes the problem by setting it to signed int of

> size ctf_type_size bits, similar to what libctf and libabigail

> do. Also add error checking on the ctf_func_type_info call.

> ---

>  gdb/ctfread.c | 10 ++++++----

>  1 file changed, 6 insertions(+), 4 deletions(-)

>

> diff --git a/gdb/ctfread.c b/gdb/ctfread.c

> index 712683b..80e20c9 100644

> --- a/gdb/ctfread.c

> +++ b/gdb/ctfread.c

> @@ -691,7 +691,8 @@ struct ctf_tid_and_type

>    type = alloc_type (of);

>

>    type->set_code (TYPE_CODE_FUNC);

> -  ctf_func_type_info (fp, tid, &cfi);

> +  if (ctf_func_type_info (fp, tid, &cfi) < 0)

> +    error(_("Error getting function type info"));


Missing space:

  error (_("..."));

Is there some information that you could include in the error message to
help any user who would see this message find what's wrong?  Maybe the
name of the function we are currently reading, an offset in the section,
etc.

>    rettype = fetch_tid_type (ccp, cfi.ctc_return);

>    TYPE_TARGET_TYPE (type) = rettype;

>    set_type_align (type, ctf_type_align (fp, tid));

> @@ -734,7 +735,6 @@ struct ctf_tid_and_type

>    struct objfile *of = ccp->of;

>    ctf_dict_t *fp = ccp->fp;

>    struct type *type, *target_type;

> -  ctf_funcinfo_t fi;

>

>    type = alloc_type (of);

>

> @@ -744,8 +744,10 @@ struct ctf_tid_and_type

>

>    type->set_code (TYPE_CODE_ENUM);

>    TYPE_LENGTH (type) = ctf_type_size (fp, tid);

> -  ctf_func_type_info (fp, tid, &fi);

> -  target_type = get_tid_type (of, fi.ctc_return);

> +  /* Create a signed integer type of size ctf_type_size bits for

> +     the underlying type.  */

> +  target_type = arch_integer_type (of->arch (), TYPE_LENGTH (type) * 8,

> +				   0, "int");


Instead of creating a new arch-owned int type for each enumeration, I
think it would be more efficient to pick one from builtin_type(arch)
(the right one based on the size).  If we really need to create new
types for this, then they should be objfile-owned, so that when the
objfile is discarded, these types are discarded too.

Simon
Lancelot SIX via Gdb-patches Oct. 15, 2021, 6:15 p.m. | #3
Thanks for your comments, Simon.

On 10/15/2021 8:41 AM, Simon Marchi wrote:
> On 2021-10-12 7:39 p.m., Weimin Pan via Gdb-patches wrote:

>> A bug was reported - incorrect underlying type setting for an

>> enumeration type, which was caused by a copy and paste error.

>> This patch fixes the problem by setting it to signed int of

>> size ctf_type_size bits, similar to what libctf and libabigail

>> do. Also add error checking on the ctf_func_type_info call.

>> ---

>>   gdb/ctfread.c | 10 ++++++----

>>   1 file changed, 6 insertions(+), 4 deletions(-)

>>

>> diff --git a/gdb/ctfread.c b/gdb/ctfread.c

>> index 712683b..80e20c9 100644

>> --- a/gdb/ctfread.c

>> +++ b/gdb/ctfread.c

>> @@ -691,7 +691,8 @@ struct ctf_tid_and_type

>>     type = alloc_type (of);

>>

>>     type->set_code (TYPE_CODE_FUNC);

>> -  ctf_func_type_info (fp, tid, &cfi);

>> +  if (ctf_func_type_info (fp, tid, &cfi) < 0)

>> +    error(_("Error getting function type info"));

> Missing space:

>

>    error (_("..."));

>

> Is there some information that you could include in the error message to

> help any user who would see this message find what's wrong?  Maybe the

> name of the function we are currently reading, an offset in the section,

> etc.


Now add type name to the error message:

    if (ctf_func_type_info (fp, tid, &cfi) < 0)
-    error(_("Error getting function type info"));
+    {
+      const char *fname = ctf_type_name_raw (fp, tid);
+      error (_("Error getting function type info: %s"),
+                  fname == nullptr ? "noname" : fname);
+    }

>>     rettype = fetch_tid_type (ccp, cfi.ctc_return);

>>     TYPE_TARGET_TYPE (type) = rettype;

>>     set_type_align (type, ctf_type_align (fp, tid));

>> @@ -734,7 +735,6 @@ struct ctf_tid_and_type

>>     struct objfile *of = ccp->of;

>>     ctf_dict_t *fp = ccp->fp;

>>     struct type *type, *target_type;

>> -  ctf_funcinfo_t fi;

>>

>>     type = alloc_type (of);

>>

>> @@ -744,8 +744,10 @@ struct ctf_tid_and_type

>>

>>     type->set_code (TYPE_CODE_ENUM);

>>     TYPE_LENGTH (type) = ctf_type_size (fp, tid);

>> -  ctf_func_type_info (fp, tid, &fi);

>> -  target_type = get_tid_type (of, fi.ctc_return);

>> +  /* Create a signed integer type of size ctf_type_size bits for

>> +     the underlying type.  */

>> +  target_type = arch_integer_type (of->arch (), TYPE_LENGTH (type) * 8,

>> +				   0, "int");

> Instead of creating a new arch-owned int type for each enumeration, I

> think it would be more efficient to pick one from builtin_type(arch)

> (the right one based on the size).  If we really need to create new

> types for this, then they should be objfile-owned, so that when the

> objfile is discarded, these types are discarded too.


Picking one from the builtin_types does seem more efficient:

-  /* Create a signed integer type of size ctf_type_size bits for
-     the underlying type.  */
-  target_type = arch_integer_type (of->arch (), TYPE_LENGTH (type) * 8,
-                                  0, "int");
+  /* Set the underlying type based on its ctf_type_size bits.  */
+  int bits = TYPE_LENGTH (type) * TARGET_CHAR_BIT;
+  if (bits == TARGET_CHAR_BIT)
+    target_type = objfile_type (of)->builtin_signed_char;
+  else if (bits == gdbarch_short_bit (of->arch ()))
+    target_type = objfile_type (of)->builtin_short;
+  else
+    {
+      if (bits != gdbarch_int_bit (of->arch ()))
+        complaint (_("read_enum_type unexpected bit size: '%d', int 
assumed"),
+                            bits);
+      target_type = objfile_type (of)->builtin_int;
+    }

>

> Simon
Simon Marchi Oct. 15, 2021, 7:51 p.m. | #4
On 2021-10-15 2:15 p.m., Wei-min Pan wrote:
> Thanks for your comments, Simon.

>

> On 10/15/2021 8:41 AM, Simon Marchi wrote:

>> On 2021-10-12 7:39 p.m., Weimin Pan via Gdb-patches wrote:

>>> A bug was reported - incorrect underlying type setting for an

>>> enumeration type, which was caused by a copy and paste error.

>>> This patch fixes the problem by setting it to signed int of

>>> size ctf_type_size bits, similar to what libctf and libabigail

>>> do. Also add error checking on the ctf_func_type_info call.

>>> ---

>>>   gdb/ctfread.c | 10 ++++++----

>>>   1 file changed, 6 insertions(+), 4 deletions(-)

>>>

>>> diff --git a/gdb/ctfread.c b/gdb/ctfread.c

>>> index 712683b..80e20c9 100644

>>> --- a/gdb/ctfread.c

>>> +++ b/gdb/ctfread.c

>>> @@ -691,7 +691,8 @@ struct ctf_tid_and_type

>>>     type = alloc_type (of);

>>>

>>>     type->set_code (TYPE_CODE_FUNC);

>>> -  ctf_func_type_info (fp, tid, &cfi);

>>> +  if (ctf_func_type_info (fp, tid, &cfi) < 0)

>>> +    error(_("Error getting function type info"));

>> Missing space:

>>

>>    error (_("..."));

>>

>> Is there some information that you could include in the error message to

>> help any user who would see this message find what's wrong?  Maybe the

>> name of the function we are currently reading, an offset in the section,

>> etc.

>

> Now add type name to the error message:

>

>    if (ctf_func_type_info (fp, tid, &cfi) < 0)

> -    error(_("Error getting function type info"));

> +    {

> +      const char *fname = ctf_type_name_raw (fp, tid);

> +      error (_("Error getting function type info: %s"),

> +                  fname == nullptr ? "noname" : fname);

> +    }


I don't know the libctf API, so I'll trust you that this does the right
thing.

>

>>>     rettype = fetch_tid_type (ccp, cfi.ctc_return);

>>>     TYPE_TARGET_TYPE (type) = rettype;

>>>     set_type_align (type, ctf_type_align (fp, tid));

>>> @@ -734,7 +735,6 @@ struct ctf_tid_and_type

>>>     struct objfile *of = ccp->of;

>>>     ctf_dict_t *fp = ccp->fp;

>>>     struct type *type, *target_type;

>>> -  ctf_funcinfo_t fi;

>>>

>>>     type = alloc_type (of);

>>>

>>> @@ -744,8 +744,10 @@ struct ctf_tid_and_type

>>>

>>>     type->set_code (TYPE_CODE_ENUM);

>>>     TYPE_LENGTH (type) = ctf_type_size (fp, tid);

>>> -  ctf_func_type_info (fp, tid, &fi);

>>> -  target_type = get_tid_type (of, fi.ctc_return);

>>> +  /* Create a signed integer type of size ctf_type_size bits for

>>> +     the underlying type.  */

>>> +  target_type = arch_integer_type (of->arch (), TYPE_LENGTH (type) * 8,

>>> +                   0, "int");

>> Instead of creating a new arch-owned int type for each enumeration, I

>> think it would be more efficient to pick one from builtin_type(arch)

>> (the right one based on the size).  If we really need to create new

>> types for this, then they should be objfile-owned, so that when the

>> objfile is discarded, these types are discarded too.

>

> Picking one from the builtin_types does seem more efficient:

>


I wrote some comments below, but what I wrote in the end probably makes
those comments moot.

> -  /* Create a signed integer type of size ctf_type_size bits for

> -     the underlying type.  */

> -  target_type = arch_integer_type (of->arch (), TYPE_LENGTH (type) * 8,

> -                                  0, "int");

> +  /* Set the underlying type based on its ctf_type_size bits.  */

> +  int bits = TYPE_LENGTH (type) * TARGET_CHAR_BIT;

> +  if (bits == TARGET_CHAR_BIT)

> +    target_type = objfile_type (of)->builtin_signed_char;


TARGET_CHAR_BIT should be avoided: it's a compile time constant that is
correct for the main target architecture of your build.  But you could
build GDB for multiple architectures that require different values, and
so TARGET_CHAR_BIT would be wrong for the other architectures.  Use
gdbarch_addressable_memory_unit_size.  That will return 1 for all
typical architectures with 8-bit bytes.  It will return 2 for an
architecture with 16-bit bytes.  So:

    int unit_size = gdbarch_addressable_memory_unit_size (arch);
    int bits = TYPE_LENGTH (type) * 8 * unit_size;
    if (bits == unit_size)
      target_type = objfile_type (of)->builtin_signed_char;
     else if ...

> +  else if (bits == gdbarch_short_bit (of->arch ()))

> +    target_type = objfile_type (of)->builtin_short;

> +  else

> +    {

> +      if (bits != gdbarch_int_bit (of->arch ()))

> +        complaint (_("read_enum_type unexpected bit size: '%d', int assumed"),

> +                            bits);

> +      target_type = objfile_type (of)->builtin_int;

> +    }


Should you cover 64 bits as well? If I make an enumeration like this,
it will be 64 bits:

    enum foo {
      bar = 0xffffffffffff,
    };

Now that I think of it, I just remembered the
"dwarf2_per_objfile::int_type" function in dwarf2/read.c.  There's
nothing inherently DWARF-specific about it, so I think that function
should be made into common util and used by both the DWARF and CTF
readers.  Note that it uses the objfile's builtin_types, rather than the
architecture's builtin_types, which makes sense (I had forgotten about
objfile's builtin_types).

Simon

Patch

diff --git a/gdb/ctfread.c b/gdb/ctfread.c
index 712683b..80e20c9 100644
--- a/gdb/ctfread.c
+++ b/gdb/ctfread.c
@@ -691,7 +691,8 @@  struct ctf_tid_and_type
   type = alloc_type (of);
 
   type->set_code (TYPE_CODE_FUNC);
-  ctf_func_type_info (fp, tid, &cfi);
+  if (ctf_func_type_info (fp, tid, &cfi) < 0)
+    error(_("Error getting function type info"));
   rettype = fetch_tid_type (ccp, cfi.ctc_return);
   TYPE_TARGET_TYPE (type) = rettype;
   set_type_align (type, ctf_type_align (fp, tid));
@@ -734,7 +735,6 @@  struct ctf_tid_and_type
   struct objfile *of = ccp->of;
   ctf_dict_t *fp = ccp->fp;
   struct type *type, *target_type;
-  ctf_funcinfo_t fi;
 
   type = alloc_type (of);
 
@@ -744,8 +744,10 @@  struct ctf_tid_and_type
 
   type->set_code (TYPE_CODE_ENUM);
   TYPE_LENGTH (type) = ctf_type_size (fp, tid);
-  ctf_func_type_info (fp, tid, &fi);
-  target_type = get_tid_type (of, fi.ctc_return);
+  /* Create a signed integer type of size ctf_type_size bits for
+     the underlying type.  */
+  target_type = arch_integer_type (of->arch (), TYPE_LENGTH (type) * 8,
+				   0, "int");
   TYPE_TARGET_TYPE (type) = target_type;
   set_type_align (type, ctf_type_align (fp, tid));