Re: SystemTap for Android - patchset

[Josh Stone <jistone@redhat.com>]

On 07/01/2016 09:15 AM, Alexander Lochmann wrote:
> 
> Hi folks!
> 
> Finally, I decided to submit my patch, which makes SystemTap work for
> Android. Moreover, it adds two new features:
> - Support for ignoring all available tapset directories, except the one
> that is provided by -K

Can you explain why you need this?

One can already override the primary tapset directory with the
environment SYSTEMTAP_TAPSET.  Ignoring -I options also seems
questionable, especially since this depends on the order -- it looks
like -I specified after -K will still be used, while those before will
be ignored.

> - Support for a pid file in staprun, parameter is -U

And you added -M on stap itself.  I don't understand either of those
letter choices.

The functionality itself is not too controversial, but I'd still like to
see your justification in the commit log, and some examples how this is
used in Android.

> I had to modify several source files of staprun. Those changes are
> mostly copied from the corresponding files contained in commit
> 2c10863bfe41b51272eff714a837f4977bdc257a. For some reasons, those ifdef
> parts have been removed. I readded them, and changed the macro, which
> activates them.

That commit is "man stap: fixed typos".  Did you mean something else?

> The patch contains two bugfixes for the SystemTap as well.
> Unfortunately, I failed to extract those fixes properly. :(

As David said, it would help us a lot if you separated each of these
changes into distinct commits.  If you have the changes in a working git
directory, "git add -p" can help you mark specific changes to be
committed.  "git-gui" is also pretty good for selecting hunks.  Feel
free to ask for help on #systemtap if you still have trouble.

> The first fix starts at line 510, and goes until line 555.
> Since an older kernel like 3.0 does not support uprobes, systemtap
> includes 'runtime/linux/task_finder_stubs.c'. That file itself does
> *not* include 'syscall.h', which declares several syscall-related functions.
> The second fix starts at line 1106. For some reasons in the Linux kernel
> 3.0 the macro cputime_to_usecs() has a semicolon at the end of its
> definition. Therefore, the defition of cputime_to_msecs() in '
> tapset/linux/task_time.stp' must be modified to deal with that fact.
> 
> Cheers,
> Alex
> 
> ---
> Technische Universität Dortmund
> Alexander Lochmann                PGP key: 0xBC3EF6FD
> Otto-Hahn-Str. 16                 phone:  +49.231.7556141
> D-44227 Dortmund                  fax:    +49.231.7556116
> http://ess.cs.tu-dortmund.de/Staff/al
>