Bug 28104 - Incorrect extraction of boolean fields in target description "struct" data type
Summary: Incorrect extraction of boolean fields in target description "struct" data type
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: gdb (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 11.1
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-19 14:41 UTC by Shahab
Modified: 2021-08-02 11:20 UTC (History)
1 user (show)

See Also:
Host: x86_64
Target: arc-elf-32
Build: x86_64
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Shahab 2021-07-19 14:41:00 UTC
Affected versions are HEAD, 11, 10, ...

This is a more elaborated bug report of 21184. That report actually mentions two bugs. This report is only about one of those.

============================================================

Assume there is an XML target description as follows:
---------------- [ target description xml ] ----------------
<?xml version="1.0"?>

<!DOCTYPE feature SYSTEM "gdb-target.dtd">
<feature name="org.gnu.gdb.arc.fpu">
  <struct id="struct_type" size="4">
    <field name="version" start="0"  end="7"  type="int" />
    <field name="g"       start="8"  end="8"  type="bool"/>
    <field name="u"       start="9"  end="9"  type="bool"/>
    <field name="b"       start="10" end="10" type="bool"/>
    <field name=""        start="11" end="31"            />
  </struct>
  <reg name="s_reg" bitsize="32" type="struct_type"/>
</feature>
------------------------------------------------------------

This corresponds to an "s_reg" register like below:
---------------- [ s_reg register format ] -----------------
      ,-----------------.---.---.---.---------.
      | dont_care_field | b | u | g | version |
      `-----------------^---^---^---^---------'
bit:   31     ...     11  10  9   8  7  ...  0
------------------------------------------------------------
 
When the reported value of "s_reg" is 0x1337:
------- [ s_reg instance with 0x1337 as its value ] --------
  0  x   1    3    3    7
       0001 0011 0011 0111
             bug  version
------------------------------------------------------------ 

GDB prints incorrect information regarding the boolean fields
that are set:
--------------------------- [gdb] --------------------------
(gdb) info reg $s_reg
s_reg          {
  version = 0x37,
  g = 0xff,
  u = 0xff,
  b = 0x0
} {
  version = 55,
  g = -1,
  u = -1,
  b = false
}
------------------------------------------------------------ 

As you can see, the "version" field is interpreted correctly,
but the "g" and "u" fields are represented by "0xff" or "-1"
instead of "1" or "true". The "b" field is fine though.
Comment 1 Shahab 2021-07-19 14:41:44 UTC
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".
Comment 2 Sourceware Commits 2021-08-02 11:07:32 UTC
The gdb-11-branch branch has been updated by Shahab Vahedi <shahab@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=e4c1aea498ff84bcb3086509d125ac8249f35db2

commit e4c1aea498ff84bcb3086509d125ac8249f35db2
Author: Shahab Vahedi <shahab@synopsys.com>
Date:   Mon Jul 19 16:13:47 2021 +0200

    gdb: Make the builtin "boolean" type an unsigned type
    
    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:
    
        val >>= lsbcount;
    
        if (...)
          {
            valmask = (((ULONGEST) 1) << bitsize) - 1;
            val &= valmask;
            if (!field_type->is_unsigned ())
              if (val & (valmask ^ (valmask >> 1)))
                  val |= ~valmask;
          }
    
        return val;
    
    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.
    
    The issue tackled here was first encountered for arc-elf32 target
    running on an x86_64 machine.  The unit-test introduced in this change
    has passed for all the targets (--enable-targets=all) running on the
    same x86_64 host.
    
    gdb/ChangeLog:
    
            PR gdb/28104
            * gdbtypes.c (gdbtypes_post_init): Use
            "arch_boolean_type (..., unsigned=1, ...) to construct
            "boolean".
            cp-valprint.c (test_print_flags): New.
            (_initialize_cp_valprint): Run the "test_print_flags" unit-test.
Comment 3 Shahab 2021-08-02 11:20:39 UTC
This PR was solved on master with this commit:

| commit   91254b918f1b35359d44b567a15013c42a931460
| Author:  Shahab Vahedi <shahab@synopsys.com>
| Date:    Mon Jul 19 16:13:47 2021 +0200
| Subject: gdb: Make the builtin "boolean" type an unsigned type

The patch above was also pushed to gdb-11-branch as:

| commit   e4c1aea498ff84bcb3086509d125ac8249f35db2
| Author:  Shahab Vahedi <shahab@synopsys.com>
| Date:    Mon Jul 19 16:13:47 2021 +0200
| Subject: gdb: Make the builtin "boolean" type an unsigned type