RE: word addressing issues

[Ashutosh Pal <Ashutosh dot Pal at synopsys dot com>]

Hi Simon,

Thanks for your prompt reply! Actually, this processor is also an internal one like in your case, so cannot be up-streamed either. But, as you suggest I will provide the patch and also the test scenarios it fixes, in a few days.
I had one more question related to handling of character size in the word-addressed architectures: Did you change the TARGET_CHAR_BIT in gdb at your side to reflect the char size, which will at least be a single word in size here? In general, is gdb prepared to handle the change in TARGET_CHAR_BIT?

Thanks and Regards,
Ashutosh


-----Original Message-----
From: Simon Marchi [mailto:simon.marchi@ericsson.com] 
Sent: Thursday, June 1, 2017 5:04 PM
To: Ashutosh Pal <Ashutosh.Pal@synopsys.com>; gdb@sourceware.org
Subject: Re: word addressing issues

Hi Ashutosh,

On 2017-06-01 10:15 AM, Ashutosh Pal wrote:
> Hi Experts,
> 
> The following gdbarch hook was introduced in GDB (I guess in v7.11) to be able to tackle architectures with word-addressed (where word = any multiple of 8-bits) memories. 
> 'gdbarch_addressable_memory_unit_size(gdbarch*)'
> 
> I have been making use of this hook for a word addressed processor architecture, in which I return back the word-size (aka addressable-unit size) of the memory. I am using gdb 7.12. Of late, I noticed a few places inside GDB code, where it seems that the word size is not being taken into account. I am listing those code snippets with some comments below:

Indeed, we contributed this gdbarch hook some time ago, exactly for this use case.  I am happy to learn that somebody is using it :).

Just to give you some context on the past work, we (@ Ericsson) work with an architecture with non-8-bit-bytes, for which we have a port of GDB.  This architecture is not externally available so it wouldn't make sense to upstream it.  However, we upstreamed the non-8-bit-bytes changes, hoping that it would be of use to somebody.  We only changed the parts of the code that are actually used in our context, so it's expected that other parts still don't consider memory unit size (e.g. things that deal with unavailable bytes).  It would be great if you could send upstream whatever fixes you end up doing related to this.

> 1. )
> I assume that now (i.e. after v7.11) the memory data transfers functions like read_value_memory, target_xfer_partial, memory_xfer_partial etc. operate at the word (or the unit) level, where the unit-size is obtained from gdbarch_addressable_memory_unit_size hook. So, for example, in case of 'memory_xfer_partial' i.e.
>                         memory_xfer_partial (struct target_ops *ops, 
> ..,  const gdb_byte *writebuf,  ULONGEST memaddr, ULONGEST len, ULONGEST *xfered_len) the arguments 'memaddr' and 'len' will contain the address and the length resp. in terms of units which can be potentially bigger than 8-bits, right? But, looking inside the body of memory_xfer_partial:
> 
> 1262 static enum target_xfer_status
> 1263 memory_xfer_partial (struct target_ops *ops, enum target_object 
> object, ..) ...
> 1284   else
> 1285     {
> 1286       gdb_byte *buf;
> 1287       struct cleanup *old_chain;
> ...
> 1295       len = min (ops->to_get_memory_xfer_limit (ops), len);
> 1296 
> 1297       buf = (gdb_byte *) xmalloc (len);
> 1298       old_chain = make_cleanup (xfree, buf);
> 1299       memcpy (buf, writebuf, len);
> ...
> Here the 'buf' is being allocated only 'len' bytes (instead of 'len' units) and similarly memcpy is copying only 'len' bytes (instead of 'len' units). I think here we should obtain the 'unit_size' from the gdbarch hook and use 'len*unit_size' instead of just 'len'. Do you agree?

Indeed, in our code we have:

  buf = (gdb_byte *) xmalloc (len * unit_size);

This was probably an oversight.

> 2.)
> The offsets for members in structs and classes when saved in the GDB's 'value' struct, should also now be saved in terms of words/units. I noticed a couple of places where the offsets are still being saved as byte offsets. For example, looking inside the 'value_primitive_field' function in value.c file i.e.
>
> 3115 struct value *
> 3116 value_primitive_field (struct value *arg1, LONGEST offset, ..) 
> ...
> 3118 {
> 3163   else if (fieldno < TYPE_N_BASECLASSES (arg_type))
> 3164     {
> ...
> 3177       if (BASETYPE_VIA_VIRTUAL (arg_type, fieldno))
> 3178   boffset = baseclass_offset (arg_type, fieldno,
> 3179             value_contents (arg1),
> 3180             value_embedded_offset (arg1),
> 3181             value_address (arg1),
> 3182             arg1);
> 3183       else
> 3184   boffset = TYPE_FIELD_BITPOS (arg_type, fieldno) / 8;
> 3185
> ....
> 3196    v->embedded_offset = offset + value_embedded_offset (arg1) + boffset;
> 
> The 'boffset' here is still being computed in terms of bytes and not units. Even the 'baseclass_offset' function returns the offset in terms of bytes. I assume the embedded_offset is now (i.e. from v7.11) saved in terms of units and so this looks like incorrect. I see the issue when accessing the "base" part of a derived object.

That's an example of code we didn't touch.  We don't use C++ with this particular architecture, so even if we made the change, we couldn't really test it.  But  your assumption looks correct to me.

> 3.)
> 
> Noticed similar issue as (2) in functions cp_print_value_fields in cp-valprint.c:
> 
> 155 void
> 156 cp_print_value_fields (struct type *type, struct type *real_type,
> 157            const gdb_byte *valaddr, LONGEST offset, ..)
> ...
> 350         else
> 351     {
> 352       struct value_print_options opts = *options;
> 353 
> 354       opts.deref_ref = 0;
> 355       val_print (TYPE_FIELD_TYPE (type, i),
> 356            valaddr,
> 357            offset + TYPE_FIELD_BITPOS (type, i) / 8,
> 358            address,
> 359            stream, recurse + 1, val, &opts,
> 360            current_language);
> 361     }
> 362       }
> 
> Here again the term ' TYPE_FIELD_BITPOS (type, i) / 8' added to 'offset' is in terms of bytes and not units.

Indeed, in our code we have

  offset_target_bytes + TYPE_FIELD_BITPOS (type, i) / TARGET_CHAR_BIT

although ideally it should use the gdbarch hook instead of TARGET_CHAR_BIT.  This was probably done before we introduced the hook and is also an oversight.

> And a similar issue in do_search_struct_field in valops.c:
> 
> 1805 static void
> 1806 do_search_struct_field (const char *name, struct value *arg1, 
> LONGEST offset, ..) ...
> 1835   if (t_field_name
> 1836       && t_field_name[0] == '\0')
> 1837     {
> 1838       struct type *field_type = TYPE_FIELD_TYPE (type, i);
> 1839
> ...
> 1864     if (TYPE_CODE (field_type) == TYPE_CODE_STRUCT
> 1865         || (TYPE_NFIELDS (field_type) > 0
> 1866       && TYPE_FIELD_BITPOS (field_type, 0) == 0))
> 1867       new_offset += TYPE_FIELD_BITPOS (type, i) / 8;

This also looks like it should use unit_size, but we did not fall in this particular trap yet.

> Could you please check and let me know if my above observations are correct and if a bug request needs to be filed?

I think your observations are correct.  You can always file a bug, but even better would be to send some patches to fix them.  If you have fixed them locally and was able to confirm that they resolved some issues, it shouldn't be too long to collect them in a patch.  I'll be happy to review that.

Out of curiosity, is this processor architecture publicly available, and if so, which one is it?

Thanks!

Simon