Re: [RFC] Hardware Breakpoint support for systemtap translator

[fche at redhat dot com (Frank Ch. Eigler)]

Prerna Saxena <prerna@linux.vnet.ibm.com> writes:

> [...]
> The attached set patches enable systemtap translator to probe kernel
> hardware breakpoints on x86.

Great.

> It introduces the following language constructs :
>
> 1. probe kernel.data(ADDRESS).write     {....}   : To probe writes at
> the given address
> 2. probe kernel.data(ADDRESS).rw         {....}   : To probe read &
> write access to the given address.
> 3. probe kernel.data("SYMBOL").write   {....}
> 4. probe kernel.data("SYMBOL").rw       {....}   : Similar to 1,2, but
> using a symbol name as argument.

These symbols are in the unified kernel+module namespace, right?


> [...]
> Index: git-3-july/tapsets.cxx
> +struct hwbkpt_derived_probe_group: public derived_probe_group
> +{
> +private:
> +
> +  multimap<string, hwbkpt_derived_probe*> probes_by_module;
> +  typedef map<string, hwbkpt_derived_probe*>::iterator p_b_m_iterator;

Do you need this multimap stuff?  In dwarfy ones we use them to
separate derived_probes by module, so that they can be treated as a
group at run time (as modules come and go).  But in your case, you
just iterate through the whole suite, so why not a plain vector<> or
set<> ?

> [...]
> +void hwbkpt_derived_probe::printsig (ostream& o) const
> +{
> +  sole_location()->print (o);
> +  o << " /* " << " Address = " << hwbkpt_addr << "*/";
> +  o << " /* " << " Symbol  = " << symbol_name << "*/";
> +  switch (hwbkpt_access)
> +	{
> +	  case HWBKPT_READ:
> +  		o << " /* " << " Access = READ " << "*/";
> +	  case HWBKPT_WRITE:
> +  		o << " /* " << " Access = WRITE " << "*/";
> +	  case HWBKPT_RW:
> +  		o << " /* " << " Access = RW " << "*/";
> +	}
> +  printsig_nested (o);
> +}

Is this extra /* .. */ verbosity necessary?  I'd think the
sole_location() printing will spell out the probe point in all the
same detail.  Dwarfy ones have extra data there to provide addresses
etc. that systemtap computed.  That does not seem to apply here.


> +void hwbkpt_derived_probe_group::enroll (hwbkpt_derived_probe* p)
> +{
> +  string hwbkpt_key;
> +  if (p->hwbkpt_addr)
> +	{
> +	  char hwbkpt_addr_str[64];
> +	  sprintf(hwbkpt_addr_str, "%u",int(p->hwbkpt_addr));
> +	  hwbkpt_key=hwbkpt_addr_str;
> +	}
> +  else
> +	  hwbkpt_key=p->symbol_name;
> +  switch (p->hwbkpt_access)
> +	{
> +	  case HWBKPT_READ:
> +		hwbkpt_key = string("R_") + hwbkpt_key;
> +	  case HWBKPT_WRITE:
> +		hwbkpt_key = string("W_") + hwbkpt_key;
> +	  case HWBKPT_RW:
> +		hwbkpt_key = string("RW") + hwbkpt_key;
> +	}
> +
> +  probes_by_module.insert (make_pair (hwbkpt_key, p));
> +}

What is the need for this?  (Was it solely to use the multimap<> ?)


> +void
> +hwbkpt_derived_probe_group::emit_module_decls (systemtap_session& s)
> +{
> +  if (probes_by_module.empty()) return;
> +
> +  s.op->newline() << "/* ---- hwbkpt-based probes ---- */";
> +  s.op->newline() << "#include <asm/hw_breakpoint.h>";
> +  s.op->newline();

It would be useful to make this break compilation more abruptly
if hw-breakpoint support for the current architecture is not available.
Whether a macro comes from a runtime/autoconf* or from a kernel header,
consider adding a ...

#ifndef HAVE_HW_BREAKPOINTS
#error "need hw breakpoints"
#endif

... at the top.


> [...]
> +void
> +hwbkpt_derived_probe_group::emit_module_init (systemtap_session& s)
> +{
> [...]
> +  s.op->newline() << "#ifdef CONFIG_X86";
> +  s.op->newline() << "	hp->info.name = (char *) hwbkpt_symbol_name;";
> +  s.op->newline() << "	hp->info.address = (unsigned long) addr;";

... and then you wouldn't need CONFIG_X86 here.

> +  s.op->newline() << "    case HWBKPT_WRITE:";
> +  s.op->newline() << "		  hp->info.type = HW_BREAKPOINT_WRITE;";

Is there some reason stap doesn't just emit the hw_breakpoint.h enum
names directly, a la HW_BREAKPOINT_WRITE, instead of the handmade
HWBKPT_WRITE / literal-numbers?


> +void
> +hwbkpt_derived_probe_group::emit_module_exit (systemtap_session& s)
> +{
> +  //Unregister hwbkpt probes by batch interfaces.
> +  s.op->newline() << "for (i=0; i<" << probes_by_module.size() << "; i++) {";
> +  s.op->newline(1) << "struct stap_hwbkpt_probe *sdp = & stap_hwbkpt_probes[i];";
> +  s.op->newline() << "struct hw_breakpoint *hp = & stap_hwbkpt_probe_array[i];";
> +  s.op->newline() << "if (! sdp->registered_p) continue;";
> +  s.op->newline() << " unregister_kernel_hw_breakpoint(hp);";
> +  s.op->newline() << "sdp->registered_p = 0;";
> +  s.op->newline(-1) << "}";
> +}

Actually this is not a batch unregistration (one call unregistering
many hw-breakpoints) at all; the comment should be fixed.


> Index: git-3-july/elaborate.cxx
> ===================================================================
> --- git-3-july.orig/elaborate.cxx
> +++ git-3-july/elaborate.cxx
> @@ -1457,6 +1457,7 @@ systemtap_session::systemtap_session ():
>    be_derived_probes(0),
>    dwarf_derived_probes(0),
>    kprobe_derived_probes(0),
> +  hwbkpt_derived_probes(0),
>    uprobe_derived_probes(0),
>    utrace_derived_probes(0),
>    itrace_derived_probes(0),

Thank you for remembering to clear that new pointer!


- FChE