Re: pre-compiled modules

[David Smith <dsmith at redhat dot com>]

See stuff below.

On Mon, 2006-06-19 at 12:40 -0400, Frank Ch. Eigler wrote:
> Hi -
> 
> dsmith wrote:
> 
> > I've been looking at pre-compiled modules, and in general they should
> > work.  I do have a few questions:
> > 

... stuff deleted ...


> Blatant modversions mismatches will be detected during the actual
> insertion attempt - IMO there is no need to make a dry run first.  We
> will need more self-protective code though for purposes of verifying
> module addresses.  Specifically, we need to find a kernel API routine
> that, given a module name, gives its loaded base address.

Hmm, I hadn't considered that.  Will this code go in the module itself
or go in the runtime?


To help move this along a bit, I've attached a patch that modifies the
systemtap front end to take 2 new options:

   -S         Save the compiled module in the current directory
   -P PRE_COMPILED_MODULE
              Run pre-compiled module

Note that the patch was generated with diff ignoring whitespace changes,
since a large part of main.cxx was re-indented so that passes 1-4 will
get skipped when '-P' is specified.

Here's how it works.  The '-S' option isn't strictly necessary since you
can accomplish the same thing with '-k' then a manual copy, but it is
nice.  To save a module, you do the following:

  # stap -S foo.stp 
  ... output from running stap_27691.ko ...
  Module saved as stap_27691.ko

It makes more sense to name the module something reasonable (and to not
run the module, just compile it).  So, you would do the following:

  # stap -m foo -p4 -S foo.stp
  Module saved as foo.ko

Now that you've got a module, you can run it like this:

  # stap -P foo.ko
  ... output from foo.ko ...

Comments on the option names and code itself appreciated.

-- 
David Smith
dsmith@redhat.com
Red Hat, Inc.
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Index: buildrun.cxx
===================================================================
RCS file: /cvs/systemtap/src/buildrun.cxx,v
retrieving revision 1.23
diff -u -p -u -p -w -r1.23 buildrun.cxx
--- buildrun.cxx	9 May 2006 09:33:19 -0000	1.23
+++ buildrun.cxx	27 Jun 2006 18:30:59 -0000
@@ -128,6 +128,9 @@ run_pass (systemtap_session& s)
   if (s.buffer_size)
     stpd_cmd += "-b " + stringify(s.buffer_size) + " ";
   
+  if (s.pre_compiled_mode)
+    stpd_cmd += s.module_name;
+  else  
   stpd_cmd += s.tmpdir + "/" + s.module_name + ".ko";
   
   if (s.verbose>1) clog << "Running " << stpd_cmd << endl;
Index: main.cxx
===================================================================
RCS file: /cvs/systemtap/src/main.cxx,v
retrieving revision 1.48
diff -u -p -u -p -w -r1.48 main.cxx
--- main.cxx	2 Jun 2006 15:54:26 -0000	1.48
+++ main.cxx	27 Jun 2006 18:30:59 -0000
@@ -57,6 +57,8 @@ usage (systemtap_session& s, int exitcod
     << endl
     << "   or: stap [options] -e SCRIPT    Run given script."
     << endl
+    << "   or: stap [options] -P PRE_COMPILED_MODULE  Run pre-compiled module."
+    << endl
     << endl
     << "Options:" << endl
     << "   --         no more options after this" << endl
@@ -93,6 +95,9 @@ usage (systemtap_session& s, int exitcod
     << endl
     << "   -x PID     sets target() to PID" << endl
     << "   -t         benchmarking timing information generated" << endl
+    << "   -S         Save the compiled module in the current directory" << endl
+    << "   -P PRE_COMPILED_MODULE" << endl
+    << "              Run pre-compiled module" << endl
     ;
   // -d: dump safety-related external references
 
@@ -138,6 +143,8 @@ main (int argc, char * const argv [])
   s.cmd = "";
   s.target_pid = 0;
   s.merge=true;
+  s.pre_compiled_mode = false;
+  s.save_module = false;
 
   const char* s_p = getenv ("SYSTEMTAP_TAPSET");
   if (s_p != NULL)
@@ -153,7 +160,7 @@ main (int argc, char * const argv [])
 
   while (true)
     {
-      int grc = getopt (argc, argv, "hVMvtp:I:e:o:R:r:m:kgc:x:D:bs:u");
+      int grc = getopt (argc, argv, "hVMvtp:I:e:o:R:r:m:kgc:x:D:bs:uP:S");
       if (grc < 0)
         break;
       switch (grc)
@@ -251,6 +258,15 @@ main (int argc, char * const argv [])
 	  s.macros.push_back (string (optarg));
 	  break;
 
+	case 'P':
+	  s.pre_compiled_mode = true;
+	  s.module_name = string(optarg);
+	  break;
+
+	case 'S':
+	  s.save_module = true;
+	  break;
+
         case 'h':
           usage (s, 0);
           break;
@@ -285,13 +301,80 @@ main (int argc, char * const argv [])
     }
 
   // need a user file
-  if (! have_script)
+  if (! have_script && ! s.pre_compiled_mode)
+    {
+      cerr << "A script or pre-compiled module must be specified." << endl;
+      usage(s, 1);
+    }
+
+  if (s.pre_compiled_mode)
+    {
+      // There isn't any point in specifying '-D' -with '-P' since the
+      // complation has already occurred.
+      if (s.macros.size() != 0)
+        {
+	  cerr << "You can't specify the -D and -P options together." << endl;
+	  usage(s, 1);
+	}
+      // There isn't any point in specifying '-I' -with '-P' since the
+      // complation has already occurred.
+      if (s.include_path.size() != 1)
+        {
+	  cerr << "You can't specify the -I and -P options together." << endl;
+	  usage(s, 1);
+	}
+      // There isn't any point in specifying '-g' -with '-P' since the
+      // complation has already occurred.
+      if (s.guru_mode)
+        {
+	  cerr << "You can't specify the -g and -P options together." << endl;
+	  usage(s, 1);
+	}
+      // There isn't any point in specifying '-u' -with '-P' since the
+      // complation has already occurred.
+      if (s.unoptimized)
+        {
+	  cerr << "You can't specify the -u and -P options together." << endl;
+	  usage(s, 1);
+	}
+      // There isn't any point in specifying '-k' -with '-P' since there
+      // is no temporary directory.
+      if (s.keep_tmpdir)
+        {
+	  cerr << "You can't specify the -k and -P options together." << endl;
+	  usage(s, 1);
+	}
+      // There isn't any point in specifying '-p' -with '-P' since
+      // we're automatically only doing step 5.
+      if (s.last_pass != 5)
     {
-      cerr << "A script must be specified." << endl;
+	  cerr << "You can't specify the -p and -P options together." << endl;
       usage(s, 1);
     }
+      // There isn't any point in specifying '-S' -with '-P' since there
+      // is no module to save.
+      if (s.save_module)
+        {
+	  cerr << "You can't specify the -S and -P options together." << endl;
+	  usage(s, 1);
+	}
+    }
+
+  if (s.save_module)
+    {
+      // With '-S', you must run at least pass 4 (the compilation
+      // phase) since there wouldn't be anything to save if we stopped
+      // before pass 4.
+      if (s.last_pass < 4)
+        {
+	  cerr << "When using the -S option, you must specify at least"
+	       << " pass 4 with the -p option." << endl;
+	  usage(s, 1);
+	}
+    }
 
   int rc = 0;
+  int pass4_rc = 0;
 
   // override PATH and LC_ALL
   char* path = "PATH=/bin:/sbin:/usr/bin:/usr/sbin";
@@ -309,6 +392,7 @@ main (int argc, char * const argv [])
 
   // Create a temporary directory to build within.
   // Be careful with this, as "s.tmpdir" is "rm -rf"'d at the end.
+  if (! s.pre_compiled_mode)
   {
     char* tmpdir_env = getenv("TMPDIR");
     if (! tmpdir_env)
@@ -320,7 +404,8 @@ main (int argc, char * const argv [])
     if (! tmpdir)
       {
         const char* e = strerror (errno);
-        cerr << "ERROR: cannot create temporary directory (\"" << tmpdirt << "\"): " << e << endl;
+	  cerr << "ERROR: cannot create temporary directory (\""
+	       << tmpdirt << "\"): " << e << endl;
         exit (1); // die
       }
     else
@@ -335,6 +420,11 @@ main (int argc, char * const argv [])
   struct timeval tv_before;
   gettimeofday (&tv_before, NULL);
 
+  unsigned _sc_clk_tck = sysconf (_SC_CLK_TCK);
+  struct tms tms_after;
+  struct timeval tv_after;
+  if (! s.pre_compiled_mode)
+    {
   // PASS 1a: PARSING USER SCRIPT
   // XXX: pass args vector, so parser (or lexer?) can substitute
   // $1..$NN with actual arguments
@@ -360,7 +450,8 @@ main (int argc, char * const argv [])
   version_suffixes.push_back ("/" + kvr);
   // add kernel version (2.6.NN) + arch
   string::size_type dash_index = kvr.find ('-');
-  if (dash_index > 0 && dash_index != string::npos) {
+      if (dash_index > 0 && dash_index != string::npos)
+        {
     kvr.erase(dash_index);
     version_suffixes.push_back ("/" + kvr + "/" + arch);
     version_suffixes.push_back ("/" + kvr);
@@ -368,7 +459,8 @@ main (int argc, char * const argv [])
   // add kernel family (2.6) + arch
   string::size_type dot1_index = kvr.find ('.');
   string::size_type dot2_index = kvr.rfind ('.');
-  while (dot2_index > dot1_index && dot2_index != string::npos) {
+      while (dot2_index > dot1_index && dot2_index != string::npos)
+        {
     kvr.erase(dot2_index);
     version_suffixes.push_back ("/" + kvr + "/" + arch);
     version_suffixes.push_back ("/" + kvr);
@@ -423,10 +515,7 @@ main (int argc, char * const argv [])
           }
     }
 
-  struct tms tms_after;
   times (& tms_after);
-  unsigned _sc_clk_tck = sysconf (_SC_CLK_TCK);
-  struct timeval tv_after;
   gettimeofday (&tv_after, NULL);
 
 #define TIMESPRINT \
@@ -521,7 +610,8 @@ main (int argc, char * const argv [])
   times (& tms_after);
   gettimeofday (&tv_after, NULL);
 
-  if (s.verbose) clog << "Pass 2: analyzed script: "
+      if (s.verbose)
+	clog << "Pass 2: analyzed script: "
                       << s.probes.size() << " probe(s), "
                       << s.functions.size() << " function(s), "
                       << s.globals.size() << " global(s) in "
@@ -552,7 +642,8 @@ main (int argc, char * const argv [])
   times (& tms_after);
   gettimeofday (&tv_after, NULL);
 
-  if (s.verbose) clog << "Pass 3: translated to C into \""
+      if (s.verbose)
+	clog << "Pass 3: translated to C into \""
                       << s.translated_source
                       << "\" in "
                       << TIMESPRINT
@@ -572,7 +663,8 @@ main (int argc, char * const argv [])
   times (& tms_after);
   gettimeofday (&tv_after, NULL);
 
-  if (s.verbose) clog << "Pass 4: compiled C into \""
+      if (s.verbose)
+	clog << "Pass 4: compiled C into \""
                       << s.module_name << ".ko"
                       << "\" in "
                       << TIMESPRINT
@@ -584,7 +676,9 @@ main (int argc, char * const argv [])
          << endl;
 
   // XXX: what to do if rc==0 && last_pass == 4?  dump .ko file to stdout?
+      pass4_rc = rc;
   if (rc || s.last_pass == 4) goto cleanup;
+    }
 
   // PASS 5: RUN
   times (& tms_before);
@@ -592,11 +686,13 @@ main (int argc, char * const argv [])
   // NB: this message is a judgement call.  The other passes don't emit
   // a "hello, I'm starting" message, but then the others aren't interactive
   // and don't take an indefinite amount of time.
-  if (s.verbose) clog << "Pass 5: starting run." << endl;
+  if (s.verbose)
+    clog << "Pass 5: starting run." << endl;
   rc = run_pass (s);
   times (& tms_after);
   gettimeofday (&tv_after, NULL);
-  if (s.verbose) clog << "Pass 5: run completed in "
+  if (s.verbose)
+    clog << "Pass 5: run completed in "
                       << TIMESPRINT
                       << endl;
 
@@ -608,6 +704,20 @@ main (int argc, char * const argv [])
   // if (rc) goto cleanup;
 
  cleanup:
+  // If the user requested that we save the module and pass 4 worked,
+  // save module.
+  if (s.save_module && !pass4_rc && s.last_pass >= 4)
+    {
+      string copycmd = "cp ";
+      copycmd += s.tmpdir + "/" + s.module_name + ".ko .";
+      if (s.verbose>1) clog << "Running " << copycmd << endl;
+      int status = system (copycmd.c_str());
+      if (status != 0)
+	clog << "Module save failed, status: " << status << endl;
+      else
+	clog << "Module saved as " << s.module_name << ".ko" << endl;
+    }
+
   // Clean up temporary directory.  Obviously, be careful with this.
   if (s.tmpdir == "")
     ; // do nothing
Index: session.h
===================================================================
RCS file: /cvs/systemtap/src/session.h,v
retrieving revision 1.7
diff -u -p -u -p -w -r1.7 session.h
--- session.h	9 May 2006 09:33:19 -0000	1.7
+++ session.h	27 Jun 2006 18:30:59 -0000
@@ -77,6 +77,8 @@ struct systemtap_session
   bool unoptimized;
   bool merge;
   int buffer_size;
+  bool pre_compiled_mode;
+  bool save_module;
 
   // temporary directory for module builds etc.
   // hazardous - it is "rm -rf"'d at exit