Re: [4/9] Unify init_type and arch_type interface and helpers

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 17-01-17 09:16 AM, Ulrich Weigand wrote:
> Simon Marchi wrote:
>> On 16-08-25 11:06 AM, Ulrich Weigand wrote:
>>> +static int
>>> +verify_floatformat (int bit, const struct floatformat **floatformats)
>>> +{
>>> +  if (bit == -1)
>>> +    {
>>> +      gdb_assert (floatformats != NULL);
>>> +      gdb_assert (floatformats[0] != NULL && floatformats[1] != NULL);
>>> +      bit = floatformats[0]->totalsize;
>>> +    }
>>> +  gdb_assert (bit >= 0);
>>> +
>>> +  if (floatformats != NULL)
>>> +    {
>>> +      size_t len = bit / TARGET_CHAR_BIT;
>>> +
>>> +      gdb_assert (len >= floatformat_totalsize_bytes (floatformats[0]));
>>> +      gdb_assert (len >= floatformat_totalsize_bytes (floatformats[1]));
>>
>> Hi Ulrich,
>>
>> I am in the process of syncing with master our port of GDB for which
>> HOST_CHAR_BIT (8) != TARGET_CHAR_BIT (16).  I believe that the piece of code
>> compares target bytes and host bytes (which is not good), whereas the old code
>> was comparing host bytes with host bytes.
> 
> Everything involved here should be target bytes (and target types).  There
> should not have been any change from the old code either:
> 
>> Here's the old one:
>>
>>> -  if (floatformats != NULL)
>>> -    {
>>> -      size_t len = TYPE_LENGTH (t);
>>> -
>>> -      gdb_assert (len >= floatformat_totalsize_bytes (floatformats[0]));
>>> -      gdb_assert (len >= floatformat_totalsize_bytes (floatformats[1]));
>>> -    }
>>> -
>>>    return t;
>>>  }
> 
> since the type "t" was just created via
>    t = arch_type (gdbarch, TYPE_CODE_FLT, bit / TARGET_CHAR_BIT, name);
> 
> so TYPE_LENGTH (t) should always equal bit / TARGET_CHAR_BIT.

The assumption that TYPE_LENGTH returns target bytes is not in line with the
documentation we added some time ago to type::length:

     Since this field is expressed in host bytes, its value is appropriate
     to pass to memcpy and such (it is assumed that GDB itself always runs
     on an 8-bits addressable architecture).  However, when using it for
     target address arithmetic (e.g. adding it to a target address), the
     type_length_units function should be used in order to get the length
     expressed in target addressable memory units.  */

But I see that even before your patch, the dwarf2read code directly assigned the value
of DW_AT_byte_size to type::length.  Since DW_AT_byte_size is assumed to be in number of
target bytes, it means the length field would be in target bytes, despite what the doc
said.

In our local gdb port, for an arch with 16-bit target bytes, we have changed dwarf2read to
set the length as host bytes, as the field documentation says.

Therefore, in the open source code, the length field (and therefore what TYPE_LENGTH returns)
actually contains target bytes, whereas in our code it's host bytes.  I think that's why your
patch is right in the context of open source, but not in our context, hence the confusion.
This should be fixed, see the proposition below.

>> The new code triggers the assert, because len is 2 target bytes and
>> floatformat_totalsize_bytes is 4 host bytes.  If it's indeed an error, I can
>> make a patch for it, but I wanted to check with you first.
> 
> Why would floatformat_totalsize_bytes be host bytes?  I think this is
> *intended* to be target bytes, as the floatformat in general is intended
> to describe the *target* floating-point format.  I do see that doublest.c
> does indeed make the assumption that a character has 8 bits, but that is
> something that will need to be fixed if you want to support targets with
> a different byte size; I don't see how this is related to this particular
> patch of mine ...

I am not sure that the intent of floatformat_totalsize_bytes was to return target bytes.
It's mainly used for memcpy's, which accepts host bytes.  I see that function as returning
the required storage space on the host for that type, so it makes sense that it returns
host bytes.  The comment in doublest.h looks quite clear in that regard:

  /* Return the floatformat's total size in host bytes.  */

  extern size_t floatformat_totalsize_bytes (const struct floatformat *fmt);

---

To fix the type::length confusion and to reduce the number of things that depend on host/target
bytes when they don't need to, I suggest we change the type::length field to be in bits.  In the
functions you added in this patchset, the type lengths are expressed in bits, and I like that.
Less chance of mix ups.  So the whole dwarf2read -> type::length path would be in bits.

Then, we need to make TYPE_LENGTH really return host bytes, and clarify its documentation.

WDYT?

Thanks,

Simon