Patch for DejaGnu test case "pointers"

Patch for DejaGnu test case "pointers"

[Orjan Friberg]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 11365 bytes --]

Hi,

I'm working at Axis Communications in Lund, Sweden, and I'm updating our
port of gdb for cross-debugging. An assign.future has been submitted
previously that covers all employees. This is my first patch submission
(more patches are on the way), so I'd appreciate feedback if any changes
are needed.

I'd like to submit a patch for the DejaGnu test case called pointers. In
pointers.c, the following variables are declared:

float F, *pF;
double D, *pD;

and later the following statements:

F = 1.25E10;
pF = &F;
D = -1.375E-123;
pD = &D;

-1.375E-123 will only fit if a double is (at least) 8 bytes. For a
4-byte double, an underflow occurs. In pointers.exp, there is a test
that prints *pD which fails. However, assigning D something that fits is
not enough since the pattern matching with respect to the decimals for
doubles and floats differ, which also seems to stem from an assumption
about their respective sizes. print *pD is required to match *exactly*,
whereas print *pF is checked only until the first decimal.

I'm not an expert on various sizes of floats and doubles, so maybe I'm
also making assumptions about sizes. From K&R, The C Programming
Language, Appendix B11, "acceptable minimum" for FLT_MIN is 1E-37. I
suggest using D = -1.25E-37, and checking only the first decimal. The
patches below to that.

*************** int more_code()
*** 194,200 ****
      L = -234;
      UL = 234;
      F = 1.25E10;
!     D = -1.375E-123;
      pC = &C;
      ppC = &pC;
      pppC = &ppC;
--- 194,200 ----
      L = -234;
      UL = 234;
      F = 1.25E10;
!     D = -1.25E-37;
      pC = &C;
      ppC = &pC;
      pppC = &ppC;


*************** gdb_expect {
*** 522,528 ****
  
  send_gdb "print *pD\n"
  gdb_expect {
!     -re ".\[0-9\]* = -1.375e-123.*$gdb_prompt $" {
          pass "print value of *pD"
        }
      -re ".*$gdb_prompt $" { fail "print value of *pD" }
--- 522,528 ----
  
  send_gdb "print *pD\n"
  gdb_expect {
!     -re ".\[0-9\]* = -1.2\[0-9\]*e\\-37.*$gdb_prompt $" {
          pass "print value of *pD"
        }
      -re ".*$gdb_prompt $" { fail "print value of *pD" }



Orjan Friberg
Axis Communications AB
E-mail: orjan.friberg@axis.com 
Phone:  +46 46 272 17 68
From hp@axis.com Thu Sep 07 07:38:00 2000
From: Hans-Peter Nilsson <hp@axis.com>
To: orjanf@axis.com
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: Patch for DejaGnu test case "pointers"
Date: Thu, 07 Sep 2000 07:38:00 -0000
Message-id: <200009071435.QAA17036@ignucius.axis.se>
References: <39B7A02B.EEB3F889@axis.com>
X-SW-Source: 2000-09/msg00059.html
Content-length: 575

> Date: Thu, 07 Sep 2000 16:03:23 +0200
> From: Orjan Friberg <orjanf@axis.com>

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

Du glömde changelog-entries, som för övrigt ska skickas som
klartext, inte patch. M-x add-change-log-entry i emacs
underlättar.  Annars är det OK till formen; patchen saknar dock
de två första raderna av någon anledning.

Se "Submitting Patches" i URL:en jag skickade tidigare,
<URL: http://gcc.gnu.org/contribute.html >.  Se även andras
patch-mail.

mvh H-P
From hp@axis.com Thu Sep 07 07:41:00 2000
From: Hans-Peter Nilsson <hp@axis.com>
To: gdb-patches@sourceware.cygnus.com
Cc: orjanf@axis.com
Subject: Re: Patch for DejaGnu test case "pointers"
Date: Thu, 07 Sep 2000 07:41:00 -0000
Message-id: <200009071440.QAA17075@ignucius.axis.se>
References: <39B7A02B.EEB3F889@axis.com>
X-SW-Source: 2000-09/msg00060.html
Content-length: 434

> Date: Thu, 07 Sep 2000 16:03:23 +0200
> From: Orjan Friberg <orjanf@axis.com>

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

Oops, sorry, CC:ed gdb-patches by mistake.  I was helping my
colleague with some slight omissions in his painful(?) first
patch.  The contents of my mail is probably mostly obvious even
though it was in Swedish :-)

brgds, H-P
From orjan.friberg@axis.com Thu Sep 07 08:16:00 2000
From: Orjan Friberg <orjan.friberg@axis.com>
To: gdb-patches@sourceware.cygnus.com
Subject: Re: Patch for DejaGnu test case "pointers"
Date: Thu, 07 Sep 2000 08:16:00 -0000
Message-id: <39B7B0B4.A53B598E@axis.com>
References: <200009071440.QAA17075@ignucius.axis.se>
X-SW-Source: 2000-09/msg00061.html
Content-length: 878

Hans-Peter Nilsson wrote:
> 
> > Date: Thu, 07 Sep 2000 16:03:23 +0200
> > From: Orjan Friberg <orjanf@axis.com>
> 
> > This is my first patch submission
> > (more patches are on the way), so I'd appreciate feedback if any changes
> > are needed.
> 
> Oops, sorry, CC:ed gdb-patches by mistake.  I was helping my
> colleague with some slight omissions in his painful(?) first
> patch.  The contents of my mail is probably mostly obvious even
> though it was in Swedish :-)
> 
> brgds, H-P

Here is the ChangeLog entry (thanks H-P):

2000-09-07  Orjan Friberg  <orjan.friberg@axis.com>

        * gdb.base/pointers.c: Do not rely on a double being 8 bytes
when
        assigning a small value	to it.
        * gdb.base/pointers.exp: Relax pattern matching of a double's
decimals.

-- 
Orjan Friberg
Axis Communications AB
E-mail: orjan.friberg@axis.com 
Phone:  +46 46 272 17 68
From jtc@redback.com Thu Sep 07 11:45:00 2000
From: jtc@redback.com (J.T. Conklin)
To: Michael Snyder <msnyder@redhat.com>
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: [RFC]: Remote Protocol "attach" command.
Date: Thu, 07 Sep 2000 11:45:00 -0000
Message-id: <5mem2wqbwi.fsf@jtc.redback.com>
References: <39AD801B.2C7C@redhat.com>
X-SW-Source: 2000-09/msg00062.html
Content-length: 2646

>>>>> "Michael" == Michael Snyder <msnyder@redhat.com> writes:
Michael> More and more embedded operating systems are multi-process
Michael> these days, and so there exists a need to have a stub or a
Michael> gdbserver already running (perhaps all the time), and have it
Michael> "attach" to another running process and debug it.  For native
Michael> Unix systems, this has traditionally been done by running 
Michael> gdbserver from a shell, giving it the path to the executable,
Michael> and letting it fork the new process.

Michael> I'd like to start a discussion about adding a new remote
Michael> protocol command with which GDB could ask the remote stub
Michael> to attach to a running process (by pid), or possibly to
Michael> open a file and fork a process (by filename/path).  Of course,
Michael> stubs that can't do that would simply return an empty reply
Michael> as always, indicating non-compliance.

I'm not opposed to this, although there appear to be some issues to be
worked out.

At present, when `target remote <foo>' is issued, GDB attaches to and
stops the target, possibly getting thread and section relocation info
in the process.  It would be easier if the `target remote <foo>' only
established communication to a target, and that a separate 'run' or
'attach' command needed to be issued to interact.  This model works
quite well, I use it in my vxWorks/WDB back end.  But I can't see how
it can be retrofited into the remote protocol.  It pretty much assumes
an interrupt driven debug agent that can handle async commands without
disturbing the target system.  I'm afraid I have more questions than 
answers here.

For the command itself, I think it probably should be defined as a
some command letter plus an 'target specific' identifier.  On UNIX
like systems, process-ids are natural.  But on others, a process or
task name may be more appropriate.

As you know, the remote debug protocol has not supported well defined
error codes.  A failure would return EXX, but there was no definition
of the different XX values.  I think that all new commands should do
so.  I'm kicking myself for not doing so when I proposed the breakpoint
extensions a year+ ago.

There already is a detach command.  I think the protocol spec could be
worded to indicate that the program under debug is released to
continue executing.  It does not appear that gdbserver understands the
command.  Question: after the inferior is detached, should you be able
to re-attach (or attach to a different process) without
re-establishing communication to the debug agent (with `target remote
<foo>')?

        --jtc

-- 
J.T. Conklin
RedBack Networks
From kevinb@cygnus.com Thu Sep 07 12:37:00 2000
From: Kevin Buettner <kevinb@cygnus.com>
To: gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH RFC] Protoize i386mach-nat.c, i960-tdep.c
Date: Thu, 07 Sep 2000 12:37:00 -0000
Message-id: <1000907193655.ZM26339@ocotillo.lan>
References: <1000905232022.ZM22471@ocotillo.lan> <kevinb@cygnus.com>
X-SW-Source: 2000-09/msg00063.html
Content-length: 208

On Sep 5,  4:20pm, Kevin Buettner wrote:

> 	* i386mach-nat.c (fetch_inferior_registers, fetch_core_registers):
> 	Protoize.
> 	* i960-tdep.c (i960_skip_prologue, leafproc_return, mem): Protoize.

Committed.
From kevinb@cygnus.com Thu Sep 07 12:38:00 2000
From: Kevin Buettner <kevinb@cygnus.com>
To: gdb-patches@sourceware.cygnus.com
Subject: [PATCH RFC] Protoize infptrace.c, infttrace.c
Date: Thu, 07 Sep 2000 12:38:00 -0000
Message-id: <1000907193843.ZM26345@ocotillo.lan>
X-SW-Source: 2000-09/msg00064.html
Content-length: 2141

More protoization...

	* infptrace.c, infttrace.c (child_xfer_memory): Protoize.

Index: infptrace.c
===================================================================
RCS file: /cvs/src/src/gdb/infptrace.c,v
retrieving revision 1.5
diff -u -r1.5 infptrace.c
--- infptrace.c	2000/07/30 01:48:25	1.5
+++ infptrace.c	2000/09/07 19:34:28
@@ -502,7 +502,7 @@
 
 /* Copy LEN bytes to or from inferior's memory starting at MEMADDR
    to debugger memory starting at MYADDR.   Copy to inferior if
-   WRITE is nonzero.
+   WRITE is nonzero.  TARGET is ignored.
 
    Returns the length copied, which is either the LEN argument or zero.
    This xfer function does not do partial moves, since child_ops
@@ -510,12 +510,8 @@
    anyway.  */
 
 int
-child_xfer_memory (memaddr, myaddr, len, write, target)
-     CORE_ADDR memaddr;
-     char *myaddr;
-     int len;
-     int write;
-     struct target_ops *target;	/* ignored */
+child_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len, int write,
+		   struct target_ops *target)
 {
   register int i;
   /* Round starting address down to longword boundary.  */
Index: infttrace.c
===================================================================
RCS file: /cvs/src/src/gdb/infttrace.c,v
retrieving revision 1.3
diff -u -r1.3 infttrace.c
--- infttrace.c	2000/07/30 01:48:25	1.3
+++ infttrace.c	2000/09/07 19:34:31
@@ -4907,7 +4907,7 @@
 
 /* Copy LEN bytes to or from inferior's memory starting at MEMADDR
    to debugger memory starting at MYADDR.   Copy to inferior if
-   WRITE is nonzero.
+   WRITE is nonzero.  TARGET is ignored.
 
    Returns the length copied, which is either the LEN argument or zero.
    This xfer function does not do partial moves, since child_ops
@@ -4915,12 +4915,8 @@
    anyway.  */
 
 int
-child_xfer_memory (memaddr, myaddr, len, write, target)
-     CORE_ADDR memaddr;
-     char *myaddr;
-     int len;
-     int write;
-     struct target_ops *target;	/* ignored */
+child_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len, int write,
+		   struct target_ops *target)
 {
   register int i;
   /* Round starting address down to longword boundary.  */

Re: Patch for DejaGnu test case "pointers"

[Kevin Buettner]

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