Re: SystemTap for Android - patchset

[Alexander Lochmann <alexander dot lochmann at tu-dortmund dot de>]

So. Let me start.
First of all, I extracted the patches properly, and attached the files.
(Btw, I found a third bug. :) )
FYI, I just fixed the bugs for the kernel versions I'm dealing with,
because I don't know which other versions are affected as well.

On 01.07.2016 19:46, Josh Stone wrote:
> 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.
> 
Honestly? I don't know. :D I had some trouble with SystemTap looking for
tapsets systemwide, which made me write that patch/fix. It's been a long
time since I wrote it. Anyway, I tried your proposed solution, and it
works. I'll adapt my build script to use the enviroment variable.
You can skip that part of the patch if you want.
>> - 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.
Did I? It might be possible that I want be able to pass the argument to
staprun.

I chose the letters randomly. For me, it doesn't matter. I just need an
argument to tell staprun to create a pid file. :)
> 
> 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.
Yeah, sure. Since I run stap on Android, I have a background service,
which periodically checks if every single stap instance is still
running. Yes, it might be possible having more than one stap script
running. :)
Moreover, if I start it more than once, I want to be able to kill
specific instance. For that I need its process id.

- Alex
> 
>> 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
>>
> 


-- 
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

From f0ca40f9aaa5ba9c39ffcefc39e450443603db44 Mon Sep 17 00:00:00 2001
From: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
Date: Wed, 6 Jul 2016 13:55:07 +0200
Subject: [PATCH 1/3] Definition of cputime_to_usecs in Linux kernel 3.0 is
 broken

The above macro is definined with a semicolon at the end. Using it as
a function argument leads to a compiler error. Hence, the necessary braces
have been added.
---
 tapset/linux/task_time.stp | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tapset/linux/task_time.stp b/tapset/linux/task_time.stp
index f86f984..f3c276c 100644
--- a/tapset/linux/task_time.stp
+++ b/tapset/linux/task_time.stp
@@ -27,8 +27,12 @@
  * Yet note some kernels (RHEL6) may already have both...  */
 #if defined(cputime_to_usecs)
 #if !defined(cputime_to_msecs)
+#if LINUX_VERSION_CODE <= KERNEL_VERSION(3,0,200) && LINUX_VERSION_CODE >= KERNEL_VERSION(3,0,0)
+#define cputime_to_msecs(__ct)		_stp_div64(NULL, ({cputime_to_usecs(__ct)}), 1000ULL)
+#else
 #define cputime_to_msecs(__ct)		_stp_div64(NULL, cputime_to_usecs(__ct), 1000ULL)
 #endif
+#endif
 
 /* Kernels before 2.6.37 have cputime_to_msecs, but not usecs. */
 #elif defined(cputime_to_msecs)
-- 
2.7.4

From 8f3817586f87cb65bc3b8b320ad8252a6a11f7e5 Mon Sep 17 00:00:00 2001
From: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
Date: Wed, 6 Jul 2016 13:59:23 +0200
Subject: [PATCH 2/3] The syscall defines were not compatible with older
 kernels, namely 3.0.x

Older kernels, at least the 3.0.x, define their syscalls in different files.
The stap runtime has to include those file properly. Moreover, for pre-uprobe kernels
no syscall file has been included at all. This job is normally done by task_finder.c.
The task_finder_stubs.c has been extended by the needed include directive.
---
 runtime/linux/autoconf-asm-syscall.c | 8 +++++++-
 runtime/linux/task_finder_stubs.c    | 1 +
 runtime/syscall.h                    | 8 +++++++-
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/runtime/linux/autoconf-asm-syscall.c b/runtime/linux/autoconf-asm-syscall.c
index bf7a273..6bfcd55 100644
--- a/runtime/linux/autoconf-asm-syscall.c
+++ b/runtime/linux/autoconf-asm-syscall.c
@@ -1,2 +1,8 @@
+#include <linux/version.h>
+#if LINUX_VERSION_CODE <= KERNEL_VERSION(3,0,200) && LINUX_VERSION_CODE >= KERNEL_VERSION(3,0,0)
+#include <asm/unistd.h>
+#include <linux/sched.h>
+#include <asm-generic/syscall.h>
+#else
 #include <asm/syscall.h>
-
+#endif
diff --git a/runtime/linux/task_finder_stubs.c b/runtime/linux/task_finder_stubs.c
index 700bb3d..39f9ec2 100644
--- a/runtime/linux/task_finder_stubs.c
+++ b/runtime/linux/task_finder_stubs.c
@@ -1,6 +1,7 @@
 #ifndef TASK_FINDER_STUBS_C
 #define TASK_FINDER_STUBS_C
 
+#include "syscall.h"
 /* Stubs of last resort for when utrace type functionality is not
    available. Nothing should actually work, but things compile
    properly, and silently return dummy data or noisily fail as
diff --git a/runtime/syscall.h b/runtime/syscall.h
index b959d46..b652946 100644
--- a/runtime/syscall.h
+++ b/runtime/syscall.h
@@ -7,7 +7,6 @@
  * Public License (GPL); either version 2, or (at your option) any
  * later version.
  */
-
 #ifndef _SYSCALL_H_ /* -*- linux-c -*- */
 #define _SYSCALL_H_
 
@@ -110,7 +109,14 @@
 #ifdef STAPCONF_ASM_SYSCALL_H
 
 /* If the system has asm/syscall.h, use defines from it. */
+#include <linux/version.h>
+#if LINUX_VERSION_CODE <= KERNEL_VERSION(3,0,200) && LINUX_VERSION_CODE >= KERNEL_VERSION(3,0,0)
+#include <asm/unistd.h>
+#include <linux/sched.h>
+#include <asm-generic/syscall.h>
+#else
 #include <asm/syscall.h>
+#endif
 
 #if defined(__arm__)
 /* The syscall_get_nr() function on 3.17.1-302.fc21.armv7hl always
-- 
2.7.4

From 4c6cb3c2d2ce969a16fe91b2a06ed78d68b33644 Mon Sep 17 00:00:00 2001
From: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
Date: Tue, 5 Jul 2016 19:05:06 +0200
Subject: [PATCH 3/3] Linux kernel 3.4 does not define PR_SET_MM_ARG_START.
 Used KERNEL_VERSION to permit compilatioon of those macros

---
 tapset/linux/aux_syscalls.stp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tapset/linux/aux_syscalls.stp b/tapset/linux/aux_syscalls.stp
index bec71c5..88db0b7 100644
--- a/tapset/linux/aux_syscalls.stp
+++ b/tapset/linux/aux_syscalls.stp
@@ -4599,12 +4599,14 @@ static const _stp_val_array const _stp_prctl_mm_option_list[] = {
 	V(PR_SET_MM_START_STACK),
 	V(PR_SET_MM_START_BRK),
 	V(PR_SET_MM_BRK),
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,4,200) && LINUX_VERSION_CODE <= KERNEL_VERSION(3,4,0)
 	V(PR_SET_MM_ARG_START),
 	V(PR_SET_MM_ARG_END),
 	V(PR_SET_MM_ENV_START),
 	V(PR_SET_MM_ENV_END),
 	V(PR_SET_MM_AUXV),
 	V(PR_SET_MM_EXE_FILE),
+#endif
 #ifdef PR_SET_MM_MAP
 	V(PR_SET_MM_MAP),
 #endif
-- 
2.7.4

Attachment: signature.asc
Description: OpenPGP digital signature