Re: [Patch] New gdbarch method "dwarf_cfa_op" and migrate SPARC to it

[Jiong Wang <jiong.wang@foss.arm.com>]

[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]

Thanks for the review.

On 21/04/17 20:37, Pedro Alves wrote:
> On 04/21/2017 03:56 PM, Jiong Wang wrote:
> Doesn't the Aarch64 version of the opcode have its own
> name, like DW_CFA_GNU_Aarch64_whatever, even if it reuses the
> opcode number?  I think that would help a lot going forward
> if it had one.  E.g., it'd avoid confusion, allow for easier
> searching, etc.

AArch64 do have a name for it, "DW_CFA_AARCH64_negate_ra_state",
I will add it to gcc/include/dwarf2.def later once it's used and will request
a sync from GCC to Bintuils.

> On the patch, I think it would be better if in execute_cfa_program:
>
>>          case DW_CFA_GNU_window_save:
>> -          /* This is SPARC-specific code, and contains hard-coded
>> -             constants for the register numbering scheme used by
>
> you removed "case CFA_GNU_window_save:" too, and moved the
> gdbarch_dwarf_cfa_op call to the default case.

Done.

> Then make the hook return a boolean indication about whether it /recognized the
> opcode, and make the caller throw an error if the hook returns
> false.

Done.

> And make that error call be a regular "error()" call instead
> of the current internal_error call.  The internal error is bogus
> here because we reach that with an unrecognized opcode that comes
> from a binary, i.e., input, not a gdb bug.

Done.

> Replace the gdb_assert in sparc_dwarf_cfa_op by returning false for
> unrecognized opcodes, and likewise change default_dwarf_cfa_op
> to return false instead of calling error itself.

Done.

> Add a comment at the hook call site about letting the backend
> handle vendor-specific opcodes, and generalize a bit the comment
> describing the hook in gdbarch.sh in that direction as well.

Done.

> Maybe rename to hook from dwarf_cfa_op to something like:
>   execute_dwarf_cfa_vendor_op

Done with above name.

Ok for master? thanks


gdb/
2017-04-25  Jiong Wang  <jiong.wang@arm.com>

         * gdbarch.sh: New gdbarch method execute_dwarf_cfa_vendor_op.
         * gdbarch.c: Regenerated.
         * gdbarch.h: Regenerated.
         * dwarf2-frame.c (dwarf2_frame_state_alloc_regs): Made the
         visibility external.
         (execute_cfa_program): Call execute_dwarf_cfa_vendor_op for CFI
         between DW_CFA_lo_user and DW_CFA_high_user inclusive.
         (enum cfa_how_kind): Move to ...
         (struct dwarf2_frame_state_reg_info): Likewise.
         (struct dwarf2_frame_state): Likewise.
         * dwarf2-frame.h: ... here.
         (dwarf2_frame_state_alloc_regs): New declaration.
         * sparc-tdep.c (sparc_execute_dwarf_cfa_vendor_op): New function.
         (sparc32_gdbarch_init): Register execute_dwarf_cfa_vendor_op hook.


[-- Attachment #2: update.patch --]
[-- Type: text/x-diff, Size: 14246 bytes --]

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 3b22e51..b1cec80 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -204,6 +204,15 @@ default_adjust_dwarf2_line (CORE_ADDR addr, int rel)
   return addr;
 }
 
+/* See arch-utils.h.  */
+
+bool
+default_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op,
+				     struct dwarf2_frame_state *fs)
+{
+  return false;
+}
+
 int
 cannot_register_not (struct gdbarch *gdbarch, int regnum)
 {
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 4d7b499..967a4b1 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -25,6 +25,7 @@ struct frame_info;
 struct minimal_symbol;
 struct type;
 struct gdbarch_info;
+struct dwarf2_frame_state;
 
 template <size_t bp_size, const gdb_byte *break_insn>
 struct bp_manipulation
@@ -130,6 +131,11 @@ CORE_ADDR default_adjust_dwarf2_addr (CORE_ADDR pc);
 
 CORE_ADDR default_adjust_dwarf2_line (CORE_ADDR addr, int rel);
 
+/* Default DWARF vendor CFI handler.  */
+
+bool default_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op,
+					  struct dwarf2_frame_state *fs);
+
 /* Version of cannot_fetch_register() / cannot_store_register() that
    always fails.  */
 
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index f8dd1df..1bdddeb 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -165,58 +165,6 @@ static CORE_ADDR read_encoded_value (struct comp_unit *unit, gdb_byte encoding,
 				     CORE_ADDR func_base);
 \f
 
-enum cfa_how_kind
-{
-  CFA_UNSET,
-  CFA_REG_OFFSET,
-  CFA_EXP
-};
-
-struct dwarf2_frame_state_reg_info
-{
-  struct dwarf2_frame_state_reg *reg;
-  int num_regs;
-
-  LONGEST cfa_offset;
-  ULONGEST cfa_reg;
-  enum cfa_how_kind cfa_how;
-  const gdb_byte *cfa_exp;
-
-  /* Used to implement DW_CFA_remember_state.  */
-  struct dwarf2_frame_state_reg_info *prev;
-};
-
-/* Structure describing a frame state.  */
-
-struct dwarf2_frame_state
-{
-  /* Each register save state can be described in terms of a CFA slot,
-     another register, or a location expression.  */
-  struct dwarf2_frame_state_reg_info regs;
-
-  /* The PC described by the current frame state.  */
-  CORE_ADDR pc;
-
-  /* Initial register set from the CIE.
-     Used to implement DW_CFA_restore.  */
-  struct dwarf2_frame_state_reg_info initial;
-
-  /* The information we care about from the CIE.  */
-  LONGEST data_align;
-  ULONGEST code_align;
-  ULONGEST retaddr_column;
-
-  /* Flags for known producer quirks.  */
-
-  /* The ARM compilers, in DWARF2 mode, assume that DW_CFA_def_cfa
-     and DW_CFA_def_cfa_offset takes a factored offset.  */
-  int armcc_cfa_offsets_sf;
-
-  /* The ARM compilers, in DWARF2 or DWARF3 mode, may assume that
-     the CFA is defined as REG - OFFSET rather than REG + OFFSET.  */
-  int armcc_cfa_offsets_reversed;
-};
-
 /* Store the length the expression for the CFA in the `cfa_reg' field,
    which is unused in that case.  */
 #define cfa_exp_len cfa_reg
@@ -224,7 +172,7 @@ struct dwarf2_frame_state
 /* Assert that the register set RS is large enough to store gdbarch_num_regs
    columns.  If necessary, enlarge the register set.  */
 
-static void
+void
 dwarf2_frame_state_alloc_regs (struct dwarf2_frame_state_reg_info *rs,
 			       int num_regs)
 {
@@ -672,31 +620,6 @@ bad CFI data; mismatched DW_CFA_restore_state at %s"),
 	      /* cfa_how deliberately not set.  */
 	      break;
 
-	    case DW_CFA_GNU_window_save:
-	      /* This is SPARC-specific code, and contains hard-coded
-		 constants for the register numbering scheme used by
-		 GCC.  Rather than having a architecture-specific
-		 operation that's only ever used by a single
-		 architecture, we provide the implementation here.
-		 Incidentally that's what GCC does too in its
-		 unwinder.  */
-	      {
-		int size = register_size (gdbarch, 0);
-
-		dwarf2_frame_state_alloc_regs (&fs->regs, 32);
-		for (reg = 8; reg < 16; reg++)
-		  {
-		    fs->regs.reg[reg].how = DWARF2_FRAME_REG_SAVED_REG;
-		    fs->regs.reg[reg].loc.reg = reg + 16;
-		  }
-		for (reg = 16; reg < 32; reg++)
-		  {
-		    fs->regs.reg[reg].how = DWARF2_FRAME_REG_SAVED_OFFSET;
-		    fs->regs.reg[reg].loc.offset = (reg - 16) * size;
-		  }
-	      }
-	      break;
-
 	    case DW_CFA_GNU_args_size:
 	      /* Ignored.  */
 	      insn_ptr = safe_read_uleb128 (insn_ptr, insn_end, &utmp);
@@ -713,8 +636,17 @@ bad CFI data; mismatched DW_CFA_restore_state at %s"),
 	      break;
 
 	    default:
-	      internal_error (__FILE__, __LINE__,
-			      _("Unknown CFI encountered."));
+	      if (insn >= DW_CFA_lo_user && insn <= DW_CFA_hi_user)
+		{
+		  /* Handle vendor-specific CFI for different architectures.  */
+		  if (!gdbarch_execute_dwarf_cfa_vendor_op (gdbarch, insn, fs))
+		    error (_("Call Frame Instruction %d in vendor extension "
+			     "space is not handled on this architecture."),
+			   insn);
+		}
+	      else
+		internal_error (__FILE__, __LINE__,
+				_("Unknown CFI encountered."));
 	    }
 	}
     }
diff --git a/gdb/dwarf2-frame.h b/gdb/dwarf2-frame.h
index 2ada436..9e8668e 100644
--- a/gdb/dwarf2-frame.h
+++ b/gdb/dwarf2-frame.h
@@ -82,6 +82,58 @@ struct dwarf2_frame_state_reg
   enum dwarf2_frame_reg_rule how;
 };
 
+enum cfa_how_kind
+{
+  CFA_UNSET,
+  CFA_REG_OFFSET,
+  CFA_EXP
+};
+
+struct dwarf2_frame_state_reg_info
+{
+  struct dwarf2_frame_state_reg *reg;
+  int num_regs;
+
+  LONGEST cfa_offset;
+  ULONGEST cfa_reg;
+  enum cfa_how_kind cfa_how;
+  const gdb_byte *cfa_exp;
+
+  /* Used to implement DW_CFA_remember_state.  */
+  struct dwarf2_frame_state_reg_info *prev;
+};
+
+/* Structure describing a frame state.  */
+
+struct dwarf2_frame_state
+{
+  /* Each register save state can be described in terms of a CFA slot,
+     another register, or a location expression.  */
+  struct dwarf2_frame_state_reg_info regs;
+
+  /* The PC described by the current frame state.  */
+  CORE_ADDR pc;
+
+  /* Initial register set from the CIE.
+     Used to implement DW_CFA_restore.  */
+  struct dwarf2_frame_state_reg_info initial;
+
+  /* The information we care about from the CIE.  */
+  LONGEST data_align;
+  ULONGEST code_align;
+  ULONGEST retaddr_column;
+
+  /* Flags for known producer quirks.  */
+
+  /* The ARM compilers, in DWARF2 mode, assume that DW_CFA_def_cfa
+     and DW_CFA_def_cfa_offset takes a factored offset.  */
+  int armcc_cfa_offsets_sf;
+
+  /* The ARM compilers, in DWARF2 or DWARF3 mode, may assume that
+     the CFA is defined as REG - OFFSET rather than REG + OFFSET.  */
+  int armcc_cfa_offsets_reversed;
+};
+
 /* Set the architecture-specific register state initialization
    function for GDBARCH to INIT_REG.  */
 
@@ -120,6 +172,12 @@ extern const struct frame_base *
 
 CORE_ADDR dwarf2_frame_cfa (struct frame_info *this_frame);
 
+/* Assert that the register set RS is large enough to store gdbarch_num_regs
+   columns.  If necessary, enlarge the register set.  */
+
+void dwarf2_frame_state_alloc_regs (struct dwarf2_frame_state_reg_info *rs,
+				    int num_regs);
+
 /* Find the CFA information for PC.
 
    Return 1 if a register is used for the CFA, or 0 if another
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 5664325..41c0866 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -275,6 +275,7 @@ struct gdbarch
   int have_nonsteppable_watchpoint;
   gdbarch_address_class_type_flags_ftype *address_class_type_flags;
   gdbarch_address_class_type_flags_to_name_ftype *address_class_type_flags_to_name;
+  gdbarch_execute_dwarf_cfa_vendor_op_ftype *execute_dwarf_cfa_vendor_op;
   gdbarch_address_class_name_to_type_flags_ftype *address_class_name_to_type_flags;
   gdbarch_register_reggroup_p_ftype *register_reggroup_p;
   gdbarch_fetch_pointer_argument_ftype *fetch_pointer_argument;
@@ -436,6 +437,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->make_symbol_special = default_make_symbol_special;
   gdbarch->adjust_dwarf2_addr = default_adjust_dwarf2_addr;
   gdbarch->adjust_dwarf2_line = default_adjust_dwarf2_line;
+  gdbarch->execute_dwarf_cfa_vendor_op = default_execute_dwarf_cfa_vendor_op;
   gdbarch->register_reggroup_p = default_register_reggroup_p;
   gdbarch->skip_permanent_breakpoint = default_skip_permanent_breakpoint;
   gdbarch->displaced_step_hw_singlestep = default_displaced_step_hw_singlestep;
@@ -634,6 +636,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of have_nonsteppable_watchpoint, invalid_p == 0 */
   /* Skip verify of address_class_type_flags, has predicate.  */
   /* Skip verify of address_class_type_flags_to_name, has predicate.  */
+  /* Skip verify of execute_dwarf_cfa_vendor_op, invalid_p == 0 */
   /* Skip verify of address_class_name_to_type_flags, has predicate.  */
   /* Skip verify of register_reggroup_p, invalid_p == 0 */
   /* Skip verify of fetch_pointer_argument, has predicate.  */
@@ -975,6 +978,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: elfcore_write_linux_prpsinfo = <%s>\n",
                       host_address_to_string (gdbarch->elfcore_write_linux_prpsinfo));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: execute_dwarf_cfa_vendor_op = <%s>\n",
+                      host_address_to_string (gdbarch->execute_dwarf_cfa_vendor_op));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: fast_tracepoint_valid_at = <%s>\n",
                       host_address_to_string (gdbarch->fast_tracepoint_valid_at));
   fprintf_unfiltered (file,
@@ -3523,6 +3529,23 @@ set_gdbarch_address_class_type_flags_to_name (struct gdbarch *gdbarch,
   gdbarch->address_class_type_flags_to_name = address_class_type_flags_to_name;
 }
 
+bool
+gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op, struct dwarf2_frame_state *fs)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->execute_dwarf_cfa_vendor_op != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_execute_dwarf_cfa_vendor_op called\n");
+  return gdbarch->execute_dwarf_cfa_vendor_op (gdbarch, op, fs);
+}
+
+void
+set_gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch,
+                                         gdbarch_execute_dwarf_cfa_vendor_op_ftype execute_dwarf_cfa_vendor_op)
+{
+  gdbarch->execute_dwarf_cfa_vendor_op = execute_dwarf_cfa_vendor_op;
+}
+
 int
 gdbarch_address_class_name_to_type_flags_p (struct gdbarch *gdbarch)
 {
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 4845f23..7617908 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -827,6 +827,13 @@ typedef const char * (gdbarch_address_class_type_flags_to_name_ftype) (struct gd
 extern const char * gdbarch_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags);
 extern void set_gdbarch_address_class_type_flags_to_name (struct gdbarch *gdbarch, gdbarch_address_class_type_flags_to_name_ftype *address_class_type_flags_to_name);
 
+/* Execute vendor-specific DWARF Call Frame Instruction.  OP is the instruction.
+   FS are passed from the generic execute_cfa_program function. */
+
+typedef bool (gdbarch_execute_dwarf_cfa_vendor_op_ftype) (struct gdbarch *gdbarch, gdb_byte op, struct dwarf2_frame_state *fs);
+extern bool gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op, struct dwarf2_frame_state *fs);
+extern void set_gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdbarch_execute_dwarf_cfa_vendor_op_ftype *execute_dwarf_cfa_vendor_op);
+
 /* Return the appropriate type_flags for the supplied address class.
    This function should return 1 if the address class was recognized and
    type_flags was set, zero otherwise. */
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index a42dc43..fa85f7c 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -708,6 +708,9 @@ v:int:cannot_step_breakpoint:::0:0::0
 v:int:have_nonsteppable_watchpoint:::0:0::0
 F:int:address_class_type_flags:int byte_size, int dwarf2_addr_class:byte_size, dwarf2_addr_class
 M:const char *:address_class_type_flags_to_name:int type_flags:type_flags
+# Execute vendor-specific DWARF Call Frame Instruction.  OP is the instruction.
+# FS are passed from the generic execute_cfa_program function.
+m:bool:execute_dwarf_cfa_vendor_op:gdb_byte op, struct dwarf2_frame_state *fs:op, fs::default_execute_dwarf_cfa_vendor_op::0
 
 # Return the appropriate type_flags for the supplied address class.
 # This function should return 1 if the address class was recognized and
diff --git a/gdb/sparc-tdep.c b/gdb/sparc-tdep.c
index 078907a..73152ce 100644
--- a/gdb/sparc-tdep.c
+++ b/gdb/sparc-tdep.c
@@ -20,6 +20,7 @@
 #include "defs.h"
 #include "arch-utils.h"
 #include "dis-asm.h"
+#include "dwarf2.h"
 #include "dwarf2-frame.h"
 #include "floatformat.h"
 #include "frame.h"
@@ -1536,6 +1537,34 @@ sparc32_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
     }
 }
 
+/* Implement the execute_dwarf_cfa_vendor_op method.  */
+
+static bool
+sparc_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_byte op,
+				   struct dwarf2_frame_state *fs)
+{
+  /* Only DW_CFA_GNU_window_save is expected on SPARC.  */
+  if (op != DW_CFA_GNU_window_save)
+    return false;
+
+  uint64_t reg;
+  int size = register_size (gdbarch, 0);
+
+  dwarf2_frame_state_alloc_regs (&fs->regs, 32);
+  for (reg = 8; reg < 16; reg++)
+    {
+      fs->regs.reg[reg].how = DWARF2_FRAME_REG_SAVED_REG;
+      fs->regs.reg[reg].loc.reg = reg + 16;
+    }
+  for (reg = 16; reg < 32; reg++)
+    {
+      fs->regs.reg[reg].how = DWARF2_FRAME_REG_SAVED_OFFSET;
+      fs->regs.reg[reg].loc.offset = (reg - 16) * size;
+    }
+
+  return true;
+}
+
 \f
 /* The SPARC Architecture doesn't have hardware single-step support,
    and most operating systems don't implement it either, so we provide
@@ -1801,6 +1830,9 @@ sparc32_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   /* Hook in the DWARF CFI frame unwinder.  */
   dwarf2_frame_set_init_reg (gdbarch, sparc32_dwarf2_frame_init_reg);
+  /* Register DWARF vendor CFI handler.  */
+  set_gdbarch_execute_dwarf_cfa_vendor_op (gdbarch,
+					   sparc_execute_dwarf_cfa_vendor_op);
   /* FIXME: kettenis/20050423: Don't enable the unwinder until the
      StackGhost issues have been resolved.  */