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

[Antoine Tremblay <antoine.tremblay@ericsson.com>]

On 09/28/2015 06:05 AM, Eli Zaretskii wrote:
>> 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?
>
> Sometimes only the target layer knows how to choose the length
> correctly.  Are we sure this isn't one of those cases, and can never
> be?
>

breakpoint_from_pc and breakpoint_from_length are target ops so indeed 
only the target knows but it's ok we can query the target properly.