PR5892: rename new -B /PATH to -r /PATH; merge functionality

11 files changed, 146 insertions, 131 deletions

diff --git a/ChangeLog b/ChangeLog
index bde86e62..00cc652f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2008-12-03  Frank Ch. Eigler  <fche@elastic.org>
+
+	PR 5982: rename -B to -r
+	* main.cxx (main): Treat -r REV and -r /PATH as complimentary.
+	Imply -p4 if target mismatches host versions.
+	(usage): Document -r more.
+	* hash.cxx (find_hash): Add in the kernel_build_tree.
+	* buildrun.cxx (compile_pass, kernel_built_uprobes): Simplify
+	logic.
+	* tapsets.cxx (setup_kernel, setup_user): Ditto.
+	* translate.cxx (emit_symbol_data): Ditto.
+	* stap.1.in: Document -r /PATH.
+
 2008-12-01  Rajan Arora  <rarora@redhat.com>
 
 	PR 5892: Add -B and parameterize all occurences of /lib/modules/...
diff --git a/NEWS b/NEWS
index 374bcec3..4c9c68fa 100644
--- a/NEWS
+++ b/NEWS
@@ -1,7 +1,8 @@
 * What's new
 
-- A new stap -B option is now available which enables user to sic systemtap
-  at a kernel build tree in any oddball directory.
+- The stap "-r DIR" option may be used to identify a hand-made kernel
+  build directory.  The tool determines the appropriate release string
+  automatically from the directory.
 
 - A serious problem associated with user-space probing in shared libraries
   was corrected, making it now possible to experiment with probe shared
diff --git a/README b/README
index 59173166..2bb0ae79 100644
--- a/README
+++ b/README
@@ -90,25 +90,18 @@ Tips:
   /lib/modules/`uname -r`/vmlinux
   /lib/modules/`uname -r`/build/vmlinux
 
+
 Building a kernel.org kernel:
 
 - Build the kernel using your normal procedures.  Enable
-  CONFIG_DEBUG_INFO, CONFIG_KPROBES, and optionally CONFIG_RELAY and
-  CONFIG_DEBUG_FS.
-- Run "make install modules_install headers_install" as needed.
+  CONFIG_DEBUG_INFO, CONFIG_KPROBES, CONFIG_RELAY and CONFIG_DEBUG_FS.
+- % make modules_install install headers_install
 - Boot into the kernel.
-- Make sure the large unstripped kernel image 'vmlinux' from your
-  build can be found by systemtap (see above)  You can just symlink
-  it to one of these locations.
-- Symlink your build directory to here:
-  /lib/modules/`uname -r`/build
-- For example, if your kernel source is in /home/me/linux/2.6.17-rc6
-  and you build your kernel in the source directory, you would do
-  this (be sure you are running the right kernel or `uname -r`
-  will be wrong:)
-     ln -s /home/me/linux/2.6.17-rc6/vmlinux /boot/vmlinux-`uname -r`
-     mkdir /usr/src/kernels  (if needed)
-     ln -s /home/me/linux/2.6.17-rc6 /usr/src/kernels/`uname -r`
-     mkdir /lib/modules/`uname -r`  (if needed)
-     ln -s /home/me/linux/2.6.17-rc6 /lib/modules/`uname -r`/build
-     ln -s /home/me/linux/2.6.17-rc6 /lib/modules/`uname -r`/source
+
+- If you wish to leave the kernel build tree in place, simply run
+  % stap -r /path/to/kernel/build/tree [...]
+  You're done.
+
+- Or else, if you wish to install the kernel build/debuginfo data into
+  a place where systemtap will find it without the "-r" option:
+  % ln -s /path/to/kernel/build/tree /lib/modules/RELEASE/build 
diff --git a/buildrun.cxx b/buildrun.cxx
index 86da95d3..c6f465c5 100644
--- a/buildrun.cxx
+++ b/buildrun.cxx
@@ -37,7 +37,7 @@ run_make_cmd(systemtap_session& s, string& make_cmd)
 {
   // Before running make, fix up the environment a bit.  PATH should
   // already be overridden.  Clean out a few variables that
-  // /lib/modules/${KVER}/build/Makefile uses.
+  // s.kernel_build_tree/Makefile uses.
   int rc = unsetenv("ARCH") || unsetenv("KBUILD_EXTMOD")
       || unsetenv("CROSS_COMPILE") || unsetenv("KBUILD_IMAGE")
       || unsetenv("KCONFIG_CONFIG") || unsetenv("INSTALL_PATH");
@@ -151,11 +151,7 @@ compile_pass (systemtap_session& s)
 
   // Generate module directory pathname and make sure it exists.
   string module_dir;
-  if (! s.kernel_build_tree.size ())
-    module_dir = string("/lib/modules/")
-      + s.kernel_release + "/build";
-  else
-    module_dir = s.kernel_build_tree;
+  module_dir = s.kernel_build_tree;
   struct stat st;
   rc = stat(module_dir.c_str(), &st);
   if (rc != 0)
@@ -181,19 +177,13 @@ static const string uprobes_home = string(PKGDATADIR "/runtime/uprobes");
 /*
  * If uprobes was built as part of the kernel build (either built-in
  * or as a module), the uprobes exports should show up in either
- * /lib/modules/`uname -r`/build/Module.symvers or in the oddball
- * directory where the user's kernel is built.  Return true if so.
+ * s.kernel_build_tree / Module.symvers.  Return true if so.
  */
 static bool
 kernel_built_uprobes (systemtap_session& s)
 {
-  string grep_cmd;
-  if (! s.kernel_build_tree.size ())
-    grep_cmd = string ("/bin/grep -q unregister_uprobe /lib/modules/")
-      + s.kernel_release + string ("/build/Module.symvers");
-  else
-    grep_cmd = string ("/bin/grep -q unregister_uprobe ") + 
-      s.kernel_build_tree + string ("/Module.symvers");
+  string grep_cmd = string ("/bin/grep -q unregister_uprobe ") + 
+    s.kernel_build_tree + string ("/Module.symvers");
   int rc = system (grep_cmd.c_str());
   return (rc == 0);
 }
diff --git a/hash.cxx b/hash.cxx
index a10815c9..fb2f15c1 100644
--- a/hash.cxx
+++ b/hash.cxx
@@ -90,6 +90,7 @@ find_hash (systemtap_session& s, const string& script)
 
   // Hash kernel release and arch.
   h.add(s.kernel_release);
+  h.add(s.kernel_build_tree);
   h.add(s.architecture);
 
   // Hash user-specified arguments (that change the generated module).
diff --git a/main.cxx b/main.cxx
index 7907080d..649da15f 100644
--- a/main.cxx
+++ b/main.cxx
@@ -102,14 +102,14 @@ usage (systemtap_session& s, int exitcode)
   clog
     << "   -D NM=VAL  emit macro definition into generated C code" << endl
     << "   -R DIR     look in DIR for runtime, instead of" << endl
-    <<      "              " << s.runtime_path << endl
-    << "   -r RELEASE cross-compile to kernel RELEASE, instead of "
-    << s.kernel_release << endl
-    << "   -m MODULE  set probe module name, instead of "
-    << s.module_name << endl
-    << "   -o FILE    send output to file, instead of stdout" << endl
-    << "   -c CMD     start the probes, run CMD, and exit when it finishes"
-    << endl
+    << "              " << s.runtime_path << endl
+    << "   -r DIR     cross-compile to kernel with given build tree; or else" << endl
+    << "   -r RELEASE cross-compile to kernel /lib/modules/RELEASE/build, instead of" << endl
+    << "              " << s.kernel_build_tree << endl
+    << "   -m MODULE  set probe module name, instead of " << endl
+    << "              " << s.module_name << endl
+    << "   -o FILE    send script output to file, instead of stdout" << endl
+    << "   -c CMD     start the probes, run CMD, and exit when it finishes" << endl
     << "   -x PID     sets target() to PID" << endl
     << "   -F         load module and start probes, then detach" << endl
     << "   -d OBJECT  add unwind/symbol data for OBJECT file";
@@ -335,6 +335,8 @@ main (int argc, char * const argv [])
   struct utsname buf;
   (void) uname (& buf);
   s.kernel_release = string (buf.release);
+  s.kernel_build_tree = "/lib/modules/" + s.kernel_release + "/build";
+
   s.architecture = string (buf.machine);
   for (unsigned i=0; i<5; i++) s.perpass_verbose[i]=0;
   s.timing = false;
@@ -434,7 +436,7 @@ main (int argc, char * const argv [])
         { "vp", 1, &long_opt, LONG_OPT_VERBOSE_PASS },
         { NULL, 0, NULL, 0 }
       };
-      int grc = getopt_long (argc, argv, "hVMvtp:I:e:o:R:r:B:m:kgPc:x:D:bs:uqwl:d:L:F",
+      int grc = getopt_long (argc, argv, "hVMvtp:I:e:o:R:r:m:kgPc:x:D:bs:uqwl:d:L:F",
                                                           long_options, NULL);
       if (grc < 0)
         break;
@@ -549,37 +551,33 @@ main (int argc, char * const argv [])
           break;
 
         case 'r':
-          s.kernel_release = string (optarg);
+          if (optarg[0] == '/') // fully specified path
+            {
+              s.kernel_build_tree = optarg;
+              string version_file_name = s.kernel_build_tree + "/include/config/kernel.release";
+              // The file include/config/kernel.release within the
+              // build tree is used to pull out the version information
+              ifstream version_file (version_file_name.c_str());
+              if (version_file.fail ())
+                {
+                  cerr << "Missing " << version_file_name << endl;
+                  usage (s, 1);
+                }
+              else 
+                {
+                  char c;
+                  s.kernel_release = "";
+                  while (version_file.get(c) && c != '\n')
+                    s.kernel_release.push_back(c);
+                }
+            }
+          else
+            {
+              s.kernel_release = string (optarg);
+              s.kernel_build_tree = "/lib/modules/" + s.kernel_release + "/build";
+            }
           break;
 
-	case 'B':
-	  s.kernel_build_tree = string (optarg);
-	  {
-	    string version_file_name = s.kernel_build_tree
-	      + "/include/config/kernel.release";
-	    // The file include/config/kernel.release within the
-	    // build tree is used to pull out the version information
-	    // : Failing which -B is ignored
-	      
-	    ifstream version_file (version_file_name.c_str());
-	    if (version_file.fail ())
-	      {
-		cerr << "error: No file: " << version_file_name 
-		     << " found" << endl;
-		cerr << "Please check path to -B again" << endl;
-		s.kernel_build_tree = "";
-	      }
-	    else 
-	      {
-		char c;
-		string kernel_build_version;
-		while (version_file.get(c) && c != '\n')
-		  kernel_build_version.push_back(c);
-		s.kernel_release = kernel_build_version;
-	      }
-	  }
-	  break;
-
         case 'k':
           s.keep_tmpdir = true;
           s.use_cache = false; /* User wants to keep a usable build tree. */
@@ -738,16 +736,15 @@ main (int argc, char * const argv [])
         s.kernel_symtab_path = string("/boot/System.map-") + s.kernel_release;
     }
 
-  //  Since the introduction of -B, the logic of -r REV implies -p4 is dropped.
-  //  If the version of kernel build tree specified by -B is not the same as 
-  //  `uname -r`, stap will give a failure message in pass5 anyways.
-  //
-  //  if (s.last_pass > 4 && release_changed)
-  //  {
-  //    if (s.verbose)
-  //      cerr << "Warning: changing last pass to 4 since cross-compiling" << endl;
-  //    s.last_pass = 4;
-  //  }
+  // Warn in case the target kernel release doesn't match the running one.
+  if (s.last_pass > 4 &&
+      (string(buf.release) != s.kernel_release ||
+       string(buf.machine) != s.architecture))
+   {
+     if(! s.suppress_warnings)
+       cerr << "WARNING: kernel release/architecture mismatch with host forces last-pass 4." << endl;
+     s.last_pass = 4;
+   }
 
   for (int i = optind; i < argc; i++)
     {
diff --git a/stap.1.in b/stap.1.in
index 39594a15..7b26bc2d 100644
--- a/stap.1.in
+++ b/stap.1.in
@@ -156,8 +156,12 @@ be used to override limit parameters described below.
 .BI \-R " DIR"
 Look for the systemtap runtime sources in the given directory.
 .TP
+.BI \-r " DIR"
+Build for kernel in given build tree.
+.TP
 .BI \-r " RELEASE"
-Build for given kernel release instead of currently running one.
+Build for kernel in build tree
+.BR /lib/modules/RELEASE/build .
 .TP
 .BI \-m " MODULE"
 Use the given name for the generated kernel object module, instead
diff --git a/tapsets.cxx b/tapsets.cxx
index 88c6ea41..178d2465 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -888,28 +888,23 @@ struct dwflpp
   }
 
 
+  // XXX: See also translate.cxx:emit_symbol_data
+
   void setup_kernel(bool debuginfo_needed = true)
   {
     if (! sess.module_cache)
       sess.module_cache = new module_cache ();
 
-    // XXX: this is where the session -R parameter could come in
-    static char debuginfo_path_arr[] = "-:.debug:/usr/lib/debug:build";
-    static char *debuginfo_env_arr = getenv("SYSTEMTAP_DEBUGINFO_PATH");
-
-    static char *debuginfo_path = (debuginfo_env_arr ?
-				   debuginfo_env_arr : sess.kernel_build_tree.size () ?
-				   (char *) sess.kernel_build_tree.c_str() : debuginfo_path_arr);
-    static const char *debug_path = (debuginfo_env_arr ?
-				     debuginfo_env_arr : sess.kernel_build_tree.size () ?
-				     sess.kernel_build_tree.c_str() : sess.kernel_release.c_str());
+    static const char *debuginfo_path_arr = "-:.debug:/usr/lib/debug:build";
+    static const char *debuginfo_env_arr = getenv("SYSTEMTAP_DEBUGINFO_PATH");
+    static const char *debuginfo_path = (debuginfo_env_arr ?: debuginfo_path_arr );
 
     static const Dwfl_Callbacks kernel_callbacks =
       {
 	dwfl_linux_kernel_find_elf,
 	dwfl_standard_find_debuginfo,
 	dwfl_offline_section_address,
-        & debuginfo_path
+        (char **) & debuginfo_path
       };
 
     dwfl = dwfl_begin (&kernel_callbacks);
@@ -917,16 +912,28 @@ struct dwflpp
       throw semantic_error ("cannot open dwfl");
     dwfl_report_begin (dwfl);
 
+    // We have a problem with -r REVISION vs -r BUILDDIR here.  If
+    // we're running against a fedora/rhel style kernel-debuginfo
+    // tree, s.kernel_build_tree is not the place where the unstripped
+    // vmlinux will be installed.  Rather, it's over yonder at
+    // /usr/lib/debug/lib/modules/$REVISION/.  It seems that there is
+    // no way to set the dwfl_callback.debuginfo_path and always
+    // passs the plain kernel_release here.  So instead we have to
+    // hard-code this magic here.
+    string elfutils_kernel_path;
+    if (sess.kernel_build_tree == string("/lib/modules/" + sess.kernel_release + "/build"))
+      elfutils_kernel_path = sess.kernel_release;
+    else
+      elfutils_kernel_path = sess.kernel_build_tree;      
+
     int rc = dwfl_linux_kernel_report_offline (dwfl,
-                                               debug_path,
+                                               elfutils_kernel_path.c_str(),
                                                NULL);
 
     if (debuginfo_needed)
-      dwfl_assert (string("missing kernel ") +
-                   sess.kernel_release +
-                   string(" ") +
-                   sess.architecture +
-                   string(" debuginfo"),
+      dwfl_assert (string("missing ") + sess.architecture +
+                   string(" kernel/module debuginfo under '") +
+                   sess.kernel_build_tree + string("'"),
                    rc);
 
     // XXX: it would be nice if we could do a single
@@ -950,18 +957,18 @@ struct dwflpp
 
   void setup_user(string module_name, bool debuginfo_needed = true)
   {
-    // XXX: this is where the session -R parameter could come in
-    static char debuginfo_path_arr[] = "-:.debug:/usr/lib/debug:build";
-    static char *debuginfo_env_arr = getenv("SYSTEMTAP_DEBUGINFO_PATH");
-    static char *debuginfo_path = (debuginfo_env_arr ?: sess.kernel_build_tree.size () ?
-				   (char *) sess.kernel_build_tree.c_str() : debuginfo_path_arr);
+    static const char *debuginfo_path_arr = "-:.debug:/usr/lib/debug:build";
+    static const char *debuginfo_env_arr = getenv("SYSTEMTAP_DEBUGINFO_PATH");
+    // NB: kernel_build_tree doesn't enter into this, as it's for
+    // kernel-side modules only.
+    static const char *debuginfo_path = (debuginfo_env_arr ?: debuginfo_path_arr);
 
     static const Dwfl_Callbacks user_callbacks =
       {
         NULL, /* dwfl_linux_kernel_find_elf, */
         dwfl_standard_find_debuginfo,
         dwfl_offline_section_address,
-        & debuginfo_path
+        (char **) & debuginfo_path
       };
 
     dwfl = dwfl_begin (&user_callbacks);
@@ -8715,14 +8722,8 @@ mark_builder::build(systemtap_session & sess,
   if (! cache_initialized)
     {
       cache_initialized = true;
-      string module_markers_path;
-      if (! sess.kernel_build_tree.size ())
-	module_markers_path = "/lib/modules/" + sess.kernel_release
-	  + "/build/Module.markers";
-      else
-	module_markers_path = sess.kernel_build_tree + "/Module.markers";
-	
-
+      string module_markers_path = sess.kernel_build_tree + "/Module.markers";
+      
       ifstream module_markers;
       module_markers.open(module_markers_path.c_str(), ifstream::in);
       if (! module_markers)
diff --git a/testsuite/ChangeLog b/testsuite/ChangeLog
index c1ae1420..1ba4e287 100644
--- a/testsuite/ChangeLog
+++ b/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2008-12-03  Frank Ch. Eigler  <fche@elastic.org>
+
+	* systemtap.base/debugpath.exp: Correct test case for new -r behavior.
+
 2008-11-29  Frank Ch. Eigler  <fche@elastic.org>
 
 	* systemtap.base/uprobes.exp: Simplify -p5 test.
diff --git a/testsuite/systemtap.base/debugpath.exp b/testsuite/systemtap.base/debugpath.exp
index bc92c081..b0f938b6 100644
--- a/testsuite/systemtap.base/debugpath.exp
+++ b/testsuite/systemtap.base/debugpath.exp
@@ -1,6 +1,6 @@
 
 set test "debugpath-bad"
-spawn env SYSTEMTAP_DEBUGINFO_PATH=/dev/null stap -e "probe kernel.function(\"sys_open\") {}" -p4
+spawn env SYSTEMTAP_DEBUGINFO_PATH=/dev/null stap -e "probe kernel.function(\"sys_open\") {}" -wp4
 expect {
 	-re {^semantic error:.*missing.*debuginfo} { pass $test }
 	timeout { fail "$test (timeout1)" }
@@ -13,15 +13,15 @@ set test "debugpath-good"
 set uname [exec /bin/uname -r]
 
 # Guess where debuginfo is installed
-if [file isdirectory /usr/lib/debug/lib/modules/$uname] {
-  set debuginfo_path "/usr/lib/debug/lib/modules/$uname"
+if [file isdirectory /usr/lib/debug] {
+  set debuginfo_path "/usr/lib/debug"
 } elseif [file isdirectory /lib/modules/$uname/build] {
   set debuginfo_path "/lib/modules/$uname/build"
 } else {
   set debuginfo_path "/lib/modules/$uname"
 }
 
-spawn env SYSTEMTAP_DEBUGINFO_PATH=$debuginfo_path stap -e "probe kernel.function(\"sys_open\") {}" -p2
+spawn env SYSTEMTAP_DEBUGINFO_PATH=$debuginfo_path stap -e "probe kernel.function(\"sys_open\") {}" -wp2
 expect {
 	-re {kernel.function.*pc=} { pass $test }
 	timeout { fail "$test (timeout2)" }
diff --git a/translate.cxx b/translate.cxx
index dc9f12bc..12d50617 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -4706,15 +4706,9 @@ emit_symbol_data (systemtap_session& s)
   unwindsym_dump_context ctx = { s, kallsyms_out, 0, s.unwindsym_modules };
 
   // XXX: copied from tapsets.cxx dwflpp::, sadly
-  static char debuginfo_path_arr[] = "-:.debug:/usr/lib/debug:build";
-  static char *debuginfo_env_arr = getenv("SYSTEMTAP_DEBUGINFO_PATH");
-
-  static char *debuginfo_path = (debuginfo_env_arr ?
-                                 debuginfo_env_arr : s.kernel_build_tree.size () ?
-				 (char *) s.kernel_build_tree.c_str() : debuginfo_path_arr);
-  static const char *debug_path = (debuginfo_env_arr ?
-				   debuginfo_env_arr : s.kernel_build_tree.size () ?
-				   s.kernel_build_tree.c_str() : s.kernel_release.c_str());
+  static const char *debuginfo_path_arr = "-:.debug:/usr/lib/debug:build";
+  static const char *debuginfo_env_arr = getenv("SYSTEMTAP_DEBUGINFO_PATH");
+  static const char *debuginfo_path = (debuginfo_env_arr ?: debuginfo_path_arr);
 
   // ---- step 1: process any kernel modules listed
   static const Dwfl_Callbacks kernel_callbacks =
@@ -4722,14 +4716,31 @@ emit_symbol_data (systemtap_session& s)
       dwfl_linux_kernel_find_elf,
       dwfl_standard_find_debuginfo,
       dwfl_offline_section_address,
-      & debuginfo_path
+      (char **) & debuginfo_path
     };
 
   Dwfl *dwfl = dwfl_begin (&kernel_callbacks);
   if (!dwfl)
     throw semantic_error ("cannot open dwfl");
   dwfl_report_begin (dwfl);
-  int rc = dwfl_linux_kernel_report_offline (dwfl, debug_path, NULL /* XXX: filtering callback */);
+
+  // We have a problem with -r REVISION vs -r BUILDDIR here.  If
+  // we're running against a fedora/rhel style kernel-debuginfo
+  // tree, s.kernel_build_tree is not the place where the unstripped
+  // vmlinux will be installed.  Rather, it's over yonder at
+  // /usr/lib/debug/lib/modules/$REVISION/.  It seems that there is
+  // no way to set the dwfl_callback.debuginfo_path and always
+  // passs the plain kernel_release here.  So instead we have to
+  // hard-code this magic here.
+  string elfutils_kernel_path;
+  if (s.kernel_build_tree == string("/lib/modules/" + s.kernel_release + "/build"))
+    elfutils_kernel_path = s.kernel_release;
+  else
+    elfutils_kernel_path = s.kernel_build_tree;      
+
+  int rc = dwfl_linux_kernel_report_offline (dwfl, 
+                                             elfutils_kernel_path.c_str(),
+                                             NULL /* XXX: filtering callback */);
   dwfl_report_end (dwfl, NULL, NULL);
   if (rc == 0) // tolerate missing data; will warn user about it anyway
     {
@@ -4752,7 +4763,7 @@ emit_symbol_data (systemtap_session& s)
       NULL, /* dwfl_linux_kernel_find_elf, */
       dwfl_standard_find_debuginfo,
       dwfl_offline_section_address,
-      & debuginfo_path
+      (char **) & debuginfo_path
     };
 
   for (std::set<std::string>::iterator it = s.unwindsym_modules.begin();