From: Yao Qi <qiyaoltc@gmail.com>
Cc: <gdb-patches@sourceware.org>
Date: Mon, 28 Sep 2015 10:54:48 +0100
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?