diff mbox

[06/13] Add enum for mips breakpoint kinds

Message ID 1472655965-12212-7-git-send-email-yao.qi@linaro.org
State New, archived
Headers

Commit Message

Yao Qi Aug. 31, 2016, 3:05 p.m. UTC 
  This patch adds an enum mips_breakpoint_kinds to avoid using magic
numbers as much as possible.

gdb:

2016-08-31  Yao Qi  <yao.qi@linaro.org>

	* mips-tdep.c (mips_breakpoint_kinds): New enum.
	(mips_breakpoint_from_pc): Use it.
	(mips_remote_breakpoint_from_pc): Likewise.
---
 gdb/mips-tdep.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

Comments

Pedro Alves Oct. 27, 2016, 2:54 p.m. UTC | #1 
On 08/31/2016 04:05 PM, Yao Qi wrote:
> This patch adds an enum mips_breakpoint_kinds to avoid using magic
> numbers as much as possible.
> 
> gdb:
> 
> 2016-08-31  Yao Qi  <yao.qi@linaro.org>
> 
> 	* mips-tdep.c (mips_breakpoint_kinds): New enum.
> 	(mips_breakpoint_from_pc): Use it.
> 	(mips_remote_breakpoint_from_pc): Likewise.
> ---
>  gdb/mips-tdep.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 4e4d79e..34df8d0 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -107,6 +107,20 @@ static const char *const mips_abi_strings[] = {
>    NULL
>  };
>  
> +/* Enum describing the different kinds of breakpoints.  */
> +
> +enum mips_breakpoint_kinds

IMO that should be singular.  Imagine you put one of these
in a variable.  Like:

  enum mips_breakpoint_kinds kind;

This would be more natural, IMO:

  enum mips_breakpoint_kind kind;

> +{
> +  /* 16-bit MIPS16 mode breakpoint */
> +  MIPS_BP_KIND_16BIT_MIPS16 = 2,
> +  /* 16-bit microMIPS mode breakpoint */
> +  MIPS_BP_KIND_16BIT_MICROMIPS = 3,
> +  /* 32-bit standard MIPS mode breakpoint */
> +  MIPS_BP_KIND_32BIT = 4,
> +  /* 32-bit microMIPS mode breakpoint */
> +  MIPS_BP_KIND_32BIT_MICROMIPS = 5,

IMO a line break between these makes it much more readable.

  /* 16-bit MIPS16 mode breakpoint */
  MIPS_BP_KIND_16BIT_MIPS16 = 2,

  /* 16-bit microMIPS mode breakpoint */
  MIPS_BP_KIND_16BIT_MICROMIPS = 3,

etc.

> +};
> +
>  /* For backwards compatibility we default to MIPS16.  This flag is
>     overridden as soon as unambiguous ELF file flags tell us the
>     compressed ISA encoding used.  */
> @@ -7143,16 +7157,7 @@ mips_breakpoint_from_pc (struct gdbarch *gdbarch,
>      }
>  }
>  
> -/* Determine the remote breakpoint kind suitable for the PC.  The following
> -   kinds are used:
> -
> -   * 2 -- 16-bit MIPS16 mode breakpoint,
> -
> -   * 3 -- 16-bit microMIPS mode breakpoint,
> -
> -   * 4 -- 32-bit standard MIPS mode breakpoint,
> -
> -   * 5 -- 32-bit microMIPS mode breakpoint.  */

In a latter patch you seem to read this comment.

> +/* Determine the remote breakpoint kind suitable for the PC.  */

Thanks,
Pedro Alves
Maciej W. Rozycki Oct. 28, 2016, 7:39 p.m. UTC | #2 
Pedro, Yao --

 I missed the MIPS parts previously, sorry.

On Thu, 27 Oct 2016, Pedro Alves wrote:

> > +/* Enum describing the different kinds of breakpoints.  */
> > +
> > +enum mips_breakpoint_kinds
> 
> IMO that should be singular.  Imagine you put one of these
> in a variable.  Like:
> 
>   enum mips_breakpoint_kinds kind;
> 
> This would be more natural, IMO:
> 
>   enum mips_breakpoint_kind kind;

 Agreed.

> > +{
> > +  /* 16-bit MIPS16 mode breakpoint */
> > +  MIPS_BP_KIND_16BIT_MIPS16 = 2,
> > +  /* 16-bit microMIPS mode breakpoint */
> > +  MIPS_BP_KIND_16BIT_MICROMIPS = 3,
> > +  /* 32-bit standard MIPS mode breakpoint */
> > +  MIPS_BP_KIND_32BIT = 4,
> > +  /* 32-bit microMIPS mode breakpoint */
> > +  MIPS_BP_KIND_32BIT_MICROMIPS = 5,
> 
> IMO a line break between these makes it much more readable.
> 
>   /* 16-bit MIPS16 mode breakpoint */
>   MIPS_BP_KIND_16BIT_MIPS16 = 2,
> 
>   /* 16-bit microMIPS mode breakpoint */
>   MIPS_BP_KIND_16BIT_MICROMIPS = 3,
> 
> etc.

 Indeed.  Also a full stop and two spaces at the end are due.

 Also how about calling them:

MIPS_BP_KIND_MIPS16
MIPS_BP_KIND_MICROMIPS16
MIPS_BP_KIND_MIPS32
MIPS_BP_KIND_MICROMIPS32

to make the names a bit shorter though still retaining the meaning?

  Maciej
diff mbox

Patch

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 4e4d79e..34df8d0 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -107,6 +107,20 @@  static const char *const mips_abi_strings[] = {
   NULL
 };
 
+/* Enum describing the different kinds of breakpoints.  */
+
+enum mips_breakpoint_kinds
+{
+  /* 16-bit MIPS16 mode breakpoint */
+  MIPS_BP_KIND_16BIT_MIPS16 = 2,
+  /* 16-bit microMIPS mode breakpoint */
+  MIPS_BP_KIND_16BIT_MICROMIPS = 3,
+  /* 32-bit standard MIPS mode breakpoint */
+  MIPS_BP_KIND_32BIT = 4,
+  /* 32-bit microMIPS mode breakpoint */
+  MIPS_BP_KIND_32BIT_MICROMIPS = 5,
+};
+
 /* For backwards compatibility we default to MIPS16.  This flag is
    overridden as soon as unambiguous ELF file flags tell us the
    compressed ISA encoding used.  */
@@ -7143,16 +7157,7 @@  mips_breakpoint_from_pc (struct gdbarch *gdbarch,
     }
 }
 
-/* Determine the remote breakpoint kind suitable for the PC.  The following
-   kinds are used:
-
-   * 2 -- 16-bit MIPS16 mode breakpoint,
-
-   * 3 -- 16-bit microMIPS mode breakpoint,
-
-   * 4 -- 32-bit standard MIPS mode breakpoint,
-
-   * 5 -- 32-bit microMIPS mode breakpoint.  */
+/* Determine the remote breakpoint kind suitable for the PC.  */
 
 static void
 mips_remote_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
@@ -7163,21 +7168,23 @@  mips_remote_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
   if (mips_pc_is_mips16 (gdbarch, pc))
     {
       *pcptr = unmake_compact_addr (pc);
-      *kindptr = 2;
+      *kindptr = MIPS_BP_KIND_16BIT_MIPS16;
     }
   else if (mips_pc_is_micromips (gdbarch, pc))
     {
       ULONGEST insn;
       int status;
-      int size;
 
       insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status);
-      size = status ? 2 : mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4;
+      if (status || (mips_insn_size (ISA_MICROMIPS, insn) == 2))
+	*kindptr = MIPS_BP_KIND_16BIT_MICROMIPS;
+      else
+	*kindptr = MIPS_BP_KIND_32BIT_MICROMIPS;
+
       *pcptr = unmake_compact_addr (pc);
-      *kindptr = size | 1;
     }
   else
-    *kindptr = 4;
+    *kindptr = MIPS_BP_KIND_32BIT;
 }
 
 /* Return non-zero if the standard MIPS instruction INST has a branch