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

[Pedro Alves <pedro at palves dot net>]

On 04/21/2017 03:56 PM, Jiong Wang wrote:

> Hi Ivo,
> 
>   Thanks very much for testing this on SPARC platform.
> 
>   What's really reused is the DWARF CFA number 0x2d behind
> DW_CFA_GNU_window_save.  It is in vendor extension space (
> DW_CFA_lo_user.. DW_CFA_hi_user) so the semantics depends on vendor
> interpreation.

Maybe the commit log could/should be simplified, because
I was confused too.

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.

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.  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.  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.

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.

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.

Maybe rename to hook from dwarf_cfa_op to something like:

  handle_dwarf_cfa_op
  handle_dwarf_cfa_vendor_op
  execute_dwarf_cfa_op
  execute_dwarf_cfa_vendor_op

too, while at it?

Thanks,
Pedro Alves