bitpos expansion patches summary

bitpos expansion patches summary

[Siddhesh Poyarekar]

Hi,

I have finally finished another instance of the bitpos changes after
reviewing the report from the processed splint output. Please see the
following discussion for context on this:

http://sourceware.org/ml/gdb-patches/2012-06/msg00851.html

I have uploaded a new version of the splint outputs and reports here:

http://siddhesh.fedorapeople.org/20120805-splint-bitpos.tar.xz

I have broken down the patch into three parts:

* The first part is the original patch with additional type expansions
  done as needed. The other major change is that I have reverted
  expansion of insert/remove watchpoint functions, since the
  ok_for_watchpoint function should take care of the sizes that go into
  them.

* The second part implements checks to ensure that types fit into
  size_t whenever they need to be written out/read using native
  functions like memcpy, memcmp, write_memory, etc.

* The final part is a small change in python-types.c. I've kept this
  separate just for the sake of clarity.

These patches and their changelogs will follow shortly. For all the
patches, I have run the testsuite to ensure that I don't introduce any
regressions on x86_64. I have also reviewed the splint output to try
and ensure that I haven't missed any places that need to be expanded.
There are however a couple of cases where I was not sure what to do and
they are marked as such in the report.

Regards,
Siddhesh

Re: bitpos expansion patches summary

[Jan Kratochvil]

Hi Siddhesh,

with FSF GDB HEAD on Fedora Rawhide x86_64 with --enable-targets=all it does
not build for me:

mips-tdep.c: In function ‘mips_n32n64_return_value’:
mips-tdep.c:5044:5: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘long int’ [-Werror=format]
cc1: all warnings being treated as errors
tic6x-tdep.c: In function ‘tic6x_push_dummy_call’:
tic6x-tdep.c:1133:8: error: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘long int’ [-Werror=format]
cc1: all warnings being treated as errors
gdbtypes.c:955:1: error: conflicting types for ‘lookup_array_range_type’
In file included from gdbtypes.c:28:0:
gdbtypes.h:1537:21: note: previous declaration of ‘lookup_array_range_type’ was here
gdbtypes.c:991:1: error: conflicting types for ‘lookup_string_range_type’
In file included from gdbtypes.c:28:0:
gdbtypes.h:1541:21: note: previous declaration of ‘lookup_string_range_type’ was here

anyway going to review it more.


Thanks,
Jan

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Tue, 7 Aug 2012 16:39:23 +0200, Jan wrote:

> Hi Siddhesh,
> 
> with FSF GDB HEAD on Fedora Rawhide x86_64 with --enable-targets=all
> it does not build for me:

Thanks, I didn't know about --enable-targets=all

> gdbtypes.c:955:1: error: conflicting types for
> ‘lookup_array_range_type’ In file included from gdbtypes.c:28:0:
> gdbtypes.h:1537:21: note: previous declaration of
> ‘lookup_array_range_type’ was here gdbtypes.c:991:1: error:
> conflicting types for ‘lookup_string_range_type’ In file included

Really sorry about that - I must have missed doing a clean build. 

Regards,
Siddhesh

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

On Tue, 7 Aug 2012 16:39:23 +0200, Jan wrote:
> mips-tdep.c: In function ‘mips_n32n64_return_value’:
> mips-tdep.c:5044:5: error: format ‘%d’ expects argument of type
> ‘int’, but argument 3 has type ‘long int’ [-Werror=format] cc1: all
> warnings being treated as errors tic6x-tdep.c: In function
> ‘tic6x_push_dummy_call’: tic6x-tdep.c:1133:8: error: format ‘%d’
> expects argument of type ‘int’, but argument 4 has type ‘long
> int’ [-Werror=format] cc1: all warnings being treated as errors
> gdbtypes.c:955:1: error: conflicting types for
> ‘lookup_array_range_type’ In file included from gdbtypes.c:28:0:
> gdbtypes.h:1537:21: note: previous declaration of
> ‘lookup_array_range_type’ was here gdbtypes.c:991:1: error:
> conflicting types for ‘lookup_string_range_type’ In file included
> from gdbtypes.c:28:0: gdbtypes.h:1541:21: note: previous declaration
> of ‘lookup_string_range_type’ was here
> 

I've fixed the build errors with the --enable-targets=all with the
attached patch. I have done a clean build with --enable-targets=all to
verify.

Regards,
Siddhesh

gdb/ChangeLog:

	* gdbtypes.h (lookup_array_range_type): Expand parameters
	LOW_BOUND, HIGH_BOUND to LONGEST.
	(lookup_string_range_type): Likewise.
	* mips-tdep.c (mips_n32n64_return_value): Use plongest to
	format print OFFSET.
	* tic6x-tdep.c (tic6x_push_dummy_call): Use plongest to format
	print LEN.

[-- Attachment #2: gdb-bitpos-buildfix.patch --]
[-- Type: text/x-patch, Size: 1951 bytes --]

diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index cab8263..ff791ab 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1534,11 +1534,11 @@ extern struct type *create_range_type (struct type *, struct type *, LONGEST,
 
 extern struct type *create_array_type (struct type *, struct type *,
 				       struct type *);
-extern struct type *lookup_array_range_type (struct type *, int, int);
+extern struct type *lookup_array_range_type (struct type *, LONGEST, LONGEST);
 
 extern struct type *create_string_type (struct type *, struct type *,
 					struct type *);
-extern struct type *lookup_string_range_type (struct type *, int, int);
+extern struct type *lookup_string_range_type (struct type *, LONGEST, LONGEST);
 
 extern struct type *create_set_type (struct type *, struct type *);
 
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 4eb91b6..5b0efe5 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -5040,8 +5040,8 @@ mips_n32n64_return_value (struct gdbarch *gdbarch, struct value *function,
 	  if (offset + xfer > TYPE_LENGTH (type))
 	    xfer = TYPE_LENGTH (type) - offset;
 	  if (mips_debug)
-	    fprintf_unfiltered (gdb_stderr, "Return struct+%d:%d in $%d\n",
-				offset, xfer, regnum);
+	    fprintf_unfiltered (gdb_stderr, "Return struct+%s:%d in $%d\n",
+				plongest (offset), xfer, regnum);
 	  mips_xfer_register (gdbarch, regcache,
 			      gdbarch_num_regs (gdbarch) + regnum,
 			      xfer, BFD_ENDIAN_UNKNOWN, readbuf, writebuf,
diff --git a/gdb/tic6x-tdep.c b/gdb/tic6x-tdep.c
index 8309d58..5e73d27 100644
--- a/gdb/tic6x-tdep.c
+++ b/gdb/tic6x-tdep.c
@@ -1130,7 +1130,8 @@ tic6x_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	    }
 	  else
 	    internal_error (__FILE__, __LINE__,
-			    _("unexpected length %d of arg %d"), len, argnum);
+			    _("unexpected length %s of arg %d"),
+			    plongest (len), argnum);
 
 	  addr = sp + stack_offset;
 	  write_memory (addr, val, len);

Re: bitpos expansion patches summary

[Jan Kratochvil]

Hi Siddhesh,

I have found one problem.  We miss the cases (there won't be too many) like:

ada-lang.c:2315
offset is LONGEST:
      long new_offset = offset;

As on x86_64 LONGEST is #defined as 'long'.  But on LLP64 (MS-Windows IA64)
	http://en.wikipedia.org/wiki/64-bit#64-bit_data_models
'long' is 32-bit but LONGEST is (presumably) still 64-bit.

'long' should be changed to 'LONGEST' at ada-lang.c:2315.

It probably requires to run splint also in 32-bit mode (on GNU/Linux).


Thanks,
Jan

Re: bitpos expansion patches summary

[Jan Kratochvil]

On Thu, 09 Aug 2012 00:43:12 +0200, Jan Kratochvil wrote:
> As on x86_64 LONGEST is #defined as 'long'.  But on LLP64 (MS-Windows IA64)

I wanted to say MS-Windows x86_64
(I do not know what model is used by MS-Windows ia64).

> 	http://en.wikipedia.org/wiki/64-bit#64-bit_data_models
> 'long' is 32-bit but LONGEST is (presumably) still 64-bit.


Jan

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Thu, 9 Aug 2012 00:43:12 +0200, Jan wrote:

> Hi Siddhesh,
> 
> I have found one problem.  We miss the cases (there won't be too
> many) like:
> 
> ada-lang.c:2315
> offset is LONGEST:
>       long new_offset = offset;

I think that is my fault - splint gave that warning, but I missed it
when copying over my annotations from the old report to the new one:

RSAFE: (ada-lang.c:2315):       VARINIT(new_offset):    (LONGEST to
LONG)       [offset]

Split gave these warnings when I ran it for the new report, which is
why I also had to modify the processing script to account for a LONG and
ULONG as opposed to LONGEST and ULONGEST. I'll review the changes and
send a fix for this on top of the current patches.

Regards,
Siddhesh

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 297 bytes --]

On Thu, 9 Aug 2012 00:43:12 +0200, Jan wrote:
> ada-lang.c:2315
> offset is LONGEST:
>       long new_offset = offset;
> 

Turns out that this is the only one I had missed, so here's the fix for
it.

Regards,
Siddhesh

	* ada-lang.c (ada_value_primitive_packed_val): Expand
	NEW_OFFSET to LONGEST.

[-- Attachment #2: bitpos-expand-mised-long.patch --]
[-- Type: text/x-patch, Size: 424 bytes --]

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index c350a0e..9c305702 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2312,7 +2312,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
 
   if (obj != NULL)
     {
-      long new_offset = offset;
+      LONGEST new_offset = offset;
 
       set_value_component_location (v, obj);
       set_value_bitpos (v, bit_offset + value_bitpos (obj));

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

Hi,

Here is an updated patch for the missed expansions. This should apply
on top of the bitpos expansion patch. I have verified that the patch
does not cause any regressions in the testsuite on x86_64. I have also
run splint and the perl scripts to see if I have missed any more.

Regards,
Siddhesh

gdb/ChangeLog:

	* ada-lang.c (ada_value_primitive_packed_val): Expand NEW_OFFSET
	to LONGEST.
	* arm-tdep.c (arm_vfp_cprc_sub_candidate): Expand COUNT to
	LONGEST.
	* dwarf2loc.c (indirect_pieced_value): Expand THIS_SIZE_BITS to
	LONGEST.
	* sh-tdep.c (sh_push_dummy_call_fpu): Expand REG_SIZE to
	ssize_t.
	(sh_push_dummy_call_nofpu): Likewise.

[-- Attachment #2: bitpos-expand-missed-expand.patch --]
[-- Type: text/x-patch, Size: 1757 bytes --]

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index c350a0e..9c305702 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -2312,7 +2312,7 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr,
 
   if (obj != NULL)
     {
-      long new_offset = offset;
+      LONGEST new_offset = offset;
 
       set_value_component_location (v, obj);
       set_value_bitpos (v, bit_offset + value_bitpos (obj));
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index dc587e1..496cc30 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3427,7 +3427,7 @@ arm_vfp_cprc_sub_candidate (struct type *t,
 
     case TYPE_CODE_STRUCT:
       {
-	int count = 0;
+	LONGEST count = 0;
 	unsigned unitlen;
 	int i;
 	for (i = 0; i < TYPE_NFIELDS (t); i++)
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index ad12e37..bef4355 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1995,7 +1995,7 @@ indirect_pieced_value (struct value *value)
   for (i = 0; i < c->n_pieces && bit_length > 0; i++)
     {
       struct dwarf_expr_piece *p = &c->pieces[i];
-      size_t this_size_bits = p->size;
+      ULONGEST this_size_bits = p->size;
 
       if (bit_offset > 0)
 	{
diff --git a/gdb/sh-tdep.c b/gdb/sh-tdep.c
index 00d7401..1f599f8 100644
--- a/gdb/sh-tdep.c
+++ b/gdb/sh-tdep.c
@@ -1068,7 +1068,7 @@ sh_push_dummy_call_fpu (struct gdbarch *gdbarch,
   CORE_ADDR regval;
   char *val;
   LONGEST len;
-  int reg_size = 0;
+  ssize_t reg_size = 0;
   int pass_on_stack = 0;
   int treat_as_flt;
   int last_reg_arg = INT_MAX;
@@ -1210,7 +1210,7 @@ sh_push_dummy_call_nofpu (struct gdbarch *gdbarch,
   CORE_ADDR regval;
   char *val;
   LONGEST len;
-  int reg_size = 0;
+  ssize_t reg_size = 0;
   int pass_on_stack = 0;
   int last_reg_arg = INT_MAX;
 

Re: bitpos expansion patches summary

[Jan Kratochvil]

Hi Siddhesh,

just some incremental notes, still looking at it.


Thanks,
Jan


 @@ -1406,6 +1406,15 @@ lookup_struct_elt_type (struct type *type, char *name, int noerr)
    error (_("Type %s has no component named %s."), typename, name);
  }
  
 +/* Ensure that the input TYPE is not larger than the maximum capacity of the
 +   host system, i.e. size_t.  */
 +void
 +ensure_type_fits_sizet (const struct type *type)
+
+Make it ensure_type_fits_size_t.  And there should be 'error' in its name.
+For example 'type_fits_size_t_or_error' or something like that.
+
+
 +{
 +  if (!TYPE_LENGTH_FITS_SIZET (type))
 +    error (_("Target object too large for host GDB memory."));
+
+Please print the sizes.  Also this message is present at multiple places so
+maybe rather make a function for unconditionally printing the error?
+
+
 +}
 +
  /* Lookup the vptr basetype/fieldno values for TYPE.
     If found store vptr_basetype in *BASETYPEP if non-NULL, and return
     vptr_fieldno.  Also, if found and basetype is from the same objfile,
 @@ -1059,6 +1059,10 @@ extern void allocate_gnat_aux_type (struct type *);
     so you only have to call check_typedef once.  Since allocate_value
     calls check_typedef, TYPE_LENGTH (VALUE_TYPE (X)) is safe.  */
  #define TYPE_LENGTH(thistype) (thistype)->length
+
+Empty line.
+
 +/* Make sure that TYPE_LENGTH fits into a size_t.  */
 +#define TYPE_LENGTH_FITS_SIZET(thistype) ((size_t) TYPE_LENGTH (thistype) \
 +					  == TYPE_LENGTH (thistype))
+
+Make it TYPE_LENGTH_FITS_SIZE_T, please.  And I think this macro is not needed,
+inline it.  (It does not access internal fields of the type structures.)
+
+And (a) check it already for ssize_t.  Because the code is not safe enough to
+properly handle unsigned sizes everywhere without overflows.  (b) Make there
+some reserve, anything close to ssize_t will never get successfully xmalloc-ed
+anyway.  Some maximum size could be: ((size_t) -1) / 4.  Depending on SSIZE_MAX
+may not be compatible enough I guess.
+
+
 +
  /* Note that TYPE_CODE can be TYPE_CODE_TYPEDEF, so if you want the real
     type, you need to do TYPE_CODE (check_type (this_type)).  */
  #define TYPE_CODE(thistype) TYPE_MAIN_TYPE(thistype)->code

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Thu, 9 Aug 2012 22:03:56 +0200, Jan wrote:
>  +void
>  +ensure_type_fits_sizet (const struct type *type)
> +
> +Make it ensure_type_fits_size_t.  And there should be 'error' in its
> name. +For example 'type_fits_size_t_or_error' or something like that.

OK, will make this type_fits_size_t_or_error.

>  +{
>  +  if (!TYPE_LENGTH_FITS_SIZET (type))
>  +    error (_("Target object too large for host GDB memory."));
> +
> +Please print the sizes.  Also this message is present at multiple
> places so +maybe rather make a function for unconditionally printing
> the error? +

Could you please give an example of this? I didn't think that there are
any such checks in the source yet.

>  +/* Make sure that TYPE_LENGTH fits into a size_t.  */
>  +#define TYPE_LENGTH_FITS_SIZET(thistype) ((size_t) TYPE_LENGTH
> (thistype) \
>  +					  == TYPE_LENGTH (thistype))
> +
> +Make it TYPE_LENGTH_FITS_SIZE_T, please.  And I think this macro is
> not needed, +inline it.  (It does not access internal fields of the
> type structures.) +

I had kept it for possible future need if someone wants to only check
if a type fits in and not throw an error. I will inline it.

> +And (a) check it already for ssize_t.  Because the code is not safe
> enough to +properly handle unsigned sizes everywhere without
> overflows.  (b) Make there +some reserve, anything close to ssize_t
> will never get successfully xmalloc-ed +anyway.  Some maximum size
> could be: ((size_t) -1) / 4.  Depending on SSIZE_MAX +may not be
> compatible enough I guess. +

I had thought of that, but I figured that instead of guessing a value,
I would be better off only doing the size_t check (i.e. code sanity) and
leave the question of whether a type gets successfully malloc'd or not
to the OS.

For ssize_t, I could add an extra boolean argument to
type_fits_size_t_or_error that indicates whether the type is signed or
not.

Regards,
Siddhesh

Re: bitpos expansion patches summary

[Jan Kratochvil]

On Fri, 10 Aug 2012 03:43:38 +0200, Siddhesh Poyarekar wrote:
> On Thu, 9 Aug 2012 22:03:56 +0200, Jan wrote:
> >  +  if (!TYPE_LENGTH_FITS_SIZET (type))
> >  +    error (_("Target object too large for host GDB memory."));
> > +
> > +Please print the sizes.  Also this message is present at multiple
> > places so +maybe rather make a function for unconditionally printing
> > the error? +
> 
> Could you please give an example of this? I didn't think that there are
> any such checks in the source yet.

      if (accumulate_size < 0)
        error (_("Total size of arguments too large for host GDB memory."));

It also should print the sizes, in some cases GDB would print the first error
above, in other cases the second error, I do not think there is a difference.


> > +Make it TYPE_LENGTH_FITS_SIZE_T, please.  And I think this macro is
> > not needed, +inline it.  (It does not access internal fields of the
> > type structures.) +
> 
> I had kept it for possible future need if someone wants to only check
> if a type fits in and not throw an error. I will inline it.

OK, I understand now the goal.  But GDB does not accept additional code which
is useful only with future extension.


> > +And (a) check it already for ssize_t.  Because the code is not safe
> > enough to +properly handle unsigned sizes everywhere without
> > overflows.  (b) Make there +some reserve, anything close to ssize_t
> > will never get successfully xmalloc-ed +anyway.  Some maximum size
> > could be: ((size_t) -1) / 4.  Depending on SSIZE_MAX +may not be
> > compatible enough I guess. +
> 
> I had thought of that, but I figured that instead of guessing a value,
> I would be better off only doing the size_t check (i.e. code sanity) and
> leave the question of whether a type gets successfully malloc'd or not
> to the OS.

The problem is if you futher add something to the value it overflows, xmalloc
will then get few bytes, which succeeds, but GDB behavior is wrong then.
You have correctly put in alpha-tdep.c the new check for such case:
      /* Check for overflow.  */
      if (accumulate_size < 0)
        error (_("Total size of arguments too large for host GDB memory."));

But I have doubts one can catch such overflows across the whole GDB codebase,
so I see some 'safe enough margin' for overflows to be more universal check
for it.


> For ssize_t, I could add an extra boolean argument to
> type_fits_size_t_or_error that indicates whether the type is signed or
> not.

We discussed before that the larger size of unsigned types vs. signed types is
not useful.  GDB will never successfully allocate more than 2GB on 32-bit host
(size_t max larger than ssize_t).  And it does not make sense to consider
anyhow sizes above 9 quintillion (ULONGEST max larger than LONGEST on any host
or size_t larger than ssize_t on 64-bit hosts)


Thanks,
Jan

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Fri, 10 Aug 2012 09:50:56 +0200, Jan wrote:
>       if (accumulate_size < 0)
>         error (_("Total size of arguments too large for host GDB
> memory."));
> 
> It also should print the sizes, in some cases GDB would print the
> first error above, in other cases the second error, I do not think
> there is a difference.

Ack.
 
> We discussed before that the larger size of unsigned types vs. signed
> types is not useful.  GDB will never successfully allocate more than
> 2GB on 32-bit host (size_t max larger than ssize_t).  And it does not
> make sense to consider anyhow sizes above 9 quintillion (ULONGEST max
> larger than LONGEST on any host or size_t larger than ssize_t on
> 64-bit hosts)

OK, I'll limit it to SIZE_MAX / 8 (i.e. SSIZE_MAX / 4).

I'll send an updated patch in this case since the size_t patch is not
that large.

Regards,
Siddhesh

Re: bitpos expansion patches summary

[Jan Kratochvil]

Hi Siddhesh,

another part of review, it is pretty long...

Also I am marking every patch chunk with corresponding 'report' line as
otherwise I do not know how we can find the reasons each of the changes is
being committed.

(Some of it was already addressed, like ' - error: llp64'.)


Thanks,
Jan



RSAFE: (ada-lang.c:579):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
RSAFE: (ada-lang.c:670):	FUNC(umax_of_size):	(ULONGEST to int)	[(t)->length]
RSAFE: (ada-lang.c:672):	FUNC(max_of_size):	(ULONGEST to int)	[(t)->length]
RSAFE: (ada-lang.c:682):	FUNC(min_of_size):	(ULONGEST to int)	[(t)->length]
RSAFE: (ada-lang.c:1622):	RET:	(ULONGEST to int)	[8 * (ada_check_typedef((((type)->main_type->flds_bnds.fields[1]).type)))->length]
RSAFE: (ada-lang.c:1688):	RET:	(ULONGEST to int)	[8 * ((((type)->main_type->flds_bnds.fields[0]).type))->length]
RSAFE: (ada-lang.c:1724):	RET:	(ULONGEST to int)	[8 * ((((type)->main_type->flds_bnds.fields[2 * i + which - 2]).type))->length]

UNSURE(Expand bitsize?): (ada-lang.c:2072):	ASSIGN:	(LONGEST to UINT)	[ (((new_type)->main_type->flds_bnds.fields[0]).bitsize) = *elt_bits]
 - That is whether to extend:
      /* Size of this field, in bits, or zero if not packed.
         If non-zero in an array type, indicates the element size in
         bits (used only in Ada at the moment).
         For an unpacked field, the field's type's length
         says how many bytes the field occupies.  */
      unsigned int bitsize : 28;
I do not know how to create such large bitfield with gcc, during such attempts
I get:
bitsize.c:7:3: error: bit-field ‘x’ has invalid type
Even if it is possible I would find OK to leave it for a different patchset.

RSAFE: (ada-lang.c:2185):	FUNC(ada_value_primitive_packed_val):	(LONGEST to int)	[bit_pos % 8]

RSAFE: (ada-lang.c:2315):	VARINIT(new_offset):	(LONGEST to LONG)	[offset]
 - error: llp64

RSAFE: (ada-lang.c:2342):	FUNC(memset):	(ULONGEST to size_t)	[(type)->length]
RSAFE: (ada-lang.c:2379):	ASSIGN:	(LONGEST to int)	[ src = targ = 0]
RSAFE: (ada-lang.c:2445):	ASSIGN:	(LONGEST to int)	[ accum_bits = 8 - src_offset]
RSAFE: (ada-lang.c:2455):	CMP:	(LONGEST to int)	[chunk_size > n]
RSAFE: (ada-lang.c:2456):	ASSIGN:	(LONGEST to int)	[ chunk_size = n]
RSAFE: (ada-lang.c:2470):	ASSIGN:	(LONGEST to UINT)	[ accum = (UCHAR)*source >> src_offset]
RSAFE: (ada-lang.c:2472):	ASSIGN:	(LONGEST to int)	[ accum_bits = 8 - src_offset]
RSAFE: (ada-lang.c:2480):	CMP:	(LONGEST to int)	[chunk_size > n]
RSAFE: (ada-lang.c:2481):	ASSIGN:	(LONGEST to int)	[ chunk_size = n]
RSAFE: (ada-lang.c:2544):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
RSAFE: (ada-lang.c:4093):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
RSAFE: (ada-lang.c:4137):	FUNC(memcpy):	(ULONGEST to size_t)	[(actual_type)->length]
RSAFE: (ada-lang.c:4162):	FUNC(C_alloca):	(ULONGEST to size_t)	[len]
RSAFE: (ada-lang.c:6430):	FUNC(ada_value_primitive_packed_val):	(LONGEST to int)	[bit_pos % 8]
RSAFE: (ada-lang.c:6488):	ASSIGN:	(LONGEST to int)	[ *bit_offset_p = bit_pos % 8]
RSAFE: (ada-lang.c:8738):	FUNC(memcmp):	(ULONGEST to size_t)	[(value_type(arg1))->length]
RSAFE: (ada-tasks.c:785):	VARINIT(target_ptr_byte):	(ULONGEST to int)	[(data->known_tasks_element)->length]
RSAFE: (ada-tasks.c:812):	VARINIT(target_ptr_byte):	(ULONGEST to int)	[(data->known_tasks_element)->length]
RSAFE: (ada-valprint.c:376):	FUNC(ada_emit_char):	(ULONGEST to int)	[(type)->length]
FIXED(expand i, rep1, reps ULONGEST): (ada-valprint.c:483):	CMP:	(ULONGEST to UINT)	[i < length]
FIXED(Likewise): (ada-valprint.c:501):	CMP:	(ULONGEST to UINT)	[rep1 < length]
FIXED(Likewise): (ada-valprint.c:553):	CMP:	(ULONGEST to UINT)	[i < length]
RSAFE: (ada-valprint.c:563):	FUNC(printstr):	(ULONGEST to int)	[(type)->length]
 - agreed, it is length of char
RSAFE: (ada-valprint.c:629):	CMP:	(UINT to LONGEST)	[temp_len < options->print_max]
RSAFE: (ada-valprint.c:631):	FUNC(char_at):	(ULONGEST to int)	[eltlen]
RSAFE: (ada-valprint.c:636):	FUNC(printstr):	(ULONGEST to int)	[eltlen]
RSAFE: (ada-valprint.c:964):	CMP:	(size_t to ULONGEST)	[((type)->main_type->target_type)->length != sizeof(char)]
RSAFE: (ada-valprint.c:1132):	FUNC(ada_value_primitive_packed_val):	(LONGEST to int)	[bit_pos % 8]
ENSURED_SIZET: (alpha-tdep.c:402):	ASSIGN:	(LONGEST to int)	[ accumulate_size = (accumulate_size + m_arg->len + 7) & ~7]
RSAFE: (alpha-tdep.c:432):	CMP:	(size_t to LONGEST)	[offset + len <= sizeof((arg_reg_buffer))]
RSAFE: (alpha-tdep.c:434):	FUNC(memcpy):	(LONGEST to size_t)	[len]
RSAFE: (alpha-tdep.c:448):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
RSAFE: (alpha-tdep.c:630):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
RSAFE: (amd64-tdep.c:635):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
RSAFE: (amd64-tdep.c:698):	FUNC(regcache_raw_read_part):	(LONGEST to int)	[((len) < (8) ? (len) : (8))]
RSAFE: (amd64-tdep.c:701):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[((len) < (8) ? (len) : (8))]
RSAFE: (amd64-tdep.c:824):	FUNC(memcpy):	(LONGEST to size_t)	[((len) < (8) ? (len) : (8))]
RSAFE: (amd64-tdep.c:845):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
RSAFE: (amd64-tdep.c:2777):	FUNC(target_read_memory):	(LONGEST to ssize_t)	[len]
RSAFE: (amd64-windows-tdep.c:112):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
FIXED(Expand param to LONGEST): (amd64-windows-tdep.c:120):	FUNC(regcache_raw_read_part):	(LONGEST to int)	[len]
FIXED(Likewise): (amd64-windows-tdep.c:122):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[len]
WPREVERTED(Only ok_for_watchpoint needs LONGEST): (arm-linux-nat.c:938):	ASSIGN:	(LONGEST to UINT)	[ mask = (1 << len) - 1]
RSAFE: (arm-tdep.c:3292):	RET:	(ULONGEST to int)	[(t)->length]
FIXED(arm_vfp_cprc_sub_candidate return LONGEST): (arm-tdep.c:3425):	RET:	(ULONGEST to int)	[(t)->length / unitlen]
 - this is correct but arm-tdep.c:3442 is wrong, I guess you did not rerun
   splint after the changes to check for new reported regressions.
RSAFE: (arm-tdep.c:3695):	FUNC(C_alloca):	(LONGEST to size_t)	[len]
RSAFE: (arm-tdep.c:3707):	VARINIT(partial_len):	(LONGEST to int)	[len < 4 ? len : 4]
RSAFE: (auxv.c:80):	VARINIT(ptr_size):	(ULONGEST to size_t)	[(ptr_type)->length]
REVERTED: (avr-tdep.c:932):	FUNC(regcache_cooked_write):	(LONGEST to int)	[lsb_reg + i]
RSAFE: (avr-tdep.c:938):	FUNC(regcache_cooked_read):	(LONGEST to int)	[lsb_reg + i]
ENSURED_SIZET: (avr-tdep.c:1184):	FUNC(xmalloc):	(LONGEST to size_t)	[len]
 - You do not, you have changed parameter len to ssize_t, you did ENSURED_SIZET on line 1298.
 - But that ENSURED_SIZET is redundant there as the code already calls value_contents (arg).
ENSURED_SIZET: (avr-tdep.c:1185):	ASSIGN:	(LONGEST to int)	[ si->len = len]
 - Likewise.
ENSURED_SIZET: (avr-tdep.c:1187):	FUNC(memcpy):	(LONGEST to size_t)	[len]
 - Likewise.
FIXED(Expand j): (avr-tdep.c:1290):	CMP:	(LONGEST to int)	[j < len]
RSAFE: (ax-gdb.c:490):	FUNC(ax_ext):	(ULONGEST to int)	[(type)->length * 8]
RSAFE: (ax-gdb.c:500):	VARINIT(bits):	(ULONGEST to int)	[(type)->length * 8]
FIXED(Expand N to LONGEST): (ax-gdb.c:516):	FUNC(ax_trace_quick):	(ULONGEST to int)	[(type)->length]
RSAFE: (ax-gdb.c:1428):	FUNC():	(LONGEST to int)	[end - start]
SAFE: (bfin-tdep.c:538):	FUNC(write_memory):	(LONGEST to ssize_t)	[container_len]
WPFIXED(Expanded bp_target_info.length): (breakpoint.c:2397):	ASSIGN:	(LONGEST to int)	[ bl->target_info.length = bl->length]
SAFE(Hardware watchpoints should fit into int): (breakpoint.c:4624):	FUNC():	(LONGEST to int)	[loc->length]
 - That's not true, s390x can set arbitrary hardware watchpoint memory range, see s390_fix_watch_points.
WPFIXED(Expand breakpoint_address_match_range len1 arg): (breakpoint.c:6611):	FUNC(breakpoint_address_match_range):	(LONGEST to int)	[bl->length]
WPFIXED(Likewise): (breakpoint.c:9837):	FUNC(breakpoint_address_match_range):	(LONGEST to int)	[bl->length]
RSAFE: (c-lang.c:291):	ASSIGN:	(ULONGEST to int)	[ width = (element_type)->length]
RSAFE: (c-lang.c:417):	FUNC(C_alloca):	(ULONGEST to size_t)	[(type)->length]
SAFE: (c-lang.c:419):	ASSIGN:	(ULONGEST to LONG)	[ (output)->temp = ((type)->length)]
RSAFE: (c-lang.c:673):	CMP:	(ULONGEST to UINT)	[(UINT)((&output)->next_free - (&output)->object_base) != (type)->length]
RSAFE: (coffread.c:2043):	ASSIGN:	(CORE_ADDR to LONGEST)	[ ((list->field).loc.bitpos) = (ms->c_value)]
RSAFE: (corefile.c:346):	FUNC(C_alloca):	(ULONGEST to size_t)	[(type)->length]
RSAFE: (corefile.c:348):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
FIXED(Expand i_offset): (cp-valprint.c:363):	VARINIT(i_offset):	(LONGEST to int)	[offset + ((((type)->main_type->flds_bnds.fields[i]).loc.bitpos) + 0) / 8]
ENSURED_SIZET: (cp-valprint.c:561):	FUNC(xmalloc):	(ULONGEST to size_t)	[(baseclass)->length]
ENSURED_SIZET: (cp-valprint.c:565):	FUNC(target_read_memory):	(ULONGEST to ssize_t)	[(baseclass)->length]
ENSURED_SIZET: (cris-tdep.c:683):	FUNC(xmalloc):	(LONGEST to size_t)	[len]
 - You did not, but ensure_type_fits_sizet at the caller is redundant as
   value_contents is already called there beforehand.
ENSURED_SIZET: (cris-tdep.c:684):	ASSIGN:	(LONGEST to int)	[ si->len = len]
 - Likewise.
ENSURED_SIZET: (cris-tdep.c:686):	FUNC(memcpy):	(LONGEST to size_t)	[len]
 - Likewise.
ENSURED_SIZET: (cris-tdep.c:899):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
 - Likewise.
SAFE: (doublest.c:816):	CMP:	(UINT to LONGEST)	[len * 8 == gdbarch_long_double_format(gdbarch)[0]->totalsize]
RSAFE: (doublest.c:879):	FUNC(memset):	(ULONGEST to size_t)	[(type)->length]
RSAFE: (doublest.c:905):	FUNC(memset):	(ULONGEST to size_t)	[(to_type)->length]
RSAFE: (doublest.c:915):	FUNC(memset):	(ULONGEST to size_t)	[(to_type)->length]
RSAFE: (doublest.c:916):	FUNC(memcpy):	(ULONGEST to size_t)	[(((from_type)->length) < ((to_type)->length) ? ((from_type)->length) : ((to_type)->length))]
SAFE(size of DW_OP_GNU_const_type): (dwarf2expr.c:452):	CMP:	(int to ULONGEST)	[(result)->length != size]
SAFE(Address size): (dwarf2expr.c:1050):	CMP:	(int to ULONGEST)	[(type)->length != addr_size]
SAFE(Address size): (dwarf2expr.c:1055):	FUNC(C_alloca):	(ULONGEST to size_t)	[(type)->length]
RSAFE: (dwarf2loc.c:1279):	FUNC(memcpy):	(ULONGEST to size_t)	[(checked_type)->length]
SAFE: (dwarf2loc.c:1496):	ASSIGN:	(ULONGEST to UINT)	[ source_offset_bits = source_offset % 8]
SAFE: (dwarf2loc.c:1498):	ASSIGN:	(ULONGEST to UINT)	[ dest_avail = 8 - dest_offset_bits % 8]
SAFE: (dwarf2loc.c:1501):	CMP:	(ULONGEST to UINT)	[dest_avail < bit_count]
SAFE: (dwarf2loc.c:1505):	FUNC(insert_bits):	(ULONGEST to UINT)	[dest_offset_bits]
SAFE: (dwarf2loc.c:1524):	FUNC(extract_bits):	(ULONGEST to int)	[bit_count]
SAFE: (dwarf2loc.c:1526):	FUNC(insert_bits):	(ULONGEST to UINT)	[dest_offset_bits]
SAFE: (dwarf2loc.c:1526):	FUNC(insert_bits):	(ULONGEST to int)	[bit_count]
SAFE: (dwarf2loc.c:1596):	CMP:	(ULONGEST to size_t)	[buffer_size < this_size]
SAFE: (dwarf2loc.c:1598):	ASSIGN:	(ULONGEST to size_t)	[ buffer_size = this_size]
 - I do not see how this can be safe, some ENSURED_SIZET is needed here.
SAFE: (dwarf2loc.c:1610):	VARINIT(reg_offset):	(LONGEST to int)	[source_offset]
SAFE: (dwarf2loc.c:1613):	CMP:	(int to ULONGEST)	[this_size < register_size(arch, gdb_regnum)]
SAFE: (dwarf2loc.c:1616):	ASSIGN:	(ULONGEST to int)	[ reg_offset = register_size(arch, gdb_regnum) - this_size]
SAFE: (dwarf2loc.c:1627):	FUNC(get_frame_register_bytes):	(ULONGEST to int)	[this_size]
RSAFE: (dwarf2loc.c:1631):	FUNC(memset):	(ULONGEST to size_t)	[this_size]
FIXED(Expand read_value_memory arg): (dwarf2loc.c:1648):	FUNC(read_value_memory):	(LONGEST to int)	[offset]
SAFE(Caller ensures size_t): (dwarf2loc.c:1651):	FUNC(read_value_memory):	(ULONGEST to size_t)	[this_size]
SAFE: (dwarf2loc.c:1656):	VARINIT(n):	(ULONGEST to size_t)	[this_size]
SAFE: (dwarf2loc.c:1658):	CMP:	(LONGEST to size_t)	[n > c->addr_size - source_offset]
SAFE: (dwarf2loc.c:1659):	CMP:	(LONGEST to int)	[c->addr_size >= source_offset]
SAFE: (dwarf2loc.c:1659):	ASSIGN:	(LONGEST to size_t)	[ n = (c->addr_size >= source_offset ? c->addr_size - source_offset : 0)]
SAFE: (dwarf2loc.c:1677):	VARINIT(n):	(ULONGEST to size_t)	[this_size]
FIXED(Expand offset): (dwarf2loc.c:1748):	CMP:	(ULONGEST to LONG)	[offset < type_len]
SAFE: (dwarf2loc.c:1787):	CMP:	(ULONGEST to size_t)	[buffer_size < this_size]
SAFE: (dwarf2loc.c:1789):	ASSIGN:	(ULONGEST to size_t)	[ buffer_size = this_size]
 - I do not see how this can be safe, some ENSURED_SIZET is needed here.
SAFE: (dwarf2loc.c:1802):	VARINIT(reg_offset):	(LONGEST to int)	[dest_offset]
SAFE: (dwarf2loc.c:1805):	CMP:	(int to ULONGEST)	[this_size <= register_size(arch, gdb_regnum)]
SAFE: (dwarf2loc.c:1807):	ASSIGN:	(ULONGEST to int)	[ reg_offset = register_size(arch, gdb_regnum) - this_size]
SAFE: (dwarf2loc.c:1816):	FUNC(get_frame_register_bytes):	(ULONGEST to int)	[this_size]
SAFE: (dwarf2loc.c:1836):	FUNC(put_frame_register_bytes):	(ULONGEST to int)	[this_size]
SAFE: (dwarf2loc.c:1860):	FUNC(write_memory):	(ULONGEST to ssize_t)	[this_size]
FIXED(Expand this_size_bits): (dwarf2loc.c:1902):	CMP:	(size_t to LONGEST)	[bit_offset >= this_size_bits]
FIXED(Expand this_size_bits): (dwarf2loc.c:2002):	CMP:	(size_t to LONGEST)	[bit_offset >= this_size_bits]
 - I do not see this_size_bits expanded, there is still: size_t this_size_bits = p->size;


--- bitpos3.patch-orig	2012-08-09 21:21:04.722849376 +0200
+++ bitpos3.patch	2012-08-12 19:51:17.978915788 +0200
@@ -651,6 +664,11 @@ index 5d3affa..de72c9c 100644
  	  break;
  	}
 +      ensure_type_fits_sizet (arg_type);
+xENSURED_SIZET: (alpha-tdep.c:402):      ASSIGN: (LONGEST to int)        [ accumulate_size = (accumulate_size + m_arg->len + 7) & ~7]
+
+Just move m_arg->contents = value_contents (arg); here and you can drop this
+call of ensure_type_fits_sizet.
+
        m_arg->len = TYPE_LENGTH (arg_type);
        m_arg->offset = accumulate_size;
        accumulate_size = (accumulate_size + m_arg->len + 7) & ~7;
@@ -658,7 +676,14 @@ index 5d3affa..de72c9c 100644
 +      /* Check for overflow.  */
 +      if (accumulate_size < 0)
 +        error (_("Total size of arguments too large for host GDB memory."));
+xENSURED_SIZET: (alpha-tdep.c:402):      ASSIGN: (LONGEST to int)        [ accumulate_size = (accumulate_size + m_arg->len + 7) & ~7]
+
+If you make accumulate_size LONGEST you can drop this check.  Then just verify
+you do not underflow SP in the line below:
+  sp -= accumulate_size;
+
 +
+xENSURED_SIZET: (alpha-tdep.c:402):      ASSIGN: (LONGEST to int)        [ accumulate_size = (accumulate_size + m_arg->len + 7) & ~7]
        m_arg->contents = value_contents (arg);
      }
  

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Sun, 12 Aug 2012 19:57:30 +0200, Jan wrote:
> (arm-tdep.c:3425):	RET:	(ULONGEST to int)
> [(t)->length / unitlen]
>  - this is correct but arm-tdep.c:3442 is wrong, I guess you did not
> rerun splint after the changes to check for new reported regressions.

I did, but I only went through it superficially.

> i] ENSURED_SIZET: (avr-tdep.c:1184):	FUNC(xmalloc):
> (LONGEST to size_t)	[len]
>  - You do not, you have changed parameter len to ssize_t, you did
> ENSURED_SIZET on line 1298.
>  - But that ENSURED_SIZET is redundant there as the code already

OK, do you want me to add the check within the function? It seemed
pointless to me since it's only used at the caller and it doesn't look
like any other function will have use of it. I could just remove the
type_is_size_t_or_error call here. Likewise for other tdep instances.

> (breakpoint.c:4624):	FUNC():	(LONGEST to int)
> [loc->length]
>  - That's not true, s390x can set arbitrary hardware watchpoint
> memory range, see s390_fix_watch_points. WPFIXED(Expand

Oh, then my assumption about watchpoint sizes would be wrong then
wouldn't it? We then need to expand the ok_for_watchpoint as well as
{insert|remove}_watchpoint. We could do it as a different changeset
since only s390x would need it.

> [buffer_size < this_size] SAFE: (dwarf2loc.c:1598):
> ASSIGN:	(ULONGEST to size_t)	[ buffer_size = this_size]
>  - I do not see how this can be safe, some ENSURED_SIZET is needed

OK, I'll make this part of the patch 2/3.

> FIXED(Expand this_size_bits): (dwarf2loc.c:2002):	CMP:
> (size_t to LONGEST)	[bit_offset >= this_size_bits]
>  - I do not see this_size_bits expanded, there is still: size_t
> this_size_bits = p->size;

Sorry, will fix.

>  +      ensure_type_fits_sizet (arg_type);
> +xENSURED_SIZET: (alpha-tdep.c:402):      ASSIGN: (LONGEST to
> int)        [ accumulate_size = (accumulate_size + m_arg->len + 7) &
> ~7] + +Just move m_arg->contents = value_contents (arg); here and you
> can drop this +call of ensure_type_fits_sizet.

OK.

>  +      if (accumulate_size < 0)
>  +        error (_("Total size of arguments too large for host GDB
> memory.")); +xENSURED_SIZET: (alpha-tdep.c:402):      ASSIGN:
> (LONGEST to int)        [ accumulate_size = (accumulate_size +
> m_arg->len + 7) & ~7] + +If you make accumulate_size LONGEST you can
> drop this check.  Then just verify +you do not underflow SP in the
> line below:

OK.


Thanks,
Siddhesh

Re: bitpos expansion patches summary

[Jan Kratochvil]

On Mon, 13 Aug 2012 04:51:24 +0200, Siddhesh Poyarekar wrote:
> > i] ENSURED_SIZET: (avr-tdep.c:1184):	FUNC(xmalloc):
> > (LONGEST to size_t)	[len]
> >  - You do not, you have changed parameter len to ssize_t, you did
> > ENSURED_SIZET on line 1298.
> >  - But that ENSURED_SIZET is redundant there as the code already
> 
> OK, do you want me to add the check within the function?

I was requesting only to remove the redundant line
	avr-tdep.c:1298:ensure_type_fits_sizet (type);

as on this line GDB already executed line
	const bfd_byte *contents = value_contents (arg);

which would already abort for too large TYPE_LENGTH.


Unrelated to this specific case but to ensure_type_fits_sizet in general:

Could you change these calls of ensure_type_fits_sizet where we already deal
with the length itself and no longer the type?

          ensure_type_fits_sizet (type);
          si = push_stack_item (si, contents, len);

I would find it easier as:
          ensure_length_fits_sizet (len);
          si = push_stack_item (si, contents, len);

Here only LEN matters.  LEN is LONGEST so we can verify whether LONGEST LEN
fits into SSIZE_T.  Reintroducing here a dependency on TYPE is a needless
little complication of the code IMO.


Moreover even when we have TYPE we could use:
          ensure_length_fits_sizet (TYPE_LENGTH (type));

ensure_type_fits_sizet uses only TYPE->LENGTH so it does not need to be passed
TYPE at all.  It would make sense if ensure_type_fits_sizet prints TYPE_NAME
in the error case etc. - but it does not use TYPE_NAME anyway..


> I could just remove the type_is_size_t_or_error call here.

Yes.


> Likewise for other tdep instances.

I do not see how it can be generalized, I may not just see what do you mean.


> > (breakpoint.c:4624):	FUNC():	(LONGEST to int)
> > [loc->length]
> >  - That's not true, s390x can set arbitrary hardware watchpoint
> > memory range, see s390_fix_watch_points. WPFIXED(Expand
> 
> Oh, then my assumption about watchpoint sizes would be wrong then
> wouldn't it? We then need to expand the ok_for_watchpoint as well as
> {insert|remove}_watchpoint. We could do it as a different changeset
> since only s390x would need it.

You are right.  Sorry I did not remember this s390x feature when we discussed
it before.


Thanks,
Jan

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Mon, 13 Aug 2012 15:49:15 +0200, Jan wrote:
> I was requesting only to remove the redundant line
> 	avr-tdep.c:1298:ensure_type_fits_sizet (type);
> 

OK, will do.

> ensure_type_fits_sizet uses only TYPE->LENGTH so it does not need to
> be passed TYPE at all.  It would make sense if ensure_type_fits_sizet
> prints TYPE_NAME in the error case etc. - but it does not use
> TYPE_NAME anyway..

Sure, I can do that.

> > Likewise for other tdep instances.
> 
> I do not see how it can be generalized, I may not just see what do
> you mean.

I meant the other instances that you pointed out as unnecessary -
sorry for the confusion.

> > > (breakpoint.c:4624):	FUNC():	(LONGEST to int)
> > > [loc->length]
> > >  - That's not true, s390x can set arbitrary hardware watchpoint
> > > memory range, see s390_fix_watch_points. WPFIXED(Expand
> > 
> > Oh, then my assumption about watchpoint sizes would be wrong then
> > wouldn't it? We then need to expand the ok_for_watchpoint as well as
> > {insert|remove}_watchpoint. We could do it as a different changeset
> > since only s390x would need it.
> 
> You are right.  Sorry I did not remember this s390x feature when we
> discussed it before.

No worries, I'll work on the watchpoint change once we get the rest of
the stuff in if it is OK. Not having it in this change itself is not
going to break something that is not already broken.

Regards,
Siddhesh

Re: bitpos expansion patches summary

[Jan Kratochvil]

On Mon, 13 Aug 2012 16:03:13 +0200, Siddhesh Poyarekar wrote:
> No worries, I'll work on the watchpoint change once we get the rest of
> the stuff in if it is OK. Not having it in this change itself is not
> going to break something that is not already broken.

I am reluctant to check in the LENGTH expansion before getting all the needed
pre-requisite bits fixed.  One cannot properly regression-splint-check it
afterwards anymore as HEAD is a moving target so it may start to be seriously
difficult to be tracking the changes happening between LENGTH expansion and
before all the other bits get it.  The splint results will have changing line
numbers etc.

Maybe it is possible to handle it somehow but I would find easier to check the
LENGTH expansion only as the last piece.


Thanks,
Jan

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Mon, 13 Aug 2012 16:12:36 +0200, Jan wrote:
> I am reluctant to check in the LENGTH expansion before getting all
> the needed pre-requisite bits fixed.  One cannot properly
> regression-splint-check it afterwards anymore as HEAD is a moving
> target so it may start to be seriously difficult to be tracking the
> changes happening between LENGTH expansion and before all the other
> bits get it.  The splint results will have changing line numbers etc.
> 
> Maybe it is possible to handle it somehow but I would find easier to
> check the LENGTH expansion only as the last piece.
> 

OK, I'll make a separate patch of it then, like I am doing for the
size_t bits.

Regards,
Siddhesh

[PATCH 4/3] bitpos: Expand parameters of watchpoint functions

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 3289 bytes --]

Hi,

The trilogy of patches now has a fourth part - watchpoint function
changes. These had to be written since Jan pointed out that s390x allows
arbitrary length watchpoints using a range. In fact, there are some
other architectures that seem to do this too.

The patch does not cause any regressions in the testsuite. It
introduces some new warnings in splint, but I found them to be safe,
since the watchpoint sizes in those cases would be bound to within
sizeof (int). E.g. i386 watchpoints.

Regards,
Siddhesh

gdb/ChangeLog:

	* arm-linux-nat.c (arm_linux_insert_watchpoint): Expand
	parameter LEN to LONGEST.
	(arm_linux_remove_watchpoint): Likewise.
	(arm_linux_watchpoint_addr_within_range): Expand parameter
	LENGTH to LONGEST.
	* i386-nat.c (i386_insert_watchpoint): Expand parameter LEN to
	LONGEST.
	(i386_remove_watchpoint): Likewise.
	* ia64-linux-nat.c (ia64_linux_insert_watchpoint): Likewise.
	(ia64_linux_remove_watchpoint): Likewise.
	* inf-ttrace.c (inf_ttrace_insert_watchpoint): Likewise.
	Expand NUM_PAGES, PAGE to LONGEST.
	(inf_ttrace_remove_watchpoint): Likewise.
	* mips-linux-nat.c (mips_linux_insert_watchpoint): Expand
	parameter LEN to LONGEST.
	(mips_linux_remove_watchpoint): Likewise.
	* nto-procfs.c (procfs_remove_hw_watchpoint): Likewise.
	(procfs_insert_hw_watchpoint): Likewise.
	* ppc-linux-nat.c (calculate_dvc): Likewise.  Expand I,
	NUM_BYTE_ENABLE to LONGEST.
	(check_condition): Expand parameter LEN to point to LONGEST.
	(ppc_linux_can_accel_watchpoint_condition): Expand parameter
	LEN to LONGEST.
	(create_watchpoint_request): Likewise.
	(ppc_linux_insert_watchpoint): Likewise.
	(ppc_linux_remove_watchpoint): Likewise.
	(ppc_linux_watchpoint_addr_within_range): Expand parameter
	LENGTH to LONGEST.
	* procfs.c (proc_set_watchpoint): Expand parameter LEN to
	LONGEST.
	(procfs_set_watchpoint): Likewise.
	(procfs_insert_watchpoint): Likewise.
	(procfs_remove_watchpoint): Likewise.
	* remote-m32r-sdi.c (m32r_insert_watchpoint): Likewise.  Use
	plongest to format print LEN.
	(m32r_remove_watchpoint): Likewise.
	* remote-mips.c (mips_insert_watchpoint): Expand parameter LEN
	to LONGEST.
	(mips_remove_watchpoint): Likewise.
	* remote.c (remote_insert_watchpoint): Likewise.
	Use phex_nz to format print LEN.
	(remote_remove_watchpoint): Likewise.
	(remote_watchpoint_addr_within_range): Expand parameter LENGTH
	to LONGEST.
	* s390-nat.c (s390_insert_watchpoint): Expand parameter LEN to
	LONGEST.
	(s390_remove_watchpoint): Likewise.
	* target.c (update_current_target): Expand parameter LEN for
	callbacks to TO_INSERT_WATCHPOINT, TO_REMOVE_WATCHPOINT,
	TO_CAN_ACCEL_WATCHPOINT_CONDITION, to LONGEST.
	(default_watchpoint_addr_within_range): Expand parameter
	LENGTH to LONGEST.
	(debug_to_can_accel_watchpoint_condition): Expand parameter LEN
	to LONGEST.  Use plongest to format print LEN.
	(debug_to_watchpoint_addr_within_range): Expand parameter LENGTH
	to LONGEST.  Use plongest to format print LENGTH.
	(debug_to_insert_watchpoint): Expand parameter LEN to LONGEST.
	Use plongest to format print LEN.
	(debug_to_remove_watchpoint): Likewise.
	* target.h (struct target_ops): Expand parameter LEN of
	TO_REMOVE_WATCHPOINT, TO_INSERT_WATCHPOINT,
	TO_WATCHPOINT_ADDR_WITHIN_RANGE and
	TO_CAN_ACCEL_WATCHPOINT_CONDITION to LONGEST.

[-- Attachment #2: bitpos-expand-watchpoint.patch --]
[-- Type: text/x-patch, Size: 21982 bytes --]

diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index 231b008..6deb23d 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -1105,7 +1105,7 @@ arm_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, LONGEST len)
 
 /* Insert a Hardware breakpoint.  */
 static int
-arm_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
+arm_linux_insert_watchpoint (CORE_ADDR addr, LONGEST len, int rw,
 			     struct expression *cond)
 {
   struct lwp_info *lp;
@@ -1123,7 +1123,7 @@ arm_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
 
 /* Remove a hardware breakpoint.  */
 static int
-arm_linux_remove_watchpoint (CORE_ADDR addr, int len, int rw,
+arm_linux_remove_watchpoint (CORE_ADDR addr, LONGEST len, int rw,
 			     struct expression *cond)
 {
   struct lwp_info *lp;
@@ -1180,7 +1180,7 @@ arm_linux_stopped_by_watchpoint (void)
 static int
 arm_linux_watchpoint_addr_within_range (struct target_ops *target,
 					CORE_ADDR addr,
-					CORE_ADDR start, int length)
+					CORE_ADDR start, LONGEST length)
 {
   return start <= addr && start + length - 1 >= addr;
 }
diff --git a/gdb/i386-nat.c b/gdb/i386-nat.c
index a10dca0..a36e84a 100644
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -592,7 +592,7 @@ i386_update_inferior_debug_regs (struct i386_debug_reg_state *new_state)
    of the type TYPE.  Return 0 on success, -1 on failure.  */
 
 static int
-i386_insert_watchpoint (CORE_ADDR addr, int len, int type,
+i386_insert_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			struct expression *cond)
 {
   struct i386_debug_reg_state *state = i386_debug_reg_state ();
@@ -629,7 +629,7 @@ i386_insert_watchpoint (CORE_ADDR addr, int len, int type,
    address ADDR, whose length is LEN bytes, and for accesses of the
    type TYPE.  Return 0 on success, -1 on failure.  */
 static int
-i386_remove_watchpoint (CORE_ADDR addr, int len, int type,
+i386_remove_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			struct expression *cond)
 {
   struct i386_debug_reg_state *state = i386_debug_reg_state ();
diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c
index 9b5fbf3..6061eae 100644
--- a/gdb/ia64-linux-nat.c
+++ b/gdb/ia64-linux-nat.c
@@ -542,7 +542,7 @@ is_power_of_2 (int val)
 }
 
 static int
-ia64_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
+ia64_linux_insert_watchpoint (CORE_ADDR addr, LONGEST len, int rw,
 			      struct expression *cond)
 {
   struct lwp_info *lp;
@@ -596,7 +596,7 @@ ia64_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
 }
 
 static int
-ia64_linux_remove_watchpoint (CORE_ADDR addr, int len, int type,
+ia64_linux_remove_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			      struct expression *cond)
 {
   int idx;
diff --git a/gdb/inf-ttrace.c b/gdb/inf-ttrace.c
index d60eddb..c33db45 100644
--- a/gdb/inf-ttrace.c
+++ b/gdb/inf-ttrace.c
@@ -313,14 +313,14 @@ inf_ttrace_disable_page_protections (pid_t pid)
    type TYPE.  */
 
 static int
-inf_ttrace_insert_watchpoint (CORE_ADDR addr, int len, int type,
+inf_ttrace_insert_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			      struct expression *cond)
 {
   const int pagesize = inf_ttrace_page_dict.pagesize;
   pid_t pid = ptid_get_pid (inferior_ptid);
   CORE_ADDR page_addr;
-  int num_pages;
-  int page;
+  LONGEST num_pages;
+  LONGEST page;
 
   gdb_assert (type == hw_write);
 
@@ -337,14 +337,14 @@ inf_ttrace_insert_watchpoint (CORE_ADDR addr, int len, int type,
    type TYPE.  */
 
 static int
-inf_ttrace_remove_watchpoint (CORE_ADDR addr, int len, int type,
+inf_ttrace_remove_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			      struct expression *cond)
 {
   const int pagesize = inf_ttrace_page_dict.pagesize;
   pid_t pid = ptid_get_pid (inferior_ptid);
   CORE_ADDR page_addr;
-  int num_pages;
-  int page;
+  LONGEST num_pages;
+  LONGEST page;
 
   gdb_assert (type == hw_write);
 
diff --git a/gdb/mips-linux-nat.c b/gdb/mips-linux-nat.c
index 5566d0c..7467d11 100644
--- a/gdb/mips-linux-nat.c
+++ b/gdb/mips-linux-nat.c
@@ -1017,7 +1017,7 @@ populate_regs_from_watches (struct pt_watch_regs *regs)
    watch.  Return zero on success.  */
 
 static int
-mips_linux_insert_watchpoint (CORE_ADDR addr, int len, int type,
+mips_linux_insert_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			      struct expression *cond)
 {
   struct pt_watch_regs regs;
@@ -1067,7 +1067,7 @@ mips_linux_insert_watchpoint (CORE_ADDR addr, int len, int type,
    Return zero on success.  */
 
 static int
-mips_linux_remove_watchpoint (CORE_ADDR addr, int len, int type,
+mips_linux_remove_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			      struct expression *cond)
 {
   int retval;
diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
index b58f318..25fecf3 100644
--- a/gdb/nto-procfs.c
+++ b/gdb/nto-procfs.c
@@ -69,10 +69,10 @@ static ptid_t do_attach (ptid_t ptid);
 
 static int procfs_can_use_hw_breakpoint (int, int, int);
 
-static int procfs_insert_hw_watchpoint (CORE_ADDR addr, int len, int type,
+static int procfs_insert_hw_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 					struct expression *cond);
 
-static int procfs_remove_hw_watchpoint (CORE_ADDR addr, int len, int type,
+static int procfs_remove_hw_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 					struct expression *cond);
 
 static int procfs_stopped_by_watchpoint (void);
@@ -1493,14 +1493,14 @@ procfs_can_use_hw_breakpoint (int type, int cnt, int othertype)
 }
 
 static int
-procfs_remove_hw_watchpoint (CORE_ADDR addr, int len, int type,
+procfs_remove_hw_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			     struct expression *cond)
 {
   return procfs_hw_watchpoint (addr, -1, type);
 }
 
 static int
-procfs_insert_hw_watchpoint (CORE_ADDR addr, int len, int type,
+procfs_insert_hw_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			     struct expression *cond)
 {
   return procfs_hw_watchpoint (addr, len, type);
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 67e1cac..abfb2fc 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1839,11 +1839,11 @@ can_use_watchpoint_cond_accel (void)
    CONDITION_VALUE will hold the value which should be put in the
    DVC register.  */
 static void
-calculate_dvc (CORE_ADDR addr, int len, CORE_ADDR data_value,
+calculate_dvc (CORE_ADDR addr, LONGEST len, CORE_ADDR data_value,
 	       uint32_t *condition_mode, uint64_t *condition_value)
 {
-  int i, num_byte_enable, align_offset, num_bytes_off_dvc,
-      rightmost_enabled_byte;
+  LONGEST i, num_byte_enable;
+  int align_offset, num_bytes_off_dvc, rightmost_enabled_byte;
   CORE_ADDR addr_end_data, addr_end_dvc;
 
   /* The DVC register compares bytes within fixed-length windows which
@@ -1930,7 +1930,7 @@ num_memory_accesses (struct value *v)
    of the constant.  */
 static int
 check_condition (CORE_ADDR watch_addr, struct expression *cond,
-		 CORE_ADDR *data_value, int *len)
+		 CORE_ADDR *data_value, LONGEST *len)
 {
   int pc = 1, num_accesses_left, num_accesses_right;
   struct value *left_val, *right_val, *left_chain, *right_chain;
@@ -1997,7 +1997,7 @@ check_condition (CORE_ADDR watch_addr, struct expression *cond,
    the condition expression, thus only triggering the watchpoint when it is
    true.  */
 static int
-ppc_linux_can_accel_watchpoint_condition (CORE_ADDR addr, int len, int rw,
+ppc_linux_can_accel_watchpoint_condition (CORE_ADDR addr, LONGEST len, int rw,
 					  struct expression *cond)
 {
   CORE_ADDR data_value;
@@ -2014,7 +2014,7 @@ ppc_linux_can_accel_watchpoint_condition (CORE_ADDR addr, int len, int rw,
 
 static void
 create_watchpoint_request (struct ppc_hw_breakpoint *p, CORE_ADDR addr,
-			   int len, int rw, struct expression *cond,
+			   LONGEST len, int rw, struct expression *cond,
 			   int insert)
 {
   if (len == 1
@@ -2059,7 +2059,7 @@ create_watchpoint_request (struct ppc_hw_breakpoint *p, CORE_ADDR addr,
 }
 
 static int
-ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
+ppc_linux_insert_watchpoint (CORE_ADDR addr, LONGEST len, int rw,
 			     struct expression *cond)
 {
   struct lwp_info *lp;
@@ -2127,7 +2127,7 @@ ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
 }
 
 static int
-ppc_linux_remove_watchpoint (CORE_ADDR addr, int len, int rw,
+ppc_linux_remove_watchpoint (CORE_ADDR addr, LONGEST len, int rw,
 			     struct expression *cond)
 {
   struct lwp_info *lp;
@@ -2267,7 +2267,7 @@ ppc_linux_stopped_by_watchpoint (void)
 static int
 ppc_linux_watchpoint_addr_within_range (struct target_ops *target,
 					CORE_ADDR addr,
-					CORE_ADDR start, int length)
+					CORE_ADDR start, LONGEST length)
 {
   int mask;
 
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 2524564..b8e9980 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -2469,7 +2469,7 @@ procfs_address_to_host_pointer (CORE_ADDR addr)
 #endif
 
 static int
-proc_set_watchpoint (procinfo *pi, CORE_ADDR addr, int len, int wflags)
+proc_set_watchpoint (procinfo *pi, CORE_ADDR addr, LONGEST len, int wflags)
 {
 #if !defined (PCWATCH) && !defined (PIOCSWATCH)
   /* If neither or these is defined, we can't support watchpoints.
@@ -4815,7 +4815,7 @@ procfs_pid_to_str (struct target_ops *ops, ptid_t ptid)
 /* Insert a watchpoint.  */
 
 static int
-procfs_set_watchpoint (ptid_t ptid, CORE_ADDR addr, int len, int rwflag,
+procfs_set_watchpoint (ptid_t ptid, CORE_ADDR addr, LONGEST len, int rwflag,
 		       int after)
 {
 #ifndef UNIXWARE
@@ -4937,7 +4937,7 @@ procfs_stopped_data_address (struct target_ops *targ, CORE_ADDR *addr)
 }
 
 static int
-procfs_insert_watchpoint (CORE_ADDR addr, int len, int type,
+procfs_insert_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			  struct expression *cond)
 {
   if (!target_have_steppable_watchpoint
@@ -4959,7 +4959,7 @@ procfs_insert_watchpoint (CORE_ADDR addr, int len, int type,
 }
 
 static int
-procfs_remove_watchpoint (CORE_ADDR addr, int len, int type,
+procfs_remove_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			  struct expression *cond)
 {
   return procfs_set_watchpoint (inferior_ptid, addr, 0, 0, 0);
diff --git a/gdb/remote-m32r-sdi.c b/gdb/remote-m32r-sdi.c
index 85268b6..a688ecc 100644
--- a/gdb/remote-m32r-sdi.c
+++ b/gdb/remote-m32r-sdi.c
@@ -1417,14 +1417,14 @@ m32r_can_use_hw_watchpoint (int type, int cnt, int othertype)
    watchpoint.  */
 
 static int
-m32r_insert_watchpoint (CORE_ADDR addr, int len, int type,
+m32r_insert_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			struct expression *cond)
 {
   int i;
 
   if (remote_debug)
-    fprintf_unfiltered (gdb_stdlog, "m32r_insert_watchpoint(%s,%d,%d)\n",
-			paddress (target_gdbarch, addr), len, type);
+    fprintf_unfiltered (gdb_stdlog, "m32r_insert_watchpoint(%s,%s,%d)\n",
+			paddress (target_gdbarch, addr), plongest (len), type);
 
   for (i = 0; i < MAX_ACCESS_BREAKS; i++)
     {
@@ -1442,14 +1442,14 @@ m32r_insert_watchpoint (CORE_ADDR addr, int len, int type,
 }
 
 static int
-m32r_remove_watchpoint (CORE_ADDR addr, int len, int type,
+m32r_remove_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			struct expression *cond)
 {
   int i;
 
   if (remote_debug)
-    fprintf_unfiltered (gdb_stdlog, "m32r_remove_watchpoint(%s,%d,%d)\n",
-			paddress (target_gdbarch, addr), len, type);
+    fprintf_unfiltered (gdb_stdlog, "m32r_remove_watchpoint(%s,%s,%d)\n",
+			paddress (target_gdbarch, addr), plongest (len), type);
 
   for (i = 0; i < MAX_ACCESS_BREAKS; i++)
     {
diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c
index db4381b..d6aa5ce 100644
--- a/gdb/remote-mips.c
+++ b/gdb/remote-mips.c
@@ -2418,7 +2418,7 @@ calculate_mask (CORE_ADDR addr, int len)
    watchpoint.  */
 
 static int
-mips_insert_watchpoint (CORE_ADDR addr, int len, int type,
+mips_insert_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			struct expression *cond)
 {
   if (mips_set_breakpoint (addr, len, type))
@@ -2430,7 +2430,7 @@ mips_insert_watchpoint (CORE_ADDR addr, int len, int type,
 /* Remove a watchpoint.  */
 
 static int
-mips_remove_watchpoint (CORE_ADDR addr, int len, int type,
+mips_remove_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			struct expression *cond)
 {
   if (mips_clear_breakpoint (addr, len, type))
diff --git a/gdb/remote.c b/gdb/remote.c
index 7ec2e7a..c6501ed 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8033,7 +8033,7 @@ watchpoint_to_Z_packet (int type)
 }
 
 static int
-remote_insert_watchpoint (CORE_ADDR addr, int len, int type,
+remote_insert_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			  struct expression *cond)
 {
   struct remote_state *rs = get_remote_state ();
@@ -8048,7 +8048,7 @@ remote_insert_watchpoint (CORE_ADDR addr, int len, int type,
   p = strchr (rs->buf, '\0');
   addr = remote_address_masked (addr);
   p += hexnumstr (p, (ULONGEST) addr);
-  xsnprintf (p, endbuf - p, ",%x", len);
+  xsnprintf (p, endbuf - p, ",%s", phex_nz (len, sizeof (len)));
 
   putpkt (rs->buf);
   getpkt (&rs->buf, &rs->buf_size, 0);
@@ -8068,7 +8068,7 @@ remote_insert_watchpoint (CORE_ADDR addr, int len, int type,
 
 static int
 remote_watchpoint_addr_within_range (struct target_ops *target, CORE_ADDR addr,
-				     CORE_ADDR start, int length)
+				     CORE_ADDR start, LONGEST length)
 {
   CORE_ADDR diff = remote_address_masked (addr - start);
 
@@ -8077,7 +8077,7 @@ remote_watchpoint_addr_within_range (struct target_ops *target, CORE_ADDR addr,
 
 
 static int
-remote_remove_watchpoint (CORE_ADDR addr, int len, int type,
+remote_remove_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			  struct expression *cond)
 {
   struct remote_state *rs = get_remote_state ();
@@ -8092,7 +8092,7 @@ remote_remove_watchpoint (CORE_ADDR addr, int len, int type,
   p = strchr (rs->buf, '\0');
   addr = remote_address_masked (addr);
   p += hexnumstr (p, (ULONGEST) addr);
-  xsnprintf (p, endbuf - p, ",%x", len);
+  xsnprintf (p, endbuf - p, ",%s", phex_nz (len, sizeof (len)));
   putpkt (rs->buf);
   getpkt (&rs->buf, &rs->buf_size, 0);
 
diff --git a/gdb/s390-nat.c b/gdb/s390-nat.c
index 4974bad..3f41519 100644
--- a/gdb/s390-nat.c
+++ b/gdb/s390-nat.c
@@ -517,7 +517,7 @@ s390_fix_watch_points (struct lwp_info *lp)
 }
 
 static int
-s390_insert_watchpoint (CORE_ADDR addr, int len, int type,
+s390_insert_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			struct expression *cond)
 {
   struct lwp_info *lp;
@@ -538,7 +538,7 @@ s390_insert_watchpoint (CORE_ADDR addr, int len, int type,
 }
 
 static int
-s390_remove_watchpoint (CORE_ADDR addr, int len, int type,
+s390_remove_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			struct expression *cond)
 {
   struct lwp_info *lp;
diff --git a/gdb/target.c b/gdb/target.c
index f7207c0..a69fb06 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -49,7 +49,8 @@ static void target_info (char *, int);
 static void default_terminal_info (char *, int);
 
 static int default_watchpoint_addr_within_range (struct target_ops *,
-						 CORE_ADDR, CORE_ADDR, int);
+						 CORE_ADDR, CORE_ADDR,
+						 LONGEST);
 
 static int default_region_ok_for_hw_watchpoint (CORE_ADDR, LONGEST);
 
@@ -114,10 +115,10 @@ static int debug_to_insert_hw_breakpoint (struct gdbarch *,
 static int debug_to_remove_hw_breakpoint (struct gdbarch *,
 					  struct bp_target_info *);
 
-static int debug_to_insert_watchpoint (CORE_ADDR, int, int,
+static int debug_to_insert_watchpoint (CORE_ADDR, LONGEST, int,
 				       struct expression *);
 
-static int debug_to_remove_watchpoint (CORE_ADDR, int, int,
+static int debug_to_remove_watchpoint (CORE_ADDR, LONGEST, int,
 				       struct expression *);
 
 static int debug_to_stopped_by_watchpoint (void);
@@ -125,11 +126,12 @@ static int debug_to_stopped_by_watchpoint (void);
 static int debug_to_stopped_data_address (struct target_ops *, CORE_ADDR *);
 
 static int debug_to_watchpoint_addr_within_range (struct target_ops *,
-						  CORE_ADDR, CORE_ADDR, int);
+						  CORE_ADDR, CORE_ADDR,
+						  LONGEST);
 
 static int debug_to_region_ok_for_hw_watchpoint (CORE_ADDR, LONGEST);
 
-static int debug_to_can_accel_watchpoint_condition (CORE_ADDR, int, int,
+static int debug_to_can_accel_watchpoint_condition (CORE_ADDR, LONGEST, int,
 						    struct expression *);
 
 static void debug_to_terminal_init (void);
@@ -751,10 +753,10 @@ update_current_target (void)
 	    (int (*) (struct gdbarch *, struct bp_target_info *))
 	    return_minus_one);
   de_fault (to_insert_watchpoint,
-	    (int (*) (CORE_ADDR, int, int, struct expression *))
+	    (int (*) (CORE_ADDR, LONGEST, int, struct expression *))
 	    return_minus_one);
   de_fault (to_remove_watchpoint,
-	    (int (*) (CORE_ADDR, int, int, struct expression *))
+	    (int (*) (CORE_ADDR, LONGEST, int, struct expression *))
 	    return_minus_one);
   de_fault (to_stopped_by_watchpoint,
 	    (int (*) (void))
@@ -767,7 +769,7 @@ update_current_target (void)
   de_fault (to_region_ok_for_hw_watchpoint,
 	    default_region_ok_for_hw_watchpoint);
   de_fault (to_can_accel_watchpoint_condition,
-            (int (*) (CORE_ADDR, int, int, struct expression *))
+            (int (*) (CORE_ADDR, LONGEST, int, struct expression *))
             return_zero);
   de_fault (to_terminal_init,
 	    (void (*) (void))
@@ -3558,7 +3560,7 @@ default_region_ok_for_hw_watchpoint (CORE_ADDR addr, LONGEST len)
 static int
 default_watchpoint_addr_within_range (struct target_ops *target,
 				      CORE_ADDR addr,
-				      CORE_ADDR start, int length)
+				      CORE_ADDR start, LONGEST length)
 {
   return addr >= start && addr < start + length;
 }
@@ -4263,7 +4265,7 @@ debug_to_region_ok_for_hw_watchpoint (CORE_ADDR addr, LONGEST len)
 }
 
 static int
-debug_to_can_accel_watchpoint_condition (CORE_ADDR addr, int len, int rw,
+debug_to_can_accel_watchpoint_condition (CORE_ADDR addr, LONGEST len, int rw,
 					 struct expression *cond)
 {
   int retval;
@@ -4273,8 +4275,8 @@ debug_to_can_accel_watchpoint_condition (CORE_ADDR addr, int len, int rw,
 
   fprintf_unfiltered (gdb_stdlog,
 		      "target_can_accel_watchpoint_condition "
-		      "(%s, %d, %d, %s) = %ld\n",
-		      core_addr_to_string (addr), len, rw,
+		      "(%s, %s, %d, %s) = %ld\n",
+		      core_addr_to_string (addr), plongest (len), rw,
 		      host_address_to_string (cond), (unsigned long) retval);
   return retval;
 }
@@ -4309,7 +4311,7 @@ debug_to_stopped_data_address (struct target_ops *target, CORE_ADDR *addr)
 static int
 debug_to_watchpoint_addr_within_range (struct target_ops *target,
 				       CORE_ADDR addr,
-				       CORE_ADDR start, int length)
+				       CORE_ADDR start, LONGEST length)
 {
   int retval;
 
@@ -4317,9 +4319,9 @@ debug_to_watchpoint_addr_within_range (struct target_ops *target,
 							 start, length);
 
   fprintf_filtered (gdb_stdlog,
-		    "target_watchpoint_addr_within_range (%s, %s, %d) = %d\n",
+		    "target_watchpoint_addr_within_range (%s, %s, %s) = %d\n",
 		    core_addr_to_string (addr), core_addr_to_string (start),
-		    length, retval);
+		    plongest (length), retval);
   return retval;
 }
 
@@ -4354,7 +4356,7 @@ debug_to_remove_hw_breakpoint (struct gdbarch *gdbarch,
 }
 
 static int
-debug_to_insert_watchpoint (CORE_ADDR addr, int len, int type,
+debug_to_insert_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			    struct expression *cond)
 {
   int retval;
@@ -4362,14 +4364,14 @@ debug_to_insert_watchpoint (CORE_ADDR addr, int len, int type,
   retval = debug_target.to_insert_watchpoint (addr, len, type, cond);
 
   fprintf_unfiltered (gdb_stdlog,
-		      "target_insert_watchpoint (%s, %d, %d, %s) = %ld\n",
-		      core_addr_to_string (addr), len, type,
+		      "target_insert_watchpoint (%s, %s, %d, %s) = %ld\n",
+		      core_addr_to_string (addr), plongest (len), type,
 		      host_address_to_string (cond), (unsigned long) retval);
   return retval;
 }
 
 static int
-debug_to_remove_watchpoint (CORE_ADDR addr, int len, int type,
+debug_to_remove_watchpoint (CORE_ADDR addr, LONGEST len, int type,
 			    struct expression *cond)
 {
   int retval;
@@ -4377,8 +4379,8 @@ debug_to_remove_watchpoint (CORE_ADDR addr, int len, int type,
   retval = debug_target.to_remove_watchpoint (addr, len, type, cond);
 
   fprintf_unfiltered (gdb_stdlog,
-		      "target_remove_watchpoint (%s, %d, %d, %s) = %ld\n",
-		      core_addr_to_string (addr), len, type,
+		      "target_remove_watchpoint (%s, %s, %d, %s) = %ld\n",
+		      core_addr_to_string (addr), plongest (len), type,
 		      host_address_to_string (cond), (unsigned long) retval);
   return retval;
 }
diff --git a/gdb/target.h b/gdb/target.h
index 8c0f919..dca627e 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -466,8 +466,8 @@ struct target_ops
 
     /* Documentation of what the two routines below are expected to do is
        provided with the corresponding target_* macros.  */
-    int (*to_remove_watchpoint) (CORE_ADDR, int, int, struct expression *);
-    int (*to_insert_watchpoint) (CORE_ADDR, int, int, struct expression *);
+    int (*to_remove_watchpoint) (CORE_ADDR, LONGEST, int, struct expression *);
+    int (*to_insert_watchpoint) (CORE_ADDR, LONGEST, int, struct expression *);
 
     int (*to_insert_mask_watchpoint) (struct target_ops *,
 				      CORE_ADDR, CORE_ADDR, int);
@@ -478,13 +478,13 @@ struct target_ops
     int to_have_continuable_watchpoint;
     int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
     int (*to_watchpoint_addr_within_range) (struct target_ops *,
-					    CORE_ADDR, CORE_ADDR, int);
+					    CORE_ADDR, CORE_ADDR, LONGEST);
 
     /* Documentation of this routine is provided with the corresponding
        target_* macro.  */
     int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR, LONGEST);
 
-    int (*to_can_accel_watchpoint_condition) (CORE_ADDR, int, int,
+    int (*to_can_accel_watchpoint_condition) (CORE_ADDR, LONGEST, int,
 					      struct expression *);
     int (*to_masked_watch_num_registers) (struct target_ops *,
 					  CORE_ADDR, CORE_ADDR);

Re: bitpos expansion patches summary

[Jan Kratochvil]

Hi Siddhesh,

another part of the review.


Thanks,
Jan


RSAFE: (dwarf2loc.c:2257):	FUNC(memcpy):	(ULONGEST to size_t)	[n]
SAFE(value_contents_raw ensures size_t): (dwarf2loc.c:2277):	CMP:	(ULONGEST to size_t)	[n > (type)->length]
SAFE(value_contents_raw ensures size_t): (dwarf2loc.c:2283):	ASSIGN:	(ULONGEST to size_t)	[ n = (type)->length]
RSAFE: (dwarf2read.c:7340):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
FIXED(Expand bit_offset): (dwarf2read.c:9847):	VARINIT(bit_offset):	(ULONGEST to int)	[((attr)->u.unsnd)]
SAFE(Pointer Type): (dwarf2read.c:11298):	CMP:	(int to ULONGEST)	[(type)->length != byte_size]
SAFE(Pointer Type): (dwarf2read.c:11310):	CMP:	(int to ULONGEST)	[(type)->length != byte_size]
SAFE: (dwarf2read.c:11376):	ASSIGN:	(UCHAR to ULONGEST)	[ (type)->length = cu_header->addr_size]
SAFE: (dwarf2read.c:11888):	CMP:	(int to ULONGEST)	[(int_type)->length >= addr_size]
SAFE: (dwarf2read.c:11893):	CMP:	(int to ULONGEST)	[(int_type)->length >= addr_size]
SAFE: (dwarf2read.c:11898):	CMP:	(int to ULONGEST)	[(int_type)->length >= addr_size]
SAFE: (dwarf2read.c:15218):	CMP:	(UCHAR to ULONGEST)	[(type)->length != cu_header->addr_size]
SAFE: (dwarf2read.c:15220):	FUNC(dwarf2_const_value_length_mismatch_complaint):	(UCHAR to LONGEST)	[cu_header->addr_size]
SAFE: (dwarf2read.c:15254):	CMP:	(size_t to ULONGEST)	[(type)->length != blk->size]
SAFE: (elfread.c:628):	VARINIT(ptr_size):	(ULONGEST to size_t)	[(ptr_type)->length]
SAFE: (elfread.c:869):	VARINIT(ptr_size):	(ULONGEST to size_t)	[(ptr_type)->length]
RSAFE: (eval.c:446):	FUNC(memcpy):	(ULONGEST to size_t)	[(value_type(val))->length]
RSAFE: (eval.c:489):	FUNC(memcpy):	(LONGEST to size_t)	[element_size]
RSAFE: (eval.c:498):	FUNC(memcpy):	(LONGEST to size_t)	[element_size]
SAFE: (eval.c:611):	CMP:	(int to ULONGEST)	[(type1)->length * 8 > gdbarch_double_bit(gdbarch)]
SAFE: (eval.c:612):	CMP:	(int to ULONGEST)	[(type2)->length * 8 > gdbarch_double_bit(gdbarch)]
SAFE: (eval.c:715):	CMP:	(int to ULONGEST)	[result_len > gdbarch_long_bit(gdbarch) / 8]
SAFE: (eval.c:722):	CMP:	(int to ULONGEST)	[result_len > gdbarch_long_bit(gdbarch) / 8]
RSAFE: (eval.c:996):	FUNC(memset):	(ULONGEST to size_t)	[(type)->length]
RSAFE: (eval.c:1015):	FUNC(memset):	(ULONGEST to size_t)	[(expect_type)->length]
RSAFE: (eval.c:1046):	FUNC(memcpy):	(LONGEST to size_t)	[element_size]
RSAFE: (eval.c:1069):	FUNC(memset):	(ULONGEST to size_t)	[(type)->length]
SEPARATE(Ensure that the value fits into CORE_ADDR): (eval.c:2075):	FUNC(value_from_pointer):	(LONGEST to CORE_ADDR)	[value_as_long(arg1) + mem_offset]
 - I do not understand this, you have correctly made 'long->LONGEST mem_offset;', I find it enough.
SAFE: (findcmd.c:184):	CMP:	(size_t to LONGEST)	[(val_bytes) > (sizeof(int64_t))]
 - (new entry) I miss here findcmd.c:190 fix, xrealloc 
   ULONGEST pattern_buf_size;
   pattern_buf = xrealloc (pattern_buf, pattern_buf_size);
   That second xrealloc parameter may overflow, there should be ENSURED_SIZET.
ENSURED_SIZET: (findcmd.c:218):	FUNC(memcpy):	(LONGEST to size_t)	[val_bytes]
 - Not needed, there is already called 'value_contents (v)'.
RSAFE: (findvar.c:493):	FUNC(memcpy):	(LONGEST to size_t)	[len]
RSAFE: (frame.c:872):	FUNC(memcpy):	(ULONGEST to size_t)	[(value_type(value))->length]
RSAFE: (frame.c:874):	FUNC(memset):	(ULONGEST to size_t)	[(value_type(value))->length]
UNRELATED(Expand return value): (f-valprint.c:85):	RET:	(LONGEST to int)	[((((((type))->main_type->flds_bnds.fields[0]).type))->main_type->flds_bnds.bounds->high)]
 - I agree unrelated but when found it should be fixed in a separate patch.
FIXED(Expand i): (f-valprint.c:178):	CMP:	(LONGEST to int)	[i < (f77_array_offset_tbl[nss][1])]
FIXED(Expand i): (f-valprint.c:189):	CMP:	(LONGEST to int)	[i < (f77_array_offset_tbl[nss][1])]
FIXED(Expand i): (f-valprint.c:194):	CMP:	(LONGEST to int)	[i < (f77_array_offset_tbl[nss][1])]
FIXED(Expand i): (f-valprint.c:203):	CMP:	(LONGEST to int)	[i != ((f77_array_offset_tbl[nss][1]) - 1)]
FIXED(Expand i): (f-valprint.c:207):	CMP:	(LONGEST to int)	[i != ((f77_array_offset_tbl[nss][1]) - 1)]
SAFE: (gdbtypes.c:841):	CMP:	(size_t to ULONGEST)	[(type)->length > sizeof(LONGEST)]
SAFE: (gdbtypes.c:3718):	ASSIGN:	(LONGEST to int)	[ left = (((f[0]).loc.bitpos) + 0) % alignment]
FIXED(Expand embedded_offset): (go-valprint.c:107):	FUNC(print_go_string):	(LONGEST to int)	[embedded_offset]
 - But print_go_string does not have embedded_offset expanded - failure of splint re-run.
UNSAFE_ALLOCA: (h8300-tdep.c:675):	FUNC(C_alloca):	(LONGEST to size_t)	[padded_len]
 - There should be alloca fix again with xmalloc and cleanup.
ENSURED_SIZET: (h8300-tdep.c:677):	FUNC(memset):	(LONGEST to size_t)	[padded_len]
 - This is redundant, there is already above it:
   char *contents = (char *) value_contents (args[argument]);
ENSURED_SIZET: (h8300-tdep.c:679):	FUNC(memcpy):	(LONGEST to size_t)	[len]
ENSURED_SIZET: (h8300-tdep.c:689):	FUNC(write_memory):	(LONGEST to ssize_t)	[padded_len]
ENSURED_SIZET: (h8300-tdep.c:716):	FUNC(write_memory):	(LONGEST to ssize_t)	[padded_len]
FIXED(Expand len): (h8300-tdep.c:747):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Likewise): (h8300-tdep.c:784):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Likewise): (h8300-tdep.c:851):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Likewise): (h8300-tdep.c:881):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
ENSURED_SIZET: (h8300-tdep.c:928):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
 - Redundant, caller must have provided large enough READBUF.

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Sun, 19 Aug 2012 18:41:47 +0200, Jan wrote:
> SEPARATE(Ensure that the
> value fits into CORE_ADDR): (eval.c:2075):
> FUNC(value_from_pointer):	(LONGEST to CORE_ADDR)
> [value_as_long(arg1) + mem_offset]
>  - I do not understand this, you have correctly made 'long->LONGEST
> mem_offset;', I find it enough.

You're right, I can't remember what I was thinking.

> ENSURED_SIZET: (h8300-tdep.c:677):	FUNC(memset):
> (LONGEST to size_t)	[padded_len]
>  - This is redundant, there is already above it:
>    char *contents = (char *) value_contents (args[argument]);

Already removed in the latest patch.

I'll work on the rest.

Thanks,
Siddhesh

Re: bitpos expansion patches summary

[Jan Kratochvil]

Hi Siddhesh,

another part of the review.


Thanks,
Jan


SAFE: (hppa-tdep.c:740):	FUNC(write_memory):	(ULONGEST to ssize_t)	[(type)->length]
RSAFE: (hppa-tdep.c:766):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
SAFE(Bound value): (hppa-tdep.c:867):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE(Bound value): (hppa-tdep.c:889):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (hppa-tdep.c:1026):	FUNC(regcache_cooked_write_part):	(LONGEST to int)	[len]
SAFE: (hppa-tdep.c:1062):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (hppa-tdep.c:1068):	FUNC(regcache_cooked_write_part):	(LONGEST to int)	[((len) < (8) ? (len) : (8))]
SAFE: (hppa-tdep.c:1126):	VARINIT(part):	(ULONGEST to int)	[(type)->length % 4]
SAFE: (hppa-tdep.c:1141):	CMP:	(ULONGEST to int)	[b < (type)->length]
SAFE: (hppa-tdep.c:1181):	ASSIGN:	(LONGEST to int)	[ offset = 8 - len]
SAFE: (hppa-tdep.c:1203):	ASSIGN:	(LONGEST to int)	[ offset = 8 - len]
SAFE: (hppa-tdep.c:1225):	FUNC(regcache_cooked_read_part):	(LONGEST to int)	[((len) < (8) ? (len) : (8))]
SAFE: (hppa-tdep.c:1237):	FUNC(regcache_cooked_write_part):	(LONGEST to int)	[((len) < (8) ? (len) : (8))]
WPREVERTED(Only ok_for_watchpoint needs LONGEST): (i386-nat.c:613):	FUNC(i386_length_and_rw_bits):	(LONGEST to int)	[len]
 - To be re-checked with updated watchpoints patch.
WPREVERTED(Likewise): (i386-nat.c:647):	FUNC(i386_length_and_rw_bits):	(LONGEST to int)	[len]
 - To be re-checked with updated watchpoints patch.
SAFE: (i386-tdep.c:2394):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
RSAFE: (i386-tdep.c:2470):	FUNC(memset):	(LONGEST to size_t)	[len]
RSAFE: (i386-tdep.c:2489):	FUNC(memcpy):	(LONGEST to size_t)	[len]
RSAFE: (i386-tdep.c:2496):	FUNC(memcpy):	(LONGEST to size_t)	[len - low_size]
SAFE: (i386-tdep.c:2556):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[len]
SAFE: (i386-tdep.c:2561):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[len - low_size]
FIXED(Expand len): (i386-tdep.c:2594):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (i386-tdep.c:2659):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
FIXED(Expand len): (i386-tdep.c:3046):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Expand len): (i386-tdep.c:3079):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Expand len): (i386-tdep.c:3115):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (i387-tdep.c:381):	FUNC(get_frame_register_bytes):	(ULONGEST to int)	[(type)->length]
FIXED(Expand n): (ia64-tdep.c:3234):	VARINIT(n):	(ULONGEST to int)	[(type)->length / (float_elt_type)->length]
SAFE: (ia64-tdep.c:3265):	VARINIT(reglen):	(ULONGEST to int)	[(register_type(gdbarch, (0 + 8)))->length]
SAFE: (ia64-tdep.c:3267):	VARINIT(m):	(ULONGEST to int)	[(type)->length % reglen]
SAFE: (ia64-tdep.c:3299):	VARINIT(n):	(ULONGEST to int)	[(type)->length / (float_elt_type)->length]
 - You did expand n in ia64_extract_return_value, it can be array of floats.
   I did not investigate what happens if the array of floats is too large and
   it does not fit into ia64 registers.  But I find safe to just expand it.
SAFE: (ia64-tdep.c:3315):	VARINIT(reglen):	(ULONGEST to int)	[(register_type(gdbarch, (0 + 8)))->length]
SAFE: (ia64-tdep.c:3317):	VARINIT(m):	(ULONGEST to int)	[(type)->length % reglen]
SAFE: (infcall.c:1029):	FUNC(read_value_memory):	(ULONGEST to size_t)	[(values_type)->length]
SAFE: (infcall.c:1046):	FUNC(read_value_memory):	(ULONGEST to size_t)	[(values_type)->length]
REVERTED(siginfo type should fit into size_t): (infrun.c:6677):	FUNC(xmalloc):	(ULONGEST to size_t)	[len]
 - I agree. So why have you changed 'size_t->ULONGEST len = TYPE_LENGTH (type);' ?
   That change should be therefore reverted.
SAFE: (iq2000-tdep.c:513):	VARINIT(size):	(LONGEST to int)	[len % 4 ?  : 4]
SAFE: (iq2000-tdep.c:564):	VARINIT(size):	(LONGEST to int)	[len % 4 ?  : 4]
SAFE: (iq2000-tdep.c:581):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE: (iq2000-tdep.c:739):	ASSIGN:	(LONGEST to int)	[ slacklen = (4 - (typelen % 4)) % 4]
SAFE: (iq2000-tdep.c:741):	FUNC(memcpy):	(LONGEST to size_t)	[typelen]
SAFE: (iq2000-tdep.c:772):	FUNC(write_memory):	(LONGEST to ssize_t)	[typelen]
SAFE: (iq2000-tdep.c:782):	FUNC(write_memory):	(LONGEST to ssize_t)	[typelen]
SAFE: (jit.c:343):	ASSIGN:	(ULONGEST to int)	[ ptr_size = (ptr_type)->length]
SAFE: (jit.c:384):	ASSIGN:	(ULONGEST to int)	[ ptr_size = (ptr_type)->length]
SAFE: (jv-lang.c:482):	ASSIGN:	(LONGEST to CORE_ADDR)	[ (((type)->main_type->flds_bnds.fields[i]).loc.physaddr) = (boffset)]
SAFE: (jv-lang.c:611):	RET:	(ULONGEST to int)	[(objtype)->length]
SAFE: (linux-record.c:500):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg2)))->length]
SAFE: (linux-record.c:637):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg1)))->length]
SAFE: (linux-record.c:713):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg1)))->length]
SAFE: (linux-record.c:813):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg5)))->length]
SAFE: (linux-record.c:856):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg2)))->length]
SAFE: (linux-record.c:887):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg2)))->length]
SAFE: (linux-record.c:918):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg2)))->length]
SAFE: (linux-record.c:944):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg2)))->length]
SAFE: (linux-record.c:981):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg2)))->length]
SAFE: (linux-record.c:1041):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg2)))->length]
SAFE: (linux-record.c:1055):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg1)))->length]
SAFE: (linux-record.c:1392):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg2)))->length]
SAFE: (linux-record.c:1877):	FUNC(phex_nz):	(ULONGEST to int)	[(gdbarch_register_type(get_regcache_arch(regcache), (tdep->arg2)))->length]
SAFE: (linux-tdep.c:169):	FUNC(append_composite_type_field_aligned):	(ULONGEST to int)	[(long_type)->length]
FIXED(Expand i): (m2-lang.c:121):	CMP:	(ULONGEST to UINT)	[i < length]
FIXED(Expand rep1, reps): (m2-lang.c:139):	CMP:	(ULONGEST to UINT)	[rep1 < length]
FIXED(Expand rep1, reps): (m2-lang.c:185):	CMP:	(ULONGEST to UINT)	[i < length]
SAFE: (m2-typeprint.c:377):	CMP:	(size_t to ULONGEST)	[(type)->length < sizeof(LONGEST)]
SAFE: (m32c-tdep.c:415):	VARINIT(containing_len):	(ULONGEST to int)	[(reg->rx->type)->length]
SAFE: (m32c-tdep.c:418):	VARINIT(elt_len):	(ULONGEST to int)	[(reg->type)->length]
SAFE: (m32c-tdep.c:435):	ASSIGN:	(ULONGEST to int)	[ elt_offset = (reg->rx->type)->length - elt_offset - elt_len]
SAFE: (m32c-tdep.c:451):	FUNC(memset):	(ULONGEST to size_t)	[(reg->type)->length]
SAFE: (m32c-tdep.c:479):	VARINIT(high_bytes):	(ULONGEST to int)	[(reg->rx->type)->length]
SAFE: (m32c-tdep.c:480):	VARINIT(low_bytes):	(ULONGEST to int)	[(reg->ry->type)->length]
SAFE: (m32c-tdep.c:485):	CMP:	(int to ULONGEST)	[(reg->type)->length == high_bytes + low_bytes]
SAFE: (m32c-tdep.c:510):	VARINIT(high_bytes):	(ULONGEST to int)	[(reg->rx->type)->length]
SAFE: (m32c-tdep.c:511):	VARINIT(low_bytes):	(ULONGEST to int)	[(reg->ry->type)->length]
SAFE: (m32c-tdep.c:515):	CMP:	(int to ULONGEST)	[(reg->type)->length == high_bytes + low_bytes]
SAFE: (m32c-tdep.c:539):	VARINIT(len):	(ULONGEST to int)	[(tdep->r0->type)->length]
SAFE: (m32c-tdep.c:577):	VARINIT(len):	(ULONGEST to int)	[(tdep->r0->type)->length]
SAFE: (m32c-tdep.c:2074):	VARINIT(ptr_len):	(ULONGEST to int)	[(tdep->ptr_voyd)->length]
SAFE: (m32r-tdep.c:260):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (m32r-tdep.c:744):	FUNC(memcpy):	(LONGEST to size_t)	[len]
SAFE: (m68hc11-tdep.c:1222):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (m68hc11-tdep.c:1275):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[2 - len]
SAFE: (m68hc11-tdep.c:1275):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[len]
SAFE: (m68hc11-tdep.c:1278):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[4 - len]
SAFE: (m68hc11-tdep.c:1279):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[len - 2]
FIXED(Expand len): (m68hc11-tdep.c:1294):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (m68kbsd-tdep.c:47):	VARINIT(fp_len):	(ULONGEST to int)	[(gdbarch_register_type(gdbarch, regnum))->length]
FIXED(Expand param for get_frame_register_bytes): (m68k-tdep.c:225):	FUNC(get_frame_register_bytes):	(ULONGEST to int)	[(type)->length]
 - I am not against expanding LEN of get_frame_register_bytes, it has no use
   but it is a good sanity check instead of checking all the callers.
   But in this case BTW it is not needed, only TYPE_CODE_FLT can be passed to
   get_frame_register_bytes, TYPE_CODE_FLT is always small enough.
SAFE: (m68k-tdep.c:301):	FUNC(memcpy):	(LONGEST to size_t)	[len]
SAFE: (m68k-tdep.c:306):	FUNC(memcpy):	(LONGEST to size_t)	[len - 4]
SAFE: (m68k-tdep.c:344):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[4 - len]
SAFE: (m68k-tdep.c:344):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[len]
SAFE: (m68k-tdep.c:347):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[8 - len]
SAFE: (m68k-tdep.c:348):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[len - 4]
FIXED(Expand len): (m68k-tdep.c:389):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (m68k-tdep.c:428):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE: (m68k-tdep.c:468):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE: (m68k-tdep.c:535):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (m88k-tdep.c:165):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (m88k-tdep.c:192):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Expand to size_t)|ENSURED_SIZET: (m88k-tdep.c:311):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
 - ENSURED_SIZET is redundant, there is above already:
   const bfd_byte *valbuf = value_contents (args[i]);
FIXED(Expand to LONGEST): (m88k-tdep.c:312):	VARINIT(stack_word):	(LONGEST to int)	[num_stack_words]
REVERTED(num_register_words bound by number of registers): (m88k-tdep.c:316):	VARINIT(register_word):	(LONGEST to int)	[num_register_words]
SAFE: (m88k-tdep.c:404):	FUNC(memcpy):	(LONGEST to size_t)	[len]
SAFE: (m88k-tdep.c:410):	FUNC(memcpy):	(LONGEST to size_t)	[len]
SAFE: (m88k-tdep.c:426):	FUNC(memcpy):	(LONGEST to size_t)	[len]
UNRELATED: (mdebugread.c:1016):	ASSIGN:	(int to SHORT)	[ (t)->main_type->nfields = nfields]
 - True, 'short nfields;' is not this bug.
SAFE: (mdebugread.c:1035):	CMP:	(SHORT to ULONGEST)	[(t)->length == (t)->main_type->nfields]
SAFE: (mdebugread.c:1238):	ASSIGN:	(bfd_vma to LONGEST)	[ ((*f).loc.bitpos) = (sh->value)]
 - Not so clear to me, --disable-64-bit-bfd cannot support >=2GB inferiors but it
   should support >256MB structs.  Still when it also affects only STABS I do
   not think it needs a fix.
SAFE: (microblaze-tdep.c:571):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
SAFE: (microblaze-tdep.c:608):	FUNC(memcpy):	(LONGEST to size_t)	[len]

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Sun, 26 Aug 2012 19:43:35 +0200, Jan wrote:
> (i386-nat.c:613):
> FUNC(i386_length_and_rw_bits):	(LONGEST to int)	[len]
>  - To be re-checked with updated watchpoints patch.

This is safe, since ok_for_watchpoint will restrict the incoming LEN in
insert_watchpoint.

> WPREVERTED(Likewise): (i386-nat.c:647):
> FUNC(i386_length_and_rw_bits):	(LONGEST to int)	[len]
>  - To be re-checked with updated watchpoints patch.

Likewise for remove_watchpoint.

> SAFE:
> (ia64-tdep.c:3299):	VARINIT(n):	(ULONGEST to
> int)	[(type)->length / (float_elt_type)->length]
>  - You did expand n in ia64_extract_return_value, it can be array of
> floats. I did not investigate what happens if the array of floats is
> too large and it does not fit into ia64 registers.  But I find safe
> to just expand it.

OK, I misread the HFA definition as an array/struct that *only* has a
float. I think I confused this with some other function that has this
definition.

> REVERTED(siginfo type should fit into
> size_t): (infrun.c:6677):	FUNC(xmalloc):	(ULONGEST to
> size_t)	[len]
>  - I agree. So why have you changed 'size_t->ULONGEST len =
> TYPE_LENGTH (type);' ? That change should be therefore reverted.

Right, will revert.

> FIXED(Expand param for get_frame_register_bytes):
> (m68k-tdep.c:225):	FUNC(get_frame_register_bytes):
> (ULONGEST to int)	[(type)->length]
>  - I am not against expanding LEN of get_frame_register_bytes, it has
> no use but it is a good sanity check instead of checking all the
> callers. But in this case BTW it is not needed, only TYPE_CODE_FLT
> can be passed to get_frame_register_bytes, TYPE_CODE_FLT is always
> small enough.

Yes, you had suggested this expansion as a sanity check in an earlier
review.

> FIXED(Expand to size_t)|ENSURED_SIZET: (m88k-tdep.c:311):
> VARINIT(len):	(ULONGEST to int)	[(type)->length]
>  - ENSURED_SIZET is redundant, there is above already:
>    const bfd_byte *valbuf = value_contents (args[i]);

Yes, all of the ENSURED_SIZET for -tdep have been removed for similar
reasons.

> SAFE:
> (mdebugread.c:1238):	ASSIGN:	(bfd_vma to
> LONGEST)	[ ((*f).loc.bitpos) = (sh->value)]
>  - Not so clear to me, --disable-64-bit-bfd cannot support >=2GB
> inferiors but it should support >256MB structs.  Still when it also
> affects only STABS I do not think it needs a fix.

Either ways, the point is that sizeof (bfd_vma) would always be less or
equal to than sizeof (LONGEST) and hence this ought to be safe.

Regards,
Siddhesh

Re: bitpos expansion patches summary

[Jan Kratochvil]

On Mon, 27 Aug 2012 10:09:27 +0200, Siddhesh Poyarekar wrote:
> On Sun, 26 Aug 2012 19:43:35 +0200, Jan wrote:
> > (i386-nat.c:613):
> > FUNC(i386_length_and_rw_bits):	(LONGEST to int)	[len]
> >  - To be re-checked with updated watchpoints patch.
> 
> This is safe, since ok_for_watchpoint will restrict the incoming LEN in
> insert_watchpoint.
> 
> > WPREVERTED(Likewise): (i386-nat.c:647):
> > FUNC(i386_length_and_rw_bits):	(LONGEST to int)	[len]
> >  - To be re-checked with updated watchpoints patch.
> 
> Likewise for remove_watchpoint.

OK, I agree with these two, these are already i386-specific and i386 (contrary
to s390) has very limited maximum span of a single hardward watchpoint.


> Yes, all of the ENSURED_SIZET for -tdep have been removed for similar
> reasons.

It will be fun to merge all the pending patches and reviews in the end.


> > SAFE:
> > (mdebugread.c:1238):	ASSIGN:	(bfd_vma to
> > LONGEST)	[ ((*f).loc.bitpos) = (sh->value)]
> >  - Not so clear to me, --disable-64-bit-bfd cannot support >=2GB
> > inferiors but it should support >256MB structs.  Still when it also
> > affects only STABS I do not think it needs a fix.
> 
> Either ways, the point is that sizeof (bfd_vma) would always be less or
> equal to than sizeof (LONGEST) and hence this ought to be safe.

OK, I agree, I have mistaken my review here.


Thanks,
Jan

Re: bitpos expansion patches summary

[Jan Kratochvil]

Hi Siddhesh,

another part, close to the end.

http://people.redhat.com/jkratoch/bitpos3.patch
 - so far only FYI - annotations to the patch file.
^x lines are justifications of the change according to non-safe
   splint-bitpos2.locdiff.report record below.
^y only a few of them - review of changes listed below.
   yBAD means I do not agree with the change, explained below.
It will need to be all annotated before a check-in.


[part below was already sent off-list]
We will need to re-run splint on top of FSF HEAD and then re-use the already
processed '.locdiff.report' lines but still see the newly introduced splint
warnings.

I did not yet get to this point as I still process the "old" codebase
2636a39d8bf9b24dce328e4f906e8710b52d2105 but it will need to be done before
check-in.  For:
 * newly introduced splint warnings by the int->LONGEST&co. expansion
 * newly introduced FSF HEAD code since the last splint run

I was thinking about some updating of file:lineno records in '.locdiff.report'
file to their new location and then diffing the old+reviewed '.locdiff.report'
against new file.  But I have not tried it yet in practice.


Thanks,
Jan


SAFE(Register type): (mips-tdep.c:876):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE(Likewise): (mips-tdep.c:908):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (mips-tdep.c:4318):	FUNC(store_signed_integer):	(CORE_ADDR to LONGEST)	[make_compact_addr(addr)]
SAFE: (mips-tdep.c:4400):	FUNC(phex):	(LONGEST to int)	[len]
SAFE: (mips-tdep.c:4414):	VARINIT(odd_sized_struct):	(boolean to int)	[(len > regsize && len % regsize != 0)]
SAFE: (mips-tdep.c:4422):	VARINIT(partial_len):	(LONGEST to int)	[(len < regsize ? len : regsize)]
SAFE: (mips-tdep.c:4444):	ASSIGN:	(LONGEST to int)	[ longword_offset = regsize - len]
SAFE: (mips-tdep.c:4447):	CMP:	(int to ULONGEST)	[(arg_type)->length < regsize]
SAFE: (mips-tdep.c:4448):	ASSIGN:	(LONGEST to int)	[ longword_offset = regsize - len]
SAFE(Structs dont reach here): (mips-tdep.c:4565):	CMP:	(ULONGEST to int)	[offset < (type)->length]
SAFE(Likewise): (mips-tdep.c:4569):	CMP:	(ULONGEST to int)	[offset + xfer > (type)->length]
SAFE(Likewise): (mips-tdep.c:4570):	ASSIGN:	(ULONGEST to int)	[ xfer = (type)->length - offset]
FIXED(Expand len): (mips-tdep.c:4701):	VARINIT(len):	(ULONGEST to int)	[(arg_type)->length]
 - You have fixed line 4646 but I do not see line 4701 to be fixed.
SAFE: (mips-tdep.c:4937):	CMP:	(int to ULONGEST)	[(type)->length > 2 * MIPS64_REGSIZE]
SAFE: (mips-tdep.c:4970):	FUNC(mips_xfer_register):	(ULONGEST to int)	[(type)->length]
SAFE: (mips-tdep.c:5020):	FUNC(mips_xfer_register):	(ULONGEST to int)	[((((type)->main_type->flds_bnds.fields[field]).type))->length]
SAFE(Length < 2*MIPS64_REGSIZE): (mips-tdep.c:5036):	CMP:	(ULONGEST to int)	[offset < (type)->length]
FIXED(expand offset): (mips-tdep.c:5040):	CMP:	(ULONGEST to int)	[offset + xfer > (type)->length]
 - I do not see a need for this change, it is all limited by:
   TYPE_LENGTH (type) <= 2 * MIPS64_REGSIZE
SAFE: (mips-tdep.c:5041):	ASSIGN:	(ULONGEST to int)	[ xfer = (type)->length - offset]
SAFE(non-struct type): (mips-tdep.c:5059):	CMP:	(ULONGEST to int)	[offset < (type)->length]
SAFE(Likewise): (mips-tdep.c:5063):	CMP:	(ULONGEST to int)	[offset + xfer > (type)->length]
SAFE: (mips-tdep.c:5064):	ASSIGN:	(ULONGEST to int)	[ xfer = (type)->length - offset]
SAFE(float type): (mips-tdep.c:5244):	FUNC(phex):	(LONGEST to int)	[len]
SAFE: (mips-tdep.c:5252):	FUNC(phex):	(LONGEST to int)	[len]
SAFE: (mips-tdep.c:5284):	VARINIT(partial_len):	(LONGEST to int)	[(len < MIPS32_REGSIZE ? len : MIPS32_REGSIZE)]
SAFE(Not a struct): (mips-tdep.c:5444):	FUNC(mips_xfer_register):	(ULONGEST to int)	[(type)->length]
SAFE(Likewise): (mips-tdep.c:5450):	FUNC(mips_xfer_register):	(ULONGEST to int)	[(type)->length]
SAFE(Likewise): (mips-tdep.c:5599):	CMP:	(ULONGEST to int)	[offset < (type)->length]
SAFE(Likewise): (mips-tdep.c:5603):	CMP:	(ULONGEST to int)	[offset + xfer > (type)->length]
SAFE(Likewise): (mips-tdep.c:5604):	ASSIGN:	(ULONGEST to int)	[ xfer = (type)->length - offset]
SAFE: (mips-tdep.c:5712):	FUNC(store_signed_integer):	(CORE_ADDR to LONGEST)	[make_compact_addr(addr)]
SAFE(Float type): (mips-tdep.c:5733):	FUNC(phex):	(LONGEST to int)	[len]
SAFE(Likewise): (mips-tdep.c:5737):	FUNC(phex):	(LONGEST to int)	[len]
SAFE: (mips-tdep.c:5758):	VARINIT(partial_len):	(LONGEST to int)	[(len < MIPS64_REGSIZE ? len : MIPS64_REGSIZE)]
SAFE: (mips-tdep.c:5779):	ASSIGN:	(LONGEST to int)	[ longword_offset = MIPS64_REGSIZE - len]
SAFE: (mips-tdep.c:5785):	FUNC(paddress):	(LONGEST to CORE_ADDR)	[stack_offset]
SAFE(Not a struct): (mips-tdep.c:5909):	FUNC(mips_xfer_register):	(ULONGEST to int)	[(type)->length]
SAFE(Likewise): (mips-tdep.c:5915):	FUNC(mips_xfer_register):	(ULONGEST to int)	[(type)->length]
SAFE(Likewise): (mips-tdep.c:5927):	CMP:	(ULONGEST to int)	[offset < (type)->length]
SAFE(Likewise): (mips-tdep.c:5931):	CMP:	(ULONGEST to int)	[offset + xfer > (type)->length]
SAFE(Likewise): (mips-tdep.c:5932):	ASSIGN:	(ULONGEST to int)	[ xfer = (type)->length - offset]
FIXED(Expand len): (mn10300-tdep.c:174):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Likewise): (mn10300-tdep.c:203):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (mt-tdep.c:353):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE: (mt-tdep.c:361):	FUNC(write_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE: (mt-tdep.c:386):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
SAFE(stack_dest should be CORE_ADDR, but safe for now): (mt-tdep.c:786):	VARINIT(stack_dest):	(CORE_ADDR to LONGEST)	[sp]
 - There should be checked overflow of sp - length in general in these
   *_push_dummy_call functions but passing >2GB structs by value on stack is
   probably not worth fixing.
   But I do not see why you did not choose CORE_ADDR for stack_dest here.
SAFE: (mt-tdep.c:827):	ASSIGN:	(LONGEST to int)	[ split_param_len = typelen]
SAFE: (mt-tdep.c:828):	FUNC(memcpy):	(LONGEST to size_t)	[typelen]
SAFE: (mt-tdep.c:838):	FUNC(write_memory):	(LONGEST to CORE_ADDR)	[stack_dest]
SAFE: (mt-tdep.c:838):	FUNC(write_memory):	(LONGEST to ssize_t)	[typelen]
SAFE: (mt-tdep.c:851):	ASSIGN:	(LONGEST to int)	[ slacklen = (4 - (typelen % 4)) % 4]
UNSAFE_ALLOCA: (mt-tdep.c:852):	FUNC(C_alloca):	(LONGEST to size_t)	[typelen + slacklen]
 - Just drop alloca, two write_memory calls are enough.
UNSAFE_ALLOCA: (mt-tdep.c:853):	FUNC(memcpy):	(LONGEST to size_t)	[typelen]
 - Just drop alloca, two write_memory calls are enough.
SAFE: (mt-tdep.c:857):	FUNC(write_memory):	(LONGEST to CORE_ADDR)	[stack_dest]
SAFE: (mt-tdep.c:857):	FUNC(write_memory):	(LONGEST to ssize_t)	[typelen + slacklen]
SAFE: (mt-tdep.c:865):	FUNC(write_memory):	(LONGEST to CORE_ADDR)	[stack_dest]
SAFE: (mt-tdep.c:882):	RET:	(LONGEST to CORE_ADDR)	[stack_dest]
FIXED(Expand i): (objc-lang.c:371):	CMP:	(ULONGEST to UINT)	[i < length]
FIXED(Expand rep1): (objc-lang.c:389):	CMP:	(ULONGEST to UINT)	[rep1 < length]
FIXED: (objc-lang.c:435):	CMP:	(ULONGEST to UINT)	[i < length]
SAFE: (opencl-lang.c:196):	FUNC(memcpy):	(LONGEST to size_t)	[elsize]
SAFE: (opencl-lang.c:236):	FUNC(memcpy):	(LONGEST to size_t)	[elsize]
FIXED(Expand startrest): (opencl-lang.c:253):	VARINIT(startrest):	(LONGEST to int)	[offset % elsize]
 - 'elsize' line is longer than 80 columns.  One should check the whole patch.
   (In these cases a new helper variable makes it easier to conform to the GNU indentation style.)
FIXED(Expand endrest, comp_offset, comp_length): (opencl-lang.c:255):	VARINIT(endrest):	(LONGEST to int)	[(offset + length) % elsize]
FIXED: (opencl-lang.c:268):	COND:	(LONGEST to int)	[]
FIXED: (opencl-lang.c:307):	VARINIT(startrest):	(LONGEST to int)	[offset % elsize]
FIXED: (opencl-lang.c:309):	VARINIT(endrest):	(LONGEST to int)	[(offset + length) % elsize]
FIXED: (opencl-lang.c:322):	COND:	(LONGEST to int)	[]
SAFE: (opencl-lang.c:428):	FUNC(memcpy):	(ULONGEST to size_t)	[(elm_type)->length]
 - I miss here FIX of: opencl-lang.c:450
   int src_len; LONGEST lowb, highb; src_len = highb - lowb + 1;
   It will affect also at least create_value().
SAFE: (opencl-lang.c:581):	FUNC(memset):	(ULONGEST to size_t)	[(eltype)->length]
SAFE: (opencl-lang.c:681):	FUNC(memset):	(ULONGEST to size_t)	[(eltype1)->length]
 - I miss here opencl-lang.c:895 where variable "i" should have been expanded.
   int i; LONGEST lowb1, highb1; for (i = 0; i < highb1 - lowb1 + 1; i++)
SAFE: (opencl-lang.c:901):	FUNC(memcpy):	(ULONGEST to size_t)	[(eltype2)->length]
SAFE: (p-lang.c:233):	ASSIGN:	(ULONGEST to int)	[ width = (type)->length]
FIXED(Expand i): (p-lang.c:249):	CMP:	(ULONGEST to UINT)	[i < length]
FIXED(Expand rep1): (p-lang.c:271):	CMP:	(ULONGEST to UINT)	[rep1 < length]
FIXED: (p-lang.c:319):	CMP:	(ULONGEST to UINT)	[i < length]
WPREVERTED(Only ok_for_watchpoint needs LONGEST len): (ppc-linux-nat.c:2030):	FUNC(calculate_dvc):	(LONGEST to int)	[len]
SAFE: (ppc-linux-nat.c:2053):	ASSIGN:	(LONGEST to uint64_t)	[ p->addr2 = (uint64_t)addr + len]
SAFE: (ppc-linux-nat.c:2057):	ASSIGN:	(int to uint32_t)	[ p->trigger_type = get_trigger_type(rw)]
SAFE: (ppc-sysv-tdep.c:162):	FUNC(align_up):	(LONGEST to int)	[len]
 - I would say at ppc-sysv-tdep.c:124 'len' it should be int->ssize_t instead of int->LONGEST
   as there is already value_contents making the checks somehow easier.
SAFE: (ppc-sysv-tdep.c:164):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (ppc-sysv-tdep.c:193):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (ppc-sysv-tdep.c:215):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (ppc-sysv-tdep.c:250):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (ppc-sysv-tdep.c:304):	FUNC(align_up):	(LONGEST to int)	[len]
SAFE: (ppc-sysv-tdep.c:308):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (ppc-sysv-tdep.c:380):	FUNC(align_up):	(LONGEST to int)	[len]
SAFE: (ppc-sysv-tdep.c:382):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (ppc-sysv-tdep.c:395):	FUNC(write_memory):	(ULONGEST to ssize_t)	[(eltype)->length]
SAFE(Vectors may not be larger than int): (ppc-sysv-tdep.c:447):	CMP:	(LONGEST to int)	[i < len / 16]
 - This is dependency on external producer (gcc).
   GDB should verify it, such as ENSURE_SIZET or a proper handling.
   External producer could incorrectly mark DW_AT_GNU_vector some large type
   and GDB would process it incorrectly.
SAFE: (ppc-sysv-tdep.c:537):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (ppc-sysv-tdep.c:550):	FUNC(memcpy):	(LONGEST to size_t)	[len]
FIXED(Expand argspace): (ppc-sysv-tdep.c:574):	ASSIGN:	(LONGEST to int)	[ argspace = argoffset]
SAFE: (ppc-sysv-tdep.c:814):	CMP:	(int to ULONGEST)	[(type)->length <= tdep->wordsize]
REVERTED(vectors < 16 bytes): (ppc-sysv-tdep.c:852):	VARINIT(regnum):	(LONGEST to int)	[tdep->ppc_fp0_regnum + 1 + i]
 - it still could REVERT ppc-sysv-tdep.c:844 and ppc-sysv-tdep.c:848.
REVERTED: (ppc-sysv-tdep.c:871):	VARINIT(regnum):	(LONGEST to int)	[tdep->ppc_gp0_regnum + 3 + i]
SAFE(int sufficient for OpenCL vectors): (ppc-sysv-tdep.c:897):	VARINIT(n_regs):	(ULONGEST to int)	[(type)->length / 16]
 - While true again it is dependency on external producer (gcc).
SAFE: (ppc-sysv-tdep.c:984):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (ppc-sysv-tdep.c:1019):	CMP:	(int to ULONGEST)	[(type)->length > tdep->wordsize]
SAFE: (ppc-sysv-tdep.c:1022):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
SAFE: (ppc-sysv-tdep.c:1031):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
SAFE: (ppc-sysv-tdep.c:1034):	CMP:	(int to ULONGEST)	[(type)->length > tdep->wordsize]
SAFE: (ppc-sysv-tdep.c:1304):	FUNC(write_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE: (ppc-sysv-tdep.c:1360):	FUNC(write_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE(OpenCL Vector): (ppc-sysv-tdep.c:1375):	VARINIT(nelt):	(ULONGEST to int)	[(type)->length / (eltype)->length]
SAFE(OpenCL Vector): (ppc-sysv-tdep.c:1448):	CMP:	(ULONGEST to int)	[i < (type)->length / 16]
 - While true again it is dependency on external producer (gcc).
SAFE: (ppc-sysv-tdep.c:1487):	FUNC(write_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE(OpenCL Vector): (ppc-sysv-tdep.c:1502):	FUNC(write_memory):	(ULONGEST to ssize_t)	[(type)->length]
FIXED(Expand byte): (ppc-sysv-tdep.c:1561):	CMP:	(ULONGEST to int)	[byte < (type)->length]
 - OK although ssize_t would be enough here.
FIXED(Expand len): (ppc-sysv-tdep.c:1567):	VARINIT(len):	(ULONGEST to int)	[(type)->length - byte]
 - OK although ssize_t would be enough here.
SAFE: (ppc-sysv-tdep.c:1597):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
 - OK although ssize_t would be enough here.
SAFE: (ppc-sysv-tdep.c:1599):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (ppc-sysv-tdep.c:1792):	VARINIT(nelt):	(ULONGEST to int)	[(valtype)->length / (eltype)->length]
SAFE: (ppc-sysv-tdep.c:1796):	VARINIT(offset):	(ULONGEST to int)	[i * (eltype)->length]
REVERTED(OpenCL vectors don't need LONGEST): (ppc-sysv-tdep.c:1851):	VARINIT(regnum):	(LONGEST to int)	[tdep->ppc_vr0_regnum + 2 + i]
SAFE: (ppc-sysv-tdep.c:1869):	VARINIT(offset):	(ULONGEST to int)	[(register_size(gdbarch, tdep->ppc_gp0_regnum + 3) - (valtype)->length)]
SAFE: (ppc-sysv-tdep.c:1873):	FUNC(regcache_cooked_write_part):	(ULONGEST to int)	[(valtype)->length]
SAFE: (ppc-sysv-tdep.c:1876):	FUNC(regcache_cooked_read_part):	(ULONGEST to int)	[(valtype)->length]
SAFE: (ppc-sysv-tdep.c:1900):	CMP:	(ULONGEST to int)	[i < (valtype)->length / 8]
SAFE: (printcmd.c:378):	CMP:	(size_t to ULONGEST)	[len > sizeof(LONGEST)]
SAFE: (printcmd.c:385):	FUNC(print_octal_chars):	(ULONGEST to UINT)	[len]
SAFE: (printcmd.c:389):	FUNC(print_decimal_chars):	(ULONGEST to UINT)	[len]
SAFE: (printcmd.c:392):	FUNC(print_binary_chars):	(ULONGEST to UINT)	[len]
SAFE: (printcmd.c:398):	FUNC(print_char_chars):	(ULONGEST to UINT)	[len]
SAFE: (printcmd.c:419):	CMP:	(size_t to ULONGEST)	[len < sizeof(LONGEST)]
SAFE: (printcmd.c:2117):	VARINIT(wcwidth):	(ULONGEST to int)	[(wctype)->length]
SAFE: (printcmd.c:2176):	FUNC(convert_between_encodings):	(ULONGEST to UINT)	[(valtype)->length]
SAFE: (printcmd.c:2177):	FUNC(convert_between_encodings):	(ULONGEST to int)	[(valtype)->length]
SAFE: (printcmd.c:2269):	VARINIT(param_len):	(ULONGEST to UINT)	[(param_type)->length]
 - While it is technically right I would expand it here, at the point where it
   is computed it is still not clear the type is TYPE_CODE_DECFLOAT.
SAFE: (p-valprint.c:212):	FUNC(xmalloc):	(LONGEST to size_t)	[length_size]
 - Missing ENSURED_SIZET.  Normally it cannot happen but broken compiler can
   produce arbitrarily long length-of-string field which this way could be
   incorrectly processed by GDB without an error.
 - extract_unsigned_integer may also need similar handling as size_t may be > int.
SAFE: (p-valprint.c:213):	FUNC(read_memory):	(LONGEST to ssize_t)	[length_size]
SAFE: (p-valprint.c:323):	ASSIGN:	(ULONGEST to UINT)	[ len = extract_unsigned_integer(valaddr + embedded_offset + length_pos, length_size, byte_order)]
 - Again, like above.
ENSURED_SIZET: (p-valprint.c:804):	FUNC(xmalloc):	(ULONGEST to size_t)	[(baseclass)->length]
ENSURED_SIZET: (p-valprint.c:809):	FUNC(target_read_memory):	(ULONGEST to ssize_t)	[(baseclass)->length]
FIXED(Use PyLong_FromLongLong): (python/py-type.c:179):	FUNC(PyLong_FromLong):	(LONGEST to LONG)	[((((type)->main_type->flds_bnds.fields[field]).loc.bitpos) + 0)]
 - This is not correct but it has been correctly checked in in the meantime.
   [PATCH 3/3] bitpos: Minor python changes for bitpos expansion
   http://sourceware.org/ml/gdb-patches/2012-08/msg00146.html
FIXED(Likewise): (python/py-type.c:686):	FUNC(PyLong_FromLong):	(ULONGEST to LONG)	[(type)->length]
 - Likewise.
SAFE: (python/py-value.c:983):	FUNC(decimal_is_zero):	(ULONGEST to int)	[(type)->length]
SAFE: (regcache.c:128):	ASSIGN:	(ULONGEST to LONG)	[ descr->sizeof_register[i] = (descr->register_type[i])->length]
SAFE: (regcache.c:138):	ASSIGN:	(ULONGEST to LONG)	[ descr->sizeof_register[i] = (descr->register_type[i])->length]
SAFE: (remote.c:8051):	FUNC(xsnprintf):	(int to size_t)	[endbuf - p]
SAFE: (remote.c:8095):	FUNC(xsnprintf):	(int to size_t)	[endbuf - p]
WPREVERT(Only ok_for_watchpoint needs LONGEST): (remote-m32r-sdi.c:1434):	ASSIGN:	(LONGEST to UINT)	[ ab_size[i] = len]
WPREVERT(Likewise): (remote-mips.c:2458):	FUNC(mips_common_breakpoint):	(LONGEST to int)	[len]
WPREVERT(Likewise): (remote-mips.c:2467):	FUNC(mips_common_breakpoint):	(LONGEST to int)	[len]
SAFE: (rl78-tdep.c:1027):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (rs6000-aix-tdep.c:277):	CMP:	(LONGEST to int)	[argbytes < len]
SAFE: (rs6000-aix-tdep.c:280):	FUNC(memset):	(int to size_t)	[reg_size]
SAFE: (rs6000-aix-tdep.c:299):	VARINIT(adj):	(LONGEST to int)	[gdbarch_byte_order(gdbarch) == BFD_ENDIAN_BIG ? reg_size - len : 0]
SAFE: (rs6000-aix-tdep.c:304):	FUNC(memcpy):	(LONGEST to size_t)	[len]
SAFE: (rs6000-aix-tdep.c:366):	FUNC(write_memory):	(LONGEST to ssize_t)	[len - argbytes]
SAFE: (rs6000-aix-tdep.c:393):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (rs6000-aix-tdep.c:494):	CMP:	(int to ULONGEST)	[(valtype)->length <= tdep->wordsize]
SAFE(Not struct): (rx-tdep.c:640):	ASSIGN:	(ULONGEST to int)	[ p_arg_size = (p_arg_type)->length]
SAFE: (s390-tdep.c:2495):	FUNC(is_power_of_two):	(ULONGEST to UINT)	[(type)->length]
FIXED(Expand n): (s390-tdep.c:2516):	VARINIT(length):	(ULONGEST to UINT)	[(type)->length]
 - I do not see 'n' anywhere, you have modified s390_function_arg_float
   (s390-tdep.c:2505) but s390_function_arg_integer (s390-tdep.c:2516)
   also should be / is not expanded.
SAFE: (s390-tdep.c:2555):	ASSIGN:	(ULONGEST to int)	[ alignment = (type)->length]
SAFE: (s390-tdep.c:2675):	FUNC(write_memory):	(ULONGEST to ssize_t)	[length]
SAFE: (s390-tdep.c:2699):	FUNC(regcache_cooked_write_part):	(ULONGEST to int)	[length]
SAFE: (s390-tdep.c:2707):	FUNC(write_memory):	(ULONGEST to ssize_t)	[length]
SAFE: (s390-tdep.c:2710):	CMP:	(int to ULONGEST)	[length <= word_size]
SAFE: (s390-tdep.c:2728):	CMP:	(int to ULONGEST)	[length == 2 * word_size]
SAFE: (s390-tdep.c:2744):	FUNC(write_memory):	(ULONGEST to ssize_t)	[length]
SAFE: (s390-tdep.c:2840):	FUNC(regcache_cooked_write_part):	(LONGEST to int)	[length]
SAFE: (s390-tdep.c:2877):	FUNC(regcache_cooked_read_part):	(LONGEST to int)	[length]
SAFE: (s390-tdep.c:2883):	FUNC(regcache_cooked_read_part):	(LONGEST to int)	[length]
SAFE(Not struct): (score-tdep.c:465):	CMP:	(ULONGEST to int)	[offset < (type)->length]
SAFE: (score-tdep.c:470):	CMP:	(ULONGEST to int)	[offset + xfer > (type)->length]
SAFE: (score-tdep.c:471):	ASSIGN:	(ULONGEST to int)	[ xfer = (type)->length - offset]
SAFE: (score-tdep.c:592):	VARINIT(partial_len):	(LONGEST to int)	[arglen < 4 ? arglen : 4]
SAFE: (sh64-tdep.c:1116):	FUNC(memcpy):	(LONGEST to size_t)	[len]
SAFE: (sh64-tdep.c:1118):	FUNC(memcpy):	(LONGEST to size_t)	[len]
SAFE: (sh64-tdep.c:1269):	ASSIGN:	(LONGEST to int)	[ offset = register_size(gdbarch, DEFAULT_RETURN_REGNUM) - len]
SAFE: (sh64-tdep.c:1273):	FUNC(memcpy):	(LONGEST to size_t)	[len]
SAFE: (sh64-tdep.c:1298):	CMP:	(LONGEST to int)	[i < len]
SAFE: (sh64-tdep.c:1318):	ASSIGN:	(LONGEST to int)	[ offset = register_size(gdbarch, return_register) - len]
SAFE: (sh64-tdep.c:1320):	FUNC(memcpy):	(LONGEST to size_t)	[len]
FIXED(Expand len): (sh-tdep.c:808):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED: (sh-tdep.c:828):	CMP:	(int to ULONGEST)	[((((type)->main_type->flds_bnds.fields[0]).type))->length == len]
FIXED(Expand len, sh_justify_value_in_reg arg): (sh-tdep.c:1097):	ASSIGN:	(ULONGEST to int)	[ len = (type)->length]
 - Excessive ensure_type_fits_sizet in your patch at sh-tdep.c:1129
   because there is sh_justify_value_in_reg above it.
SAFE: (sh-tdep.c:1146):	CMP:	(int to ULONGEST)	[(type)->length == 2 * reg_size]
FIXED(Expand len, sh_justify_value_in_reg arg): (sh-tdep.c:1234):	ASSIGN:	(ULONGEST to int)	[ len = (type)->length]
 - Excessive ensure_type_fits_sizet in your patch at sh-tdep.c:1258
   because there is sh_justify_value_in_reg above it.
SAFE: (sh-tdep.c:1321):	CMP:	(LONGEST to int)	[i < len]
SAFE(Float type): (sh-tdep.c:1335):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Expand i): (sh-tdep.c:1371):	CMP:	(LONGEST to int)	[i < len]
SAFE(Floaty type): (sh-tdep.c:1383):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (solib-darwin.c:140):	ASSIGN:	(ULONGEST to int)	[ len = 4 + 4 + 2 * ptr_type->length]
SAFE: (solib-darwin.c:250):	VARINIT(ptr_len):	(ULONGEST to int)	[(ptr_type)->length]
SAFE: (solib-svr4.c:714):	VARINIT(pbuf_size):	(ULONGEST to int)	[(ptr_type)->length]
SAFE: (solib-svr4.c:893):	VARINIT(l_name_size):	(ULONGEST to int)	[(ptr_type)->length]
SAFE: (sparc64-tdep.c:67):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (sparc64-tdep.c:74):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (sparc64-tdep.c:94):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (sparc64-tdep.c:114):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE(Used only for floating types): (sparc64-tdep.c:645):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (sparc64-tdep.c:666):	ASSIGN:	(LONGEST to int)	[ regnum = SPARC64_D0_REGNUM + element + bitpos / 64]
SAFE: (sparc64-tdep.c:674):	ASSIGN:	(LONGEST to int)	[ regnum = SPARC_F0_REGNUM + element * 2 + bitpos / 32]
SAFE(Float type): (sparc64-tdep.c:721):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (sparc64-tdep.c:728):	ASSIGN:	(LONGEST to int)	[ regnum = SPARC64_Q0_REGNUM + bitpos / 128]
SAFE: (sparc64-tdep.c:735):	ASSIGN:	(LONGEST to int)	[ regnum = SPARC64_D0_REGNUM + bitpos / 64]
SAFE: (sparc64-tdep.c:743):	ASSIGN:	(LONGEST to int)	[ regnum = SPARC_F0_REGNUM + bitpos / 32]
SAFE: (sparc64-tdep.c:816):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
FIXED(Expand len): (sparc64-tdep.c:887):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Expand len): (sparc64-tdep.c:1024):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Expand len): (sparc64-tdep.c:1074):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (sparc-tdep.c:181):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (sparc-tdep.c:214):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (sparc-tdep.c:233):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (sparc-tdep.c:486):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (sparc-tdep.c:524):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE(Not struct): (sparc-tdep.c:1254):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE(Not struct): (sparc-tdep.c:1308):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (sparc-tdep.c:1379):	FUNC(read_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE: (spu-tdep.c:323):	VARINIT(preferred_slot):	(LONGEST to int)	[len < 4 ? 4 - len : 0]
FIXED(Expand len): (spu-tdep.c:1296):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
 - This is not needed, it has to fit in inferior registers.
FIXED(Likewise): (spu-tdep.c:1321):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
 - Likewise.
FIXED(Eliminate len and expand n_regs): (spu-tdep.c:1376):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
FIXED(Expand len): (spu-tdep.c:1409):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (spu-tdep.c:1475):	CMP:	(int to ULONGEST)	[(type)->length <= (SPU_ARGN_REGNUM - SPU_ARG1_REGNUM + 1) * 16]
SAFE: (stabsread.c:785):	ASSIGN:	(ULONGEST to LONG)	[ ((&objfile->objfile_obstack))->temp = (((dbl_type)->length))]
SAFE: (stabsread.c:1069):	CMP:	(int to ULONGEST)	[((sym)->type)->length < gdbarch_int_bit(gdbarch) / 8]
SAFE: (stabsread.c:2961):	CMP:	(ULONGEST to UINT)	[((fip->list->field).bitsize) == 8 * (field_type)->length]
FIXED(Expand highest_offset, start, print_frame_nameless_args arg 2): (stack.c:548):	ASSIGN:	(LONGEST to LONG)	[ highest_offset = current_offset]
 - current_offset/highest_offset could be split out, it is a different bug.
   But fine with merged it here.
SAFE: (stap-probe.c:1215):	FUNC(target_read_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE: (stap-probe.c:1233):	FUNC(target_write_memory):	(ULONGEST to ssize_t)	[(type)->length]
SAFE(Not struct): (tic6x-tdep.c:746):	VARINIT(len):	(ULONGEST to int)	[(valtype)->length]
SAFE(likewise): (tic6x-tdep.c:791):	VARINIT(len):	(ULONGEST to int)	[(valtype)->length]
SAFE(len <= 8): (tic6x-tdep.c:866):	VARINIT(len):	(ULONGEST to int)	[(check_typedef(type))->length]
FIXED(Expand len): (tic6x-tdep.c:974):	VARINIT(len):	(ULONGEST to int)	[(arg_type)->length]
 - references_offset is incorrectly not expanded.
SAFE: (tilegx-tdep.c:210):	CMP:	(int to ULONGEST)	[(type)->length > (1 + TILEGX_R9_REGNUM - TILEGX_R0_REGNUM) * tilegx_reg_size]
FIXED(Expand len, i): (tilegx-tdep.c:221):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
 - not needed, it is used only if !tilegx_use_struct_convention
SAFE: (tilegx-tdep.c:241):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
FIXED(Expand len, i): (tilegx-tdep.c:246):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
 - not needed, it is used only if !tilegx_use_struct_convention
FIXED(Expand typelen): (tilegx-tdep.c:308):	ASSIGN:	(ULONGEST to int)	[ typelen = (value_enclosing_type(args[i]))->length]
 - For example stacklen and alignlen are incorrectly not expanded.
FIXED: (tilegx-tdep.c:334):	ASSIGN:	(ULONGEST to int)	[ typelen = (value_enclosing_type(args[j]))->length]
UNSAFE_ALLOCA: (tilegx-tdep.c:347):	ASSIGN:	(ULONGEST to int)	[ typelen = (value_enclosing_type(args[j]))->length]
 - Please fix as an unrelated patch; it should be also ENSURED properly.
FIXED(Expand memrange last arg): (tracepoint.c:1005):	FUNC(add_memrange):	(ULONGEST to ULONG)	[len]
SAFE: (tracepoint.c:1016):	CMP:	(int to ULONGEST)	[len > register_size(gdbarch, reg)]
FIXED: (tracepoint.c:1034):	FUNC(add_memrange):	(ULONGEST to ULONG)	[len]
SAFE: (tracepoint.c:1037):	ASSIGN:	(LONGEST to UINT)	[ reg = (sym)->ginfo.value.ivalue]
FIXED: (tracepoint.c:1046):	FUNC(add_memrange):	(ULONGEST to ULONG)	[len]
SAFE: (tracepoint.c:1049):	ASSIGN:	(LONG to UINT)	[ reg = frame_regno]
FIXED: (tracepoint.c:1058):	FUNC(add_memrange):	(ULONGEST to ULONG)	[len]
SAFE: (tracepoint.c:1485):	FUNC(add_memrange):	(CORE_ADDR to bfd_signed_vma)	[addr]
FIXED: (tracepoint.c:1485):	FUNC(add_memrange):	(ULONGEST to ULONG)	[len]
 - Expansion of 'addr' is correct but unrelated.
FIXED(Expand len): (v850-tdep.c:851):	ASSIGN:	(ULONGEST to int)	[ len = (value_type(*args))->length]
 - Not needed to expand v850-tdep.c:893 due to !v850_use_struct_convention by its caller.
 - Not needed to expand v850-tdep.c:920 due to !v850_use_struct_convention by its caller.
SAFE: (v850-tdep.c:929):	CMP:	(LONGEST to int)	[i < len]
SAFE(string/char types): (valarith.c:713):	ASSIGN:	(ULONGEST to int)	[ inval2len = (type2)->length]
SAFE: (valarith.c:757):	ASSIGN:	(ULONGEST to int)	[ inval1len = (type1)->length]
 - TYPE_CODE_STRING can be also >2GB, it is in fact a sort of array.
   For example Fortran uses these.
   Also it needs alloca expansion.
SAFE: (valarith.c:758):	ASSIGN:	(ULONGEST to int)	[ inval2len = (type2)->length]
 - Likewise.
SAFE: (valarith.c:895):	ASSIGN:	(ULONGEST to int)	[ *len_x = (type1)->length]
SAFE: (valarith.c:901):	ASSIGN:	(ULONGEST to int)	[ *len_x = (type2)->length]
SAFE: (valarith.c:914):	ASSIGN:	(ULONGEST to int)	[ *len_y = (type2)->length]
SAFE: (valarith.c:920):	ASSIGN:	(ULONGEST to int)	[ *len_y = (type1)->length]
SAFE: (valarith.c:973):	ASSIGN:	(ULONGEST to int)	[ len_v = (result_type)->length]
SAFE(Vector types): (valarith.c:1414):	ASSIGN:	(ULONGEST to int)	[ elsize = (eltype1)->length]
SAFE(likewise): (valarith.c:1417):	CMP:	(ULONGEST to int)	[elsize != (eltype2)->length]
SAFE: (valarith.c:1487):	FUNC(decimal_is_zero):	(ULONGEST to int)	[(type1)->length]
FIXED(Expand len, i): (valarith.c:1512):	VARINIT(len):	(LONGEST to int)	[len1 < len2 ? len1 : len2]
SAFE: (valarith.c:1628):	FUNC(memcmp):	(ULONGEST to size_t)	[(type1)->length]
SAFE: (valarith.c:1718):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
SAFE: (valarith.c:1739):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (valarith.c:1772):	FUNC(memcpy):	(ULONGEST to size_t)	[(eltype)->length]
SAFE: (valarith.c:1809):	FUNC(memcpy):	(ULONGEST to size_t)	[(eltype)->length]
SAFE: (valops.c:474):	VARINIT(dec_len):	(ULONGEST to int)	[(type)->length]
SAFE: (valops.c:480):	FUNC(decimal_convert):	(ULONGEST to int)	[(type2)->length]
SAFE: (valops.c:569):	FUNC(memcpy):	(ULONGEST to size_t)	[(eltype)->length]
SAFE: (valops.c:884):	FUNC(decimal_from_string):	(ULONGEST to int)	[(type)->length]
SAFE: (valops.c:910):	FUNC(memcpy):	(ULONGEST to size_t)	[(eltype)->length]
SAFE: (valops.c:1026):	FUNC(read_value_memory):	(LONGEST to size_t)	[length]
SAFE: (valops.c:1284):	CMP:	(int to ULONGEST)	[(type)->length <= (int)sizeof(LONGEST)]
SAFE: (valops.c:1293):	FUNC(read_memory):	(LONGEST to ssize_t)	[changed_len]
SAFE: (valops.c:1305):	FUNC(write_memory_with_notification):	(LONGEST to ssize_t)	[changed_len]
SAFE: (valops.c:1352):	FUNC(get_frame_register_bytes):	(LONGEST to CORE_ADDR)	[offset]
SAFE: (valops.c:1366):	FUNC(put_frame_register_bytes):	(LONGEST to CORE_ADDR)	[offset]
SAFE: (valops.c:1372):	FUNC(put_frame_register_bytes):	(LONGEST to CORE_ADDR)	[value_offset(toval)]
FIXED(Expanded put_frame_register_bytes arg 4): (valops.c:1373):	FUNC(put_frame_register_bytes):	(ULONGEST to int)	[(type)->length]
 - I do not see any expansion and I do not see why.
   lval_register values can never be too large.
SAFE: (valops.c:1457):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
SAFE: (valops.c:1491):	FUNC(read_value_memory):	(ULONGEST to size_t)	[(value_enclosing_type(val))->length]
FIXED(Expand value_cstring arg 2, highbound to LONGEST): (valops.c:1849):	VARINIT(highbound):	(ULONGEST to int)	[len / (char_type)->length]
 - value_cstring's parameter 'int->LONGEST len' should have been only ssize_t.
   It cannot be larger than 'char *ptr' memory.
SAFE: (valops.c:1854):	FUNC(memcpy):	(int to size_t)	[len]
FIXED: (valops.c:1872):	VARINIT(highbound):	(ULONGEST to int)	[len / (char_type)->length]
 - Likewise for the 'len' parameter.
SAFE: (valops.c:1877):	FUNC(memcpy):	(int to size_t)	[len]
SAFE: (valops.c:1891):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
 - While TYPE_CODE_BITSTRING is not used in practice I do not see why it should not be expanded(=fixed).
SAFE: (valops.c:2137):	FUNC(target_read_memory):	(ULONGEST to ssize_t)	[(basetype)->length]
FIXED(Expand boffset - update_search_result,search_struct_field,do_search_struct_field): (valops.c:2173):	FUNC(update_search_result):	(LONGEST to int)	[boffset]
ENSURED_SIZET: (valops.c:2291):	FUNC(xmalloc):	(ULONGEST to size_t)	[(baseclass)->length]
ENSURED_SIZET: (valops.c:2296):	FUNC(target_read_memory):	(ULONGEST to ssize_t)	[(baseclass)->length]
SAFE: (valops.c:3858):	FUNC(memcpy):	(ULONGEST to size_t)	[(real_type)->length]
SAFE: (valops.c:3860):	FUNC(memcpy):	(ULONGEST to size_t)	[(real_type)->length]
SAFE: (valops.c:3878):	FUNC(memcpy):	(ULONGEST to size_t)	[(val_real_type)->length]
SAFE: (valops.c:3881):	FUNC(memcpy):	(ULONGEST to size_t)	[(val_real_type)->length]

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Sun, 2 Sep 2012 20:15:15 +0200, Jan wrote:
> I was thinking about some updating of file:lineno records in
> '.locdiff.report' file to their new location and then diffing the
> old+reviewed '.locdiff.report' against new file.  But I have not
> tried it yet in practice.

This will be heuristic to some extent, which is why I avoided it. 

> FIXED(Expand len):
> (mips-tdep.c:4701):	VARINIT(len):	(ULONGEST to
> int)	[(arg_type)->length]
>  - You have fixed line 4646 but I do not see line 4701 to be fixed.

Probably a case of different line numbers due to using a different
updated version? I was rebasing every now and then, which gave me
different line numbers for some warnings. I see this on line 4695 now.

> FIXED(expand offset): (mips-tdep.c:5040):	CMP:
> (ULONGEST to int)	[offset + xfer > (type)->length]
>  - I do not see a need for this change, it is all limited by:
>    TYPE_LENGTH (type) <= 2 * MIPS64_REGSIZE

Right, reverted.

> SAFE(stack_dest should
> be CORE_ADDR, but safe for now): (mt-tdep.c:786):
> VARINIT(stack_dest):	(CORE_ADDR to LONGEST)	[sp]
>  - There should be checked overflow of sp - length in general in these
>    *_push_dummy_call functions but passing >2GB structs by value on
> stack is probably not worth fixing.
>    But I do not see why you did not choose CORE_ADDR for stack_dest

Because it would change the sign, which is something we agreed we don't
want to do for this patch.

> UNSAFE_ALLOCA: (mt-tdep.c:852):
> FUNC(C_alloca):	(LONGEST to size_t)	[typelen + slacklen]
>  - Just drop alloca, two write_memory calls are enough.

This has already been replaced with xmalloc/xfree in cvs. But yes, two
write_memory calls should have been enough.

> UNSAFE_ALLOCA: (mt-tdep.c:853):	FUNC(memcpy):	(LONGEST
> to size_t)	[typelen]
>  - Just drop alloca, two write_memory calls are enough.

Likewise.

> FIXED(Expand startrest):
> (opencl-lang.c:253):	VARINIT(startrest):	(LONGEST to
> int)	[offset % elsize]
>  - 'elsize' line is longer than 80 columns.  One should check the
> whole patch. (In these cases a new helper variable makes it easier to
> conform to the GNU indentation style.)

OK, fixed.

> SAFE: (opencl-lang.c:428):	FUNC(memcpy):	(ULONGEST to
> size_t)	[(elm_type)->length]
>  - I miss here FIX of: opencl-lang.c:450
>    int src_len; LONGEST lowb, highb; src_len = highb - lowb + 1;
>    It will affect also at least create_value().

This is called only for a vector, so highb - lowb should be bound by
vector size. You made a point later about gcc possibly generating
incorrect code and hence giving us a large value. Do you want me to
expand this to cater for such a situation?

> SAFE: (opencl-lang.c:681):
> FUNC(memset):	(ULONGEST to size_t)	[(eltype1)->length]
>  - I miss here opencl-lang.c:895 where variable "i" should have been
> expanded. int i; LONGEST lowb1, highb1; for (i = 0; i < highb1 -
> lowb1 + 1; i++) 

Again, this should be limited by vector size.

> SAFE:
> (ppc-sysv-tdep.c:162):	FUNC(align_up):	(LONGEST to
> int)	[len]
>  - I would say at ppc-sysv-tdep.c:124 'len' it should be int->ssize_t
> instead of int->LONGEST as there is already value_contents making the
> checks somehow easier. 

OK, fixed.

> SAFE(Vectors may not be larger than int):
> (ppc-sysv-tdep.c:447):	CMP:	(LONGEST to int)	[i
> < len / 16]
>  - This is dependency on external producer (gcc).
>    GDB should verify it, such as ENSURE_SIZET or a proper handling.
>    External producer could incorrectly mark DW_AT_GNU_vector some
> large type and GDB would process it incorrectly.

OK, fixed.

> REVERTED(vectors < 16 bytes):
> (ppc-sysv-tdep.c:852):	VARINIT(regnum):	(LONGEST to
> int)	[tdep->ppc_fp0_regnum + 1 + i]
>  - it still could REVERT ppc-sysv-tdep.c:844 and ppc-sysv-tdep.c:848.

OK.

> SAFE(int
> sufficient for OpenCL vectors): (ppc-sysv-tdep.c:897):
> VARINIT(n_regs):	(ULONGEST to int)	[(type)->length / 16]
>  - While true again it is dependency on external producer (gcc).

OK.

> SAFE(OpenCL Vector):
> (ppc-sysv-tdep.c:1448):	CMP:	(ULONGEST to int)
> [i < (type)->length / 16]
>  - While true again it is dependency on external producer (gcc).

OK.

> FIXED(Expand byte):
> (ppc-sysv-tdep.c:1561):	CMP:	(ULONGEST to int)
> [byte < (type)->length]
>  - OK although ssize_t would be enough here.

OK.

> FIXED(Expand len): (ppc-sysv-tdep.c:1567):
> VARINIT(len):	(ULONGEST to int)	[(type)->length - byte]
>  - OK although ssize_t would be enough here.

OK.

> SAFE: (ppc-sysv-tdep.c:1597):	FUNC(write_memory):
> (LONGEST to ssize_t)	[len]
>  - OK although ssize_t would be enough here.

OK.

> > [(valtype)->length] SAFE: (printcmd.c:2269):
> > VARINIT(param_len):	(ULONGEST to UINT)
> > [(param_type)->length]
>  - While it is technically right I would expand it here, at the point
> where it is computed it is still not clear the type is
> TYPE_CODE_DECFLOAT.

I am actually more inclined to eliminate the single-use variable and use
TYPE_LENGTH directly. What do you think?

> SAFE: (p-valprint.c:212):
> FUNC(xmalloc):	(LONGEST to size_t)	[length_size]
>  - Missing ENSURED_SIZET.  Normally it cannot happen but broken
> compiler can produce arbitrarily long length-of-string field which
> this way could be incorrectly processed by GDB without an error.
>  - extract_unsigned_integer may also need similar handling as size_t
> may be > int. 

OK, will do.

> SAFE: (p-valprint.c:323):	ASSIGN:	(ULONGEST to
> UINT)	[ len = extract_unsigned_integer(valaddr +
> embedded_offset + length_pos, length_size, byte_order)]
>  - Again, like above.

OK.

> [(type)->length] FIXED(Expand n): (s390-tdep.c:2516):
> VARINIT(length):	(ULONGEST to UINT)	[(type)->length]
>  - I do not see 'n' anywhere, you have modified

Looks like I missed this due to some misplaced comment. length needs to
be expanded instead here. Fixed.

> FIXED(Expand len, sh_justify_value_in_reg arg):
> (sh-tdep.c:1097):	ASSIGN:	(ULONGEST to int)
> [ len = (type)->length]
>  - Excessive ensure_type_fits_sizet in your patch at sh-tdep.c:1129
>    because there is sh_justify_value_in_reg above it.

I think I have removed this in a later version of the patch.

> [(type)->length == 2 * reg_size] FIXED(Expand len,
> sh_justify_value_in_reg arg): (sh-tdep.c:1234):	ASSIGN:
> (ULONGEST to int)	[ len = (type)->length]
>  - Excessive ensure_type_fits_sizet in your patch at sh-tdep.c:1258
>    because there is sh_justify_value_in_reg above it.

Likewise.

> - len : 0] FIXED(Expand len): (spu-tdep.c:1296):
> VARINIT(len):	(ULONGEST to int)	[(type)->length]
>  - This is not needed, it has to fit in inferior registers.

spu_push_dummy_call also calls spu_value_to_regcache with a type
without checking the size of the type.

> FIXED(Likewise): (spu-tdep.c:1321):	VARINIT(len):
> (ULONGEST to int)	[(type)->length]
>  - Likewise.

OK, fixed.

> FIXED(Expand highest_offset, start,
> print_frame_nameless_args arg 2): (stack.c:548):
> ASSIGN:	(LONGEST to LONG)	[ highest_offset =
> current_offset]
>  - current_offset/highest_offset could be split out, it is a
> different bug. But fine with merged it here.

I don't know what you mean by splitting it out - could you please
elaborate?

> FIXED(Expand len):
> (tic6x-tdep.c:974):	VARINIT(len):	(ULONGEST to
> int)	[(arg_type)->length]
>  - references_offset is incorrectly not expanded.

OK.

> FIXED(Expand len, i):
> (tilegx-tdep.c:221):	VARINIT(len):	(ULONGEST to
> int)	[(type)->length]
>  - not needed, it is used only if !tilegx_use_struct_convention

OK.

> FIXED(Expand len, i):
> (tilegx-tdep.c:246):	VARINIT(len):	(ULONGEST to
> int)	[(type)->length]
>  - not needed, it is used only if !tilegx_use_struct_convention

OK.

> FIXED(Expand typelen): (tilegx-tdep.c:308):	ASSIGN:
> (ULONGEST to int)	[ typelen =
> (value_enclosing_type(args[i]))->length]
>  - For example stacklen and alignlen are incorrectly not expanded.

OK.

> UNSAFE_ALLOCA: (tilegx-tdep.c:347):	ASSIGN:	(ULONGEST
> to int)	[ typelen = (value_enclosing_type(args[j]))->length]
>  - Please fix as an unrelated patch; it should be also ENSURED

Already done.

> FIXED: (tracepoint.c:1485):	FUNC(add_memrange):
> (ULONGEST to ULONG)	[len]
>  - Expansion of 'addr' is correct but unrelated.

OK. I'll keep it as is since it is trivial anyway.

> FIXED(Expand len): (v850-tdep.c:851):	ASSIGN:	(ULONGEST
> to int)	[ len = (value_type(*args))->length]
>  - Not needed to expand v850-tdep.c:893 due
> to !v850_use_struct_convention by its caller.
>  - Not needed to expand v850-tdep.c:920 due
> to !v850_use_struct_convention by its caller. 

OK.

> SAFE:
> (valarith.c:757):	ASSIGN:	(ULONGEST to int)
> [ inval1len = (type1)->length]
>  - TYPE_CODE_STRING can be also >2GB, it is in fact a sort of array.
>    For example Fortran uses these.
>    Also it needs alloca expansion.

OK will do.

> SAFE: (valarith.c:758):	ASSIGN:	(ULONGEST to
> int)	[ inval2len = (type2)->length]
>  - Likewise.

OK.

> FIXED(Expanded put_frame_register_bytes arg 4):
> (valops.c:1373):	FUNC(put_frame_register_bytes):
> (ULONGEST to int)	[(type)->length]
>  - I do not see any expansion and I do not see why.
>    lval_register values can never be too large.

OK, I missed it and I was right ;)

> FIXED(Expand value_cstring arg
> 2, highbound to LONGEST): (valops.c:1849):
> VARINIT(highbound):	(ULONGEST to int)	[len /
> (char_type)->length]
>  - value_cstring's parameter 'int->LONGEST len' should have been only
> ssize_t. It cannot be larger than 'char *ptr' memory.

OK.

> size_t)	[len] FIXED: (valops.c:1872):
> VARINIT(highbound):	(ULONGEST to int)	[len /
> (char_type)->length]
>  - Likewise for the 'len' parameter.

OK.

> SAFE: (valops.c:1891):
> FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
>  - While TYPE_CODE_BITSTRING is not used in practice I do not see why
> it should not be expanded(=fixed).

I don't get this. I think I have the wrong line numbers and hence not
able to see what you're seeing.


Regards,
Siddhesh

Re: bitpos expansion patches summary

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 4072 bytes --]

On Fri, 07 Sep 2012 12:51:58 +0200, Siddhesh Poyarekar wrote:
> On Sun, 2 Sep 2012 20:15:15 +0200, Jan wrote:
> > I was thinking about some updating of file:lineno records in
> > '.locdiff.report' file to their new location and then diffing the
> > old+reviewed '.locdiff.report' against new file.  But I have not
> > tried it yet in practice.
> 
> This will be heuristic to some extent, which is why I avoided it. 

The current locdiff output is already heuristic.  With the attached source:

perl -pe 's/\Q{+\E(.*)?\Q+}\E/$1/g;s/\Q[-\E(.*)?\Q-]\E//g' <5.s >5p.c;perl -pe 's/\Q{+\E(.*)?\Q+}\E//g;s/\Q[-\E(.*)?\Q-]\E/$1/g' <5.s >5m.c;diff -u 5m.c 5p.c;for i in 5m 5p;do gcc -o $i $i.c -Wall -g;splint -showcolumn $i.c|sed "s/$i[.]c/5.c/"|perl -lpe 's/^(\s*)(\S*?\.[ch]:\d+):/$1LOC $2\n/'|tee $i.splint|perl -lpe 's{^(\s*)LOC (.*)$}{$1LOC}' >$i.splintloc;done;diff -U-1 5{m,p}.splintloc;perl -le 'print "-"x80';/tmp/splint-bitpos/splint-locdiff 5{m,p}.splint

as explanation of the script above the source line
  [-int-]{+long+} len;
gets translated in the two sources accordingly:
  int len;
->
  long len;

--- 5m.splintloc	2012-09-11 20:46:27.727578194 +0200
+++ 5p.splintloc	2012-09-11 20:46:27.785578114 +0200
@@ -1,11 +1,11 @@
 5.c: (in function stub1)
 LOC
  Variable stubj initialized to type long int, expects int: stubi
   To ignore type qualifiers in type comparisons use +ignorequals.
-5.c: (in function f)
+5.c: (in function g)
 LOC
  Assignment of long int to int: b.len = a.len
 5.c: (in function stub2)
 LOC
  Variable stubj initialized to type long int, expects int: stubi
 
--------------------------------------------------------------------------------
LOC 5.c:6
 Variable stubj initialized to type long int, expects int: stubi
  To ignore type qualifiers in type comparisons use +ignorequals.
LOC 5.c:49
 Variable stubj initialized to type long int, expects int: stubi


The first part shows there is only very minor difference of the splint output
while the splint-locdiff already contains no traces of it.

And still the change caused a type safety regression in function g.

I hope such cases do not happen in real but it is still a heuristic hope only.


How do you want to check it in without doing several rebases?

Also I have found several missed expansions only by hand, one needs to do full
re-run of splint on the patched sources.  As the patched sources change line
numbers a bit it already means some sort of rebase.

Also we have now about a month old analysis, all the code checked in the last
month may be broken.

Additionally I am pretty sure the codebase will get broken soon again as it is
common GDB practice to use 'int' for every length and I do not review very
every check-in.  So it would be nice to possibly be able to do such
incremental re-checks in the future; although I am not sure it will be done.


Also IMO (any feedback from other maintainers?) we need full annotation of the
patch file as with such large number of change there is not clear which
changes are justified and whether there are no excessive changes.
	http://people.redhat.com/jkratoch/bitpos3.patch
	(lines starting with 'x')


> Probably a case of different line numbers due to using a different
> updated version? I was rebasing every now and then, which gave me
> different line numbers for some warnings. I see this on line 4695 now.

I stay with 2636a39d8bf9b24dce328e4f906e8710b52d2105 which is a commit very
close to the time you generated the output.

We should always stick to a single commit and document which commit, otherwise
it is difficult to interchange the data.


I will reply to the specific items in next mail.  Then we need some new full
run of splint on patched sources and reuse the existing reviews in some
hauristic way.

And to complete the patch annotation.  I wrote it in a way to make it
automatically cross-checkable.  That each 'x' line in the patch has
corresponding 'report' line and that each FIXED 'report' line has at least one
'x' line in the patch.  I still have no script for checking the 'x' lines.


Thanks,
Jan

[-- Attachment #2: 5.s --]
[-- Type: text/plain, Size: 531 bytes --]

static long used;

static void
stub1 (long stubi)
{
  int stubj = stubi;

  used = stubj;
}

struct s1
{
  long len;
};

struct s2
{
  [-int-]{+long+} len;
};

struct s3
{
  int len;
};

static void
f (void)
{
  struct s1 a = { 0 };
  struct s2 b;

  b.len = a.len;
  used = b.len;
}

static void
g (void)
{
  struct s2 a = { 0 };
  struct s3 b;

  b.len = a.len;
  used = b.len;
}

static void
stub2 (long stubi)
{
  int stubj = stubi;

  used = stubj;
}

int main (void)
{
  stub1 (0);
  stub2 (0);
  f ();
  g ();
  return 0;
}

Re: bitpos expansion patches summary

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> Additionally I am pretty sure the codebase will get broken soon
Jan> again as it is common GDB practice to use 'int' for every length
Jan> and I do not review very every check-in.  So it would be nice to
Jan> possibly be able to do such incremental re-checks in the future;
Jan> although I am not sure it will be done.

Perhaps you could post something here describing your intended rules.

It could be part of patch review.  That obviously won't catch
everything, but we can make an effort at least.

Jan> Also IMO (any feedback from other maintainers?) we need full
Jan> annotation of the patch file as with such large number of change
Jan> there is not clear which changes are justified and whether there
Jan> are no excessive changes.
Jan> 	http://people.redhat.com/jkratoch/bitpos3.patch
Jan> 	(lines starting with 'x')

Ouch, 168 hits.

I guess I'm not really sure what you mean by a full annotation.

Tom

Re: bitpos expansion patches summary

[Jan Kratochvil]

On Tue, 11 Sep 2012 21:26:06 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> Additionally I am pretty sure the codebase will get broken soon
> Jan> again as it is common GDB practice to use 'int' for every length
> Jan> and I do not review very every check-in.  So it would be nice to
> Jan> possibly be able to do such incremental re-checks in the future;
> Jan> although I am not sure it will be done.
> 
> Perhaps you could post something here describing your intended rules.

Yes, I will try to make some summary for further reviews.


> It could be part of patch review.  That obviously won't catch
> everything, but we can make an effort at least.

Yes, it should be.


> Jan> Also IMO (any feedback from other maintainers?) we need full
> Jan> annotation of the patch file as with such large number of change
> Jan> there is not clear which changes are justified and whether there
> Jan> are no excessive changes.
> Jan> 	http://people.redhat.com/jkratoch/bitpos3.patch
> Jan> 	(lines starting with 'x')
> 
> Ouch, 168 hits.
> 
> I guess I'm not really sure what you mean by a full annotation.

$ wget -q -O - http://people.redhat.com/jkratoch/bitpos3.patch|perl -lne '$pn=/^[+](?![+][+] )/;(/^x/?$done:$missing)++ if $p&&!$pn;$p=$pn;END{print "done=$done,missing=$missing";}'
done=120,missing=574

I have annotated 120 changes but still 574 changes need to be annotated.
(A few of them were commented by a suggestion for their removal etc.)


This case I call 'done', annotated:

 /* Append a trace_quick instruction to EXPR, to record N bytes.  */
-extern void ax_trace_quick (struct agent_expr *EXPR, int N);
+extern void ax_trace_quick (struct agent_expr *EXPR, LONGEST N);
xFIXED(Expand N to LONGEST): (ax-gdb.c:516):     FUNC(ax_trace_quick):   (ULONGEST to int)       [(type)->length]


This case I call 'missing', not yet annotated:
   long reg_r0, reg_r1, reg_r2;
-  int total_len = 0;
+  LONGEST total_len = 0;
   enum bfin_abi abi = bfin_abi (gdbarch);



Thanks,
Jan

Re: bitpos expansion patches summary

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

On Tue, 11 Sep 2012 21:04:21 +0200, Jan Kratochvil wrote:
> Also I have found several missed expansions only by hand, one needs to do full
> re-run of splint on the patched sources.  As the patched sources change line
> numbers a bit it already means some sort of rebase.

Attaching some script for remapping the old->new line numbers in a .report
file, I use it locally to play with it without any real results yet.


Jan

[-- Attachment #2: diffmap --]
[-- Type: text/plain, Size: 1146 bytes --]

#! /usr/bin/perl
use strict;
use warnings;

die "$0 'git diff -U999999 old..new|' <old.report >new.report\n" if @ARGV!=1;
my %h;
{
  local *DIFF;
  open DIFF,$ARGV[0] or die $ARGV[0];
  my($from,$fromline,$to,$toline);
  local $_;
  while (<DIFF>) {
    chomp;
    next if /^diff /;
    next if /^index /;
    next if /^new file mode /;
    next if /^deleted file mode /;
    if (m{^--- a/(\S+)$}) {
      $from=$1;
      $fromline=0;
      next;
    }
    if (m{^\Q+++\E b/(\S+)$}) {
      $to=$1;
      $toline=0;
      next;
    }
    next if /^@@ -[10](?:,\d+)? [+][10](?:,\d+)? @@(?:)$/;
    if (/^ /) {
      $fromline++;
      $toline++;
    } elsif (/^-/) {
      $fromline++;
    } elsif (/^[+]/) {
      $toline++;
    } else {
      die;
    }
    $h{$from}{$fromline}=[$to,$toline];
  }
  close DIFF or die $ARGV[0];
}
{
  local $_;
  while (<STDIN>) {
    s{[(]([^():\s]+):(\d+)[)]}{
      my($basename,$line)=($1,$2);
      my $filename="gdb/$basename";
      my $r=$h{$filename}{$line};
      if ($r) {
	die "$filename->".$r->[0] if $r->[0] ne $filename;
	$line=$r->[1];
      }
      "($basename:$line)";
    }e;
    print;
  }
}

Re: bitpos expansion patches summary

[Jan Kratochvil]

On Fri, 07 Sep 2012 12:51:58 +0200, Siddhesh Poyarekar wrote:
> OK, fixed.

+ others, I do not see you have sent any patch update.


Jan

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Thu, 13 Sep 2012 18:48:06 +0200, Jan wrote:
> On Fri, 07 Sep 2012 12:51:58 +0200, Siddhesh Poyarekar wrote:
> > OK, fixed.
> 
> + others, I do not see you have sent any patch update.
> 

I meant fixed in my stash. I intended to finish off and post it earlier
this week, but couldn't get to it yet. I'll get on this today.

Regards,
Siddhesh

Re: bitpos expansion patches summary

[Jan Kratochvil]

Hi Siddhesh,

here is the last part of this first phase.

I will now check some new splint output with the various forgotten cases.


Thanks,
Jan


SAFE: (valprint.c:848):	CMP:	(size_t to ULONGEST)	[(type)->length > sizeof(LONGEST)]
SAFE(float): (valprint.c:1024):	VARINIT(len):	(ULONGEST to UINT)	[(type)->length]
SAFE: (valprint.c:1096):	VARINIT(len):	(ULONGEST to UINT)	[(type)->length]
FIXED(Expand i. Callers could be expanded separately): (valprint.c:1607):	CMP:	(ULONGEST to UINT)	[i < len]
 - Callers should be expanded because of splint warnings there.
   But for example ada-valprint.c:700 is not reported in this file at all, why?
FIXED(Expand rep1): (valprint.c:1631):	CMP:	(ULONGEST to UINT)	[rep1 < len]
FIXED: (valprint.c:1666):	CMP:	(ULONGEST to UINT)	[i < len]
SAFE: (valprint.c:1979):	FUNC(C_alloca):	(ULONGEST to size_t)	[(type)->length]
SAFE: (valprint.c:1982):	FUNC(make_wchar_iterator):	(ULONGEST to size_t)	[(type)->length]
SAFE: (valprint.c:1983):	FUNC(make_wchar_iterator):	(ULONGEST to size_t)	[(type)->length]
SAFE: (valprint.c:2023):	FUNC(print_wchar):	(ULONGEST to int)	[(type)->length]
SAFE: (valprint.c:2030):	FUNC(print_wchar):	(ULONGEST to int)	[(type)->length]
SAFE(char width): (valprint.c:2071):	VARINIT(width):	(ULONGEST to int)	[(type)->length]
SAFE: (valprint.c:2079):	CMP:	(int to ULONGEST)	[length == -1]
 - But as LENGTH is just a length of character the expansion
   'unsigned int'->ULONGEST should be reverted.
SAFE: (valprint.c:2109):	FUNC(make_wchar_iterator):	(ULONGEST to size_t)	[length * width]
SAFE: (valprint.c:2313):	VARINIT(width):	(ULONGEST to int)	[(elttype)->length]
SAFE: (value.c:568):	FUNC(memcmp):	(LONGEST to size_t)	[length]
ENSURED_SIZET: (value.c:697):	FUNC(xzalloc):	(ULONGEST to size_t)	[(val->enclosing_type)->length]
SAFE: (value.c:949):	FUNC(memcpy):	(LONGEST to size_t)	[length]
 - The expansion of int dst_offset, src_offset and length could be just ssize_t (instead of LONGEST).
SAFE: (value.c:1047):	FUNC(memcmp):	(LONGEST to size_t)	[len]
SAFE: (value.c:1402):	FUNC(memcpy):	(ULONGEST to size_t)	[(value_enclosing_type(arg))->length]
SAFE: (value.c:1430):	FUNC(memcpy):	(ULONGEST to size_t)	[(enc_type)->length]
SAFE: (value.c:1957):	FUNC(memcpy):	(ULONGEST to size_t)	[(value_type(newval))->length]
SAFE: (value.c:2421):	VARINIT(len):	(ULONGEST to int)	[(type)->length]
SAFE: (value.c:2477):	ASSIGN:	(ULONGEST to int)	[ len = (type)->length]
ENSURED_SIZET: (value.c:2600):	FUNC(xrealloc):	(ULONGEST to size_t)	[(new_encl_type)->length]
SAFE: (value.c:2647):	CMP:	(int to ULONGEST)	[(type)->length <= (int)sizeof(LONGEST)]
SAFE: (value.c:2648):	ASSIGN:	(LONGEST to int)	[ v->bitpos = bitpos % container_bitsize]
 - The local variable container_bitsize does not need to be LONGEST.
   bitfield cannot be too large, at most about 64 bits or so.
   'type' there is the containing the of the bitfield (such as long long),
   not the whole struct the bitfield is contained in.
   There could be some ENSURE instead as some bogus DWARF could violate that.
SAFE: (value.c:2650):	ASSIGN:	(LONGEST to int)	[ v->bitpos = bitpos % 8]
SAFE: (value.c:2816):	ASSIGN:	(LONGEST to int)	[ bytes_read = ((bitpos % 8) + bitsize + 7) / 8]
SAFE: (value.c:2818):	ASSIGN:	(ULONGEST to int)	[ bytes_read = (field_type)->length]
SAFE: (value.c:2833):	ASSIGN:	(LONGEST to int)	[ lsbcount = (bytes_read * 8 - bitpos % 8 - bitsize)]
SAFE: (value.c:2835):	ASSIGN:	(LONGEST to int)	[ lsbcount = (bitpos % 8)]
SAFE: (value.c:3028):	ASSIGN:	(ULONGEST to int)	[ len = (type)->length]
SAFE: (value.c:3063):	ASSIGN:	(ULONGEST to int)	[ len = (type)->length]
ENSURED_SIZET(allocate_value_lazy): (value.c:3144):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
 - I do not see any need and any change of source code here, it is protected by ENSURE in allocate_value_contents.
ENSURED_SIZET: (value.c:3160):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
 - Likewise.
ENSURED_SIZET: (value.c:3186):	FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
 - Likewise.
UNRELATED(children needs to be expanded maybe later as arrays fix): (varobj.c:3153):	ASSIGN:	(ULONGEST to int)	[ children = (type)->length / (target)->length]
 - You already made some such patch, this issue should be addressed.
   Currently the implementation cannot cope with high number of children
   but that is a different problem, GDB would overflow host memory.
SAFE: (vax-tdep.c:132):	FUNC(write_memory):	(LONGEST to ssize_t)	[len]
SAFE: (vax-tdep.c:227):	FUNC(read_memory):	(LONGEST to ssize_t)	[len]
SAFE: (vax-tdep.c:239):	FUNC(memcpy):	(LONGEST to size_t)	[len]
SAFE: (vax-tdep.c:244):	FUNC(memcpy):	(LONGEST to size_t)	[len]
SAFE: (xstormy16-tdep.c:287):	ASSIGN:	(LONGEST to int)	[ slacklen = typelen & 1]
UNSAFE_ALLOCA: (xstormy16-tdep.c:288):	FUNC(C_alloca):	(LONGEST to size_t)	[typelen + slacklen]
 - Yes, fix it, please.
ENSURED_SIZET: (xstormy16-tdep.c:289):	FUNC(memcpy):	(LONGEST to size_t)	[typelen]
ENSURED_SIZET: (xstormy16-tdep.c:293):	FUNC(write_memory):	(LONGEST to ssize_t)	[typelen + slacklen]
SAFE: (xtensa-tdep.c:1621):	ASSIGN:	(LONGEST to int)	[ offset = 4 - len]
SAFE: (xtensa-tdep.c:1626):	FUNC(regcache_raw_read_part):	(LONGEST to int)	[len]
SAFE: (xtensa-tdep.c:1671):	ASSIGN:	(LONGEST to int)	[ offset = 4 - len]
SAFE: (xtensa-tdep.c:1676):	FUNC(regcache_raw_write_part):	(LONGEST to int)	[len]
SAFE: (xtensa-tdep.c:1822):	ASSIGN:	(ULONGEST to int)	[ info->align = (arg_type)->length]
SAFE: (xtensa-tdep.c:1856):	ASSIGN:	(LONGEST to int)	[ info->u.regno = (gdbarch_tdep(gdbarch)->call_abi == CallAbiCall0Only ? (gdbarch_tdep(gdbarch)->a0_base + 2) : (gdbarch_tdep(gdbarch)->a0_base + 6)) + size / 4]
FIXED(Expand n): (xtensa-tdep.c:1886):	VARINIT(n):	(LONGEST to int)	[info->length]
 - info->length could be just ssize_t; +swap the initialization lines:
     info->length = TYPE_LENGTH (arg_type);
     info->contents = value_contents (arg);
   Then also this n would be just ssize_t.
   Just a nitpick.
SAFE: (xtensa-tdep.c:1897):	FUNC(write_memory):	(LONGEST to ssize_t)	[info->length]
 - And this warning would no longer exist.
FIXED(Expand n): (xtensa-tdep.c:1902):	VARINIT(n):	(LONGEST to int)	[info->length]
 - Likewise.

-------------------------
Warnings I haven't figured out:

LOC breakpoint.c:15874 (breakpoint.c:15873)
 Function observer_attach_memory_changed expects arg 1 to be observer_memory_changed_ftype * gets [function (CORE_ADDR, LONGEST, bfd_byte *) returns void]: invalidate_bp_value_on_memory_change
*** SAFE
 - Again LONGEST len could be just ssize_t.

LOC findcmd.c:184
 Conditional clauses are not of same type: (val_bytes) (LONGEST), (sizeof(int64_t)) (size_t)
*** SAFE
 - Missing some ENSURE.

LOC i386-nat.c:531 (i386-nat.c:553)
 Conditional clauses are not of same type: (max_wp_len - 1) (int), len - 1 (LONGEST)
*** WATCHPOINT
 - I see no problem there now.  i386 does not support large HW watchpoints,
   it just needs to reject gigantic (>4GB) LEN requests.

LOC memrange.c:77
 Conditional clauses are not of same type: (ra->length) (LONGEST), ((rb->start - ra->start) + rb->length) (ULONGEST)
*** SAFE

LOC python/py-type.c:197
 Clauses exit with arg referencing fresh storage in true branch, local storage in false branch
*** SAFE
 - I do not see what does it reference, the line numbers are shifted somehow, I use:
   2636a39d8bf9b24dce328e4f906e8710b52d2105

LOC rs6000-aix-tdep.c:284 (rs6000-aix-tdep.c:283)
 Conditional clauses are not of same type: reg_size (int), len - argbytes (LONGEST)
*** SAFE

LOC s390-tdep.c:2520
 Operands of || are non-boolean (int): is_integer_like(type) || is_pointer_like(type)
*** SAFE

LOC sh-tdep.c:1322
 Function regcache_raw_read expects arg 3 to be gdb_byte * gets char *: (char *)valbuf + i
*** SAFE

LOC stack.c:540 (stack.c:536)
 Assignment of arbitrary UINTegral type to LONGEST: current_offset = ((current_offset + arg_size + sizeof(int) - 1) & ~(sizeof(int) - 1))
*** SAFE

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Tue, 4 Sep 2012 17:03:25 +0200, Jan wrote:
> here is the last part of this first phase.
> 
> I will now check some new splint output with the various forgotten
> cases.

Thanks. I'll look into the last two reviews in this week.

Regards,
Siddhesh

Re: bitpos expansion patches summary

[Siddhesh Poyarekar]

On Tue, 4 Sep 2012 17:03:25 +0200, Jan wrote:
> here is the last part of this first phase.

PHEW!
 
> FIXED(Expand i. Callers could be expanded separately):
> (valprint.c:1607):	CMP:	(ULONGEST to UINT)	[i <
> len]
>  - Callers should be expanded because of splint warnings there.
>    But for example ada-valprint.c:700 is not reported in this file at
> all, why? 

Because callers will be sending in a smaller size, which does not
count as a problem.

> SAFE: (valprint.c:2079):
> CMP:	(int to ULONGEST)	[length == -1]
>  - But as LENGTH is just a length of character the expansion
>    'unsigned int'->ULONGEST should be reverted.

length would be the length of the string; width is the length of
character.

> SAFE:
> (value.c:949):	FUNC(memcpy):	(LONGEST to size_t)
> [length]
>  - The expansion of int dst_offset, src_offset and length could be
> just ssize_t (instead of LONGEST). 

OK. Likewise for value_contents_copy then?

> SAFE: (value.c:2648):	ASSIGN:	(LONGEST to int)
> [ v->bitpos = bitpos % container_bitsize]
>  - The local variable container_bitsize does not need to be LONGEST.
>    bitfield cannot be too large, at most about 64 bits or so.
>    'type' there is the containing the of the bitfield (such as long
> long), not the whole struct the bitfield is contained in.
>    There could be some ENSURE instead as some bogus DWARF could
> violate that.

OK.

> = (type)->length] ENSURED_SIZET(allocate_value_lazy):
> (value.c:3144):	FUNC(memcpy):	(ULONGEST to
> size_t)	[(type)->length]
>  - I do not see any need and any change of source code here, it is
> protected by ENSURE in allocate_value_contents. 

It could be called independently of allocate_value_contents.

> UNRELATED(children needs to be expanded maybe later as arrays fix):
> (varobj.c:3153):	ASSIGN:	(ULONGEST to int)
> [ children = (type)->length / (target)->length]
>  - You already made some such patch, this issue should be addressed.
>    Currently the implementation cannot cope with high number of
> children but that is a different problem, GDB would overflow host
> memory.

Right, I did, but I wanted to stop myself from entering this
rabbit-hole too :)

> [ slacklen = typelen & 1] UNSAFE_ALLOCA:
> (xstormy16-tdep.c:288):	FUNC(C_alloca):	(LONGEST to
> size_t)	[typelen + slacklen]
>  - Yes, fix it, please.

Already fixed and committed.

> FIXED(Expand n):
> (xtensa-tdep.c:1886):	VARINIT(n):	(LONGEST to
> int)	[info->length]
>  - info->length could be just ssize_t; +swap the initialization lines:
>      info->length = TYPE_LENGTH (arg_type);
>      info->contents = value_contents (arg);
>    Then also this n would be just ssize_t.
>    Just a nitpick.

OK.

> LOC breakpoint.c:15874 (breakpoint.c:15873)
>  Function observer_attach_memory_changed expects arg 1 to be
> observer_memory_changed_ftype * gets [function (CORE_ADDR, LONGEST,
> bfd_byte *) returns void]: invalidate_bp_value_on_memory_change ***
> SAFE
>  - Again LONGEST len could be just ssize_t.

OK
 
> LOC findcmd.c:184
>  Conditional clauses are not of same type: (val_bytes) (LONGEST),
> (sizeof(int64_t)) (size_t) *** SAFE
>  - Missing some ENSURE.

OK.

> LOC i386-nat.c:531 (i386-nat.c:553)
>  Conditional clauses are not of same type: (max_wp_len - 1) (int),
> len - 1 (LONGEST) *** WATCHPOINT
>  - I see no problem there now.  i386 does not support large HW
> watchpoints, it just needs to reject gigantic (>4GB) LEN requests.

The separate watchpoints patch will have done this already.


Regards,
Siddhesh