Re: (patch) hpjyg15: hppah-nat.c & related

Re: (patch) hpjyg15: hppah-nat.c & related

[Stan Shebs]

   Date: Thu, 11 Nov 1999 16:31:10 -0800 (PST)
   From: Jimmy Guo <guo@cup.hp.com>

   I'm aware of this problem and I'm trying to resolve this.  Interestly,
   yes I'm reversing what indent did, _not_ to create noises, but to remove
   them.  If you run indent on, say hppah-nat.c, straight from the 19991108
   snapshot, you will see that it's not in GDB's coding style (since indent
   is not happy with the way the code is!)

I've seen some inconsistent behavior from indent, and was planning to
investigate further one of these days.  Nothing to get too excited
about, the oddities amounting to about ~100 in 400K lines of code,
but good to be aware of.  diff -w is useful for separating out the
"real" diffs from the uninteresting ones.

							Stan
From guo@cup.hp.com Thu Nov 11 16:47:00 1999
From: Jimmy Guo <guo@cup.hp.com>
To: Andrew Cagney <ac131313@cygnus.com>
Cc: gdb-patches@sourceware.cygnus.com
Subject: (patch) hpjyg15a, was Re: (patch) hpjyg15: hppah-nat.c & related
Date: Thu, 11 Nov 1999 16:47:00 -0000
Message-id: <Pine.LNX.4.10.9911111640190.6426-100000@hpcll168.cup.hp.com>
References: <382B5609.A4548C6D@cygnus.com>
X-SW-Source: 1999-q4/msg00244.html
Content-length: 3763

>I'd prefer the name (target_signal_p()) (Well actually I don't prefer
>that name but its more consistent with the general naming convetion :-).
>
>I'll check in a change to target.[ch] that adds the target_singal_p(). 
>Hmm, I might do it slightly differently - were getting too many separate
>places where there is code like:
>
>	#if SIG...
>	   case TARGET_SIGNAL...
>	#endif
>
>With that in, can the hppa-nat.c be re-submitted?  BTW, you might also
>split it, I think it contains two changes - the code that uses this new
>function and something else.

Sure.  Here is the part which makes use of target_signal_p (the
target.[ch] changes you are making).

Ignore the hpjyg15 patch, but use this (hpjyg15a), and the sequel I'm
cooking (hpjug15b).

ChangeLog:

1999-11-08	Jimmy Guo	<guo@cup.hp.com>

	* hppah-nat.c (require_notification_of_events): start by
        ignoring all signals and then adding back in ones we're
        interested in.

        * infrun.c (handle_command): On HP, call
          require_notification_of_events to ignore signals we don't
          care.

Index: gdb/hppah-nat.c
/opt/gnu/bin/diff -r -c -N  /view/guo.wdb.c//CLO/Components/WDB/Src/gnu/gdb/hppah-nat.c gdb/hppah-nat.c
*** /view/guo.wdb.c//CLO/Components/WDB/Src/gnu/gdb/hppah-nat.c	Thu Nov 11 15:59:07 1999
--- gdb/hppah-nat.c	Thu Nov 11 16:38:20 1999
***************
*** 698,703 ****
--- 698,705 ----
  #if defined(PT_SET_EVENT_MASK)
    int pt_status;
    ptrace_event_t ptrace_events;
+   int nsigs;
+   int signum;
  
    /* Instruct the kernel as to the set of events we wish to be
       informed of.  (This support does not exist before HPUX 10.0.
***************
*** 709,715 ****
       the kernel to keep certain signals hidden from us, we do it
       by calling sigdelset (ptrace_events.pe_signals, signal) for
       each such signal here, before doing PT_SET_EVENT_MASK.  */
!   sigemptyset (&ptrace_events.pe_signals);
  
    ptrace_events.pe_set_event = 0;
  
--- 711,740 ----
       the kernel to keep certain signals hidden from us, we do it
       by calling sigdelset (ptrace_events.pe_signals, signal) for
       each such signal here, before doing PT_SET_EVENT_MASK.  */
!   /* RM: The above comment is no longer true. We start with ignoring
!    * all signals, and then add the ones we are interested in. We could
!    * do it the other way: start by looking at all signals and then
!    * deleting the ones that we aren't interested in, except that
!    * multiple gdb signals may be mapped to the same host signal
!    * (eg. TARGET_SIGNAL_IO and TARGET_SIGNAL_POLL both get mapped to
!    * signal 22 on HPUX 10.20) We want to be notified if we are
!    * interested in either signal.
!    */
!   sigfillset (&ptrace_events.pe_signals);
! 
!   /* RM: Let's not bother with signals we don't care about */
!   nsigs = (int) TARGET_SIGNAL_LAST;
!   for (signum = nsigs; signum > 0; signum--)
!     {
!       if ((signal_stop_state (signum)) ||
! 	  (signal_print_state (signum)) ||
! 	  (!signal_pass_state (signum)))
! 	{
! 	  if (target_signal_p (signum))
! 	    sigdelset (&ptrace_events.pe_signals,
! 		       target_signal_to_host (signum));
! 	}
!     }
  
    ptrace_events.pe_set_event = 0;
  
Index: gdb/infrun.c
/opt/gnu/bin/diff -r -c -N  /view/guo.wdb.c//CLO/Components/WDB/Src/gnu/gdb/infrun.c gdb/infrun.c
*** /view/guo.wdb.c//CLO/Components/WDB/Src/gnu/gdb/infrun.c	Fri Nov  5 18:37:38 1999
--- gdb/infrun.c	Fri Nov  5 18:38:41 1999
***************
*** 3621,3626 ****
--- 3621,3633 ----
  	}
      }
  
+ 
+ #ifdef GDB_TARGET_IS_HPPA
+   /* RM: Use OS interface to ignore signals we don't care about */
+   if (target_has_execution)
+     require_notification_of_events (inferior_pid);
+ #endif
+ 
    do_cleanups (old_chain);
  }
  

Re: (patch) hpjyg15: hppah-nat.c & related

[Kevin Buettner]

On Nov 11,  4:46pm, Stan Shebs wrote:

> I've seen some inconsistent behavior from indent, and was planning to
> investigate further one of these days.  Nothing to get too excited
> about, the oddities amounting to about ~100 in 400K lines of code,
> but good to be aware of.  diff -w is useful for separating out the
> "real" diffs from the uninteresting ones.

The output of indent should be idempotent wrt indent; i.e, if you run
indent on the output of a previous indent run, you should get the same
output from the second run of indent.  E.g,

	indent foo.c
	cp foo.c foo1.c
	indent foo1.c
	diff foo.c foo1.c

The diff at the end should show no changes.

Unfortunately, this is not the case.  In particular, indent seems to
want to move comments around and add/delete whitespace from the ends
of lines.

I used to have some patches (for a much older version of indent) that
fixed many of these problems at least for the code that I was
indenting at the time.  If there is sufficient interest, I can rumage
around on my backup tapes to see if I can find these patches.

Kevin
From kevinb@cygnus.com Fri Nov 12 10:42:00 1999
From: Kevin Buettner <kevinb@cygnus.com>
To: cagney@cygnus.com
Cc: gdb-patches@sourceware.cygnus.com
Subject: RFA: PACKET_OVERHEAD constant added to remote.c
Date: Fri, 12 Nov 1999 10:42:00 -0000
Message-id: <991112184146.ZM22052@ocotillo.lan>
X-SW-Source: 1999-q4/msg00247.html
Content-length: 2427

Hi Andrew,

Jesper Skov alerted me to the fact that we were getting some "Remote
packet too long" messages when attempting to debug using a gdbserver
for i386 linux.  The problem was that the memory packet size
computations were not taking into account the packet overhead.  This
would've been a one line fix, but I decided to define PACKET_OVERHEAD
instead of adding another hard-coded instance of the constant 32.

Kevin


	* remote.c (PACKET_OVERHEAD): Define.
	(get_memory_packet_size): Take PACKET_OVERHEAD into account when
	computing memory packet size.
	(build_remote_packet_sizes): Use PACKET_OVERHEAD instead of
	hard-coded constants.

Index: remote.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/remote.c,v
retrieving revision 1.258
diff -u -r1.258 remote.c
--- remote.c	1999/11/08 13:14:17	1.258
+++ remote.c	1999/11/12 17:57:13
@@ -315,6 +315,11 @@
 /* compatibility. */
 #define PBUFSIZ (remote_packet_size)
 
+/* Overhead for packet header / footer.  NOTE: cagney/1999-10-26: I suspect
+   that 8 (``$NN:G...#NN'') is a better guess, the below has been padded a
+   little. */
+#define PACKET_OVERHEAD 32
+
 /* User configurable variables for the number of characters in a
    memory read/write packet.  MIN (PBUFSIZ, g-packet-size) is the
    default.  Some targets need smaller values (fifo overruns, et.al.)
@@ -358,7 +363,7 @@
     }
   else
     {
-      what_they_get = remote_packet_size;
+      what_they_get = remote_packet_size - PACKET_OVERHEAD;
       /* Limit the packet to the size specified by the user. */
       if (config->size > 0
 	  && what_they_get > config->size)
@@ -502,12 +507,9 @@
   remote_packet_size = 400;
   /* Should REGISTER_BYTES needs more space than the default, adjust
      the size accordingly. Remember that each byte is encoded as two
-     characters. 32 is the overhead for the packet header /
-     footer. NOTE: cagney/1999-10-26: I suspect that 8
-     (``$NN:G...#NN'') is a better guess, the below has been padded a
-     little. */
-  if (REGISTER_BYTES > ((remote_packet_size - 32) / 2))
-    remote_packet_size = (REGISTER_BYTES * 2 + 32);
+     characters.  */
+  if (REGISTER_BYTES > ((remote_packet_size - PACKET_OVERHEAD) / 2))
+    remote_packet_size = (REGISTER_BYTES * 2 + PACKET_OVERHEAD);
   
   /* This one is filled in when a ``g'' packet is received. */
   actual_register_packet_size = 0;

Re: (patch) hpjyg15: hppah-nat.c & related

[Andrew Cagney]

Jeffrey A Law wrote:
> 
>   In message < Pine.LNX.4.10.9912150951120.29808-100000@hpcll168.cup.hp.com >you
> write:
>   > Since I'm splitting apart changes made by HP, in various areas, over 12
>   > months, into multiple patches, there's a need for me to run indent on
>   > the patched source to ensure that the GNU coding style in gdb source is
>   > adhered to.  I will make sure that pure formatting changes (line breaks)
>   > made by HP are not introduced along with real changes.
> Well, there may be instances where you have a real change that required a
> formatting update.  ie consider if you had
> 
>   if (foo)
>     blah

> You missed another alternative, which I'd tend to favor.
> 
> 1. Run intent on the gdb source files.  Submit that as a patch.  That should be
>    acceptable without problems.
> 
> 2. Apply the patch from wdb using whatever means are possible (patch -l ?).
> 
> 3. Re-run indent on the changed files.
> 
> 4. Submit the patch resulting from diffing the output of steps #1 and #3.

From memory, one of the original problems Jim[HP] found was that
re-running indent further changed the file :-(

	Andrew
From guo@cup.hp.com Thu Dec 16 00:39:00 1999
From: Jimmy Guo <guo@cup.hp.com>
To: Jeffrey A Law <law@cygnus.com>
Cc: Andrew Cagney <ac131313@cygnus.com>, gdb-patches@sourceware.cygnus.com
Subject: Re: (patch) hpjyg15: hppah-nat.c & related 
Date: Thu, 16 Dec 1999 00:39:00 -0000
Message-id: <Pine.LNX.4.10.9912160026010.32740-100000@hpcll168.cup.hp.com>
References: <4534.945330639@upchuck>
X-SW-Source: 1999-q4/msg00402.html
Content-length: 1215

>You missed another alternative, which I'd tend to favor.
>
>1. Run intent on the gdb source files.  Submit that as a patch.  That should be
>   acceptable without problems.
>
>2. Apply the patch from wdb using whatever means are possible (patch -l ?).
>
>3. Re-run indent on the changed files.
>
>4. Submit the patch resulting from diffing the output of steps #1 and #3.

    This is very doable.  Painful as it may appear, it's more so in the
    steps involved than the actual overhead ... and I will definitely
    write a simple script to generate patch for #1 and #3 in one shot ...

>Yes, this is painful.  But it avoids introducing new formatting problems in
>the sources (in fact via step #1 it will tend to fix formatting problems that
>have slipped in accidentally).

    I agree.

>Of course we only need to go through this pain as we flush out your old
>patches.  Newer stuff should be done using GNU standards and this kind of
>painful process shouldn't be necessary.

    Since I'm 'chopping' changes apart to limit patch scope and
    'cooking' patches during patch testing cycles, the above steps are
    pretty much what should be done for my future patches as well.

- Jimmy Guo, guo@cup.hp.com

Re: (patch) hpjyg15: hppah-nat.c & related

[Jeffrey A Law]

  In message < Pine.LNX.4.10.9912150951120.29808-100000@hpcll168.cup.hp.com >you 
write:
  > Since I'm splitting apart changes made by HP, in various areas, over 12
  > months, into multiple patches, there's a need for me to run indent on
  > the patched source to ensure that the GNU coding style in gdb source is
  > adhered to.  I will make sure that pure formatting changes (line breaks)
  > made by HP are not introduced along with real changes.
Well, there may be instances where you have a real change that required a 
formatting update.  ie consider if you had

  if (foo)
    blah

And you changed it to

  if (foo)
    {
      bar
      blah
    }

In that case you do want the formatting change.  So you have to be careful
about blindly ignoring whitespace differences.

  > However as we've discovered, due to a combination of a potential indent
  > program problem and the fact that there're places in snapshot sources
  > where formatting is actually necessary, there's another kind of pure
  > formatting changes which can come from this combination (and my running
  > indent before making the patch).  There're several ways to handle this:
  > 
  > 1) generate patch using GNU diff -w option (ignore white space), and
  >    apply patch using patch -l (--ignore-whitespace) option.
  >    :: I like this approach and I'll do it if you guys are willing to
  >    give this a try.
It runs afoul of the problem noted above.  And thus will tend to introduce new
formatting problems in the gdb sources.  It may be a reasonable thing to do
in limited cases though.

  > 2) don't run indent before generating patch, have maintainers do
  >    that before checking into CVS.
  >    :: could work.  If necessary I can provide a ksh script to do this,
  >    incorporating the special-case handlings documented by Stan during
  >    the GDB reformat (so one do not indent a file which is not supposed
  >    to be indented).
  > 
  > 3) patch submitter manually remove pure formatting change noises from
  >    patch.
  >    :: least favored.

You missed another alternative, which I'd tend to favor.

1. Run intent on the gdb source files.  Submit that as a patch.  That should be
   acceptable without problems.

2. Apply the patch from wdb using whatever means are possible (patch -l ?).

3. Re-run indent on the changed files.

4. Submit the patch resulting from diffing the output of steps #1 and #3.


Yes, this is painful.  But it avoids introducing new formatting problems in
the sources (in fact via step #1 it will tend to fix formatting problems that
have slipped in accidentally).

Of course we only need to go through this pain as we flush out your old
patches.  Newer stuff should be done using GNU standards and this kind of
painful process shouldn't be necessary.

jeff