Re: [PATCH v2] Expand bitpos and type.length to LONGEST and ULONGEST

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

Hi Jan,

I'm back to working on this. A couple of clarifications on the review:

> >  static void
> > -copy_bitwise (gdb_byte *dest, unsigned int dest_offset_bits,
> > -	      const gdb_byte *source, unsigned int
> > source_offset_bits,
> > -	      unsigned int bit_count,
> > +copy_bitwise (gdb_byte *dest, ULONGEST dest_offset_bits,
> > +	      const gdb_byte *source, ULONGEST source_offset,
> > +	      ULONGEST bit_count,
> >  	      int bits_big_endian)
> >  {
> > -  unsigned int dest_avail;
> > +  unsigned int dest_avail, source_offset_bits;
> >    int datum;
> >  
> >    /* Reduce everything to byte-size pieces.  */
> >    dest += dest_offset_bits / 8;
> >    dest_offset_bits %= 8;
> > -  source += source_offset_bits / 8;
> > -  source_offset_bits %= 8;
> > +  source += source_offset / 8;
> > +  source_offset_bits = source_offset % 8;
> 
> I do not fully understand this whole change but it looks unrelated to
> this patch to me.

I had to split up the source_offset and source_offset bits because
source_offset can be LONGEST but the bits would always be less than 8.
I did this because otherwise I would have had to change signatures of
functions that use source_offset_bits even when it is not really needed
(extract_bits and extract_bits_primitive).

> > size of this argument and rounding to an int boundary.  */
> > diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
> > index 91125e1..a3c15d0 100644
> > --- a/gdb/symfile-mem.c
> > +++ b/gdb/symfile-mem.c
> > @@ -55,6 +55,11 @@
> >  #include "auxv.h"
> >  #include "elf/common.h"
> >  
> > +static int symfile_target_read_memory (CORE_ADDR memaddr, gdb_byte
> > *myaddr,
> > +				       int len)
> > +{
> > +	return target_read_memory (memaddr, myaddr, len);
> > +}
> 
> Formatting should be (see also tab -> 2 spaces):
> 
> static int
> symfile_target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int
> len) {
>   return target_read_memory (memaddr, myaddr, len);
> }
> 
> But I proposed to rather target_read_memory use size_t and we should
> then proposed to bfd/ that it also uses size_t for
> bfd_elf_bfd_from_remote_memory.

Perhaps it will be helpful if I keep this change (and consequently the
rest of the read/write_memory function changes) as a separate patch?
AFAICT the only place where not having these changes as part of the
original patch will potentially cause problems is in
do_search_struct_field. That way I'll also do the size_t changes right
away instead of changing and then reverting the change.

> >  void
> >  modify_field (struct type *type, gdb_byte *addr,
> > -	      LONGEST fieldval, int bitpos, int bitsize)
> > +	      LONGEST fieldval, LONGEST bitpos, unsigned int
> > bitsize)
> 
> WHy to make bitsize unsigned here?

It's a minor nit to make the arguments match the comment above it:

  Requires 0 < BITSIZE <= lbits, 0 <= BITPOS % 8 + BITSIZE <= lbits, and
   0 <= BITPOS, where lbits is the size of a LONGEST in bits.  */

but this does not belong in this patch. I'll have to get together these
smaller patches too so that they're not missed out.
 
I'm still working on the rest of the points, which is mainly maintain
signedness when expanding variables and a couple of other problems.


Regards,
Siddhesh