This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/2] Support the new PPC476 processor -- Arch Dependent
- From: Thiago Jung Bauermann <bauerman at br dot ibm dot com>
- To: Eli Zaretskii <eliz at gnu dot org>, gdb-patches at sourceware dot org
- Cc: luisgpm at linux dot vnet dot ibm dot com, tyrlik at us dot ibm dot com
- Date: Wed, 30 Dec 2009 01:20:02 -0200
- Subject: Re: [PATCH 2/2] Support the new PPC476 processor -- Arch Dependent
- References: <200912161847.20313.sergiodj@linux.vnet.ibm.com> <83ws0k5znd.fsf@gnu.org>
On Fri 18 Dec 2009 08:45:26 Eli Zaretskii wrote:
> > From: Sérgio_Durigan_Júnior <sergiodj@linux.vnet.ibm.com>
> > Date: Wed, 16 Dec 2009 18:47:20 -0200
> > Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>, Luis Machado
> > <luisgpm@linux.vnet.ibm.com>, Matt Tyrlik <tyrlik@us.ibm.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?
I hope Luis was able to explain a bit about that in his e-mail.
> 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).
We are of course a bit biased because we were thinking about the BookE
features as we were working on this code, but we tried to implement this
support in as general a way as we could think of.
> > (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?
I agree, and refactored the ppc-linux-nat.c part of the code. We don't have a
dozen of similar functions anymore, and I reduced the number of booke_*
functions to four. This change is already present in the 4 patches series I
sent last week.
> > -#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)
> > +#endif
> > +
> > +#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) +#endif
> > +
> > +/* 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)
> > +#endif
> > +
> > +#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)
> > +#endif
>
> 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)
> target_region_ok_for_hw_watchpoint above?
Ok, changed to lowercase.
--
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center