[Bug tapsets/10466] New: stap -L misrepresents variables available to multi-probe aliases

[Bug tapsets/10466] New: stap -L misrepresents variables available to multi-probe aliases

[jistone at redhat dot com]

The listing mode attempts to clean up the output by only printing probe points
once, even if that name is an alias for several probe points underneath. 
However, -L only considers the variables from the first underlying probe point,
which is an issue if the multiple probe points don't have the same available
variables.

Usually the tapset writers try to define the same local variables for each
branch, but the recent addition of dwarf variables to -L exposes the difference
in the raw probe points.

$ stap -L signal.send
signal.send name:string shared:long sig:long task:long sinfo:long
send2queue:long sig_name:string sig_pid:long pid_name:string si_code:string
$sig:int $info:struct siginfo* $t:struct task_struct* $group:int $pending:struct
sigpending* $q:struct sigqueue*

Some of the $vars are only present in one part of signal.send, so they're not
actually usable in a signal.send probe body.

Rather than showing the variables of just one branch, -L should show the
set-intersection of each branch, so that every variable listed is actually useful.

-- 
           Summary: stap -L misrepresents variables available to multi-probe
                    aliases
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: tapsets
        AssignedTo: systemtap at sources dot redhat dot com
        ReportedBy: jistone at redhat dot com


http://sourceware.org/bugzilla/show_bug.cgi?id=10466

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug tapsets/10466] stap -L misrepresents variables available to multi-probe aliases

[wenji dot huang at oracle dot com]

------- Additional Comments From wenji dot huang at oracle dot com  2009-10-29 08:53 -------
Created an attachment (id=4336)
 --> (http://sourceware.org/bugzilla/attachment.cgi?id=4336&action=view)
patch

It's difficult to discriminate between multi probes like signal.* and one name
with several expanded probe alias like signal.send. Moreover the variable list
is in string stream, to make intersection is a little arduous.

Maybe the following way can be a good choice.

$ ./stap -L signal.send
signal.send /* kernel.function("send_signal@kernel/signal.c:816") */
name:string shared:long sig:long task:long sinfo:long send2queue:long
sig_name:string sig_pid:long pid_name:string si_code:string $sig:int
$info:struct siginfo* $t:struct task_struct* $group:int $pending:struct
sigpending*

signal.send /* kernel.function("send_sigqueue@kernel/signal.c:1309") */ 
name:string task:long sig:long sinfo:long shared:long send2queue:long
sig_name:string sig_pid:long pid_name:string si_code:string $q:struct sigqueue*
$t:struct task_struct* $group:int $sig:int $flags:long unsigned int

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10466

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug tapsets/10466] stap -L misrepresents variables available to multi-probe aliases

[jistone at redhat dot com]

------- Additional Comments From jistone at redhat dot com  2009-10-29 23:29 -------
(In reply to comment #1)
> Created an attachment (id=4336)
 --> (http://sourceware.org/bugzilla/attachment.cgi?id=4336&action=view)
> patch
> 
> It's difficult to discriminate between multi probes like signal.* and one name
> with several expanded probe alias like signal.send. Moreover the variable list
> is in string stream, to make intersection is a little arduous.

It doesn't have to stay as a string stream though.  The probes could instead
keep a set<string> of "name:type", to be reconciled at the higher level.

> Maybe [listing expansions separately] can be a good choice.

I don't think it's right to make the user figure out the common variables...

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10466

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug tapsets/10466] stap -L misrepresents variables available to multi-probe aliases

[wenji dot huang at oracle dot com]

------- Additional Comments From wenji dot huang at oracle dot com  2009-11-03 05:37 -------
Created an attachment (id=4353)
 --> (http://sourceware.org/bugzilla/attachment.cgi?id=4353&action=view)
Updated patch

As jistone suggested, keep the variables/arguments as set<string>
of "name:type" and make set-intersection before printing.

$ ./stap -L signal.send
signal.send name:string pid_name:string send2queue:long shared:long
si_code:string sig:long sig_name:string sig_pid:long sinfo:long task:long
$group:int $sig:int $t:struct task_struct*

Now, only those items existing in all branch will be printed.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #4336 is|0                           |1
           obsolete|                            |


http://sourceware.org/bugzilla/show_bug.cgi?id=10466

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug tapsets/10466] stap -L misrepresents variables available to multi-probe aliases

[jistone at redhat dot com]

------- Additional Comments From jistone at redhat dot com  2009-11-03 19:42 -------
(In reply to comment #3)
> Created an attachment (id=4353)
 --> (http://sourceware.org/bugzilla/attachment.cgi?id=4353&action=view)
> Updated patch

Great, that's exactly the output I was hoping for!  I have some comments for
cleaning up the code, and then please go ahead and commit this.

> map<string,set<unsigned>&> probe_list;

The reference value here seems dubious, and I also wonder why not just keep the
probe pointer directly instead of indexes?

  map<string, set<derived_probe*> > probe_list;

You can also take advantage that the default set constructor is an empty set, so
this:

> if (probe_list.find (pp) == probe_list.end())
>   {
>     set<unsigned> *probe_id = new set<unsigned>();
>     (*probe_id).insert(i);
>     probe_list.insert(pair<string,set<unsigned>&>(pp,*probe_id));
>   }
> else
>   probe_list.find(pp)->second.insert(i);

becomes:

  probe_list[pp].insert(p);

This same idiom will work for updating the counts in var_list and arg_list.

  var_list[tmps.str()]++;
...
  arg_list[*ia]++;

> set<string> *arg_set = new set<string>();
> p->getargs(*arg_set);

There's no reason for this to be heap allocated, which you're then leaking. 
Just create it directly:

  set<string> arg_set;
  p->getargs(arg_set);

>         var_list.clear();
>         arg_list.clear();
>       }
>     o << endl;
>   }
> probe_list.clear();

These clear() calls are redundant, as the maps are already going out of scope
there and will be deconstructed.

> dwarf_derived_probe::getargs(std::set<std::string> &arg_set) const
> {
>   for (set<string>::iterator it = args.begin(); it != args.end(); it++)
>     arg_set.insert(*it);
> }

Note that set::insert can take iterators:

  arg_set.insert(args.begin(), args.end());


Thanks!


-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10466

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug tapsets/10466] stap -L misrepresents variables available to multi-probe aliases

[wenji dot huang at oracle dot com]

------- Additional Comments From wenji dot huang at oracle dot com  2009-11-04 03:09 -------
Thanks for the comments.

commit	c39cdd5565f718302057242bbfe50e71b69c4f4d

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10466

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug tapsets/10466] stap -L misrepresents variables available to multi-probe aliases

[jistone at redhat dot com]

------- Additional Comments From jistone at redhat dot com  2009-11-04 19:21 -------
Fix confirmed.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


http://sourceware.org/bugzilla/show_bug.cgi?id=10466

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.