Re: [PATCH 2/2] Support the new PPC476 processor -- Arch Dependent

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

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