[PATCH] Cleanup i386-nat.c

[PATCH] Cleanup i386-nat.c

[Mark Kettenis]

The coding style in this file had some quirks, so I committed the
attached.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>
 
	* i386-nat.c: Reformat to be closer to coding standards.
	(i386_handle_nonaligned_watchpoint): Rename local variable `rv' to
	`retval'.  Make variables `align' and `size' local to while-loop.
	(i386_stopped_data_address): Rename local variable `ret' to `addr'.
	(_initialize_i386_nat): New prototype.

Index: i386-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-nat.c,v
retrieving revision 1.6
diff -u -p -r1.6 i386-nat.c
--- i386-nat.c 17 Aug 2003 18:22:25 -0000 1.6
+++ i386-nat.c 28 Feb 2004 15:14:22 -0000
@@ -1,5 +1,6 @@
-/* Intel x86 (a.k.a. ia32) native-dependent code.
-   Copyright (C) 2001 Free Software Foundation, Inc.
+/* Native-dependent code for the i386.
+
+   Copyright 2001, 2004 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -23,24 +24,24 @@
 #include "command.h"
 #include "gdbcmd.h"
 
-/* Support for hardware watchpoints and breakpoints using the x86
+/* Support for hardware watchpoints and breakpoints using the i386
    debug registers.
 
    This provides several functions for inserting and removing
-   hardware-assisted breakpoints and watchpoints, testing if
-   one or more of the watchpoints triggered and at what address,
-   checking whether a given region can be watched, etc.
-
-   A target which wants to use these functions should define
-   several macros, such as `target_insert_watchpoint' and
-   `target_stopped_data_address', listed in target.h, to call
-   the appropriate functions below.  It should also define
+   hardware-assisted breakpoints and watchpoints, testing if one or
+   more of the watchpoints triggered and at what address, checking
+   whether a given region can be watched, etc.
+
+   A target which wants to use these functions should define several
+   macros, such as `target_insert_watchpoint' and
+   `target_stopped_data_address', listed in target.h, to call the
+   appropriate functions below.  It should also define
    I386_USE_GENERIC_WATCHPOINTS in its tm.h file.
 
-   In addition, each target should provide several low-level
-   macros that will be called to insert watchpoints and hardware
-   breakpoints into the inferior, remove them, and check their
-   status.  These macros are:
+   In addition, each target should provide several low-level macros
+   that will be called to insert watchpoints and hardware breakpoints
+   into the inferior, remove them, and check their status.  These
+   macros are:
 
       I386_DR_LOW_SET_CONTROL  -- set the debug control (DR7)
 				  register to a given value
@@ -54,21 +55,20 @@
       I386_DR_LOW_GET_STATUS   -- return the value of the debug
 				  status (DR6) register.
 
-   The functions below implement debug registers sharing by
-   reference counts, and allow to watch regions up to 16 bytes
-   long.  */
+   The functions below implement debug registers sharing by reference
+   counts, and allow to watch regions up to 16 bytes long.  */
 
 #ifdef I386_USE_GENERIC_WATCHPOINTS
 
 /* Support for 8-byte wide hw watchpoints.  */
 #ifndef TARGET_HAS_DR_LEN_8
-#define TARGET_HAS_DR_LEN_8 0
+#define TARGET_HAS_DR_LEN_8	0
 #endif
 
 /* Debug registers' indices.  */
-#define DR_NADDR		4  /* the number of debug address registers */
-#define DR_STATUS		6  /* index of debug status register (DR6) */
-#define DR_CONTROL		7  /* index of debug control register (DR7) */
+#define DR_NADDR	4	/* The number of debug address registers.  */
+#define DR_STATUS	6	/* Index of debug status register (DR6).  */
+#define DR_CONTROL	7	/* Index of debug control register (DR7). */
 
 /* DR7 Debug Control register fields.  */
 
@@ -78,46 +78,46 @@
 #define DR_CONTROL_SIZE		4
 
 /* Watchpoint/breakpoint read/write fields in DR7.  */
-#define DR_RW_EXECUTE		(0x0) /* break on instruction execution */
-#define DR_RW_WRITE		(0x1) /* break on data writes */
-#define DR_RW_READ		(0x3) /* break on data reads or writes */
+#define DR_RW_EXECUTE	(0x0)	/* Break on instruction execution.  */
+#define DR_RW_WRITE	(0x1)	/* Break on data writes.  */
+#define DR_RW_READ	(0x3)	/* Break on data reads or writes.  */
 
 /* This is here for completeness.  No platform supports this
-   functionality yet (as of Mar-2001).  Note that the DE flag in the
+   functionality yet (as of March 2001).  Note that the DE flag in the
    CR4 register needs to be set to support this.  */
 #ifndef DR_RW_IORW
-#define DR_RW_IORW		(0x2) /* break on I/O reads or writes */
+#define DR_RW_IORW	(0x2)	/* Break on I/O reads or writes.  */
 #endif
 
 /* Watchpoint/breakpoint length fields in DR7.  The 2-bit left shift
    is so we could OR this with the read/write field defined above.  */
-#define DR_LEN_1		(0x0 << 2) /* 1-byte region watch or breakpt */
-#define DR_LEN_2		(0x1 << 2) /* 2-byte region watch */
-#define DR_LEN_4		(0x3 << 2) /* 4-byte region watch */
-#define DR_LEN_8		(0x2 << 2) /* 8-byte region watch (x86-64) */
+#define DR_LEN_1	(0x0 << 2) /* 1-byte region watch or breakpoint.  */
+#define DR_LEN_2	(0x1 << 2) /* 2-byte region watch.  */
+#define DR_LEN_4	(0x3 << 2) /* 4-byte region watch.  */
+#define DR_LEN_8	(0x2 << 2) /* 8-byte region watch (AMD64).  */
 
 /* Local and Global Enable flags in DR7.
 
    When the Local Enable flag is set, the breakpoint/watchpoint is
    enabled only for the current task; the processor automatically
-   clears this flag on every task switch.  When the Global Enable
-   flag is set, the breakpoint/watchpoint is enabled for all tasks;
-   the processor never clears this flag.
+   clears this flag on every task switch.  When the Global Enable flag
+   is set, the breakpoint/watchpoint is enabled for all tasks; the
+   processor never clears this flag.
 
    Currently, all watchpoint are locally enabled.  If you need to
    enable them globally, read the comment which pertains to this in
    i386_insert_aligned_watchpoint below.  */
-#define DR_LOCAL_ENABLE_SHIFT	0   /* extra shift to the local enable bit */
-#define DR_GLOBAL_ENABLE_SHIFT	1   /* extra shift to the global enable bit */
-#define DR_ENABLE_SIZE		2   /* 2 enable bits per debug register */
+#define DR_LOCAL_ENABLE_SHIFT	0 /* Extra shift to the local enable bit.  */
+#define DR_GLOBAL_ENABLE_SHIFT	1 /* Extra shift to the global enable bit.  */
+#define DR_ENABLE_SIZE		2 /* Two enable bits per debug register.  */
 
 /* Local and global exact breakpoint enable flags (a.k.a. slowdown
    flags).  These are only required on i386, to allow detection of the
    exact instruction which caused a watchpoint to break; i486 and
    later processors do that automatically.  We set these flags for
-   back compatibility.  */
+   backwards compatibility.  */
 #define DR_LOCAL_SLOWDOWN	(0x100)
-#define DR_GLOBAL_SLOWDOWN	(0x200)
+#define DR_GLOBAL_SLOWDOWN     	(0x200)
 
 /* Fields reserved by Intel.  This includes the GD (General Detect
    Enable) flag, which causes a debug exception to be generated when a
@@ -129,7 +129,7 @@
 /* Auxiliary helper macros.  */
 
 /* A value that masks all fields in DR7 that are reserved by Intel.  */
-#define I386_DR_CONTROL_MASK		(~DR_CONTROL_RESERVED)
+#define I386_DR_CONTROL_MASK	(~DR_CONTROL_RESERVED)
 
 /* The I'th debug register is vacant if its Local and Global Enable
    bits are reset in the Debug Control register.  */
@@ -168,13 +168,13 @@
 /* Mirror the inferior's DRi registers.  We keep the status and
    control registers separated because they don't hold addresses.  */
 static CORE_ADDR dr_mirror[DR_NADDR];
-static unsigned	 dr_status_mirror, dr_control_mirror;
+static unsigned dr_status_mirror, dr_control_mirror;
 
 /* Reference counts for each debug register.  */
-static int	 dr_ref_count[DR_NADDR];
+static int dr_ref_count[DR_NADDR];
 
 /* Whether or not to print the mirrored debug registers.  */
-static int	 maint_show_dr;
+static int maint_show_dr;
 
 /* Types of operations supported by i386_handle_nonaligned_watchpoint.  */
 typedef enum { WP_INSERT, WP_REMOVE, WP_COUNT } i386_wp_op_t;
@@ -182,8 +182,8 @@ typedef enum { WP_INSERT, WP_REMOVE, WP_
 /* Internal functions.  */
 
 /* Return the value of a 4-bit field for DR7 suitable for watching a
-   region of LEN bytes for accesses of type TYPE.  LEN is assumed
-   to have the value of 1, 2, or 4.  */
+   region of LEN bytes for accesses of type TYPE.  LEN is assumed to
+   have the value of 1, 2, or 4.  */
 static unsigned i386_length_and_rw_bits (int len, enum target_hw_bp_type type);
 
 /* Insert a watchpoint at address ADDR, which is assumed to be aligned
@@ -206,16 +206,17 @@ static int i386_remove_aligned_watchpoin
    number of debug registers required to watch a region at address
    ADDR whose length is LEN for accesses of type TYPE.  Return 0 on
    successful insertion or removal, a positive number when queried
-   about the number of registers, or -1 on failure.  If WHAT is not
-   a valid value, bombs through internal_error.  */
+   about the number of registers, or -1 on failure.  If WHAT is not a
+   valid value, bombs through internal_error.  */
 static int i386_handle_nonaligned_watchpoint (i386_wp_op_t what,
 					      CORE_ADDR addr, int len,
 					      enum target_hw_bp_type type);
 
 /* Implementation.  */
 
-/* Clear the reference counts and forget everything we knew about
-   the debug registers.  */
+/* Clear the reference counts and forget everything we knew about the
+   debug registers.  */
+
 void
 i386_cleanup_dregs (void)
 {
@@ -231,18 +232,22 @@ i386_cleanup_dregs (void)
 }
 
 #ifndef LINUX_CHILD_POST_STARTUP_INFERIOR
-/* Reset all debug registers at each new startup
-   to avoid missing watchpoints after restart.  */
+
+/* Reset all debug registers at each new startup to avoid missing
+   watchpoints after restart.  */
+
 void
 child_post_startup_inferior (ptid_t ptid)
 {
   i386_cleanup_dregs ();
 }
+
 #endif /* LINUX_CHILD_POST_STARTUP_INFERIOR */
 
-/* Print the values of the mirrored debug registers.
-   This is called when maint_show_dr is non-zero.  To set that
-   up, type "maint show-debug-regs" at GDB's prompt.  */
+/* Print the values of the mirrored debug registers.  This is called
+   when maint_show_dr is non-zero.  To set that up, type "maint
+   show-debug-regs" at GDB's prompt.  */
+
 static void
 i386_show_dr (const char *func, CORE_ADDR addr,
 	      int len, enum target_hw_bp_type type)
@@ -268,7 +273,8 @@ i386_show_dr (const char *func, CORE_ADD
 		     dr_control_mirror, dr_status_mirror);
   ALL_DEBUG_REGISTERS(i)
     {
-      printf_unfiltered ("\tDR%d: addr=0x%s, ref.count=%d  DR%d: addr=0x%s, ref.count=%d\n",
+      printf_unfiltered ("\
+\tDR%d: addr=0x%s, ref.count=%d  DR%d: addr=0x%s, ref.count=%d\n",
 			 i, paddr(dr_mirror[i]), dr_ref_count[i],
 			 i+1, paddr(dr_mirror[i+1]), dr_ref_count[i+1]);
       i++;
@@ -276,8 +282,9 @@ i386_show_dr (const char *func, CORE_ADD
 }
 
 /* Return the value of a 4-bit field for DR7 suitable for watching a
-   region of LEN bytes for accesses of type TYPE.  LEN is assumed
-   to have the value of 1, 2, or 4.  */
+   region of LEN bytes for accesses of type TYPE.  LEN is assumed to
+   have the value of 1, 2, or 4.  */
+
 static unsigned
 i386_length_and_rw_bits (int len, enum target_hw_bp_type type)
 {
@@ -291,18 +298,21 @@ i386_length_and_rw_bits (int len, enum t
       case hw_write:
 	rw = DR_RW_WRITE;
 	break;
-      case hw_read:	 /* x86 doesn't support data-read watchpoints */
+      case hw_read:
+	/* The i386 doesn't support data-read watchpoints.  */
       case hw_access:
 	rw = DR_RW_READ;
 	break;
 #if 0
-      case hw_io_access: /* not yet supported */
+	/* Not yet supported.  */
+      case hw_io_access:
 	rw = DR_RW_IORW;
 	break;
 #endif
       default:
 	internal_error (__FILE__, __LINE__, "\
-Invalid hw breakpoint type %d in i386_length_and_rw_bits.\n", (int)type);
+Invalid hardware breakpoint type %d in i386_length_and_rw_bits.\n",
+			(int) type);
     }
 
   switch (len)
@@ -318,7 +328,7 @@ Invalid hw breakpoint type %d in i386_le
  	  return (DR_LEN_8 | rw);
       default:
 	internal_error (__FILE__, __LINE__, "\
-Invalid hw breakpoint length %d in i386_length_and_rw_bits.\n", len);
+Invalid hardware breakpoint length %d in i386_length_and_rw_bits.\n", len);
     }
 }
 
@@ -327,6 +337,7 @@ Invalid hw breakpoint length %d in i386_
    value of the bits from DR7 which describes the length and access
    type of the region to be watched by this watchpoint.  Return 0 on
    success, -1 on failure.  */
+
 static int
 i386_insert_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
 {
@@ -364,7 +375,7 @@ i386_insert_aligned_watchpoint (CORE_ADD
   dr_ref_count[i] = 1;
   I386_DR_SET_RW_LEN (i, len_rw_bits);
   /* Note: we only enable the watchpoint locally, i.e. in the current
-     task.  Currently, no x86 target allows or supports global
+     task.  Currently, no i386 target allows or supports global
      watchpoints; however, if any target would want that in the
      future, GDB should probably provide a command to control whether
      to enable watchpoints globally or locally, and the code below
@@ -386,6 +397,7 @@ i386_insert_aligned_watchpoint (CORE_ADD
    value of the bits from DR7 which describes the length and access
    type of the region watched by this watchpoint.  Return 0 on
    success, -1 on failure.  */
+
 static int
 i386_remove_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
 {
@@ -417,42 +429,46 @@ i386_remove_aligned_watchpoint (CORE_ADD
    number of debug registers required to watch a region at address
    ADDR whose length is LEN for accesses of type TYPE.  Return 0 on
    successful insertion or removal, a positive number when queried
-   about the number of registers, or -1 on failure.  If WHAT is not
-   a valid value, bombs through internal_error.  */
+   about the number of registers, or -1 on failure.  If WHAT is not a
+   valid value, bombs through internal_error.  */
+
 static int
 i386_handle_nonaligned_watchpoint (i386_wp_op_t what, CORE_ADDR addr, int len,
 				   enum target_hw_bp_type type)
 {
-  int align;
-  int size;
-  int rv = 0, status = 0;
+  int retval = 0, status = 0;
   int max_wp_len = TARGET_HAS_DR_LEN_8 ? 8 : 4;
 
   static int size_try_array[8][8] =
   {
-    {1, 1, 1, 1, 1, 1, 1, 1},	/* trying size one */
-    {2, 1, 2, 1, 2, 1, 2, 1},	/* trying size two */
-    {2, 1, 2, 1, 2, 1, 2, 1},	/* trying size three */
-    {4, 1, 2, 1, 4, 1, 2, 1},	/* trying size four */
-    {4, 1, 2, 1, 4, 1, 2, 1},	/* trying size five */
-    {4, 1, 2, 1, 4, 1, 2, 1},	/* trying size six */
-    {4, 1, 2, 1, 4, 1, 2, 1},	/* trying size seven */
-    {8, 1, 2, 1, 4, 1, 2, 1},	/* trying size eight */
+    {1, 1, 1, 1, 1, 1, 1, 1},	/* Trying size one.  */
+    {2, 1, 2, 1, 2, 1, 2, 1},	/* Trying size two.  */
+    {2, 1, 2, 1, 2, 1, 2, 1},	/* Trying size three.  */
+    {4, 1, 2, 1, 4, 1, 2, 1},	/* Trying size four.  */
+    {4, 1, 2, 1, 4, 1, 2, 1},	/* Trying size five.  */
+    {4, 1, 2, 1, 4, 1, 2, 1},	/* Trying size six.  */
+    {4, 1, 2, 1, 4, 1, 2, 1},	/* Trying size seven.  */
+    {8, 1, 2, 1, 4, 1, 2, 1},	/* Trying size eight.  */
   };
 
   while (len > 0)
     {
-      align = addr % max_wp_len;
-      /* Four(eigth on x86_64) is the maximum length an x86 debug register
+      int align = addr % max_wp_len;
+      /* Four (eigth on AMD64) is the maximum length a debug register
 	 can watch.  */
-      size = size_try_array[len > max_wp_len ? (max_wp_len - 1) : len - 1][align];
+      int try = (len > max_wp_len ? (max_wp_len - 1) : len - 1);
+      int size = size_try_array[try][align];
+
       if (what == WP_COUNT)
-	/* size_try_array[] is defined so that each iteration through
-	   the loop is guaranteed to produce an address and a size
-	   that can be watched with a single debug register.  Thus,
-	   for counting the registers required to watch a region, we
-	   simply need to increment the count on each iteration.  */
-	rv++;
+	{
+	  /* size_try_array[] is defined such that each iteration
+	     through the loop is guaranteed to produce an address and a
+	     size that can be watched with a single debug register.
+	     Thus, for counting the registers required to watch a
+	     region, we simply need to increment the count on each
+	     iteration.  */
+	  retval++;
+	}
       else
 	{
 	  unsigned len_rw = i386_length_and_rw_bits (size, type);
@@ -475,17 +491,20 @@ Invalid value %d of operation in i386_ha
 	     to our failure to insert this watchpoint and tries to
 	     remove it.  */
 	  if (status)
-	    rv = status;
+	    retval = status;
 	}
+
       addr += size;
       len -= size;
     }
-  return rv;
+
+  return retval;
 }
 
 /* Insert a watchpoint to watch a memory region which starts at
    address ADDR and whose length is LEN bytes.  Watch memory accesses
    of the type TYPE.  Return 0 on success, -1 on failure.  */
+
 int
 i386_insert_watchpoint (CORE_ADDR addr, int len, int type)
 {
@@ -533,25 +552,26 @@ i386_remove_watchpoint (CORE_ADDR addr, 
 
 /* Return non-zero if we can watch a memory region that starts at
    address ADDR and whose length is LEN bytes.  */
+
 int
 i386_region_ok_for_watchpoint (CORE_ADDR addr, int len)
 {
+  int nregs;
+
   /* Compute how many aligned watchpoints we would need to cover this
      region.  */
-  int nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len,
-						 hw_write);
-
+  nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len, hw_write);
   return nregs <= DR_NADDR ? 1 : 0;
 }
 
 /* If the inferior has some watchpoint that triggered, return the
-   address associated with that watchpoint.  Otherwise, return
-   zero.  */
+   address associated with that watchpoint.  Otherwise, return zero.  */
+
 CORE_ADDR
 i386_stopped_data_address (void)
 {
+  CORE_ADDR addr = 0;
   int i;
-  CORE_ADDR ret = 0;
 
   dr_status_mirror = I386_DR_LOW_GET_STATUS ();
 
@@ -562,22 +582,23 @@ i386_stopped_data_address (void)
 	     watchpoint, not a hardware breakpoint.  The reason is
 	     that GDB doesn't call the target_stopped_data_address
 	     method except for data watchpoints.  In other words, I'm
-	     being paranoiac.  */
+	     being paranoid.  */
 	  && I386_DR_GET_RW_LEN (i) != 0)
 	{
-	  ret = dr_mirror[i];
+	  addr = dr_mirror[i];
 	  if (maint_show_dr)
-	    i386_show_dr ("watchpoint_hit", ret, -1, hw_write);
+	    i386_show_dr ("watchpoint_hit", addr, -1, hw_write);
 	}
     }
-  if (maint_show_dr && ret == 0)
+  if (maint_show_dr && addr == 0)
     i386_show_dr ("stopped_data_addr", 0, 0, hw_write);
 
-  return ret;
+  return addr;
 }
 
 /* Return non-zero if the inferior has some break/watchpoint that
    triggered.  */
+
 int
 i386_stopped_by_hwbp (void)
 {
@@ -612,6 +633,7 @@ i386_insert_hw_breakpoint (CORE_ADDR add
 
 /* Remove a hardware-assisted breakpoint at address ADDR.  SHADOW is
    unused.  Return 0 on success, -1 on failure.  */
+
 int
 i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow)
 {
@@ -625,8 +647,11 @@ i386_remove_hw_breakpoint (CORE_ADDR add
 }
 
 #endif /* I386_USE_GENERIC_WATCHPOINTS */
-
 \f
+
+/* Provide a prototype to silence -Wmissing-prototypes.  */
+void _initialize_i386_nat (void);
+
 void
 _initialize_i386_nat (void)
 {

Re: [PATCH] Cleanup i386-nat.c

[Eli Zaretskii]

> Date: Sat, 28 Feb 2004 16:38:23 +0100 (CET)
> From: Mark Kettenis <kettenis@chello.nl>
> 
> The coding style in this file had some quirks, so I committed the
> attached.

That's largely my code, so please explain the changes, so that I
never repeat any mistakes I've committed.  I've read the entire
patch, and I must say that I don't understand even a single change
you made.  In the comments reformatting department, I guess we have
different setting for fill-column (what is the canonical one, btw?),
but as for the rest, I don't have a clue.  So please do explain.

> -      /* Four(eigth on x86_64) is the maximum length an x86 debug register
> +      int align = addr % max_wp_len;
> +      /* Four (eigth on AMD64) is the maximum length a debug register

Here, there's a real typo in the comment, but it wasn't fixed.

Re: [PATCH] Cleanup i386-nat.c

[Mark Kettenis]

   Date: 29 Feb 2004 08:06:56 +0200
   From: Eli Zaretskii <eliz@elta.co.il>

   > Date: Sat, 28 Feb 2004 16:38:23 +0100 (CET)
   > From: Mark Kettenis <kettenis@chello.nl>
   > 
   > The coding style in this file had some quirks, so I committed the
   > attached.

   That's largely my code, so please explain the changes, so that I
   never repeat any mistakes I've committed.  I've read the entire
   patch, and I must say that I don't understand even a single change
   you made.

* I did some s/x86/i386/g because i386-nat.c was the only file talking
  about x86.

* The coding standards say that every comment should be a full
  sentence, starting with a capital and ending with a full stop (a
  dot).  This also applies to comments on lines with code.  To prevent
  some unnecessary line-wrapping, I used M-; to start them at what's
  the canonical column according to Emacs.

* There were some lines in the code that were too long.  Some of these
  were hard to break, so I used some creativity here.

   In the comments reformatting department, I guess we have different
   setting for fill-column (what is the canonical one, btw?), but as
   for the rest, I don't have a clue.  So please do explain.

According to Emacs, the default setting for the fill-column is 70.

   > -      /* Four(eigth on x86_64) is the maximum length an x86 debug register
   > +      int align = addr % max_wp_len;
   > +      /* Four (eigth on AMD64) is the maximum length a debug register

   Here, there's a real typo in the comment, but it wasn't fixed.

Hmm, sorry about that.  Will fix it sometime after Andrew's cut the
branch.

Mark

Re: [PATCH] Cleanup i386-nat.c

[Eli Zaretskii]

> Date: Sun, 29 Feb 2004 10:41:14 +0100 (CET)
> From: Mark Kettenis <kettenis@chello.nl>
> 
>    That's largely my code, so please explain the changes, so that I
>    never repeat any mistakes I've committed.  I've read the entire
>    patch, and I must say that I don't understand even a single change
>    you made.

Well, thanks for the info, but I'm still not out of the woods, see
below.

> * I did some s/x86/i386/g because i386-nat.c was the only file talking
>   about x86.

I'm not sure this is a good change: it's IMHO confusing to talk about
i386 when almost no one uses that chip.  So if we want consistency,
I'd change all i386's in x86's.  But if you insist on i386, I wouldn't
argue.

> * The coding standards say that every comment should be a full
>   sentence, starting with a capital and ending with a full stop (a
>   dot).  This also applies to comments on lines with code.

Really?  That is one heck of a strange requirement!  A short comment
that follows a line of code is sometimes more than enough to explain
the code of that line; making it a full English statement will usually
prolong it beyond fill-column, thus making a short, concise comment
into a long and less clear one.

FWIW, many GDB source files use short comments after the code like I
did; see, for example, breakpoint.c, lines 1555, 1684, 1689, 1717,
and 1722.

Moreover, the section in standards.texi that seems to require that
comments be full sentences contradicts itself:

 #ifdef foo
   ...
 #else /* not foo */ <=== this isn't a full sentence

>   To prevent
>   some unnecessary line-wrapping, I used M-; to start them at what's
>   the canonical column according to Emacs.

I never treated the default setting of comment-column as a canonical
one, merely as a starting place.  If the code on which we comment
extends beyond that column, I reset the comment-column's value to a
larger value with "C-x ;".  What is wrong with that?

>    In the comments reformatting department, I guess we have different
>    setting for fill-column (what is the canonical one, btw?), but as
>    for the rest, I don't have a clue.  So please do explain.
> 
> According to Emacs, the default setting for the fill-column is 70.

I was asking about the canonical value for GDB.  The Emacs default is
not necessarily that.

Even if we use 70, I'm not sure we should blindly follow it when the
results are ugly.  For example, consider the following hunk of your
change:

	This provides several functions for inserting and removing
    -   hardware-assisted breakpoints and watchpoints, testing if
    -   one or more of the watchpoints triggered and at what address,
    -   checking whether a given region can be watched, etc.
    -
    -   A target which wants to use these functions should define
    -   several macros, such as `target_insert_watchpoint' and
    -   `target_stopped_data_address', listed in target.h, to call
    -   the appropriate functions below.  It should also define
    +   hardware-assisted breakpoints and watchpoints, testing if one or
    +   more of the watchpoints triggered and at what address, checking
    +   whether a given region can be watched, etc.
    +
    +   A target which wants to use these functions should define several
    +   macros, such as `target_insert_watchpoint' and
    +   `target_stopped_data_address', listed in target.h, to call the
    +   appropriate functions below.  It should also define
	I386_USE_GENERIC_WATCHPOINTS in its tm.h file.

I think the last paragraph now looks ugly because its 1st line is
very long, while the second line is very short.  The original was
balanced much better, IMHO.

Then there are more changes that I don't understand and that you
didn't explain:

    -#define TARGET_HAS_DR_LEN_8 0
    +#define TARGET_HAS_DR_LEN_8	0

Is there some rule that a TAB should be used here rather than space(s)?

     /* This is here for completeness.  No platform supports this
    -   functionality yet (as of Mar-2001).  Note that the DE flag in the
    +   functionality yet (as of March 2001).  Note that the DE flag in the
	CR4 register needs to be set to support this.  */

What's wrong with "Mar-2001"?

     /* Local and global exact breakpoint enable flags (a.k.a. slowdown
	flags).  These are only required on i386, to allow detection of the
	exact instruction which caused a watchpoint to break; i486 and
	later processors do that automatically.  We set these flags for
    -   back compatibility.  */
    +   backwards compatibility.  */

Is "back compatibility" bad techspeak?

    -static unsigned	 dr_status_mirror, dr_control_mirror;
    +static unsigned dr_status_mirror, dr_control_mirror;

What's wrong with using a TAB in the original?  (It was probably
produced by Emacs's indent-relative, to align both variables one
below the other, but that's immaterial.)

    -  int rv = 0, status = 0;
    +  int retval = 0, status = 0;

What was the need to rename "rv" into "retval"?

	   if (what == WP_COUNT)
    -	/* size_try_array[] is defined so that each iteration through
    -	   the loop is guaranteed to produce an address and a size
    -	   that can be watched with a single debug register.  Thus,
    -	   for counting the registers required to watch a region, we
    -	   simply need to increment the count on each iteration.  */
    -	rv++;
    +	{
    +	  /* size_try_array[] is defined such that each iteration
    +	     through the loop is guaranteed to produce an address and a
    +	     size that can be watched with a single debug register.
    +	     Thus, for counting the registers required to watch a
    +	     region, we simply need to increment the count on each
    +	     iteration.  */
    +	  retval++;
    +	}

What was the need to add braces?  There's only one executable sentence
in the if-clause.

     i386_region_ok_for_watchpoint (CORE_ADDR addr, int len)
     {
    +  int nregs;
    +
       /* Compute how many aligned watchpoints we would need to cover this
	  region.  */
    -  int nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len,
    -						 hw_write);
    -
    +  nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len, hw_write);

Is there something wrong with declaration and initialization in one
go?

     i386_stopped_data_address (void)
     {
    +  CORE_ADDR addr = 0;
       int i;
    -  CORE_ADDR ret = 0;

Why the different order, and why the rename?

    -	     being paranoiac.  */
    +	     being paranoid.  */

That was supposed to be funny, now it's dull...

TIA

Re: [PATCH] Cleanup i386-nat.c

[Eli Zaretskii]

Ping!  I still am waiting for some response to my last message.  See

  http://sources.redhat.com/ml/gdb-patches/2004-02/msg00902.html

I'd really appreciate a reply.  Thanks.

Re: [PATCH] Cleanup i386-nat.c

[Mark Kettenis]

   Date: Sun, 29 Feb 2004 20:47:57 +0200
   From: "Eli Zaretskii" <eliz@elta.co.il>

   > Date: Sun, 29 Feb 2004 10:41:14 +0100 (CET)
   > From: Mark Kettenis <kettenis@chello.nl>
   > 
   >    That's largely my code, so please explain the changes, so that I
   >    never repeat any mistakes I've committed.  I've read the entire
   >    patch, and I must say that I don't understand even a single change
   >    you made.

   Well, thanks for the info, but I'm still not out of the woods, see
   below.

Eli,

I get the felling I somehow stepped on your toes.  I'm sorry for that.
Although there were some genuine style "problems", my main motivation
for changing the coding style was the fact that it was mere
inconsistent with the rest of the i386-specific files in GDB.  Since
I, as the i386 target maintainer feel somehow repsonsible for this
file, I tried to resolve the inconsistencies.  In no way do I blame
you for those inconsistencies.

   > * The coding standards say that every comment should be a full
   >   sentence, starting with a capital and ending with a full stop (a
   >   dot).  This also applies to comments on lines with code.

   Really?  That is one heck of a strange requirement!  A short comment
   that follows a line of code is sometimes more than enough to explain
   the code of that line; making it a full English statement will usually
   prolong it beyond fill-column, thus making a short, concise comment
   into a long and less clear one.

   FWIW, many GDB source files use short comments after the code like I
   did; see, for example, breakpoint.c, lines 1555, 1684, 1689, 1717,
   and 1722.

There are many inconsistencies in the GDB source files.  We should
work towards removing these inconsitencies, but we should realize that
we can't resolve them all immediately.

   Moreover, the section in standards.texi that seems to require that
   comments be full sentences contradicts itself:

    #ifdef foo
      ...
    #else /* not foo */ <=== this isn't a full sentence



   >   To prevent
   >   some unnecessary line-wrapping, I used M-; to start them at what's
   >   the canonical column according to Emacs.

   I never treated the default setting of comment-column as a canonical
   one, merely as a starting place.  If the code on which we comment
   extends beyond that column, I reset the comment-column's value to a
   larger value with "C-x ;".  What is wrong with that?

Nothing I guess.  In fact it illustrates what I was doing.  By
starting the comments at a suitable point, you can prevent uneccesary
line breaks.

   >    In the comments reformatting department, I guess we have different
   >    setting for fill-column (what is the canonical one, btw?), but as
   >    for the rest, I don't have a clue.  So please do explain.
   > 
   > According to Emacs, the default setting for the fill-column is 70.

   I was asking about the canonical value for GDB.  The Emacs default is
   not necessarily that.

I think it is.  The default settings for c-mode are those that
correspond to the GNU coding standards.  I'm fairly certain this
extends to the fill-column too.

   Even if we use 70, I'm not sure we should blindly follow it when the
   results are ugly.  For example, consider the following hunk of your
   change:

	   This provides several functions for inserting and removing
       -   hardware-assisted breakpoints and watchpoints, testing if
       -   one or more of the watchpoints triggered and at what address,
       -   checking whether a given region can be watched, etc.
       -
       -   A target which wants to use these functions should define
       -   several macros, such as `target_insert_watchpoint' and
       -   `target_stopped_data_address', listed in target.h, to call
       -   the appropriate functions below.  It should also define
       +   hardware-assisted breakpoints and watchpoints, testing if one or
       +   more of the watchpoints triggered and at what address, checking
       +   whether a given region can be watched, etc.
       +
       +   A target which wants to use these functions should define several
       +   macros, such as `target_insert_watchpoint' and
       +   `target_stopped_data_address', listed in target.h, to call the
       +   appropriate functions below.  It should also define
	   I386_USE_GENERIC_WATCHPOINTS in its tm.h file.

   I think the last paragraph now looks ugly because its 1st line is
   very long, while the second line is very short.  The original was
   balanced much better, IMHO.

Yep.  Your origional looks better to me too.  And it doesn't take any
vertical space.  I just blindly hit M-q :-(.

   Then there are more changes that I don't understand and that you
   didn't explain:

       -#define TARGET_HAS_DR_LEN_8 0
       +#define TARGET_HAS_DR_LEN_8	0

   Is there some rule that a TAB should be used here rather than space(s)?

Not really.  I remember adding the TAB to get some extra whitespace
between the 8 and the 0, to prevent myself from reading it as the
number 80.

	/* This is here for completeness.  No platform supports this
       -   functionality yet (as of Mar-2001).  Note that the DE flag in the
       +   functionality yet (as of March 2001).  Note that the DE flag in the
	   CR4 register needs to be set to support this.  */

   What's wrong with "Mar-2001"?

I think spelling it out is better than saving two characters.

	/* Local and global exact breakpoint enable flags (a.k.a. slowdown
	   flags).  These are only required on i386, to allow detection of the
	   exact instruction which caused a watchpoint to break; i486 and
	   later processors do that automatically.  We set these flags for
       -   back compatibility.  */
       +   backwards compatibility.  */

   Is "back compatibility" bad techspeak?

It's unknown to me.

       -static unsigned	 dr_status_mirror, dr_control_mirror;
       +static unsigned dr_status_mirror, dr_control_mirror;

   What's wrong with using a TAB in the original?  (It was probably
   produced by Emacs's indent-relative, to align both variables one
   below the other, but that's immaterial.)

Hmm.  I thought the coding standards had an explicit warning against
trying to make variables line up.

       -  int rv = 0, status = 0;
       +  int retval = 0, status = 0;

   What was the need to rename "rv" into "retval"?

I think rc is too short a name to be meaningfull.

	      if (what == WP_COUNT)
       -	/* size_try_array[] is defined so that each iteration through
       -	   the loop is guaranteed to produce an address and a size
       -	   that can be watched with a single debug register.  Thus,
       -	   for counting the registers required to watch a region, we
       -	   simply need to increment the count on each iteration.  */
       -	rv++;
       +	{
       +	  /* size_try_array[] is defined such that each iteration
       +	     through the loop is guaranteed to produce an address and a
       +	     size that can be watched with a single debug register.
       +	     Thus, for counting the registers required to watch a
       +	     region, we simply need to increment the count on each
       +	     iteration.  */
       +	  retval++;
       +	}

   What was the need to add braces?  There's only one executable sentence
   in the if-clause.

They're there to guide the eye.  The actual statement follows a
five-line comment which makes it diffucult to recognize as a branch.

	i386_region_ok_for_watchpoint (CORE_ADDR addr, int len)
	{
       +  int nregs;
       +
	  /* Compute how many aligned watchpoints we would need to cover this
	     region.  */
       -  int nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len,
       -						 hw_write);
       -
       +  nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len, hw_write);

   Is there something wrong with declaration and initialization in one
   go?

In general: no.  In this case: splitting this up makes it possible to
have all function arguments on one line.

	i386_stopped_data_address (void)
	{
       +  CORE_ADDR addr = 0;
	  int i;
       -  CORE_ADDR ret = 0;

   Why the different order, and why the rename?

The name `addr' is more meaningful than the name `ret'.  About
reordering: no good reason for that, except that I think it lookes
more balanced this way.

       -	     being paranoiac.  */
       +	     being paranoid.  */

   That was supposed to be funny, now it's dull...

Sorry about that one ;-).

Mark

Re: [PATCH] Cleanup i386-nat.c

[Eli Zaretskii]

> Date: Wed, 3 Mar 2004 21:30:57 +0100 (CET)
> From: Mark Kettenis <kettenis@chello.nl>
> 
> I get the felling I somehow stepped on your toes.  I'm sorry for that.
> Although there were some genuine style "problems", my main motivation
> for changing the coding style was the fact that it was mere
> inconsistent with the rest of the i386-specific files in GDB.  Since
> I, as the i386 target maintainer feel somehow repsonsible for this
> file, I tried to resolve the inconsistencies.  In no way do I blame
> you for those inconsistencies.

Well, no offense, but it surely feels like nitpicking that got out of
proportions.  I labored on that code not only to make it right, but
also so that it looks good.  Most of those ``style problems'' are
really conscious decisions on my part, meant to make the code
aesthetically pleasant.

I understand that our styles may well be different a bit, but since
GDB is a program maintained by lots of people, it is no surprise that
the code style isn't uniform.  I don't see anything wrong with that,
provided that the basic GNU coding conventions are preserved.  E.g., I
don't see any need to insist on a specific value of comment-column, or
refill comment blocks, unless they are badly malformed.  Do we really
need this kind of ``style police''?

I guess bottom line is, I think that minor style differences shouldn't
be a reason for such extensive changes, especially when an area
maintainer intends to exercise his/her privilege of committing such
changes in someone else's code without any discussion.

Am I the only one who thinks so?

Re: [PATCH] Cleanup i386-nat.c

[Eli Zaretskii]

> Date: Wed, 3 Mar 2004 21:30:57 +0100 (CET)
> From: Mark Kettenis <kettenis@chello.nl>
> 
> I get the felling I somehow stepped on your toes.  I'm sorry for that.
> Although there were some genuine style "problems", my main motivation
> for changing the coding style was the fact that it was mere
> inconsistent with the rest of the i386-specific files in GDB.  Since
> I, as the i386 target maintainer feel somehow repsonsible for this
> file, I tried to resolve the inconsistencies.  In no way do I blame
> you for those inconsistencies.

Well, no offense, but it surely feels like nitpicking that got out of
proportions.  I labored on that code not only to make it right, but
also so that it looks good.  Most of those ``style problems'' are
really conscious decisions on my part, meant to make the code
aesthetically pleasant.

I understand that our styles may well be different a bit, but since
GDB is a program maintained by lots of people, it is no surprise that
the code style isn't uniform.  I don't see anything wrong with that,
provided that the basic GNU coding conventions are preserved.  E.g., I
don't see any need to insist on a specific value of comment-column, or
refill comment blocks, unless they are badly malformed.  Do we really
need this kind of ``style police''?

I guess bottom line is, I think that minor style differences shouldn't
be a reason for such extensive changes, especially when an area
maintainer intends to exercise his/her privilege of committing such
changes in someone else's code without any discussion.

Am I the only one who thinks so?

Re: [PATCH] Cleanup i386-nat.c

[Mark Kettenis]

   Date: Sun, 29 Feb 2004 20:47:57 +0200
   From: "Eli Zaretskii" <eliz@elta.co.il>

   > Date: Sun, 29 Feb 2004 10:41:14 +0100 (CET)
   > From: Mark Kettenis <kettenis@chello.nl>
   > 
   >    That's largely my code, so please explain the changes, so that I
   >    never repeat any mistakes I've committed.  I've read the entire
   >    patch, and I must say that I don't understand even a single change
   >    you made.

   Well, thanks for the info, but I'm still not out of the woods, see
   below.

Eli,

I get the felling I somehow stepped on your toes.  I'm sorry for that.
Although there were some genuine style "problems", my main motivation
for changing the coding style was the fact that it was mere
inconsistent with the rest of the i386-specific files in GDB.  Since
I, as the i386 target maintainer feel somehow repsonsible for this
file, I tried to resolve the inconsistencies.  In no way do I blame
you for those inconsistencies.

   > * The coding standards say that every comment should be a full
   >   sentence, starting with a capital and ending with a full stop (a
   >   dot).  This also applies to comments on lines with code.

   Really?  That is one heck of a strange requirement!  A short comment
   that follows a line of code is sometimes more than enough to explain
   the code of that line; making it a full English statement will usually
   prolong it beyond fill-column, thus making a short, concise comment
   into a long and less clear one.

   FWIW, many GDB source files use short comments after the code like I
   did; see, for example, breakpoint.c, lines 1555, 1684, 1689, 1717,
   and 1722.

There are many inconsistencies in the GDB source files.  We should
work towards removing these inconsitencies, but we should realize that
we can't resolve them all immediately.

   Moreover, the section in standards.texi that seems to require that
   comments be full sentences contradicts itself:

    #ifdef foo
      ...
    #else /* not foo */ <=== this isn't a full sentence



   >   To prevent
   >   some unnecessary line-wrapping, I used M-; to start them at what's
   >   the canonical column according to Emacs.

   I never treated the default setting of comment-column as a canonical
   one, merely as a starting place.  If the code on which we comment
   extends beyond that column, I reset the comment-column's value to a
   larger value with "C-x ;".  What is wrong with that?

Nothing I guess.  In fact it illustrates what I was doing.  By
starting the comments at a suitable point, you can prevent uneccesary
line breaks.

   >    In the comments reformatting department, I guess we have different
   >    setting for fill-column (what is the canonical one, btw?), but as
   >    for the rest, I don't have a clue.  So please do explain.
   > 
   > According to Emacs, the default setting for the fill-column is 70.

   I was asking about the canonical value for GDB.  The Emacs default is
   not necessarily that.

I think it is.  The default settings for c-mode are those that
correspond to the GNU coding standards.  I'm fairly certain this
extends to the fill-column too.

   Even if we use 70, I'm not sure we should blindly follow it when the
   results are ugly.  For example, consider the following hunk of your
   change:

	   This provides several functions for inserting and removing
       -   hardware-assisted breakpoints and watchpoints, testing if
       -   one or more of the watchpoints triggered and at what address,
       -   checking whether a given region can be watched, etc.
       -
       -   A target which wants to use these functions should define
       -   several macros, such as `target_insert_watchpoint' and
       -   `target_stopped_data_address', listed in target.h, to call
       -   the appropriate functions below.  It should also define
       +   hardware-assisted breakpoints and watchpoints, testing if one or
       +   more of the watchpoints triggered and at what address, checking
       +   whether a given region can be watched, etc.
       +
       +   A target which wants to use these functions should define several
       +   macros, such as `target_insert_watchpoint' and
       +   `target_stopped_data_address', listed in target.h, to call the
       +   appropriate functions below.  It should also define
	   I386_USE_GENERIC_WATCHPOINTS in its tm.h file.

   I think the last paragraph now looks ugly because its 1st line is
   very long, while the second line is very short.  The original was
   balanced much better, IMHO.

Yep.  Your origional looks better to me too.  And it doesn't take any
vertical space.  I just blindly hit M-q :-(.

   Then there are more changes that I don't understand and that you
   didn't explain:

       -#define TARGET_HAS_DR_LEN_8 0
       +#define TARGET_HAS_DR_LEN_8	0

   Is there some rule that a TAB should be used here rather than space(s)?

Not really.  I remember adding the TAB to get some extra whitespace
between the 8 and the 0, to prevent myself from reading it as the
number 80.

	/* This is here for completeness.  No platform supports this
       -   functionality yet (as of Mar-2001).  Note that the DE flag in the
       +   functionality yet (as of March 2001).  Note that the DE flag in the
	   CR4 register needs to be set to support this.  */

   What's wrong with "Mar-2001"?

I think spelling it out is better than saving two characters.

	/* Local and global exact breakpoint enable flags (a.k.a. slowdown
	   flags).  These are only required on i386, to allow detection of the
	   exact instruction which caused a watchpoint to break; i486 and
	   later processors do that automatically.  We set these flags for
       -   back compatibility.  */
       +   backwards compatibility.  */

   Is "back compatibility" bad techspeak?

It's unknown to me.

       -static unsigned	 dr_status_mirror, dr_control_mirror;
       +static unsigned dr_status_mirror, dr_control_mirror;

   What's wrong with using a TAB in the original?  (It was probably
   produced by Emacs's indent-relative, to align both variables one
   below the other, but that's immaterial.)

Hmm.  I thought the coding standards had an explicit warning against
trying to make variables line up.

       -  int rv = 0, status = 0;
       +  int retval = 0, status = 0;

   What was the need to rename "rv" into "retval"?

I think rc is too short a name to be meaningfull.

	      if (what == WP_COUNT)
       -	/* size_try_array[] is defined so that each iteration through
       -	   the loop is guaranteed to produce an address and a size
       -	   that can be watched with a single debug register.  Thus,
       -	   for counting the registers required to watch a region, we
       -	   simply need to increment the count on each iteration.  */
       -	rv++;
       +	{
       +	  /* size_try_array[] is defined such that each iteration
       +	     through the loop is guaranteed to produce an address and a
       +	     size that can be watched with a single debug register.
       +	     Thus, for counting the registers required to watch a
       +	     region, we simply need to increment the count on each
       +	     iteration.  */
       +	  retval++;
       +	}

   What was the need to add braces?  There's only one executable sentence
   in the if-clause.

They're there to guide the eye.  The actual statement follows a
five-line comment which makes it diffucult to recognize as a branch.

	i386_region_ok_for_watchpoint (CORE_ADDR addr, int len)
	{
       +  int nregs;
       +
	  /* Compute how many aligned watchpoints we would need to cover this
	     region.  */
       -  int nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len,
       -						 hw_write);
       -
       +  nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len, hw_write);

   Is there something wrong with declaration and initialization in one
   go?

In general: no.  In this case: splitting this up makes it possible to
have all function arguments on one line.

	i386_stopped_data_address (void)
	{
       +  CORE_ADDR addr = 0;
	  int i;
       -  CORE_ADDR ret = 0;

   Why the different order, and why the rename?

The name `addr' is more meaningful than the name `ret'.  About
reordering: no good reason for that, except that I think it lookes
more balanced this way.

       -	     being paranoiac.  */
       +	     being paranoid.  */

   That was supposed to be funny, now it's dull...

Sorry about that one ;-).

Mark

Re: [PATCH] Cleanup i386-nat.c

[Eli Zaretskii]

Ping!  I still am waiting for some response to my last message.  See

  http://sources.redhat.com/ml/gdb-patches/2004-02/msg00902.html

I'd really appreciate a reply.  Thanks.