[PATCH] bfd: Use size_t for length argument totarget_read_memory function passed into bfd_elf_bfd_from_remote_memory

[PATCH] bfd: Use size_t for length argument totarget_read_memory function passed into bfd_elf_bfd_from_remote_memory

[Siddhesh Poyarekar]

Hi,

The target_read_memory function pointer that
bfd_elf_bfd_from_remote_memory accepts current accepts int for length.
I have attached a patch which changes this argument to size_t. This
change is needed because I'm looking to make analogous changes in gdb to
ensure consistency of storage sizes passed across functions to ensure
that larger values are not truncated.

I could write a wrapper around or cast the function pointer explicitly,
but as Jan Kratochvil suggested, it would be cleaner to just make a
change in bfd since it should be taking size_t values anyway. The
conversation thread is here for reference:

http://sourceware.org/ml/gdb-patches/2012-05/msg00909.html

Attached are two patches, the first being changes to bfd and the second
is the change I need to make in gdb to make it work with the changes in
bfd.

Regards,
Siddhesh

bfd/ChangeLog:

2012-05-28  Siddhesh Poyarekar  <siddhesh@redhat.com>

	* bfd-in.h (bfd_elf_bfd_from_remote_memory): Make LEN argument
	of target_read_memory as size_t.
	* bfd-in2.h: Regenerate.
	* elf-bfd.h (elf_backend_bfd_from_remote_memory): Make LEN
	  argument of target_read_memory as size_t.
	(_bfd_elf32_bfd_from_remote_memory): Likewise.
	(_bfd_elf64_bfd_from_remote_memory): Likewise.
	* elf.c (bfd_elf_bfd_from_remote_memory): Likewise.
	* enfcode.h (NAME): Likewise.

gdb/ChangeLog:

2012-05-28  Siddhesh Poyarekar  <siddhesh@redhat.com>

	* target.c (target_read_memory): Make LEN argument as size_t.
	* target.h (target_read_memory): Likewise.

Re: [PATCH] bfd: Use size_t for length argument totarget_read_memory function passed into bfd_elf_bfd_from_remote_memory

[Jan Kratochvil]

On Fri, 01 Jun 2012 20:24:14 +0200, Hans-Peter Nilsson wrote:
> There's always bfd_size_type, though I haven't checked if it
> fits your needs.

bfd_size_type seems to be always >= size_t so it should work.


> For reference, the m32r-elf fail:

I can confirm it with
	./configure --target=m32r-elf; make


On Fri, 01 Jun 2012 21:54:10 +0200, Siddhesh Poyarekar wrote:
> On Fri, 1 Jun 2012 20:24:14 +0200, Hans-Peter wrote:
> > There's always bfd_size_type, though I haven't checked if it
> > fits your needs.
> 
> Should be OK as long as it always matches the size_t definition in the
> stddef.h that gcc ships.

It is larger in some cases than size_t; but GDB can adapt, it is still better
than it was smaller before.


> They're both typically unsigned long, but if
> that is so, then bfd_size_type should have been typedef'd to size_t
> anyway.

Unfortunately bfd_size_type is not always type-compatible with size_t so
passing pointers to prototyped functions would not work.


> > Should bfd.h include sysdep.h or what is missing?

sysdep.h inclusion has caused problems which were avoided by
	Re: recent change broke gdb build
	http://sourceware.org/ml/binutils/2012-05/msg00224.html
but they can be hit in other cases like here, so I do not think bfd.h can
include sysdep.h.


> An stddef.h include in bfd.h should fix this.

stddef.h may not exist on the host system, it should include sysdep.h instead.


> That or I can fix my patch to use bfd_size_type provided its size is always
> equal to size_t, so that the include is not needed. Which way would be
> preferable?

I think the bfd/ part should use bfd_size_type and the gdb/ part should
continue to use size_t where possible, only in some few cases it needs to be
prototype-compatible with bfd/ it would use bfd_size_type.

Going to post a patch today as keeping HEAD broken is not good.


Thanks,
Jan

Re: [PATCH] bfd: Use size_t for length argument totarget_read_memory function passed into bfd_elf_bfd_from_remote_memory

[Siddhesh Poyarekar]

On Fri, 1 Jun 2012 20:24:14 +0200, Hans-Peter wrote:
> There's always bfd_size_type, though I haven't checked if it
> fits your needs.

Should be OK as long as it always matches the size_t definition in the
stddef.h that gcc ships. They're both typically unsigned long, but if
that is so, then bfd_size_type should have been typedef'd to size_t
anyway.
 
> This caused failure to build for simulators for (at least) the
> following targets:
> 
> cris-elf, frv-elf, h8300-elf, iq2000-elf, m32r-elf, mips-elf, and
> mn10300-elf.
> 
> They fail building either of sim/common/cgen-utils.c,
> sim/common/sim-command.c, sim/mips/interp.c, or
> sim/common/nrun.c all due to lack of size_t definition; a
> missing include of stddef.h before its use.
> 
> Should bfd.h include sysdep.h or what is missing?

An stddef.h include in bfd.h should fix this. That or I can fix my
patch to use bfd_size_type provided its size is always equal to
size_t, so that the include is not needed. Which way would be
preferable?

Regards,
Siddhesh

Re: [PATCH] bfd: Use size_t for length argument totarget_read_memory function passed into bfd_elf_bfd_from_remote_memory

[Hans-Peter Nilsson]

> From: Siddhesh Poyarekar <siddhesh@redhat.com>

> The target_read_memory function pointer that
> bfd_elf_bfd_from_remote_memory accepts current accepts int for length.
> I have attached a patch which changes this argument to size_t. This
> change is needed because I'm looking to make analogous changes in gdb to
> ensure consistency of storage sizes passed across functions to ensure
> that larger values are not truncated.

There's always bfd_size_type, though I haven't checked if it
fits your needs.

> 2012-05-28  Siddhesh Poyarekar  <siddhesh@redhat.com>
> 
> 	* bfd-in.h (bfd_elf_bfd_from_remote_memory): Make LEN argument
> 	of target_read_memory as size_t.
> 	* bfd-in2.h: Regenerate.
> 	* elf-bfd.h (elf_backend_bfd_from_remote_memory): Make LEN
> 	  argument of target_read_memory as size_t.
> 	(_bfd_elf32_bfd_from_remote_memory): Likewise.
> 	(_bfd_elf64_bfd_from_remote_memory): Likewise.
> 	* elf.c (bfd_elf_bfd_from_remote_memory): Likewise.
> 	* enfcode.h (NAME): Likewise.

This caused failure to build for simulators for (at least) the
following targets:

cris-elf, frv-elf, h8300-elf, iq2000-elf, m32r-elf, mips-elf, and
mn10300-elf.

They fail building either of sim/common/cgen-utils.c,
sim/common/sim-command.c, sim/mips/interp.c, or
sim/common/nrun.c all due to lack of size_t definition; a
missing include of stddef.h before its use.

Should bfd.h include sysdep.h or what is missing?

For reference, the m32r-elf fail:

gcc -DHAVE_CONFIG_H   -DWITH_DEFAULT_MODEL='"m32r/d"'  -DPROFILE=1 -DWITH_PROFILE=-1   -DWITH_ALIGNMENT=STRICT_ALIGNMENT  -DWITH_TARGET_BYTE_ORDER=BIG_ENDIAN -DWITH_ENVIRONMENT=ALL_ENVIRONMENT   -DWITH_HOST_BYTE_ORDER=LITTLE_ENDIAN     -DWITH_SCACHE=16384       -DM32R_ELF   -I. -I/tmp/hpautotest-sim/src/sim/m32r -I../common -I/tmp/hpautotest-sim/src/sim/m32r/../common -I../../include -I/tmp/hpautotest-sim/src/sim/m32r/../../include -I../../bfd -I/tmp/hpautotest-sim/src/sim/m32r/../../bfd -I../../opcodes -I/tmp/hpautotest-sim/src/sim/m32r/../../opcodes  -g -O2 -c -o cgen-utils.o -MT cgen-utils.o -MMD -MP -MF .deps/cgen-utils.Tpo /tmp/hpautotest-sim/src/sim/m32r/../common/cgen-utils.c
In file included from /tmp/hpautotest-sim/src/sim/m32r/../common/cgen-utils.c:21:
../../bfd/bfd.h:708: error: expected declaration specifiers or '...' before 'size_t'

brgds, H-P

Re: [PATCH] bfd: Use size_t for length argument totarget_read_memory function passed into bfd_elf_bfd_from_remote_memory

[Jan Kratochvil]

On Mon, 28 May 2012 13:03:13 +0200, Alan Modra wrote:
> Since Jan has already given the OK, and these functions are only used
> by gdb, binutils maintainers hardly need to look at the patch.  OK
> anyway, but please fix the ChangeLog

I would like to make the function at least formally really working with the new
types, therefore the patch below.

OK from me for the gdb/ part.


Thanks,
Jan


bfd/
2012-05-28  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* elfcode.h (NAME(_bfd_elf,bfd_from_remote_memory)): Make
	contents_size type bfd_size_type.

diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index c985c63..1ff3f07 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1623,7 +1623,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   Elf_Internal_Phdr *i_phdrs, *last_phdr;
   bfd *nbfd;
   struct bfd_in_memory *bim;
-  int contents_size;
+  bfd_size_type contents_size;
   bfd_byte *contents;
   int err;
   unsigned int i;

Re: [PATCH] bfd: Use size_t for length argument totarget_read_memory function passed into bfd_elf_bfd_from_remote_memory

[Siddhesh Poyarekar]

On Mon, 28 May 2012 20:33:13 +0930, Alan wrote:

> On Mon, May 28, 2012 at 02:35:20PM +0530, Siddhesh Poyarekar wrote:
> > 2012-05-28  Siddhesh Poyarekar  <siddhesh@redhat.com>
> > 
> > 	* bfd-in.h (bfd_elf_bfd_from_remote_memory): Make LEN
> > argument of target_read_memory as size_t.
> > 	* bfd-in2.h: Regenerate.
> > 	* elf-bfd.h (elf_backend_bfd_from_remote_memory): Make LEN
> > 	  argument of target_read_memory as size_t.
> > 	(_bfd_elf32_bfd_from_remote_memory): Likewise.
> > 	(_bfd_elf64_bfd_from_remote_memory): Likewise.
> > 	* elf.c (bfd_elf_bfd_from_remote_memory): Likewise.
> > 	* enfcode.h (NAME): Likewise.
> 
> Since Jan has already given the OK, and these functions are only used
> by gdb, binutils maintainers hardly need to look at the patch.  OK
> anyway, but please fix the ChangeLog
> 
> 	* elfcode.h (NAME(_bfd_elf,bfd_from_remote_memory)): Likewise.
> 

Thanks, fixed ChangeLog for bfd below as per your suggestion.

Regards,
Siddhesh

2012-05-28  Siddhesh Poyarekar  <siddhesh@redhat.com>

	* bfd-in.h (bfd_elf_bfd_from_remote_memory): Make LEN argument
	of target_read_memory as size_t.
	* bfd-in2.h: Regenerate.
	* elf-bfd.h (elf_backend_bfd_from_remote_memory): Make LEN
	argument of target_read_memory as size_t.
	(_bfd_elf32_bfd_from_remote_memory): Likewise.
	(_bfd_elf64_bfd_from_remote_memory): Likewise.
	* elf.c (bfd_elf_bfd_from_remote_memory): Likewise.
	* enfcode.h (NAME(_bfd_elf,bfd_from_remote_memory)): Likewise.

Re: [PATCH] bfd: Use size_t for length argument totarget_read_memory function passed into bfd_elf_bfd_from_remote_memory

[Alan Modra]

On Mon, May 28, 2012 at 02:35:20PM +0530, Siddhesh Poyarekar wrote:
> 2012-05-28  Siddhesh Poyarekar  <siddhesh@redhat.com>
> 
> 	* bfd-in.h (bfd_elf_bfd_from_remote_memory): Make LEN argument
> 	of target_read_memory as size_t.
> 	* bfd-in2.h: Regenerate.
> 	* elf-bfd.h (elf_backend_bfd_from_remote_memory): Make LEN
> 	  argument of target_read_memory as size_t.
> 	(_bfd_elf32_bfd_from_remote_memory): Likewise.
> 	(_bfd_elf64_bfd_from_remote_memory): Likewise.
> 	* elf.c (bfd_elf_bfd_from_remote_memory): Likewise.
> 	* enfcode.h (NAME): Likewise.

Since Jan has already given the OK, and these functions are only used
by gdb, binutils maintainers hardly need to look at the patch.  OK
anyway, but please fix the ChangeLog

	* elfcode.h (NAME(_bfd_elf,bfd_from_remote_memory)): Likewise.

-- 
Alan Modra
Australia Development Lab, IBM

[PATCH] bfd: Use size_t for length argument totarget_read_memory function passed into bfd_elf_bfd_from_remote_memory

[Siddhesh Poyarekar]

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

Hi,

The target_read_memory function pointer that
bfd_elf_bfd_from_remote_memory accepts current accepts int for length.
I have attached a patch which changes this argument to size_t. This
change is needed because I'm looking to make analogous changes in gdb to
ensure consistency of storage sizes passed across functions to ensure
that larger values are not truncated.

I could write a wrapper around or cast the function pointer explicitly,
but as Jan Kratochvil suggested, it would be cleaner to just make a
change in bfd since it should be taking size_t values anyway. The
conversation thread is here for reference:

http://sourceware.org/ml/gdb-patches/2012-05/msg00909.html

Attached are two patches, the first being changes to bfd and the second
is the change I need to make in gdb to make it work with the changes in
bfd.

Regards,
Siddhesh

bfd/ChangeLog:

2012-05-28  Siddhesh Poyarekar  <siddhesh@redhat.com>

	* bfd-in.h (bfd_elf_bfd_from_remote_memory): Make LEN argument
	of target_read_memory as size_t.
	* bfd-in2.h: Regenerate.
	* elf-bfd.h (elf_backend_bfd_from_remote_memory): Make LEN
	  argument of target_read_memory as size_t.
	(_bfd_elf32_bfd_from_remote_memory): Likewise.
	(_bfd_elf64_bfd_from_remote_memory): Likewise.
	* elf.c (bfd_elf_bfd_from_remote_memory): Likewise.
	* enfcode.h (NAME): Likewise.

gdb/ChangeLog:

2012-05-28  Siddhesh Poyarekar  <siddhesh@redhat.com>

	* target.c (target_read_memory): Make LEN argument as size_t.
	* target.h (target_read_memory): Likewise.

[-- Attachment #2: 0001-bfd-changes-for-target_read_memory.patch --]
[-- Type: text/x-patch, Size: 3176 bytes --]

diff --git a/bfd/bfd-in.h b/bfd/bfd-in.h
index a61e721..9617428 100644
--- a/bfd/bfd-in.h
+++ b/bfd/bfd-in.h
@@ -698,7 +698,7 @@ extern int bfd_get_elf_phdrs
    the remote memory.  */
 extern bfd *bfd_elf_bfd_from_remote_memory
   (bfd *templ, bfd_vma ehdr_vma, bfd_vma *loadbasep,
-   int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr, int len));
+   int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr, size_t len));
 
 extern struct bfd_section *_bfd_elf_tls_setup
   (bfd *, struct bfd_link_info *);
diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index efd542f..585a54a 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -705,7 +705,7 @@ extern int bfd_get_elf_phdrs
    the remote memory.  */
 extern bfd *bfd_elf_bfd_from_remote_memory
   (bfd *templ, bfd_vma ehdr_vma, bfd_vma *loadbasep,
-   int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr, int len));
+   int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr, size_t len));
 
 extern struct bfd_section *_bfd_elf_tls_setup
   (bfd *, struct bfd_link_info *);
diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 5426c93..fcfb42a 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -1186,7 +1186,7 @@ struct elf_backend_data
      see elf.c, elfcode.h.  */
   bfd *(*elf_backend_bfd_from_remote_memory)
      (bfd *templ, bfd_vma ehdr_vma, bfd_vma *loadbasep,
-      int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr, int len));
+      int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr, size_t len));
 
   /* This function is used by `_bfd_elf_get_synthetic_symtab';
      see elf.c.  */
@@ -2260,10 +2260,10 @@ extern char *elfcore_write_register_note
 
 extern bfd *_bfd_elf32_bfd_from_remote_memory
   (bfd *templ, bfd_vma ehdr_vma, bfd_vma *loadbasep,
-   int (*target_read_memory) (bfd_vma, bfd_byte *, int));
+   int (*target_read_memory) (bfd_vma, bfd_byte *, size_t));
 extern bfd *_bfd_elf64_bfd_from_remote_memory
   (bfd *templ, bfd_vma ehdr_vma, bfd_vma *loadbasep,
-   int (*target_read_memory) (bfd_vma, bfd_byte *, int));
+   int (*target_read_memory) (bfd_vma, bfd_byte *, size_t));
 
 extern bfd_vma bfd_elf_obj_attr_size (bfd *);
 extern void bfd_elf_set_obj_attr_contents (bfd *, bfd_byte *, bfd_vma);
diff --git a/bfd/elf.c b/bfd/elf.c
index c5b04ac..7a06fb4 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -9556,7 +9556,7 @@ bfd_elf_bfd_from_remote_memory
   (bfd *templ,
    bfd_vma ehdr_vma,
    bfd_vma *loadbasep,
-   int (*target_read_memory) (bfd_vma, bfd_byte *, int))
+   int (*target_read_memory) (bfd_vma, bfd_byte *, size_t))
 {
   return (*get_elf_backend_data (templ)->elf_backend_bfd_from_remote_memory)
     (templ, ehdr_vma, loadbasep, target_read_memory);
diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index c985c63..2c8fe2b 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1615,7 +1615,7 @@ NAME(_bfd_elf,bfd_from_remote_memory)
   (bfd *templ,
    bfd_vma ehdr_vma,
    bfd_vma *loadbasep,
-   int (*target_read_memory) (bfd_vma, bfd_byte *, int))
+   int (*target_read_memory) (bfd_vma, bfd_byte *, size_t))
 {
   Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
   Elf_Internal_Ehdr i_ehdr;	/* Elf file header, internal form */
-- 
1.7.7.6


[-- Attachment #3: 0002-gdb-changes-for-target_read_memory.patch --]
[-- Type: text/x-patch, Size: 1043 bytes --]

diff --git a/gdb/target.c b/gdb/target.c
index 151209e..91b4b47 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1756,7 +1756,7 @@ target_xfer_partial (struct target_ops *ops,
    it makes no progress, and then return how much was transferred).  */
 
 int
-target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
+target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, size_t len)
 {
   /* Dispatch to the topmost target, not the flattened current_target.
      Memory accesses check target->to_has_(all_)memory, and the
diff --git a/gdb/target.h b/gdb/target.h
index f80fba0..f3ef33a 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -996,7 +996,8 @@ extern void target_dcache_invalidate (void);
 
 extern int target_read_string (CORE_ADDR, char **, int, int *);
 
-extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len);
+extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
+			       size_t len);
 
 extern int target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, int len);
 
-- 
1.7.7.6