[RFC/RFA] target.c: Check current_target in target_resize_to_sections

[RFC/RFA] target.c: Check current_target in target_resize_to_sections

[Nathan J. Williams]

I recently came across an internal error/malloc failure while running
the gdb.threads/print-threads.exp test with gdbserver. The problem is
related to the target stack and to_sections being copied as part of
the stack. In brief, the sequence of commands that reproduces the
problem is:

$ gdb print-threads
(gdb) set solib-absolute-prefix ...
(gdb) b main
(gdb) target remote remotehost:1234
(gdb) c
(gdb) target remote remotehost:1235
(gdb) c

(with instances of gdbserver running on both ports on the remotehost).

The errors I see are:
gdb in realloc(): warning: modified (page-) pointer
../../../gdb-w/gdb/utils.c:994: internal-error: virtual memory exhausted: can't allocate 2032 bytes

(the former error is from the NetBSD realloc call)

The problem is that update_current_inferior() will copy a valid
to_sections value out of one of the targets in the stack into
current_target, but that pointer may become invalid later when
target_resize_to_sections() is called again on any target using the
same pointer. Finally, when handle_inferior_event() calls
SOLIB_ADD(.., &current_target, ...), target_resize_to_sections() calls
realloc() again on a pointer that's already been realloc'd. "Boom."

My fix is to make target_resize_to_sections update current_target as
well as all of the targets in target_structs. Seems to do the job,
though I can't say it thrills me.

Comments? Suggestions for better approaches? It definitely fixes the
problem, and doesn't seem any messier than the rest of the target
stack stuff. It might be better to not use current_target with
anything that looks at to_sections, but I've no idea how difficult it
might be to do that.

        - Nathan

2004-08-25  Nathan J. Williams  <nathanw@wasabisystems.com>

	* target.c (target_resize_to_sections): Check
	current_target.to_sections for an old value when updating.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.78
diff -u -r1.78 target.c
--- target.c	3 Aug 2004 00:57:26 -0000	1.78
+++ target.c	25 Aug 2004 20:35:37 -0000
@@ -1415,6 +1415,11 @@
 	      (*t)->to_sections_end = target->to_sections_end;
 	    }
 	}
+      if (current_target.to_sections == old_value)
+	{
+	  current_target.to_sections = target->to_sections;
+	  current_target.to_sections_end = target->to_sections_end;
+	}
     }
   
   return old_count;

Re: [RFC/RFA] target.c: Check current_target in target_resize_to_sections

[Andrew Cagney]

> (with instances of gdbserver running on both ports on the remotehost).
> 
> The errors I see are:
> gdb in realloc(): warning: modified (page-) pointer
> ../../../gdb-w/gdb/utils.c:994: internal-error: virtual memory exhausted: can't allocate 2032 bytes
> 
> (the former error is from the NetBSD realloc call)
> 
> The problem is that update_current_inferior() will copy a valid
> to_sections value out of one of the targets in the stack into
> current_target, but that pointer may become invalid later when
> target_resize_to_sections() is called again on any target using the
> same pointer. Finally, when handle_inferior_event() calls
> SOLIB_ADD(.., &current_target, ...), target_resize_to_sections() calls
> realloc() again on a pointer that's already been realloc'd. "Boom."
> 
> My fix is to make target_resize_to_sections update current_target as
> well as all of the targets in target_structs. Seems to do the job,
> though I can't say it thrills me.
> 
> Comments? Suggestions for better approaches? It definitely fixes the
> problem, and doesn't seem any messier than the rest of the target
> stack stuff. It might be better to not use current_target with
> anything that looks at to_sections, but I've no idea how difficult it
> might be to do that.
> 
>         - Nathan
> 
> 2004-08-25  Nathan J. Williams  <nathanw@wasabisystems.com>
> 
> 	* target.c (target_resize_to_sections): Check
> 	current_target.to_sections for an old value when updating.

Can you just add some sort of brief comment noting why current_target 
also needs to be checked.  With that it's ok (but post the revised patch 
when committing).

Long term `current_target', along with this problem, will go away.

Andrew


> Index: target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.78
> diff -u -r1.78 target.c
> --- target.c	3 Aug 2004 00:57:26 -0000	1.78
> +++ target.c	25 Aug 2004 20:35:37 -0000
> @@ -1415,6 +1415,11 @@
>  	      (*t)->to_sections_end = target->to_sections_end;
>  	    }
>  	}
> +      if (current_target.to_sections == old_value)
> +	{
> +	  current_target.to_sections = target->to_sections;
> +	  current_target.to_sections_end = target->to_sections_end;
> +	}
>      }
>    
>    return old_count;
> 
> 

Re: [RFC/RFA] target.c: Check current_target in target_resize_to_sections

[Nathan J. Williams]

Andrew Cagney <cagney@gnu.org> writes:

> > 2004-08-25  Nathan J. Williams  <nathanw@wasabisystems.com>
> > 	* target.c (target_resize_to_sections): Check
> > 	current_target.to_sections for an old value when updating.
> 
> Can you just add some sort of brief comment noting why current_target
> also needs to be checked.  With that it's ok (but post the revised
> patch when committing).

Sure. Should I add the comment to the ChangeLog, the code, or both?
(The code already says what it's trying to do by updating all this
stuff, and while it's not obvious when something like current_target
is missing, it's pretty obvious when it's there).

        - Nathan

Re: [RFC/RFA] target.c: Check current_target in target_resize_to_sections

[Andrew Cagney]

> Andrew Cagney <cagney@gnu.org> writes:
> 
> 
>>>> > 2004-08-25  Nathan J. Williams  <nathanw@wasabisystems.com>
>>>> > 	* target.c (target_resize_to_sections): Check
>>>> > 	current_target.to_sections for an old value when updating.
>>
>>> 
>>> Can you just add some sort of brief comment noting why current_target
>>> also needs to be checked.  With that it's ok (but post the revised
>>> patch when committing).
> 
> 
> Sure. Should I add the comment to the ChangeLog, the code, or both?
> (The code already says what it's trying to do by updating all this
> stuff, and while it's not obvious when something like current_target
> is missing, it's pretty obvious when it's there).

The code.  The ChangeLog contains what was changed, the code why.
It's not so obvious, as otherwize you'd have not found yourself fixing 
the bug :-)  Just some sort of gentle reminder that current_target 
contains a flattened copy of the target stack and hence also needs to be 
updated.

Andrew

Re: [RFC/RFA] target.c: Check current_target in target_resize_to_sections

[Nathan J. Williams]

Committed.

        - Nathan

2004-08-27  Nathan J. Williams  <nathanw@wasabisystems.com>

	* target.c (target_resize_to_sections): Check
	current_target.to_sections for an old value when updating.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.78
diff -u -r1.78 target.c
--- target.c	3 Aug 2004 00:57:26 -0000	1.78
+++ target.c	27 Aug 2004 18:02:57 -0000
@@ -1415,6 +1415,13 @@
 	      (*t)->to_sections_end = target->to_sections_end;
 	    }
 	}
+      /* There is a flattened view of the target stack in current_target,
+	 so its to_sections pointer might also need updating. */
+      if (current_target.to_sections == old_value)
+	{
+	  current_target.to_sections = target->to_sections;
+	  current_target.to_sections_end = target->to_sections_end;
+	}
     }
   
   return old_count;