This is the mail archive of the
mailing list for the GDB project.
Re: [PATCH 2/2] Support the new PPC476 processor -- Arch Dependent
- From: Eli Zaretskii <eliz at gnu dot org>
- To: Sérgio Durigan Júnior <sergiodj at linux dot vnet dot ibm dot com>
- Cc: gdb-patches at sourceware dot org, bauerman at br dot ibm dot com, luisgpm at linux dot vnet dot ibm dot com, tyrlik at us dot ibm dot com
- Date: Fri, 18 Dec 2009 12:45:26 +0200
- Subject: Re: [PATCH 2/2] Support the new PPC476 processor -- Arch Dependent
- References: <email@example.com>
- Reply-to: Eli Zaretskii <eliz at gnu dot org>
> From: Sérgio_Durigan_Júnior <firstname.lastname@example.org>
> Date: Wed, 16 Dec 2009 18:47:20 -0200
> Cc: Thiago Jung Bauermann <email@example.com>, Luis Machado <firstname.lastname@example.org>, Matt Tyrlik <email@example.com>
> It also defines a bunch of new methods in the target_ops, so that
> other archs can implement them in the future.
I wonder why architecture-specific options need to be exposed to the
target.c level. Can't a target be smart enough to decide which kind
of watchpoint to pass to the kernel in each case?
In any case, if we do that, we need to be sure that the exposed APIs
are sufficiently general to be useful for other archs. We had in the
past a similar problem with hardware watchpoint support code that was
written to support a single architecture (Sparc, I believe) without
any attempt to generalize the interface between the low-level stuff
and application-level GDB code. It took years to get that right
again, once hardware watchpoints began being supported by a radically
different architecture (x86).
> (booke_linux_insert_normal_watchpoint): New function.
> (booke_linux_remove_normal_watchpoint): Ditto.
> (booke_linux_insert_ranged_watchpoint): Ditto.
> (booke_linux_remove_ranged_watchpoint): Ditto.
> (booke_linux_insert_watchpoint): Ditto.
> (booke_linux_remove_watchpoint): Ditto.
> (booke_linux_insert_mask_watchpoint): Ditto.
> (booke_linux_remove_mask_watchpoint): Ditto.
> (booke_linux_fill_cond_mode): Ditto.
> (booke_linux_insert_cond_accel_watchpoint): Ditto.
> (booke_linux_remove_cond_accel_watchpoint): Ditto.
I wonder why we need a dozen of functions whose innards are almost
entirely identical, with only minor variations. Isn't it better to
have a single low-level function that sets the ppc_hw_breakpoint
struct and calls booke_insert_point, and have all the others modify
what it does by passing appropriate arguments?
Also, with so many booke_* functions, isn't it better to have them on
a separate source file?
> -#define target_region_ok_for_hw_watchpoint(addr, len) \
> - (*current_target.to_region_ok_for_hw_watchpoint) (addr, len)
> +/* Returns non-zero if the target supports the special type of hardware
> + breakpoint/watchpoint represented by FLAG. */
> +#ifndef TARGET_CAN_USE_SPECIAL_HW_POINT_P
> +#define TARGET_CAN_USE_SPECIAL_HW_POINT_P(flag) \
> + (*current_target.to_can_use_special_hw_point_p) (flag)
> +#ifndef TARGET_REGION_OK_FOR_HW_WATCHPOINT
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr, len, is_big_blob) \
> + (*current_target.to_region_ok_for_hw_watchpoint) (addr, len, is_big_blob)
> +/* Returns greater than zero if the target supports hardware-accelerated
> + condition. */
> +#ifndef TARGET_CAN_USE_WATCHPOINT_COND_ACCEL_P
> +#define TARGET_CAN_USE_WATCHPOINT_COND_ACCEL_P(loc) \
> + (*current_target.to_can_use_watchpoint_cond_accel_p) (loc)
> +#ifndef TARGET_GET_WATCHPOINT_COND_ACCEL_ADDR
> +#define TARGET_GET_WATCHPOINT_COND_ACCEL_ADDR(loc, addr) \
> + (*current_target.to_get_watchpoint_cond_accel_addr) (loc, addr)
Why is this introducing back the TARGET_* macros that we have just got
rid of not long ago? Can't you go with the style of (lower-case)