Re: [patch 4/6] Prepare linux_find_memory_regions_full & co. for move

[Aleksandar Ristovski <aristovski at qnx dot com>]

On 13-04-02 09:07 AM, Jan Kratochvil wrote:
> On Mon, 01 Apr 2013 21:44:04 +0200, Aleksandar Ristovski wrote:
> [...]
> > --- a/gdb/target.c
> > +++ b/gdb/target.c
> [...]
> > +static LONGEST
> > +target_fileio_read_alloc_1 (const char *filename,
> > +			    gdb_byte **buf_p, int padding)
> > +{
> > +  struct cleanup *close_cleanup;
> > +  int fd, target_errno;
> > +  void *memory_to_free = NULL;
> > +  LONGEST retval;
> > +
> > +  fd = target_fileio_open (filename, FILEIO_O_RDONLY, 0700, &target_errno);
> > +  if (fd == -1)
> > +    return -1;
> > +
> > +  close_cleanup = make_cleanup (target_fileio_close_cleanup, &fd);
> > +
> > +  make_cleanup (free_current_contents, &memory_to_free);
> > +  retval = read_alloc (buf_p, fd, target_fileio_read_alloc_1_pread, padding,
> > +		       &memory_to_free);
>
>
> > +  if (retval >= 0)
> > +    {
> > +      /* Returned allocated memory is interesting for the caller.  */
> > +      memory_to_free = NULL;
> >       }
>
> BTW ">= 0" is incorrect, the decision of filled in *BUF_P should be "> 0", see
> target_fileio_read_stralloc in FSF GDB:
>    if (transferred < 0)
>      return NULL;
>    if (transferred == 0)
>      return xstrdup ("");
>    bufstr[transferred] = 0;
> or even the documentation of target_fileio_read_alloc:
>     If a positive value is returned, a
>     sufficiently large buffer will be allocated using xmalloc and
>     returned in *BUF_P containing the contents of the object.
>
>
> But I find you wrote a workaround of my bug in the code of read_alloc, the
> should have been:
>            if (n < 0 || (n == 0 && buf_pos == 0))
> 	    xfree (buf);
>            else
>              *buf_p = buf;
> 	  if (memory_to_free_ptr != NULL)
> 	    *memory_to_free_ptr = NULL;
>
> Then target_fileio_read_alloc_1 can contain just (if you want):
> 	gdb_assert (memory_to_free == NULL);

Ok, diff from the code at patch 4 (i.e. after applying patches 1-4) that 
I will incorporate into patch 4 is pasted below.

I did not put any assert in target_fileio_read_alloc_1 - I don't think 
it's useful.

Thanks,

Aleksandar




diff --git a/gdb/target.c b/gdb/target.c
index d8ab056..ed0f3bf 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3522,17 +3522,11 @@ read_alloc (gdb_byte **buf_p, int handle, 
read_alloc_pre
       if (n <= 0)
        {
          if (n < 0 || (n == 0 && buf_pos == 0))
-           {
-             if (memory_to_free_ptr != NULL)
-               {
-                 gdb_assert (buf == *memory_to_free_ptr);
-                 *memory_to_free_ptr = NULL;
-               }
-             xfree (buf);
-           }
+           xfree (buf);
          else
            *buf_p = buf;
-
+         if (memory_to_free_ptr != NULL)
+           *memory_to_free_ptr = NULL;
          if (n < 0)
            {
              /* An error occurred.  */
@@ -3576,11 +3570,6 @@ target_fileio_read_alloc_1 (const char *filename,
   make_cleanup (free_current_contents, &memory_to_free);
   retval = read_alloc (buf_p, fd, target_fileio_read_alloc_1_pread, 
padding,
                       &memory_to_free);
-  if (retval >= 0)
-    {
-      /* Returned allocated memory is interesting for the caller.  */
-      memory_to_free = NULL;
-    }
   do_cleanups (close_cleanup);
   return retval;
 }