gdb: Make the builtin "boolean" type an unsigned type

Message ID 20210719163136.23631-1-shahab.vahedi@gmail.com
State New
Headers show
Series
  • gdb: Make the builtin "boolean" type an unsigned type
Related show

Commit Message

Philippe Waroquiers via Gdb-patches July 19, 2021, 4:31 p.m.
From: Shahab Vahedi <shahab@synopsys.com>


When printing the fields of a register that is of a custom struct type,
the "unpack_bits_as_long ()" function is used:

  do_val_print (...)
    cp_print_value_fields (...)
      value_field_bitfield (...)
        unpack_value_bitfield (...)
          unpack_bits_as_long (...)

This function may sign-extend the extracted field while returning it:
------------------ 8< ------------------
  val >>= lsbcount;

  if (...)
    {
      valmask = (((ULONGEST) 1) << bitsize) - 1;
      val &= valmask;
      if (!field_type->is_unsigned ())
	  if (val & (valmask ^ (valmask >> 1)))
	      val |= ~valmask;
    }

  return val;
------------------ >8 ------------------

lsbcount:   Number of lower bits to get rid of.
bitsize:    The bit length of the field to be extracted.
val:        The register value.
field_type: The type of field that is being handled.

While the logic here is correct, there is a problem when it is
handling "field_type"s of "boolean".  Those types are NOT marked
as "unsigned" and therefore they end up being sign extended.
Although this is not a problem for "false" (0), it definitely
causes trouble for "true".

This patch constructs the builtin boolean type as such that it is
marked as an "unsigned" entity.

gdb/ChangeLog:

	PR gdb/28104
	* gdbtypes.c (gdbtypes_post_init): Use
	"arch_boolean_type (..., unsigned=1, ...) to construct
	"boolean".
---
 gdb/gdbtypes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.32.0

Comments

Joel Brobecker July 25, 2021, 4:34 p.m. | #1
Hi Shahab,

On Mon, Jul 19, 2021 at 06:31:36PM +0200, Shahab Vahedi via Gdb-patches wrote:
> From: Shahab Vahedi <shahab@synopsys.com>

> 

> When printing the fields of a register that is of a custom struct type,

> the "unpack_bits_as_long ()" function is used:

> 

>   do_val_print (...)

>     cp_print_value_fields (...)

>       value_field_bitfield (...)

>         unpack_value_bitfield (...)

>           unpack_bits_as_long (...)

> 

> This function may sign-extend the extracted field while returning it:

> ------------------ 8< ------------------

>   val >>= lsbcount;

> 

>   if (...)

>     {

>       valmask = (((ULONGEST) 1) << bitsize) - 1;

>       val &= valmask;

>       if (!field_type->is_unsigned ())

> 	  if (val & (valmask ^ (valmask >> 1)))

> 	      val |= ~valmask;

>     }

> 

>   return val;

> ------------------ >8 ------------------

> 

> lsbcount:   Number of lower bits to get rid of.

> bitsize:    The bit length of the field to be extracted.

> val:        The register value.

> field_type: The type of field that is being handled.

> 

> While the logic here is correct, there is a problem when it is

> handling "field_type"s of "boolean".  Those types are NOT marked

> as "unsigned" and therefore they end up being sign extended.

> Although this is not a problem for "false" (0), it definitely

> causes trouble for "true".

> 

> This patch constructs the builtin boolean type as such that it is

> marked as an "unsigned" entity.

> 

> gdb/ChangeLog:

> 

> 	PR gdb/28104

> 	* gdbtypes.c (gdbtypes_post_init): Use

> 	"arch_boolean_type (..., unsigned=1, ...) to construct

> 	"boolean".


The change makes sense to me. I'm wondering if it's possible to
create a testcase, though. You mention in your explanation
that you found the source of the issue when trying to print
the fields of a register. But it seems to me it should be possible
to construct an example with user-defined struct type where
the fields are one-bit booleans. Wouldn't it?

It would also be useful to specify which problem you had problems
with, and which platform you tested this change on.

> ---

>  gdb/gdbtypes.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

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

> index 1a261719422..9a8ac896fca 100644

> --- a/gdb/gdbtypes.c

> +++ b/gdb/gdbtypes.c

> @@ -6087,7 +6087,7 @@ gdbtypes_post_init (struct gdbarch *gdbarch)

>    builtin_type->builtin_string

>      = arch_type (gdbarch, TYPE_CODE_STRING, TARGET_CHAR_BIT, "string");

>    builtin_type->builtin_bool

> -    = arch_type (gdbarch, TYPE_CODE_BOOL, TARGET_CHAR_BIT, "bool");

> +    = arch_boolean_type (gdbarch, TARGET_CHAR_BIT, 1, "bool");

>  

>    /* The following three are about decimal floating point types, which

>       are 32-bits, 64-bits and 128-bits respectively.  */

> -- 

> 2.32.0

> 


-- 
Joel
Philippe Waroquiers via Gdb-patches July 29, 2021, 12:19 p.m. | #2
Hi Joel,

On 7/25/21 6:34 PM, Joel Brobecker wrote:
> 

> The change makes sense to me. I'm wondering if it's possible to

> create a testcase, though. You mention in your explanation

> that you found the source of the issue when trying to print

> the fields of a register. But it seems to me it should be possible

> to construct an example with user-defined struct type where

> the fields are one-bit booleans. Wouldn't it?

> 

> It would also be useful to specify which problem you had problems

> with, and which platform you tested this change on.


I addressed both of your concerns in the 2nd version of the patch [1].

If it seems alright to you, could you set the target milestone for
the bug report [2] to gdb 11?


[1] [PATCH v2] gdb: Make the builtin "boolean" type an unsigned type
https://sourceware.org/pipermail/gdb-patches/2021-July/181221.html

[2] Incorrect extraction of boolean fields in target description...
https://sourceware.org/bugzilla/show_bug.cgi?id=28104


Cheers,
Shahab

Patch

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 1a261719422..9a8ac896fca 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -6087,7 +6087,7 @@  gdbtypes_post_init (struct gdbarch *gdbarch)
   builtin_type->builtin_string
     = arch_type (gdbarch, TYPE_CODE_STRING, TARGET_CHAR_BIT, "string");
   builtin_type->builtin_bool
-    = arch_type (gdbarch, TYPE_CODE_BOOL, TARGET_CHAR_BIT, "bool");
+    = arch_boolean_type (gdbarch, TARGET_CHAR_BIT, 1, "bool");
 
   /* The following three are about decimal floating point types, which
      are 32-bits, 64-bits and 128-bits respectively.  */