Re: [PATCH v3 06/10] Replace breakpoint_reinsert_addr by get_next_pcs operation in GDBServer.

Pedro Alves
2015-11-26T10:50:44Z

On 11/23/2015 02:12 PM, Antoine Tremblay wrote:
> In this v3:
>
> * Changed get_next_pcs signature.
> * Remove common/get-next-pcs.h file.
> * CORE_ADDR is now in linux-low.h temporarely.
>
> ---
>
> This patch in preparation for software single step support on ARM. It refactors
> breakpoint_reinsert_addr into get_next_pcs so that multiple location can be
> returned.
>
> When software single stepping there can be multiple possible next addresses
> because we're stepping over a conditional branch instruction for example.
>
> The operation get_next_pcs handles that by returning a vector of all the
> possible next addresses.
>
> Software breakpoints are installed at each location returned.
>

Thanks for doing this. This interface does look cleaner to me, and a
more direct mapping C++ (so easier to clean up in the future).

>
> +/* Install breakpoints for software single stepping. */
> +
> +static void
> +install_software_single_step_breakpoints (struct lwp_info *lwp)
> +{
> + int i;
> + CORE_ADDR pc;
> + struct regcache *regcache = get_thread_regcache (current_thread, 1);
> + VEC (CORE_ADDR) *next_pcs = NULL;

I think this should be freed with a cleanup.

> +
> + pc = get_pc (lwp);
> + next_pcs = (*the_low_target.get_next_pcs) (pc, regcache);
> +
> + for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); ++i)
> + {
> + set_reinsert_breakpoint (pc);
> + }

Single-line statements don't get braces.

Otherwise LGTM.

Thanks,
Pedro Alves