[PATCH] sim: allow memory maps to default to mapped files

[PATCH] sim: allow memory maps to default to mapped files

[Mike Frysinger]

I find it annoying when using --memory-mapfile that I also need to look
up and manually specify the file size to the following --memory-region
option.  So make a length of 0 in the following --memory-region trigger
an auto-sizing of the map to the length of the file being mapped.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>

2010-12-27  Mike Frysinger  <vapier@gentoo.org>

	* sim-memopt.c (do_memopt_add): Set nr_bytes to s.st_size before
	bytes has been calculated and when mmap_next_fd is valid and
	nr_bytes is 0.
	(memory_option_handler): Allow missing size when mmap_next_fd is
	valid.
---
 sim/common/sim-memopt.c |   40 ++++++++++++++++++++++++++++++----------
 1 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/sim/common/sim-memopt.c b/sim/common/sim-memopt.c
index 99e6add..0f71262 100644
--- a/sim/common/sim-memopt.c
+++ b/sim/common/sim-memopt.c
@@ -154,7 +154,27 @@ do_memopt_add (SIM_DESC sd,
       /* Allocate new well-aligned buffer, just as sim_core_attach(). */
       void *aligned_buffer;
       int padding = (addr % sizeof (unsigned64));
-      unsigned long bytes = (modulo == 0 ? nr_bytes : modulo) + padding;
+      unsigned long bytes;
+
+#ifdef HAVE_MMAP
+      struct stat s;
+
+      if (mmap_next_fd >= 0)
+	{
+	  /* Check that given file is big enough. */
+	  int rc = fstat (mmap_next_fd, &s);
+
+	  if (rc < 0)
+	    sim_io_error (sd, "Error, unable to stat file: %s\n",
+			  strerror (errno));
+
+	  /* Autosize the mapping to the file length.  */
+	  if (bytes == 0)
+	    nr_bytes = s.st_size;
+	}
+#endif
+
+      bytes = (modulo == 0 ? nr_bytes : modulo) + padding;
 
       free_buffer = NULL;
       free_length = bytes;
@@ -163,14 +183,9 @@ do_memopt_add (SIM_DESC sd,
       /* Memory map or malloc(). */
       if (mmap_next_fd >= 0)
 	{
-	  /* Check that given file is big enough. */
-	  struct stat s;
-	  int rc;
-
 	  /* Some kernels will SIGBUS the application if mmap'd file
 	     is not large enough.  */ 
-	  rc = fstat (mmap_next_fd, &s);
-	  if (rc < 0 || s.st_size < bytes)
+	  if (s.st_size < bytes)
 	    {
 	      sim_io_error (sd,
 			    "Error, cannot confirm that mmap file is large enough "
@@ -383,10 +398,15 @@ memory_option_handler (SIM_DESC sd, sim_cpu *cpu, int opt,
 	chp = parse_addr (chp, &level, &space, &addr);
 	if (*chp != ',')
 	  {
-	    sim_io_eprintf (sd, "Missing size for memory-region\n");
-	    return SIM_RC_FAIL;
+	    /* let the file autosize */
+	    if (mmap_next_fd == -1)
+	      {
+		sim_io_eprintf (sd, "Missing size for memory-region\n");
+		return SIM_RC_FAIL;
+	      }
 	  }
-	chp = parse_size (chp + 1, &nr_bytes, &modulo);
+	else
+	  chp = parse_size (chp + 1, &nr_bytes, &modulo);
 	/* old style */
 	if (*chp == ',')
 	  modulo = strtoul (chp + 1, &chp, 0);
-- 
1.7.3.1

Re: [PATCH] sim: allow memory maps to default to mapped files

[Joel Brobecker]

> I find it annoying when using --memory-mapfile that I also need to look
> up and manually specify the file size to the following --memory-region
> option.  So make a length of 0 in the following --memory-region trigger
> an auto-sizing of the map to the length of the file being mapped.

Generally speaking, and that's not your fault, it would be nice to
have some documentation about this module in the GDB Users' Manual.
Would you be willing to help us in that department? - nothing fancy,
but something very basic that quickly lists all available option
and what they mean.  This can be treated as a separate patch.

In terms of the user interface for this change, why not just make
the size optional? So, either the user specifies "ADDR,SIZE", or
he says "ADDR".  That way, you do not need to treat zero as special.

I see, now, that this patch has already been checked in - I must have
missed the approval email.  But the suggestions are both still interesting,
IMO.

-- 
Joel

Re: [PATCH] sim: allow memory maps to default to mapped files

[Joel Brobecker]

> I see, now, that this patch has already been checked in - I must have
> missed the approval email.  But the suggestions are both still interesting,
> IMO.

Actually, it looks like the patch was not approved! Can you clarify
a bit its status? Minor comments below:

+	  /* Check that given file is big enough. */

Second space after the period.

+	    /* let the file autosize */

Period and two spaces at end of sentence.

-- 
Joel

Re: [PATCH] sim: allow memory maps to default to mapped files

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1446 bytes --]

On Tuesday, December 28, 2010 01:35:36 Joel Brobecker wrote:
> > I find it annoying when using --memory-mapfile that I also need to look
> > up and manually specify the file size to the following --memory-region
> > option.  So make a length of 0 in the following --memory-region trigger
> > an auto-sizing of the map to the length of the file being mapped.
> 
> Generally speaking, and that's not your fault, it would be nice to
> have some documentation about this module in the GDB Users' Manual.
> Would you be willing to help us in that department? - nothing fancy,
> but something very basic that quickly lists all available option
> and what they mean.  This can be treated as a separate patch.

i dont mind writing documentation, but i dont understand texi, so ive avoided 
the format when possible.  i believe all of the gdb documentation is based in 
that format ?

> In terms of the user interface for this change, why not just make
> the size optional? So, either the user specifies "ADDR,SIZE", or
> he says "ADDR".  That way, you do not need to treat zero as special.

that's basically what my change does.  "addr,0" or just "addr" now work.  
sorry if my commit msg wasnt clear.

> I see, now, that this patch has already been checked in - I must have
> missed the approval email.  But the suggestions are both still interesting,
> IMO.

i dont recall committing this, nor do i see it in cvs ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sim: allow memory maps to default to mapped files

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 377 bytes --]

On Tuesday, December 28, 2010 01:40:29 Joel Brobecker wrote:
> +	  /* Check that given file is big enough. */
> 
> Second space after the period.
> 
> +	    /* let the file autosize */
> 
> Period and two spaces at end of sentence.

i was trying to follow the style of the surrounding code, but if that style is 
offf, i'll just go with what you suggest here
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sim: allow memory maps to default to mapped files

[Joel Brobecker]

> > I see, now, that this patch has already been checked in - I must have
> > missed the approval email.  But the suggestions are both still interesting,
> > IMO.
> 
> i dont recall committing this, nor do i see it in cvs ...

Apologies. I must have been something really good, earlier today...

-- 
Joel

Re: [PATCH] sim: allow memory maps to default to mapped files

[Joel Brobecker]

> i dont mind writing documentation, but i dont understand texi, so ive
> avoided the format when possible.  i believe all of the gdb
> documentation is based in that format ?

Yes, that's correct. texi isn't very difficult, and we have Eli who
helps with any issues around it. There are @[cmd] markers that help
describe the text, and once you know that, you should be able to write
documentation by copy/paste. I found an "introduction to Texinfo" page,
which might be helpful giving you the basics in a short amount of time:

http://lilypond.org/doc/v2.12/Documentation/devel/contrib-guide/Texinfo-introduction-and-usage-policy

> that's basically what my change does.  "addr,0" or just "addr" now work.  
> sorry if my commit msg wasnt clear.

OK.  The patch looks good to me, except for the little formatting nits
that I already pointed out. But can you give it a few more days for
others to comment?  In particular, Doug Evans is more proficient with
the simulator than I am, I believe (which is not hard, considering
that I have barely ever touched that code).

-- 
Joel

Re: [PATCH] sim: allow memory maps to default to mapped files

[Eli Zaretskii]

> From: Mike Frysinger <vapier@gentoo.org>
> Date: Tue, 28 Dec 2010 02:38:02 -0500
> Cc: gdb-patches@sourceware.org, toolchain-devel@blackfin.uclinux.org
> 
> i dont mind writing documentation, but i dont understand texi, so ive avoided 
> the format when possible.

If you can write it as plain text, I can add the Texinfo markup.

Thanks.

Re: [PATCH] sim: allow memory maps to default to mapped files

[Eli Zaretskii]

> Date: Tue, 28 Dec 2010 16:01:20 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org, toolchain-devel@blackfin.uclinux.org
> 
> http://lilypond.org/doc/v2.12/Documentation/devel/contrib-guide/Texinfo-introduction-and-usage-policy

IMO, this mixes too much Lilypond-specific stuff with general-purpose
Texinfo information, so much si that someone who knows no Texinfo will
have hard time figuring out what is specific to Lilypond and what is
general.

Re: [PATCH] sim: allow memory maps to default to mapped files

[Michael Snyder]

Mike Frysinger wrote:
> On Tuesday, December 28, 2010 01:35:36 Joel Brobecker wrote:
>>> I find it annoying when using --memory-mapfile that I also need to look
>>> up and manually specify the file size to the following --memory-region
>>> option.  So make a length of 0 in the following --memory-region trigger
>>> an auto-sizing of the map to the length of the file being mapped.
>> Generally speaking, and that's not your fault, it would be nice to
>> have some documentation about this module in the GDB Users' Manual.
>> Would you be willing to help us in that department? - nothing fancy,
>> but something very basic that quickly lists all available option
>> and what they mean.  This can be treated as a separate patch.
> 
> i dont mind writing documentation, but i dont understand texi, so ive avoided 
> the format when possible.  i believe all of the gdb documentation is based in 
> that format ?

Eli has been very generous about taking plain-text submissions
and tex-ifying them.

[PATCH v2] sim: allow memory maps to default to mapped files

[Mike Frysinger]

I find it annoying when using --memory-mapfile that I also need to look
up and manually specify the file size to the following --memory-region
option.  So make a length of 0 in the following --memory-region trigger
an auto-sizing of the map to the length of the file being mapped.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>

2010-12-27  Mike Frysinger  <vapier@gentoo.org>

	* sim-memopt.c (do_memopt_add): Set nr_bytes to s.st_size before
	bytes has been calculated and when mmap_next_fd is valid and
	nr_bytes is 0.
	(memory_option_handler): Allow missing size when mmap_next_fd is
	valid.
---
v2
	- fix typo in nr_bytes==0 handling

 sim/common/sim-memopt.c |   40 ++++++++++++++++++++++++++++++----------
 1 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/sim/common/sim-memopt.c b/sim/common/sim-memopt.c
index 99e6add..5ef48cb 100644
--- a/sim/common/sim-memopt.c
+++ b/sim/common/sim-memopt.c
@@ -154,7 +154,27 @@ do_memopt_add (SIM_DESC sd,
       /* Allocate new well-aligned buffer, just as sim_core_attach(). */
       void *aligned_buffer;
       int padding = (addr % sizeof (unsigned64));
-      unsigned long bytes = (modulo == 0 ? nr_bytes : modulo) + padding;
+      unsigned long bytes;
+
+#ifdef HAVE_MMAP
+      struct stat s;
+
+      if (mmap_next_fd >= 0)
+	{
+	  /* Check that given file is big enough. */
+	  int rc = fstat (mmap_next_fd, &s);
+
+	  if (rc < 0)
+	    sim_io_error (sd, "Error, unable to stat file: %s\n",
+			  strerror (errno));
+
+	  /* Autosize the mapping to the file length.  */
+	  if (nr_bytes == 0)
+	    nr_bytes = s.st_size;
+	}
+#endif
+
+      bytes = (modulo == 0 ? nr_bytes : modulo) + padding;
 
       free_buffer = NULL;
       free_length = bytes;
@@ -163,14 +183,9 @@ do_memopt_add (SIM_DESC sd,
       /* Memory map or malloc(). */
       if (mmap_next_fd >= 0)
 	{
-	  /* Check that given file is big enough. */
-	  struct stat s;
-	  int rc;
-
 	  /* Some kernels will SIGBUS the application if mmap'd file
 	     is not large enough.  */ 
-	  rc = fstat (mmap_next_fd, &s);
-	  if (rc < 0 || s.st_size < bytes)
+	  if (s.st_size < bytes)
 	    {
 	      sim_io_error (sd,
 			    "Error, cannot confirm that mmap file is large enough "
@@ -383,10 +398,15 @@ memory_option_handler (SIM_DESC sd, sim_cpu *cpu, int opt,
 	chp = parse_addr (chp, &level, &space, &addr);
 	if (*chp != ',')
 	  {
-	    sim_io_eprintf (sd, "Missing size for memory-region\n");
-	    return SIM_RC_FAIL;
+	    /* let the file autosize */
+	    if (mmap_next_fd == -1)
+	      {
+		sim_io_eprintf (sd, "Missing size for memory-region\n");
+		return SIM_RC_FAIL;
+	      }
 	  }
-	chp = parse_size (chp + 1, &nr_bytes, &modulo);
+	else
+	  chp = parse_size (chp + 1, &nr_bytes, &modulo);
 	/* old style */
 	if (*chp == ',')
 	  modulo = strtoul (chp + 1, &chp, 0);
-- 
1.7.3.1

Re: [PATCH] sim: allow memory maps to default to mapped files

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 462 bytes --]

On Tuesday, December 28, 2010 07:01:20 Joel Brobecker wrote:
> OK.  The patch looks good to me, except for the little formatting nits
> that I already pointed out. But can you give it a few more days for
> others to comment?  In particular, Doug Evans is more proficient with
> the simulator than I am, I believe (which is not hard, considering
> that I have barely ever touched that code).

i'll commit in a day or two if no one has any more feedback ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sim: allow memory maps to default to mapped files

[Doug Evans]

On Sat, Jan 8, 2011 at 7:39 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday, December 28, 2010 07:01:20 Joel Brobecker wrote:
>> OK.  The patch looks good to me, except for the little formatting nits
>> that I already pointed out. But can you give it a few more days for
>> others to comment?  In particular, Doug Evans is more proficient with
>> the simulator than I am, I believe (which is not hard, considering
>> that I have barely ever touched that code).
>
> i'll commit in a day or two if no one has any more feedback ...

Hi.  The only comment I have is that it would be nice to keep "struct
stat s;" in its original location, but it's not necessary.

Re: [toolchain-devel] [PATCH] sim: allow memory maps to default to mapped files

[Mike Frysinger]

On Tue, Jan 11, 2011 at 10:07, Doug Evans wrote:
> On Sat, Jan 8, 2011 at 7:39 PM, Mike Frysinger wrote:
>> On Tuesday, December 28, 2010 07:01:20 Joel Brobecker wrote:
>>> OK.  The patch looks good to me, except for the little formatting nits
>>> that I already pointed out. But can you give it a few more days for
>>> others to comment?  In particular, Doug Evans is more proficient with
>>> the simulator than I am, I believe (which is not hard, considering
>>> that I have barely ever touched that code).
>>
>> i'll commit in a day or two if no one has any more feedback ...
>
> Hi.  The only comment I have is that it would be nice to keep "struct
> stat s;" in its original location, but it's not necessary.

i couldnt do that without duplicating the bytes and free_length setup,
and i thought not duplicating those was more important.
-mike

Re: [toolchain-devel] [PATCH] sim: allow memory maps to default to mapped files

[Doug Evans]

On Tue, Jan 11, 2011 at 8:04 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tue, Jan 11, 2011 at 10:07, Doug Evans wrote:
>> On Sat, Jan 8, 2011 at 7:39 PM, Mike Frysinger wrote:
>>> On Tuesday, December 28, 2010 07:01:20 Joel Brobecker wrote:
>>>> OK.  The patch looks good to me, except for the little formatting nits
>>>> that I already pointed out. But can you give it a few more days for
>>>> others to comment?  In particular, Doug Evans is more proficient with
>>>> the simulator than I am, I believe (which is not hard, considering
>>>> that I have barely ever touched that code).
>>>
>>> i'll commit in a day or two if no one has any more feedback ...
>>
>> Hi.  The only comment I have is that it would be nice to keep "struct
>> stat s;" in its original location, but it's not necessary.
>
> i couldnt do that without duplicating the bytes and free_length setup,
> and i thought not duplicating those was more important.

Well, free_length can easily be moved down, it's really only bytes
that's the concern.
It's not worth spending time on it, but having the two ifdefs and having

  int a;
#ifdef FOO
  int b;

  mumble
#endif

feels like it could be cleaner, but for this it's not worth spending time on.

Re: [toolchain-devel] [PATCH] sim: allow memory maps to default to mapped files

[Doug Evans]

On Tue, Jan 11, 2011 at 8:04 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tue, Jan 11, 2011 at 10:07, Doug Evans wrote:
>> On Sat, Jan 8, 2011 at 7:39 PM, Mike Frysinger wrote:
>>> On Tuesday, December 28, 2010 07:01:20 Joel Brobecker wrote:
>>>> OK.  The patch looks good to me, except for the little formatting nits
>>>> that I already pointed out. But can you give it a few more days for
>>>> others to comment?  In particular, Doug Evans is more proficient with
>>>> the simulator than I am, I believe (which is not hard, considering
>>>> that I have barely ever touched that code).
>>>
>>> i'll commit in a day or two if no one has any more feedback ...
>>
>> Hi.  The only comment I have is that it would be nice to keep "struct
>> stat s;" in its original location, but it's not necessary.
>
> i couldnt do that without duplicating the bytes and free_length setup,
> and i thought not duplicating those was more important.

Well, free_length can easily be moved down, it's really only bytes
that's the concern.
It's not worth spending time on it, but having the two ifdefs and having

  int a;
#ifdef FOO
  int b;

  mumble
#endif

feels like it could be cleaner, but for this it's not worth spending time on.

Re: [PATCH] sim: allow memory maps to default to mapped files

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 376 bytes --]

On Monday, December 27, 2010 00:07:51 Mike Frysinger wrote:
> I find it annoying when using --memory-mapfile that I also need to look
> up and manually specify the file size to the following --memory-region
> option.  So make a length of 0 in the following --memory-region trigger
> an auto-sizing of the map to the length of the file being mapped.

ive merged this now
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]