Re: Patch for DejaGnu test case "pointers"

[Kevin Buettner <kevinb@cygnus.com>]

On Sep 7,  4:03pm, Orjan Friberg wrote:

> [...] This is my first patch submission
> (more patches are on the way), so I'd appreciate feedback if any changes
> are needed.

In addition to providing ChangeLog entries (as noted by Hans-Peter
Nilsson), you should keep the diff headers intact.  I.e, you should
preserve the lines that appeared in your diff output immediately
before the following lines:

> *************** int more_code()
> *** 194,200 ****

Kevin
From jtc@redback.com Thu Sep 07 13:15:00 2000
From: jtc@redback.com (J.T. Conklin)
To: gdb-patches@sourceware.cygnus.com
Subject: [PATCH]: move i386nbsd_
Date: Thu, 07 Sep 2000 13:15:00 -0000
Message-id: <5m1yywq7rk.fsf@jtc.redback.com>
X-SW-Source: 2000-09/msg00066.html
Content-length: 1743

Someone reported a problem building a NetBSD/i386 cross-debugger because
i386nbsd_use_struct_convention() was in a native-only file instead of a
target file.  I applied the enclosed patch to fix this.

        --jtc

2000-09-07  J.T. Conklin  <jtc@redback.com>

	* config/i386/nbsd.mt (TDEPFILES): Add i386nbsd-tdep.o.
	* i386nbsd-nat.c (i386nbsd_use_struct_convention): Moved from here.
	* i386nbsd-tdep.c (i386nbsd_use_struct_convention): To here.
        * i386nbsd-tdep.c: New file.

Index: i386nbsd-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/i386nbsd-nat.c,v
retrieving revision 1.4
diff -c -r1.4 i386nbsd-nat.c
*** i386nbsd-nat.c	2000/07/30 01:48:25	1.4
--- i386nbsd-nat.c	2000/09/07 18:22:15
***************
*** 147,161 ****
  	  (PTRACE_ARG3_TYPE) &inferior_fpregisters, 0);
  }
  \f
- int
- i386nbsd_use_struct_convention (int gcc_p, struct type *type)
- {
-   return !(TYPE_LENGTH (type) == 1
- 	   || TYPE_LENGTH (type) == 2
- 	   || TYPE_LENGTH (type) == 4
- 	   || TYPE_LENGTH (type) == 8);
- }
- \f
  struct md_core
  {
    struct reg intreg;
--- 147,152 ----
Index: config/i386/nbsd.mt
===================================================================
RCS file: /cvs/src/src/gdb/config/i386/nbsd.mt,v
retrieving revision 1.3
diff -c -r1.3 nbsd.mt
*** nbsd.mt	2000/05/24 04:16:27	1.3
--- nbsd.mt	2000/09/07 18:22:24
***************
*** 1,5 ****
  # Target: Intel 386 running NetBSD
! TDEPFILES= i386-tdep.o i387-tdep.o
  TM_FILE= tm-nbsd.h
  
  GDBSERVER_DEPFILES= low-nbsd.o
--- 1,5 ----
  # Target: Intel 386 running NetBSD
! TDEPFILES= i386-tdep.o i387-tdep.o i386nbsd-tdep.o
  TM_FILE= tm-nbsd.h
  
  GDBSERVER_DEPFILES= low-nbsd.o

-- 
J.T. Conklin
RedBack Networks
From kettenis@wins.uva.nl Thu Sep 07 13:19:00 2000
From: Mark Kettenis <kettenis@wins.uva.nl>
To: gdb-patches@sourceware.cygnus.com
Subject: [PATCH] lin-lwp.c fix
Date: Thu, 07 Sep 2000 13:19:00 -0000
Message-id: <200009072019.e87KJMm00944@delius.kettenis.local>
X-SW-Source: 2000-09/msg00067.html
Content-length: 6408

The attached patch (checked in) fixes a fairly critical bug in the new
Linux threads code (debugging threaded code might cause GDB to
terminate with one of the real-time signals).  I reorganized the
signal handling stuff a bit so that now the SIGCHLD signal is no
longer automatically blocked in the inferior.  This makes the failures
in gdb.base/sigall.exp disappear :-).

Mark


2000-09-06  Mark Kettenis  <kettenis@gnu.org>

	* lin-lwp.c (normal_mask, blocked_mask): New variables.
	(lin_lwp_wait): Block SIGCHLD here if it isn't already blocked.
	(lin_lwp_mourn_inferior): Restore the origional signal mask, and
	reset the mask of blocked signals.
	(_initialize_lin_lwp): Don't block SIGCHLD here, but do initialize
	suspend_mask and blocked_mask.  This makes us pass
	gdb.base/sigall.exp for Linux/x86 now.
	(lin_thread_get_thread_signals): Treat the LinuxThreads "cancel"
	signal similarly to SIGCHLD in the generic code.  Avoids GDB being
	terminated by a Real-time signal.


Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.1
diff -u -p -r1.1 lin-lwp.c
--- lin-lwp.c	2000/09/03 18:41:28	1.1
+++ lin-lwp.c	2000/09/07 20:05:28
@@ -55,9 +55,11 @@ extern const char *strsignal (int sig);
    code:
 
    - In general one should specify the __WCLONE flag to waitpid in
-     order to make it report events for any of the cloned processes.
-     However, if a cloned process has exited the exit status is only
-     reported if the __WCLONE flag is absent.
+     order to make it report events for any of the cloned processes
+     (and leave it out for the initial process).  However, if a cloned
+     process has exited the exit status is only reported if the
+     __WCLONE flag is absent.  Linux 2.4 has a __WALL flag, but we
+     cannot use it since GDB must work on older systems too.
 
    - When a traced, cloned process exits and is waited for by the
      debugger, the kernel reassigns it to the origional parent and
@@ -126,9 +128,26 @@ static struct target_ops lin_lwp_ops;
 /* The standard child operations.  */
 extern struct target_ops child_ops;
 
+/* Since we cannot wait (in lin_lwp_wait) for the initial process and
+   any cloned processes with a single call to waitpid, we have to use
+   use the WNOHANG flag and call waitpid in a loop.  To optimize
+   things a bit we use `sigsuspend' to wake us up when a process has
+   something to report (it will send us a SIGCHLD if it has).  To make
+   this work we have to juggle with the signal mask.  We save the
+   origional signal mask such that we can restore it before creating a
+   new process in order to avoid blocking certain signals in the
+   inferior.  We then block SIGCHLD during the waitpid/sigsuspend
+   loop.  */
+
+/* Origional signal mask.  */
+static sigset_t normal_mask;
+
 /* Signal mask for use with sigsuspend in lin_lwp_wait, initialized in
    _initialize_lin_lwp.  */
 static sigset_t suspend_mask;
+
+/* Signals to block to make that sigsuspend work.  */
+static sigset_t blocked_mask;
 \f
 
 /* Prototypes for local functions.  */
@@ -479,7 +498,7 @@ stop_wait_callback (struct lwp_info *lp,
 		     is_cloned (lp->pid) ? __WCLONE : 0);
       if (pid == -1 && errno == ECHILD)
 	/* OK, the proccess has disappeared.  We'll catch the actual
-           exit event in lin_lwp_wait.  */
+	   exit event in lin_lwp_wait.  */
 	return 0;
 
       gdb_assert (pid == GET_LWP (lp->pid));
@@ -575,6 +594,13 @@ lin_lwp_wait (int pid, struct target_wai
   int options = 0;
   int status = 0;
 
+  /* Make sure SIGCHLD is blocked.  */
+  if (! sigismember (&blocked_mask, SIGCHLD))
+    {
+      sigaddset (&blocked_mask, SIGCHLD);
+      sigprocmask (SIG_BLOCK, &blocked_mask, NULL);
+    }
+
  retry:
 
   /* First check if there is a LWP with a wait status pending.  */
@@ -659,7 +685,8 @@ lin_lwp_wait (int pid, struct target_wai
 	      lp = add_lwp (BUILD_LWP (lwpid, inferior_pid));
 	      if (threaded)
 		{
-		  gdb_assert (WIFSTOPPED (status) && WSTOPSIG (status) == SIGSTOP);
+		  gdb_assert (WIFSTOPPED (status)
+			      && WSTOPSIG (status) == SIGSTOP);
 		  lp->signalled = 1;
 
 		  if (! in_thread_list (inferior_pid))
@@ -863,6 +890,10 @@ lin_lwp_mourn_inferior (void)
 
   trap_pid = 0;
 
+  /* Restore the origional signal mask.  */
+  sigprocmask (SIG_SETMASK, &normal_mask, NULL);
+  sigemptyset (&blocked_mask);
+
 #if 0
   target_beneath = find_target_beneath (&lin_lwp_ops);
 #else
@@ -978,7 +1009,6 @@ void
 _initialize_lin_lwp (void)
 {
   struct sigaction action;
-  sigset_t mask;
 
   extern void thread_db_init (struct target_ops *);
 
@@ -986,18 +1016,19 @@ _initialize_lin_lwp (void)
   add_target (&lin_lwp_ops);
   thread_db_init (&lin_lwp_ops);
 
+  /* Save the origional signal mask.  */
+  sigprocmask (SIG_SETMASK, NULL, &normal_mask);
+
   action.sa_handler = sigchld_handler;
   sigemptyset (&action.sa_mask);
   action.sa_flags = 0;
   sigaction (SIGCHLD, &action, NULL);
 
-  /* We block SIGCHLD throughout this code ...  */
-  sigemptyset (&mask);
-  sigaddset (&mask, SIGCHLD);
-  sigprocmask (SIG_BLOCK, &mask, &suspend_mask);
-
-  /* ... except during a sigsuspend.  */
+  /* Make sure we don't block SIGCHLD during a sigsuspend.  */
+  sigprocmask (SIG_SETMASK, NULL, &suspend_mask);
   sigdelset (&suspend_mask, SIGCHLD);
+
+  sigemptyset (&blocked_mask);
 }
 \f
 
@@ -1030,8 +1061,8 @@ get_signo (const char *name)
 void
 lin_thread_get_thread_signals (sigset_t *set)
 {
-  int restart;
-  int cancel;
+  struct sigaction action;
+  int restart, cancel;
 
   sigemptyset (set);
 
@@ -1045,4 +1076,21 @@ lin_thread_get_thread_signals (sigset_t 
 
   sigaddset (set, restart);
   sigaddset (set, cancel);
+
+  /* The LinuxThreads library makes terminating threads send a special
+     "cancel" signal instead of SIGCHLD.  Make sure we catch those (to
+     prevent them from terminating GDB itself, which is likely to be
+     their default action) and treat them the same way as SIGCHLD.  */
+
+  action.sa_handler = sigchld_handler;
+  sigemptyset (&action.sa_mask);
+  action.sa_flags = 0;
+  sigaction (cancel, &action, NULL);
+
+  /* We block the "cancel" signal throughout this code ...  */
+  sigaddset (&blocked_mask, cancel);
+  sigprocmask (SIG_BLOCK, &blocked_mask, NULL);
+
+  /* ... except during a sigsuspend.  */
+  sigdelset (&suspend_mask, cancel);
 }
From ac131313@cygnus.com Thu Sep 07 23:13:00 2000
From: Andrew Cagney <ac131313@cygnus.com>
To: GDB Patches <gdb-patches@sourceware.cygnus.com>
Subject: [patch] Re-generate aclocal.m4
Date: Thu, 07 Sep 2000 23:13:00 -0000
Message-id: <39B882E9.1F2444B1@cygnus.com>
X-SW-Source: 2000-09/msg00068.html
Content-length: 285

FYI,

I've re-generated aclocal.m4 and along with everything else.  Recent
../bfd changes didn't get correctly propogated into GDB.

	Andrew

Thu Sep  7 21:59:23 2000  Andrew Cagney  <cagney@b1.cygnus.com>

        * aclocal.m4: Regenerate.
        * config.in, configure: Regenerate.
From ac131313@cygnus.com Thu Sep 07 23:43:00 2000
From: Andrew Cagney <ac131313@cygnus.com>
To: Kevin Buettner <kevinb@cygnus.com>, Daniel Berlin <dan@cgsoftware.com>
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH] C++ patches
Date: Thu, 07 Sep 2000 23:43:00 -0000
Message-id: <39B88998.3DE3966@cygnus.com>
References: <Pine.LNX.4.21.0009062118160.17951-200000@propylaea.anduin.com> <1000907045542.ZM25066@ocotillo.lan>
X-SW-Source: 2000-09/msg00069.html
Content-length: 2039

Kevin Buettner wrote:
> 
> On Sep 6,  9:35pm, Daniel Berlin wrote:
> 
> > I've rerun indent on the files this affects, because the formatting has
> > gotten way out of whack with the GNU standards.
> > I hope nobody minds, if somebody does, i'll undo the whitespace changes in
> > my patch
> > (diff -c3pwBb, remove the unbreaking of lines it did).
> 
> I think the generally agreed upon practice is to do two separate
> commits; the first being the actual changes you made and the second
> being the reformatting.  This way it's easier to review the first
> patch.

Either order.  The main thing is to keep indentation and real codeing
changes separate.  A maintainer shouldn't be presented with a patch that
contains a combination of both.  Reviewing it is impossible :-(.

Don't forget to clean the diff up a little (strip out the ChangeLog etc)
next time :-)

> > Please note we now have one memory leak in lookup_symbol. The demangler
> > gives us a buffer we never free. I'd need to free it at every exit point
> > from the function, of which we have 12 right now, all of which end almost
> > the exact same way (most return fixup_symbol_section(sym), a few just
> > return sym. Is this incorrect?).
> >
> > I'm thinking that i'll change it so we just have a label at the end, and
> > 12 gotos, rather than 12 exit points, and do the cleanup of the
> > buffer there.
> > It would make it about 50x easier for someone who needed to do something
> > that requires cleanup when we exit.
> 
> I think gotos in this case are preferable.

Gotos are bad.  M'kay?  We've so far spent three years trying to
eliminate all the goto's in WFI, adding more will just make me depressed
:-)

Why not push the ALL_SYMTABS() loop into a sub function parameterized by
``name'' - I'm assuming that it is the loop that has the N different
exit points?  That way you get your goto without actually having a
goto...  Hmm, I suspect that ``name'' will need a cleanup.  Throw an
error and, regardless of the gotos, it will leak memory.

Keep it up.

	Andrew
From dberlin@redhat.com Fri Sep 08 06:36:00 2000
From: Daniel Berlin <dberlin@redhat.com>
To: Andrew Cagney <ac131313@cygnus.com>
Cc: Kevin Buettner <kevinb@cygnus.com>, Daniel Berlin <dan@cgsoftware.com>, gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH] C++ patches
Date: Fri, 08 Sep 2000 06:36:00 -0000
Message-id: <m366o7j9a0.fsf@dan2.cygnus.com>
References: <Pine.LNX.4.21.0009062118160.17951-200000@propylaea.anduin.com> <1000907045542.ZM25066@ocotillo.lan> <39B88998.3DE3966@cygnus.com>
X-SW-Source: 2000-09/msg00070.html
Content-length: 2610

Andrew Cagney <ac131313@cygnus.com> writes:

> Kevin Buettner wrote:
> > 
> > On Sep 6,  9:35pm, Daniel Berlin wrote:
> > 
> > > I've rerun indent on the files this affects, because the formatting has
> > > gotten way out of whack with the GNU standards.
> > > I hope nobody minds, if somebody does, i'll undo the whitespace changes in
> > > my patch
> > > (diff -c3pwBb, remove the unbreaking of lines it did).
> > 
> > I think the generally agreed upon practice is to do two separate
> > commits; the first being the actual changes you made and the second
> > being the reformatting.  This way it's easier to review the first
> > patch.
> 
> Either order.  The main thing is to keep indentation and real codeing
> changes separate.  A maintainer shouldn't be presented with a patch that
> contains a combination of both.  Reviewing it is impossible :-(.

Yup.
Dunno what i was thinking.
Brain slipped.

> 
> Don't forget to clean the diff up a little (strip out the ChangeLog etc)
> next time :-)
> 
> > > Please note we now have one memory leak in lookup_symbol. The demangler
> > > gives us a buffer we never free. I'd need to free it at every exit point
> > > from the function, of which we have 12 right now, all of which end almost
> > > the exact same way (most return fixup_symbol_section(sym), a few just
> > > return sym. Is this incorrect?).
> > >
> > > I'm thinking that i'll change it so we just have a label at the end, and
> > > 12 gotos, rather than 12 exit points, and do the cleanup of the
> > > buffer there.
> > > It would make it about 50x easier for someone who needed to do something
> > > that requires cleanup when we exit.
> > 
> > I think gotos in this case are preferable.
> 
> Gotos are bad.  M'kay?  We've so far spent three years trying to
> eliminate all the goto's in WFI, adding more will just make me depressed
> :-)
> 
> Why not push the ALL_SYMTABS() loop into a sub function parameterized by
> ``name'' - I'm assuming that it is the loop that has the N different
> exit points?  That way you get your goto without actually having a
> goto...  Hmm, I suspect that ``name'' will need a cleanup.  Throw an
> error and, regardless of the gotos, it will leak memory.
I'm not staring at the code right now, but IIRC, part of the problem
was that they aren't all inside one loop.

I have IMHO a better solution, like the above. I'll split
lookup_symbol into lookup_symbol and lookup_symbol_aux, do the case
sensitivity/demangling in lookup_symbol, then call lookup_symbol_aux,
do the cleanup, and retunr whatever aux gave us.
Is this acceptable?

> 
> Keep it up.
> 
> 	Andrew