Re: [PATCH 2/5] Make breakpoint and breakpoint_len local variables in GDBServer.

[Yao Qi <qiyaoltc at gmail dot com>]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> @@ -774,8 +800,9 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
>  {
>    int err_ignored;
>  
> +  /* default breakpoint_len will be initialized downstream.  */
>    return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
> -			 where, breakpoint_len, handler,
> +			 where, 0, handler,
>  			 &err_ignored);
>  }

Why do you set breakpoint length to zero?  I know you'll set the length
downstream properly, but we should compute the breakpoint length here.

After thinking about it a bit more, I think this reveals some design
issues in GDBserver brekapoint, nowadays, GDBserver inserts its own
breakpoints and breakpoints requested by GDB.  After this patch series,
GDBserver should be able to:

  1) choose the right breakpoint instruction for its own breakpoints,
  according to the breakpoint address, status register flag, etc,
  through API set_breakpoint_at,
  2) choose the right breakpoint instruction for breakpoints requested
  by GDB, according to the information in Z packets, through API
  set_gdb_breakpoint

there should be two paths for them, and each path needs different target
hook to choose breakpoint instructions.  breakpoint_from_pc is needed for
#1, and breakpoint_from_length is needed for #2.  In your current patch
set (in patch 4/5), two things are mixed together, which doesn't look
good to me.  The current functions calls in GDBserver to create
breakpoint is like

  set_breakpoint_at
  set_gdb_breakpoint_1
     |
     `--> set_breakpoint
             |
             `-->set_raw_breakpoint_at
                    |
                    `--> the_target->insert_point

we are handling the breakpoint length at the lowest level, in
insert_memory_breakpoint, and we use breakpoint_from_pc and
breakpoint_from_length together there, which looks unclean.  Ideally, we
can move these code handling breakpoint length (breakpoint_from_pc and
breakpoint_from_length) to upper levels, like this,

  set_breakpoint_at (call breakpoint_from_pc)
  set_gdb_breakpoint_1 (call breakpoint_from_length)
     |
     `--> set_breakpoint
             |
             `-->set_raw_breakpoint_at
                    |
                    `--> the_target->insert_point

all needed information is saved in struct breakpoint or struct
raw_breakpoint, and function set_breakpoint and it callees can use
breakpoint or raw_breakpoint directly.  It'll be cleaner in this way,
let me know what do you think?

-- 
Yao (éå)