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

[Aleksandar Ristovski <aristovski@qnx.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;
  }