[PATCH 0/4] bitpos expansion summary reloaded

[PATCH 0/4] bitpos expansion summary reloaded

[Siddhesh Poyarekar]

Hi,

This is iteration (insert-large-number-here) of the bitpos expansion
patch with all its reviews, etc.  The patch is now split into 4 parts
as follows:

1) The main bitpos and type.length expansion changes to generic code
2) Additional validation in some places to ensure that a size fits into
   memory (ensure_size_t)
3) Changes to watchpoint functions
4) bitpos and type length related changes to tdep files.  I separated
   these out since they had a related pattern of change, i.e. pushing
   of variables on stack and getting/storing return values.

The 4th part is new, but the content is not very different.  Over time
I have also picked out portions of the patch and pushed them as
independent changes (some of which I rightly got flak for ;)) and as a
result I have been able to shave off a total of about 1k lines off the
patch, making it a bit easier to review.  The split is also aimed at
easing the review.

There is also a 5th patch that Tom has already acked, but I have held
back (and will commit with these patches) since it will be complete only
with the bitpos change:

http://sourceware.org/ml/gdb-patches/2012-08/msg00562.html

I have also updated my repository of splint output reports here:

http://git.siddhesh.in/cgit.cgi/splint.git/

The reports generated earlier were splint-bitpos2.*, the noteworthy ones
being splint-bitpos2.locdiff.processed and
splint-bitpos2.locdiff.report.  The new report generated is now called
splint-bitpos3.locdiff.processed.

There are a few improvements in the scripts that generate these reports
(splint-locdiff and splint-siddhesh-process-locdiff), the main one
being replacement of "arbitrary signed integer" with ssize_t and
"arbitrary unsigned integer" with size_t.  Additionally, some more
warnings are ignored by the script, like assignments from int to
size_t, etc. that are safe.

There is an additional step of processing I have brought in (sorry),
which is done by checkreport.pl.  The way to run it is as follows:

diff -U-1 \
	splint-bitpos2.locdiff.processed \
	splint-bitpos3.locdiff.processed | \
	grep "^[-+](" | \
	perl -e 'while(<>){s{^(.)(\([^:]+):([0-9]+)\):(.*)}{print("$2):$4\t\t$1$3\n")}egm}' | \
	sort | \
	./checkreport.pl | \
	grep -v "^+++" | \
	grep -v "^----" | \
	grep "^+" | sed 's/^.\(.*\)/\1/' > report

This generated the 'report' file in the repo, which I copied over to
report.done and analyzed warnings.  This report contains the new warnings
that are not in splint-bitpos2.locdiff.processed.  The report to view is:

http://git.siddhesh.in/cgit.cgi/splint.git/tree/report.done

Now patches coming up next.

Regards,
Siddhesh

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

Hi Siddhesh,

is anywhere FSF GDB GIT commit hash which all this work is for?


On Thu, 27 Sep 2012 15:30:53 +0200, Siddhesh Poyarekar wrote:
> I have also updated my repository of splint output reports here:
> 
> http://git.siddhesh.in/cgit.cgi/splint.git/

BTW I cannot "git clone" it.  Possibly using downloads of each file from this
web interface I see no logs or timestamps and there are too many files, most of
which are probably no longer worth checking.

So far I do not see how to review the patch as it is not annotated which
change is for which valid splint warning.  Comment from other maintainers is
welcome, annotating will take several more weeks of work.  Without the
annotation the only risk is that some changes are accidentally needless.
As "annotation" I call the /^x/ line below:
       if (info->onstack)
        {
-         int n = info->length;
+         LONGEST n = info->length;
xFIXED(Expand n): (xtensa-tdep.c:1886):  VARINIT(n):     (LONGEST to int)        [info->length]
          CORE_ADDR offset = sp + info->u.offset;


But it is also known that the changeset is not absolutely minimal, in some
cases the type was extended as it is very unclear the LONGEST type is in fact
not needed there and int would be enough.  I do not have such example now but
IIRC I just left some such extensions without comment as valid ones.



Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Siddhesh Poyarekar]

On Fri, 28 Sep 2012 13:20:24 +0200, Jan wrote:
> Hi Siddhesh,
> 
> is anywhere FSF GDB GIT commit hash which all this work is for?

cec90d9d386f57f116e114c50e4287281420f531

> On Thu, 27 Sep 2012 15:30:53 +0200, Siddhesh Poyarekar wrote:
> > I have also updated my repository of splint output reports here:
> > 
> > http://git.siddhesh.in/cgit.cgi/splint.git/
> 
> BTW I cannot "git clone" it.  Possibly using downloads of each file
> from this web interface I see no logs or timestamps and there are too
> many files, most of which are probably no longer worth checking.

git clone http://git.siddhesh.in/splint.git

should work.
 
> So far I do not see how to review the patch as it is not annotated
> which change is for which valid splint warning.  Comment from other

Oh I'm sorry, I thought you intended to do that as part of your review,
so I did not do it.  The older warnings (processed) are in
splint-bitpos2.locdiff.processed and the new ones are in
splint-bitpos3.locdiff.processed.  I generated the reports.done so that
you don't have to redo the earlier review.

Regards,
Siddhesh

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Fri, 28 Sep 2012 13:39:37 +0200, Siddhesh Poyarekar wrote:
> On Fri, 28 Sep 2012 13:20:24 +0200, Jan wrote:
> > > http://git.siddhesh.in/cgit.cgi/splint.git/
> > 
> > BTW I cannot "git clone" it.  Possibly using downloads of each file
> > from this web interface I see no logs or timestamps and there are too
> > many files, most of which are probably no longer worth checking.
> 
> git clone http://git.siddhesh.in/splint.git

$ git clone http://git.siddhesh.in/splint.git
Cloning into 'splint'...
fatal: http://git.siddhesh.in/splint.git/info/refs not found: did you run git update-server-info on the server?
$ git clone http://git.siddhesh.in/cgit.cgi/splint.git/
Cloning into 'splint'...
<hang>
$ rpm -q git
git-1.7.7.6-1.fc16.x86_64


> > So far I do not see how to review the patch as it is not annotated
> > which change is for which valid splint warning.  Comment from other
> 
> Oh I'm sorry, I thought you intended to do that as part of your review,

I do not mind who will do so, I was just clarifying it as an open problem that
someone needs to do it, whether it should be done, whether there is not
a better / automated way to do it etc.


Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Siddhesh Poyarekar]

On Fri, 28 Sep 2012 14:06:42 +0200, Jan wrote:
> $ git clone http://git.siddhesh.in/splint.git
> Cloning into 'splint'...
> fatal: http://git.siddhesh.in/splint.git/info/refs not found: did you
> run git update-server-info on the server?

Ahh sorry. I had not run git update-server-info.  I have now and it
should clone.  It's a lot of data, so it might take a while for the
clone to finish and since it's over http, it will not show (or do) the
usual compression bits and progress.  Unfortunately, my hosting
provider doesn't have native git support yet.

regards,
Siddhesh

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

Hi Siddhesh,

I have reported without effect:
	But for example ada-valprint.c:700 is not reported in this file at all, why?

ada-valprint.c:700 is the last line of this extracted code:
  LONGEST offset;
  int offset_aligned;
  const gdb_byte *valaddr;
  const gdb_byte *ada_aligned_value_addr (struct type *type, const gdb_byte *valaddr);
  offset_aligned = offset + ada_aligned_value_addr (type, valaddr) - valaddr;

Reduced testcase:
int main (void) {
  const char *a = "", *b = "";
  int reported, missed;
  long longvar = 0;
  reported = longvar;
  missed = longvar + a - b;
  return reported + missed; }

splint -hints -linelen 999:
../../../t/t.c: (in function main)
../../../t/t.c:5:3: Assignment of long int to int: reported = longvar

splint -hints -linelen 999 -checks:
../../../t/t.c: (in function main)
../../../t/t.c:5:3: Assignment of long int to int: reported = longvar
../../../t/t.c:6:22: Pointer arithmetic involving possibly null pointer a: longvar + a

splint -hints -linelen 999 -strict:
../../../t/t.c: (in function main)
../../../t/t.c:5:3: Assignment of long int to int: reported = longvar
../../../t/t.c:6:12: Pointer arithmetic (long int, char *): longvar + a
../../../t/t.c:6:22: Pointer arithmetic involving possibly null pointer a: longvar + a
../../../t/t.c:6:12: Operands of - are non-numeric (char *): longvar + a - b

These warnings are in fact OK but the problematic assignment
of ptrdiff_t to int variable 'missed' is not reported at all.

We need to either fix splint or (more likely) use a different tool, there are
too many cases missed this way.


Regards,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Sat, 29 Sep 2012 19:39:38 +0200, Jan Kratochvil wrote:
> We need to either fix splint or (more likely) use a different tool, there are
> too many cases missed this way.

clang-3.0+ (Fedora-17+) can do it:

$ clang -o t t.c -Wshorten-64-to-32
t.c:5:14: warning: implicit conversion loses integer precision: 'long' to 'int' [-Wshorten-64-to-32]
  reported = longvar;
           ~ ^~~~~~~
t.c:6:12: warning: implicit conversion loses integer precision: 'long' to 'int' [-Wshorten-64-to-32]
  missed = longvar + a - b;
         ~ ^~~~~~~~~~~~~~~
2 warnings generated.

For GDB:
CC=clang CFLAGS="-Wshorten-64-to-32" ./configure --disable-werror;make

I did not analyse how to possibly reformat the warnings for their easier later
processing.  The patchset will need some extensions but I think the current
completed work can be completely easily reused.


Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Sat, 29 Sep 2012 20:11:41 +0200, Jan Kratochvil wrote:
> On Sat, 29 Sep 2012 19:39:38 +0200, Jan Kratochvil wrote:
> > We need to either fix splint or (more likely) use a different tool, there are
> > too many cases missed this way.
> 
> clang-3.0+ (Fedora-17+) can do it:
> 
> $ clang -o t t.c -Wshorten-64-to-32
> t.c:5:14: warning: implicit conversion loses integer precision: 'long' to 'int' [-Wshorten-64-to-32]
>   reported = longvar;
>            ~ ^~~~~~~
> t.c:6:12: warning: implicit conversion loses integer precision: 'long' to 'int' [-Wshorten-64-to-32]
>   missed = longvar + a - b;
>          ~ ^~~~~~~~~~~~~~~
> 2 warnings generated.

Or gcc with -Wconversion:
t.c: In function ‘main’:
t.c:5:3: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
t.c:6:24: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]

With gcc-4.8+ featuring the carets:
t.c: In function ‘main’:
t.c:5:3: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
   reported = longvar;
   ^
t.c:6:24: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
   missed = longvar + a - b;
                        ^

This way one can use --enable-targets=all currently incompatible with clang.


Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Siddhesh Poyarekar]

On Sun, 30 Sep 2012 08:52:11 +0200, Jan wrote:
> Or gcc with -Wconversion:
> t.c: In function ‘main’:
> t.c:5:3: warning: conversion to ‘int’ from ‘long int’ may alter its
> value [-Wconversion] t.c:6:24: warning: conversion to ‘int’ from
> ‘long int’ may alter its value [-Wconversion]

This seems good.  However, how about committing these patches if
they're correct and then doing a run with -Wconversion?  The patches
are kinda unwieldy to maintain like this since it's quite painful
to review the entire thing repeatedly and edit changelogs, etc. Besides,
they should be safe for mainline since expansion of types should not
pose a regression risk. I volunteer to do this of course.

Regards,
Siddhesh

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Mon, 01 Oct 2012 07:20:48 +0200, Siddhesh Poyarekar wrote:
> However, how about committing these patches if
> they're correct and then doing a run with -Wconversion?  The patches
> are kinda unwieldy to maintain like this since it's quite painful
> to review the entire thing repeatedly and edit changelogs, etc. Besides,
> they should be safe for mainline since expansion of types should not
> pose a regression risk. I volunteer to do this of course.

I believe the only problem is with ChangeLogs.  When your patch is just
a proposal it is OK to review it without a ChangeLog, ChangeLog can be written
only after its approval when it is like in this case (a) a significant work to
write it and (b) it does not benefit the review process or self-review of the
patch which in this case it IMO does not.

The problem with checking it in partially is that this patch introduces new
conversion regressions which were not caught by splint such as:
  int len1 = TYPE_LENGTH (type);
  void *address1, *address2;
  int len2 = len1 + address1 - address2;
->
  LONGEST len1 = TYPE_LENGTH (type);
  void *address1, *address2;
  int len2 = len1 + address1 - address2;

After such patch gets checked in we will no longer find we need to expand also
LEN2.  Sure we cannot check in the type->LENGTH expansion itself due to the
same reason.

I believe (I may be wrong) there won't be too many new expansions from the
-Wconversion re-verification so you could just incrementally update the
existing patch incl. its ChangeLog.

I am thinking how to review it and I will have to create the patch again
anyway otherwise I do not know how to find out the annotations for it.


Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Siddhesh Poyarekar]

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

On Sun, 30 Sep 2012 08:52:11 +0200, Jan wrote:
> Or gcc with -Wconversion:
> t.c: In function ‘main’:
> t.c:5:3: warning: conversion to ‘int’ from ‘long int’ may alter its
> value [-Wconversion] t.c:6:24: warning: conversion to ‘int’ from
> ‘long int’ may alter its value [-Wconversion]

I played around with this briefly today and there seem to be an
additional 175 warnings that gcc finds and splint doesn't on x86_64.
Here's how I've gone about it:

First, I configured gdb as:

CC="gcc -Wconversion -Wno-sign-conversion" \
CXX="g++ -Wconversion -Wno-sign-conversion" \
../gdb.git/configure --enable-targets=all --disable-werror

So that I get only the truncation warnings and none of the sign
conversion ones.

I built the master and bitpos-expand branches (the latter being the one
with my patches):

make 2> build-warnings.out

and then filtered only the "warning: " lines.

I then processed the two files with this wonderful looking command:

diff -u build-master.out build-bitpos.out |
	perl -e 'while(<>){s{^(.)(.*)}{print "$2$1\n"}egm}' | sort |
	perl -e 'while(<>){s{^([^\]]+\])(.)$}{print "$2$1\n"}egm}' |
	grep "^[-+]" | ./checkreport2.pl | grep -v "^----" |
	grep -v "^+++" | grep "^+" |
	perl -e \
	'while(<>){s{^([^\/]+/)+([^:]+:[^:]+):.*}{print "$2\n"}egm}' |
	while read loc; do
		grep -q "$loc" splint-bitpos3.locdiff.processed
		if [ $? -eq 0 ]; then
			echo "FOUND: $loc"
		else
			echo "NOTFOUND: $loc"
		fi
	done |
	grep "^NOTFOUND"

The checkreport2.pl is attached.  I'll look through this list and see
if any of the warnings are relevant.

Regards,
Siddhesh

[-- Attachment #2: checkreport2.pl --]
[-- Type: application/x-perl, Size: 2148 bytes --]

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Wed, 03 Oct 2012 15:11:55 +0200, Siddhesh Poyarekar wrote:
> I then processed the two files with this wonderful looking command:

It would work as long as both the compilation and
splint-bitpos3.locdiff.processed are whole from the exact one FSF GDB GIT
commit, as your scripting is doing the filtering just based on
filename:lineno.

You may know the best but so far even you said that you were rebasing codebase
multiple times to various commits and I even failed to find some of the
reported lines.

As long as the common commit is right I am fine with your solution.


Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Siddhesh Poyarekar]

On Wed, 3 Oct 2012 20:38:42 +0200, Jan wrote:
> It would work as long as both the compilation and
> splint-bitpos3.locdiff.processed are whole from the exact one FSF GDB
> GIT commit, as your scripting is doing the filtering just based on
> filename:lineno.
> 
> You may know the best but so far even you said that you were rebasing
> codebase multiple times to various commits and I even failed to find
> some of the reported lines.
> 
> As long as the common commit is right I am fine with your solution.

Yeah I had stuck to the same commit.


Siddhesh

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Wed, 03 Oct 2012 15:11:55 +0200, Siddhesh Poyarekar wrote:
[...]
> make 2> build-warnings.out

Probably only in gdb/ directory otherwise it would create more needless output
to deal with.


[...]
> I then processed the two files with this wonderful looking command:
> 
> diff -u build-master.out build-bitpos.out |
> 	perl -e 'while(<>){s{^(.)(.*)}{print "$2$1\n"}egm}' | sort |
> 	perl -e 'while(<>){s{^([^\]]+\])(.)$}{print "$2$1\n"}egm}' |
> 	grep "^[-+]" | ./checkreport2.pl | grep -v "^----" |
> 	grep -v "^+++" | grep "^+" |
> 	perl -e \
> 	'while(<>){s{^([^\/]+/)+([^:]+:[^:]+):.*}{print "$2\n"}egm}' |
> 	while read loc; do
> 		grep -q "$loc" splint-bitpos3.locdiff.processed

This 'grep' has too relaxed search string, for example warnings in "main.c"
(there are some 'conversion' warnings) will match _29_ other files creating
false FOUND lines and therefore fogetting about possibly valid type safety
conversion problems.

There should be sure some fgrep -q "($loc:" or so.


Thanks,
Jan


break-interp-main.c
dmsym_main.c
dw2-cp-infcall-ref-static-main.c
dw2-entry-value-main.c
dw2-inline-param-main.c
dw2-noloc-main.c
dw2-param-error-main.c
dw2-ref-missing-frame-main.c
dw2-unresolved-main.c
gcore-relro-main.c
gdb1555-main.c
jit-dlmain.c
jit-main.c
main.c
mi-main.c
mips16-thunks-inmain.c
mips16-thunks-main.c
mips16-thunks-sinmain.c
print-file-var-main.c
shmain.c
skip-solib-main.c
solib-display-main.c
solib-list-main.c
solib-main.c
solib-overlap-main.c
solib-symbol-main.c
tls-main.c
tls-var-main.c
type-opaque-main.c

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Wed, 03 Oct 2012 21:56:27 +0200, Jan Kratochvil wrote:
> > 		grep -q "$loc" splint-bitpos3.locdiff.processed
> 
> This 'grep' has too relaxed search string, for example warnings in "main.c"
> (there are some 'conversion' warnings) will match _29_ other files creating
> false FOUND lines and therefore fogetting about possibly valid type safety
> conversion problems.
> 
> There should be sure some fgrep -q "($loc:" or so.

$loc includes the line number which also can have false positives on partial
match without anchor so probably something like:
	fgrep -q "($loc)"


Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Siddhesh Poyarekar]

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

Hi,

Here is a fix on top of the bitpos fixes based on the warnings
generated from gcc -Wconversion.  I have also attached the report for
review; I have not rebased since the last submission to ensure that
the line numbers don't go awry. Most of the extra warnings were either
unrelated or were the length parameter to
(store|extract)_(un)?signed_integer functions that are safe.

I have also verified that this does not cause any regressions in the
testsuite and that the gcc warnings generated after this were safe.

Regards,
Siddhesh

gdb/ChangeLog:

	* ada-lang.c (ada_lookup_struct_elt_type): Expand parameter
	DISPP to point to LONGEST.  Expand DISP to LONGEST.
	* breakpoint.c (insert_watchpoint): Expand LENGTH to LONGEST.
	(remove_watchpoint): Likewise.
	(resources_needed_watchpoint): Likewise.
	* hppa-tdep.c (hppa64_push_dummy_call): Expand OFFSET, REGNUM to
	LONGEST.
	* m32r-tdep.c (m32r_push_dummy_call): Expand STACK_ALLOC to
	LONGEST.
	* rs6000-aix-tdep.c (rs6000_push_dummy_call): Expand II to
	LONGEST.
	* tic6x-tdep.c (tic6x_push_dummy_call): Expand STACK_OFFSET to
	LONGEST.
	* valops.c (value_slice): Expand parameters LOWBOUND, LENGTH to
	LONGEST.
	* value.h (value_slice): Likewise.


[-- Attachment #2: extra-fixes.patch --]
[-- Type: text/x-patch, Size: 5423 bytes --]

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 146e733..df499db 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -152,7 +152,7 @@ static struct symbol *find_old_style_renaming_symbol (const char *,
 						      struct block *);
 
 static struct type *ada_lookup_struct_elt_type (struct type *, char *,
-                                                int, int, int *);
+                                                int, int, LONGEST *);
 
 static struct value *evaluate_subexp_type (struct expression *, int *);
 
@@ -6766,7 +6766,7 @@ ada_value_struct_elt (struct value *arg, char *name, int no_err)
 
 static struct type *
 ada_lookup_struct_elt_type (struct type *type, char *name, int refok,
-                            int noerr, int *dispp)
+                            int noerr, LONGEST *dispp)
 {
   int i;
 
@@ -6811,7 +6811,7 @@ ada_lookup_struct_elt_type (struct type *type, char *name, int refok,
     {
       const char *t_field_name = TYPE_FIELD_NAME (type, i);
       struct type *t;
-      int disp;
+      LONGEST disp;
 
       if (t_field_name == NULL)
         continue;
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a7d5e32..e5788eb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10309,7 +10309,7 @@ static int
 insert_watchpoint (struct bp_location *bl)
 {
   struct watchpoint *w = (struct watchpoint *) bl->owner;
-  int length = w->exact ? 1 : bl->length;
+  LONGEST length = w->exact ? 1 : bl->length;
 
   return target_insert_watchpoint (bl->address, length, bl->watchpoint_type,
 				   w->cond_exp);
@@ -10321,7 +10321,7 @@ static int
 remove_watchpoint (struct bp_location *bl)
 {
   struct watchpoint *w = (struct watchpoint *) bl->owner;
-  int length = w->exact ? 1 : bl->length;
+  LONGEST length = w->exact ? 1 : bl->length;
 
   return target_remove_watchpoint (bl->address, length, bl->watchpoint_type,
 				   w->cond_exp);
@@ -10363,7 +10363,7 @@ static int
 resources_needed_watchpoint (const struct bp_location *bl)
 {
   struct watchpoint *w = (struct watchpoint *) bl->owner;
-  int length = w->exact? 1 : bl->length;
+  LONGEST length = w->exact? 1 : bl->length;
 
   return target_region_ok_for_hw_watchpoint (bl->address, length);
 }
diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
index e66f17f..f8444bc 100644
--- a/gdb/hppa-tdep.c
+++ b/gdb/hppa-tdep.c
@@ -951,7 +951,8 @@ hppa64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  int i, offset = 0;
+  int i;
+  LONGEST offset = 0;
   CORE_ADDR gp;
 
   /* "The outgoing parameter area [...] must be aligned at a 16-byte
@@ -965,7 +966,7 @@ hppa64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
       LONGEST len = TYPE_LENGTH (type);
       const bfd_byte *valbuf;
       bfd_byte fptrbuf[8];
-      int regnum;
+      LONGEST regnum;
 
       /* "Each parameter begins on a 64-bit (8-byte) boundary."  */
       offset = align_up (offset, 8);
diff --git a/gdb/m32r-tdep.c b/gdb/m32r-tdep.c
index 438f482..d424776 100644
--- a/gdb/m32r-tdep.c
+++ b/gdb/m32r-tdep.c
@@ -688,7 +688,8 @@ m32r_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		      CORE_ADDR struct_addr)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  int stack_offset, stack_alloc;
+  int stack_offset;
+  LONGEST stack_alloc;
   int argreg = ARG1_REGNUM;
   int argnum;
   struct type *type;
diff --git a/gdb/rs6000-aix-tdep.c b/gdb/rs6000-aix-tdep.c
index 37353f6..6353aa8 100644
--- a/gdb/rs6000-aix-tdep.c
+++ b/gdb/rs6000-aix-tdep.c
@@ -196,7 +196,7 @@ rs6000_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  int ii;
+  LONGEST ii;
   LONGEST len = 0;
   int argno;			/* current argument number */
   LONGEST argbytes;		/* current argument byte */
diff --git a/gdb/tic6x-tdep.c b/gdb/tic6x-tdep.c
index 85fd433..915605a 100644
--- a/gdb/tic6x-tdep.c
+++ b/gdb/tic6x-tdep.c
@@ -895,7 +895,7 @@ tic6x_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 {
   int argreg = 0;
   int argnum;
-  int stack_offset = 4;
+  LONGEST stack_offset = 4;
   LONGEST references_offset = 4;
   CORE_ADDR func_addr = find_function_addr (function, NULL);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
diff --git a/gdb/valops.c b/gdb/valops.c
index 94e8f67..89f3bb7 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -3737,7 +3737,7 @@ value_of_this_silent (const struct language_defn *lang)
    bound as the original ARRAY.  */
 
 struct value *
-value_slice (struct value *array, int lowbound, int length)
+value_slice (struct value *array, LONGEST lowbound, LONGEST length)
 {
   struct type *slice_range_type, *slice_type, *range_type;
   LONGEST lowerbound, upperbound;
diff --git a/gdb/value.h b/gdb/value.h
index 33199f3..2796262 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -936,7 +936,7 @@ extern void preserve_one_value (struct value *, struct objfile *, htab_t);
 
 extern struct value *varying_to_slice (struct value *);
 
-extern struct value *value_slice (struct value *, int, int);
+extern struct value *value_slice (struct value *, LONGEST, LONGEST);
 
 extern struct value *value_literal_complex (struct value *, struct value *,
 					    struct type *);

[-- Attachment #3: gcc-warnings.out.report --]
[-- Type: application/octet-stream, Size: 22254 bytes --]

SAFE: ada-lang.c:2239:34: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: ada-lang.c:2484:39: warning: conversion to ‘gdb_byte’ from ‘unsigned int’ may alter its value [-Wconversion]
SAFE: ada-lang.c:3478:29: warning: conversion to ‘int’ from ‘size_t’ may alter its value [-Wconversion]
SAFE: ada-lang.c:4168:3: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ada-lang.c:4496:26: warning: conversion to ‘int’ from ‘size_t’ may alter its value [-Wconversion]
FIXED(Expand disp, dispp): ada-lang.c:6822:20: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
FIXED: ada-lang.c:6834:24: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
FIXED: ada-lang.c:6864:28: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: ada-lang.c:7416:3: warning: conversion to ‘short int’ from ‘int’ may alter its value [-Wconversion]
SAFE: ada-lang.c:7418:271: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ada-lang.c:7651:11: warning: conversion to ‘short int’ from ‘int’ may alter its value [-Wconversion]
SAFE: ada-lang.c:7653:276: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ada-lang.c:7696:3: warning: conversion to ‘short int’ from ‘int’ may alter its value [-Wconversion]
SAFE: ada-lang.c:7698:288: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ada-lang.c:8712:45: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: alpha-tdep.c:540:48: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: amd64-tdep.c:2764:55: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: arm-tdep.c:3696:4: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: arm-tdep.c:3701:11: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:3805:27: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:3808:3: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: arm-tdep.c:4258:45: warning: conversion to ‘short unsigned int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRElATED: arm-tdep.c:4273:8: warning: conversion to ‘unsigned int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: arm-tdep.c:4277:42: warning: conversion to ‘short unsigned int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:4280:5: warning: conversion to ‘unsigned int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: arm-tdep.c:4319:42: warning: conversion to ‘short unsigned int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:4322:5: warning: conversion to ‘unsigned int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:5265:19: warning: conversion to ‘int’ from ‘CORE_ADDR’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:5268:21: warning: conversion to ‘int’ from ‘CORE_ADDR’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:5274:65: warning: conversion to ‘int’ from ‘CORE_ADDR’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:5277:24: warning: conversion to ‘int’ from ‘CORE_ADDR’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:5881:3: warning: conversion to ‘unsigned char:4’ from ‘unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:5882:3: warning: conversion to ‘unsigned char:1’ from ‘int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:5883:3: warning: conversion to ‘unsigned char:1’ from ‘int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:6037:3: warning: conversion to ‘unsigned char:4’ from ‘unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:6038:3: warning: conversion to ‘unsigned char:1’ from ‘int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:6538:3: warning: conversion to ‘unsigned char:1’ from ‘int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:6979:3: warning: conversion to ‘unsigned char:1’ from ‘int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:6980:3: warning: conversion to ‘unsigned char:1’ from ‘int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:6981:3: warning: conversion to ‘unsigned char:1’ from ‘int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:6982:41: warning: conversion to ‘unsigned char:4’ from ‘unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:7092:46: warning: conversion to ‘unsigned char:1’ from ‘short unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:7093:43: warning: conversion to ‘unsigned char:1’ from ‘short unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:7094:3: warning: conversion to ‘unsigned char:1’ from ‘int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:7930:53: warning: conversion to ‘unsigned int’ from ‘long int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:7933:35: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:8011:32: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: arm-tdep.c:8015:28: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: arm-tdep.c:9163:13: warning: conversion to ‘int’ from ‘size_t’ may alter its value [-Wconversion]
SAFE: avr-tdep.c:1289:11: warning: conversion to ‘int’ from ‘ssize_t’ may alter its value [-Wconversion]
SAFE: avr-tdep.c:304:42: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: avr-tdep.c:310:42: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: avr-tdep.c:321:44: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
FIXED(Expand length): breakpoint.c:10312:29: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
FIXED: breakpoint.c:10324:29: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
FIXED: breakpoint.c:10366:28: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE(MEMBERPTR type): cp-valprint.c:795:11: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE(printable character type): c-valprint.c:191:12: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: dwarf2expr.c:1056:38: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: dwarf2expr.c:265:35: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: dwarf2loc.c:1525:8: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: eval.c:524:3: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: eval.c:524:64: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: eval.c:524:3: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: eval.c:524:64: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: findvar.c:333:47: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: findvar.c:342:45: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: findvar.c:353:38: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: findvar.c:362:36: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: findvar.c:465:59: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: frv-tdep.c:1123:19: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE(Structs passed as pointers): frv-tdep.c:1249:23: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: frv-tdep.c:1331:19: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: gnu-v3-abi.c:534:23: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: gnu-v3-abi.c:537:25: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: gnu-v3-abi.c:645:12: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: gnu-v3-abi.c:655:56: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRElATED: gnu-v3-abi.c:758:35: warning: conversion to ‘hashval_t’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: h8300-tdep.c:697:12: warning: conversion to ‘int’ from ‘ssize_t’ may alter its value [-Wconversion]
SAFE: h8300-tdep.c:796:45: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: h8300-tdep.c:860:53: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: h8300-tdep.c:864:53: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: h8300-tdep.c:890:53: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: h8300-tdep.c:894:53: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: hppa-tdep.c:1052:43: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: hppa-tdep.c:1069:11: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
FIXED(Expand offset): hppa-tdep.c:1075:14: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: i386-darwin-tdep.c:132:18: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: i386-nat.c:531:54: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: ia64-tdep.c:3241:11: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ia64-tdep.c:3258:45: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ia64-tdep.c:3306:11: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ia64-tdep.c:3750:31: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: ia64-tdep.c:3823:9: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: iq2000-tdep.c:118:38: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: iq2000-tdep.c:96:44: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: lm32-tdep.c:285:59: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: lm32-tdep.c:323:45: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: lm32-tdep.c:352:19: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: m32c-tdep.c:2515:38: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: m32c-tdep.c:2530:46: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
FIXED(Expand stack_alloc): m32r-tdep.c:719:17: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: m68hc11-tdep.c:1194:12: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: m88k-tdep.c:286:14: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: mep-tdep.c:2149:18: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: mep-tdep.c:2177:22: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: mep-tdep.c:2345:65: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: mips-tdep.c:4361:10: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: mips-tdep.c:4367:9: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: mips-tdep.c:4446:8: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: mips-tdep.c:5290:8: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: mips-tdep.c:5757:10: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: mips-tdep.c:5761:9: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: mips-tdep.c:5779:4: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: mips-tdep.c:7791:45: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: mt-tdep.c:374:43: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ppc-sysv-tdep.c:1768:46: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ppc-sysv-tdep.c:1831:16: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ppc-sysv-tdep.c:1870:24: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ppc-sysv-tdep.c:824:43: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: ppc-sysv-tdep.c:883:16: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: p-valprint.c:109:8: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: p-valprint.c:159:15: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: p-valprint.c:214:11: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: p-valprint.c:312:61: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: p-valprint.c:324:12: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: python/py-value.c:983:12: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: rl78-tdep.c:731:38: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: rl78-tdep.c:743:44: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: rs6000-aix-tdep.c:300:42: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
FIXED(Expand ii): rs6000-aix-tdep.c:368:7: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
FIXED: rs6000-aix-tdep.c:394:7: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: rs6000-aix-tdep.c:503:46: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: s390-tdep.c:2539:44: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: s390-tdep.c:2542:42: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: score-tdep.c:588:25: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: sh-tdep.c:1134:21: warning: conversion to ‘int’ from ‘ssize_t’ may alter its value [-Wconversion]
SAFE: sh-tdep.c:1140:8: warning: conversion to ‘int’ from ‘ssize_t’ may alter its value [-Wconversion]
SAFE: sh-tdep.c:1156:12: warning: conversion to ‘int’ from ‘ssize_t’ may alter its value [-Wconversion]
SAFE: sh-tdep.c:1164:8: warning: conversion to ‘int’ from ‘ssize_t’ may alter its value [-Wconversion]
SAFE: sh-tdep.c:1261:21: warning: conversion to ‘int’ from ‘ssize_t’ may alter its value [-Wconversion]
SAFE: sh-tdep.c:1267:8: warning: conversion to ‘int’ from ‘ssize_t’ may alter its value [-Wconversion]
SAFE: sh-tdep.c:935:17: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: sparc64-tdep.c:797:21: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE(Structure or union arguments are always <= 16): sparc64-tdep.c:974:15: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: sparc-tdep.c:494:17: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: sparc-tdep.c:504:17: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: spu-tdep.c:1387:14: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: spu-tdep.c:409:38: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: spu-tdep.c:420:44: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: stap-probe.c:1221:50: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: stap-probe.c:1230:40: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: target.c:1515:5: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
UNRELATED: target.c:1517:26: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: tic6x-tdep.c:1024:11: warning: conversion to ‘int’ from ‘ssize_t’ may alter its value [-Wconversion]
FIXED(Expand stack_offset): tic6x-tdep.c:936:20: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: tic6x-tdep.c:984:15: warning: conversion to ‘int’ from ‘ssize_t’ may alter its value [-Wconversion]
UNRELATED: tracepoint.c:3993:24: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: tracepoint.c:3995:20: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: tracepoint.c:4084:7: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: tracepoint.c:4139:50: warning: conversion to ‘int’ from ‘size_t’ may alter its value [-Wconversion]
UNRELATED: tracepoint.c:4144:3: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: utils.c:3615:13: warning: conversion to ‘int’ from ‘size_t’ may alter its value [-Wconversion]
SAFE: valarith.c:1062:21: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: valarith.c:1210:25: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: valarith.c:1340:23: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: valops.c:1014:56: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: valops.c:3175:15: warning: conversion to ‘unsigned int’ from ‘size_t’ may alter its value [-Wconversion]
UNRELATED: valops.c:3177:11: warning: conversion to ‘unsigned int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: valops.c:503:38: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: valprint.c:852:53: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
UNRELATED: value.c:1206:3: warning: conversion to ‘unsigned char:1’ from ‘int’ may alter its value [-Wconversion]
SAFE: value.c:2449:37: warning: conversion to ‘long int’ from ‘DOUBLEST’ may alter its value [-Wconversion]
SAFE: value.c:2454:34: warning: conversion to ‘long int’ from ‘DOUBLEST’ may alter its value [-Wconversion]
SAFE: value.c:3019:3: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: value.c:3028:3: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
SAFE: xstormy16-tdep.c:625:44: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: xstormy16-tdep.c:650:38: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: xtensa-tdep.c:1828:60: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: xtensa-tdep.c:1830:58: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]
SAFE: xtensa-tdep.c:1835:56: warning: conversion to ‘int’ from ‘long unsigned int’ may alter its value [-Wconversion]

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Tom Tromey]

>>>>> "Siddhesh" == Siddhesh Poyarekar <siddhesh@redhat.com> writes:

Siddhesh> Here is a fix on top of the bitpos fixes based on the warnings
Siddhesh> generated from gcc -Wconversion.  I have also attached the
Siddhesh> report for review; I have not rebased since the last
Siddhesh> submission to ensure that the line numbers don't go awry. Most
Siddhesh> of the extra warnings were either unrelated or were the length
Siddhesh> parameter to (store|extract)_(un)?signed_integer functions
Siddhesh> that are safe.

Siddhesh> I have also verified that this does not cause any regressions in the
Siddhesh> testsuite and that the gcc warnings generated after this were safe.

IIUC, this patch fixes some subset of -Wconversion warnings but leaves
the rest untouched.

Would it be very hard or ugly if we just tried to fix them all, and then
enabled -Wconversion in configure?  Aside from maybe some code ugliness,
I wonder what the downsides would be.

The reason I ask is that I'm concerned about our ability to maintain
this change properly, and I wonder if this would be a cheap way to
handle the more mechanical aspects.

Tom

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Mon, 22 Oct 2012 22:45:35 +0200, Tom Tromey wrote:
> IIUC, this patch fixes some subset of -Wconversion warnings but leaves
> the rest untouched.

Yes.  The goal of all this work has been to automatically separate out which
of these warnings are safe and which are not.


> Would it be very hard or ugly if we just tried to fix them all, and then
> enabled -Wconversion in configure?  Aside from maybe some code ugliness,
> I wonder what the downsides would be.

So far I did not consider such task as doable.  This work was about ~660
warnings while I have checked -Wconversion is at least 2500 warnings to solve.
With the FSF ChangeLogs and reviewing rules the situation is more difficult.


> The reason I ask is that I'm concerned about our ability to maintain
> this change properly, and I wonder if this would be a cheap way to
> handle the more mechanical aspects.

Cheap for future reviewing.  But more expensive for the initial change.


Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Tue, 23 Oct 2012 03:34:38 +0200, Jan Kratochvil wrote:
> On Mon, 22 Oct 2012 22:45:35 +0200, Tom Tromey wrote:
> > The reason I ask is that I'm concerned about our ability to maintain
> > this change properly, and I wonder if this would be a cheap way to
> > handle the more mechanical aspects.
> 
> Cheap for future reviewing.  But more expensive for the initial change.

To make clear what is the problem here:

Primarily I admit I did not know until recently there is -Wconversion at all.

The most easy would be to extend to 64-bit any automvaribles handling
TYPE_LENGTH (and some similar fields also being extended to 64-bit).
But this would include 64-bit lengths of scalar types which has been
considered as both performance wasteful and needless work to change them all.

The most difficult work on this patch has been (for me) to always judge
whether a type at that place of code has to be always scalar or not.

It is only about the autovariables as TYPE_LENGTH itself of scalar types has
to be 64-bit already (because that struct type->length field has to be 64-bit
for struct/array types).

IMO we could extend 64-bit length handling everywhere and nothing happens.
But I always expected other maintainers will not accept that as it may have
performance impact on various non-x86* slow 32-bit host GDB platforms.

IMO there are more serious (=algorighmic complexity ones) performance problems
of GDB than some worse constant complexity of 64-bit types.

I do not see how to easily measure the performance impact without implementing
the full 64-bit extension first (but I also believe more work could be
invested in fixing the more serious performance problems than trying to
micro-optimize this 32-bit vs. 64-bit autovariables).

Maybe there are no concerns about extending even scalars to 64-bit?


Regards,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Siddhesh Poyarekar]

On Tue, 23 Oct 2012 03:58:35 +0200, Jan wrote:
> The most easy would be to extend to 64-bit any automvaribles handling
> TYPE_LENGTH (and some similar fields also being extended to 64-bit).
> But this would include 64-bit lengths of scalar types which has been
> considered as both performance wasteful and needless work to change
> them all.

Most of the warnings could be resolved by adding explicit casts
wherever we know for sure that the value is safe to truncate.


Siddhesh

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Tue, 23 Oct 2012 04:28:34 +0200, Siddhesh Poyarekar wrote:
> On Tue, 23 Oct 2012 03:58:35 +0200, Jan wrote:
> > The most easy would be to extend to 64-bit any automvaribles handling
> > TYPE_LENGTH (and some similar fields also being extended to 64-bit).
> > But this would include 64-bit lengths of scalar types which has been
> > considered as both performance wasteful and needless work to change
> > them all.
> 
> Most of the warnings could be resolved by adding explicit casts
> wherever we know for sure that the value is safe to truncate.

This would be the case if we want to enable -Wconversion and also we want to
keep scalar types with 32-bit only variables.

While doable this is the most difficult variant to implement.  One has to
properly verify all the callers/callees pass only scalar types and also never
forget to assert there only a scalar type has been passed - to prevent safety
regression with future changes.


Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Tom Tromey]

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

Jan> So far I did not consider such task as doable.  This work was about
Jan> ~660 warnings while I have checked -Wconversion is at least 2500
Jan> warnings to solve.

Thanks.  I'm sorry to hear it.

Tom

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

Hi Siddhesh,

in the mail
	http://sourceware.org/ml/gdb-patches/2012-09/msg00706.html

I have reported bitpos-patched GDB has the problem on line ada-valprint.c:700.

I do see this incomplete extension neither fixed nor listed in the
gcc-warnings.out.report file.

I see why, the line had the -Wconversion warning even before so it correctly
did not appear in the .report.  But there is missing a bitpos extension now.

When I run -Wconversion by hand I do get:
	ada-valprint.c: In function ‘ada_val_print_1’:
	ada-valprint.c:700:68: error: conversion to ‘int’ from ‘long int’ may alter its value [-Werror=conversion]
while on the unpatched source tree one gets:
	ada-valprint.c: In function ‘ada_val_print_1’:
	ada-valprint.c:699:68: error: conversion to ‘int’ from ‘long int’ may alter its value [-Werror=conversion]

Unless one carefully checks all the uses of any extended variable - which
I was doing occasionally by hand but I understand it is very fragile to do in
all the cases by hand - this method does not work.

So there are these options:

(1) Check in the patchset as is while it is known not all type safety
    regressions have been caught.

(2) Fix all -Wconversion warnings, either by cast or by type extension,
    depending on the case.  But this can be done anytime later.

The whole review effort improved the quality of (1) but it is still not a 100%
fix.

Fortunately (2) can be done in parts, (1) could not have been done in parts as
one cannot separate former -Wconversion vs. latter -Wconversion messages. But
it has been shown (1) cannot be done perfectly without (2).



Regards,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Tom Tromey]

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

Jan> (1) Check in the patchset as is while it is known not all type safety
Jan>     regressions have been caught.

Jan> (2) Fix all -Wconversion warnings, either by cast or by type extension,
Jan>     depending on the case.  But this can be done anytime later.

I think we should start with (1).

My rationale is that I consider the current patch set an improvement.
It may not be perfect, but it doesn't hurt anything, and I think the
various threads have shown that perfecting it as a precondition for
checking it in is too much to ask.


I was less convinced about (2), but while writing up my reasons why, I
convinced myself that it is a good idea.  It will let us notice new
introductions of value truncation problems.  We may make mistakes while
fixing the current code, specifically by introducing incorrect casts --
but whenever we do this, we are not regressing anything, all we are
doing is hiding a latent bug from a warning that we currently do not
enable.  That is, no real change.

This patch may make gdb uglier, in that we'll most likely add many new
casts to the code.  I think the resulting additional safety is probably
worth it.


I'm curious what your opinion is.  I didn't see it in your message.

Tom

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Wed, 24 Oct 2012 20:32:52 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> (1) Check in the patchset as is while it is known not all type safety
> Jan>     regressions have been caught.
> 
> Jan> (2) Fix all -Wconversion warnings, either by cast or by type extension,
> Jan>     depending on the case.  But this can be done anytime later.
> 
> I think we should start with (1).
> 
> My rationale is that I consider the current patch set an improvement.
> It may not be perfect, but it doesn't hurt anything, and I think the
> various threads have shown that perfecting it as a precondition for
> checking it in is too much to ask.

To restate the perfection precondition reason - it was required before it has
been found out for the really right solution we need to do full (2) anyway.
Trying to automatically find all TYPE_LENGTH-derived values truncations would
be impossible anymore after checking-in partial (1) and without the (2) fix.


> This patch may make gdb uglier, in that we'll most likely add many new
> casts to the code.  I think the resulting additional safety is probably
> worth it.

Yes.


> I'm curious what your opinion is.  I didn't see it in your message.

I think (2) - fixing -Wconversion should be done for GDB.  With GDB there only
always remains a question what is a priority of this task.

This whole work has one flaw that GDB cannot access pieces of inferior objects.
Therefore most of naive user operations with >4GB inferior objects will fail
anyway.  Which reminds priorities of GDB TODO items.


Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Tom Tromey]

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

Jan> To restate the perfection precondition reason - it was required
Jan> before it has been found out for the really right solution we need
Jan> to do full (2) anyway.  Trying to automatically find all
Jan> TYPE_LENGTH-derived values truncations would be impossible anymore
Jan> after checking-in partial (1) and without the (2) fix.

I don't think I follow.  It seems to me that the patches in (1) are a
subset of those required for (2).

Jan> I think (2) - fixing -Wconversion should be done for GDB.  With GDB
Jan> there only always remains a question what is a priority of this
Jan> task.

Yeah.  I find this difficult to prioritize.

Tom

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

Hi Siddhesh,

we will go with the fix without the full -Wconversion fix.

As most of the patches are very mechanical could you write a script which will
try to separate the "clear" and human-review parts?

"clear" I find that the patch is understood by the script (that is it is some
s/int/LONGEST/ etc.) and that it does remove >= 1 -Wconversion warnings.

Otherwise I will write such script.  I hope it would "approve" most of the
patches.


Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Siddhesh Poyarekar]

On Thu, 25 Oct 2012 17:54:12 +0200, Jan wrote:
> we will go with the fix without the full -Wconversion fix.
> 
> As most of the patches are very mechanical could you write a script
> which will try to separate the "clear" and human-review parts?
> 
> "clear" I find that the patch is understood by the script (that is it
> is some s/int/LONGEST/ etc.) and that it does remove >= 1
> -Wconversion warnings.
> 
> Otherwise I will write such script.  I hope it would "approve" most
> of the patches.

It would be great if you could do this.  I won't be able to get on it
this week.

Thanks,
Siddhesh

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Thu, 25 Oct 2012 18:51:23 +0200, Siddhesh Poyarekar wrote:
> On Thu, 25 Oct 2012 17:54:12 +0200, Jan wrote:
> > we will go with the fix without the full -Wconversion fix.
> > 
> > As most of the patches are very mechanical could you write a script
> > which will try to separate the "clear" and human-review parts?
> > 
> > "clear" I find that the patch is understood by the script (that is it
> > is some s/int/LONGEST/ etc.) and that it does remove >= 1
> > -Wconversion warnings.
> > 
> > Otherwise I will write such script.  I hope it would "approve" most
> > of the patches.
> 
> It would be great if you could do this.  I won't be able to get on it
> this week.

I have created the scripts below so far:
	http://people.redhat.com/jkratoch/conversiontest.pl
	http://people.redhat.com/jkratoch/buffer.c
	http://people.redhat.com/jkratoch/conversionsplit.pl

But it will be still several weeks of work to split and verify all the patch
chunks and after it is all done GDB will be still buggy wrt the 64-bit offsets
safety as was shown below.

Therefore going to drop it and start just normally fixing all the warnings for
-Wconversion -Wno-sign-conversion where the number of warnings is several
times larger but each such warning is simple and clear to fix.  Unless someone
has a better idea.


Regards,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Tue, 06 Nov 2012 21:01:17 +0100, Jan Kratochvil wrote:
> start just normally fixing all the warnings for -Wconversion

other possibility is just to target the 64-bit offsets avoid the -Wconversion
fixes mostly unrelated to it.

This is a repeating issue like the checked-in cu_offset vs. sect_offset,
therefore to use something like:

typedef struct
{
  int64_t x;
} ssize64_type;

(There was some complain on reversed *_t types, therefore I used _type.))))

I will yet see how beneficial are the -Wconversion fixes, I believe not much
so the ssize64_type way may be at least easier to implement.

BTW it would be all sure much easier with C++ and its operator overloading.


Regards,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Tom Tromey]

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

Jan> This is a repeating issue like the checked-in cu_offset vs. sect_offset,
Jan> therefore to use something like:
Jan> typedef struct
Jan> {
Jan>   int64_t x;
Jan> } ssize64_type;

I'm concerned about how invasive this might turn out to be.
Maybe you could say something reassuring.

Jan> BTW it would be all sure much easier with C++ and its operator
Jan> overloading.

Yeah, but I think it is clear now that this will never happen.
We just have to make the best of the tools we do have.

Tom

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

On Tue, 13 Nov 2012 20:46:07 +0100, Tom Tromey wrote:
> Jan> BTW it would be all sure much easier with C++ and its operator
> Jan> overloading.
> 
> Yeah, but I think it is clear now that this will never happen.
> We just have to make the best of the tools we do have.

Pedro told me there were some reasons in the thread but I do not remember any
valid one, sizes were made invalid.  I will have to re-read the threads again.

There are only abstract opinions that there may exist systems not supporting
this or that but no valid system.

Developing GDB is not constructive this way, it is still about repeated
catching of crashes from forgotten destructor or exception here and there, the
question of next crash is not if but when.


Regards,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Jan Kratochvil]

Hi Siddhesh,

On Sun, 21 Oct 2012 09:35:46 +0200, Siddhesh Poyarekar wrote:
> I have also verified that this does not cause any regressions in the
> testsuite

this patch extra-fixes.patch is not applicable to the commit
cec90d9d386f57f116e114c50e4287281420f531 which we established for this work.
Moreover it is not applicable even to FSF GDB HEAD.


Thanks,
Jan

Re: [PATCH 0/4] bitpos expansion summary reloaded

[Siddhesh Poyarekar]

On Thu, 1 Nov 2012 16:23:45 +0100, Jan wrote:
> this patch extra-fixes.patch is not applicable to the commit
> cec90d9d386f57f116e114c50e4287281420f531 which we established for
> this work. Moreover it is not applicable even to FSF GDB HEAD.
> 

It should apply on top of the earlier patches, not directly on top of
cec90d9d386f57f116e114c50e4287281420f531.  The sequence should be:


bitpos and type.length changes
f77 bounds fixes
ensure size_t
watchpoint changes
tdep changes
extra fixes


Siddhesh