PR15481: Only destroy mutexes from stapdyn itself

The primary bug was since commit 8ca891c, _stp_shm_destroy was calling
_stp_runtime_context_free, which calls pthread_mutex_destroy on the
context locks.  But _stp_shm_destroy is only meant to destroy *one*
processes view of shm -- it still needs to leave the contents intact for
other processes!  This was making it so a child process ended and
destroyed the contexts, and thus in stapdyn it couldn't grab a context
to run the end probes.

* runtime/dyninst/shm.c (_stp_shm_destroy): Don't destroy contexts!
* runtime/dyninst/runtime_context.h (_stp_runtime_contexts_free): Remove
  the __-prefixed variant, and explain in comments who should call this.
* tapsets.cxx (common_probe_entryfn_epilogue): Only call put_context if
  we had successfully called get_context before.
* testsuite/systemtap.base/be_loaded.*: Test the simple begin/end case
  with a child command loaded.
* translate.cxx (c_unparser::emit_module_exit): This generated exit
  function is the best place to free the contexts and also shutdown the
  transport, because this is the final call among all processes.
* runtime/dyninst/runtime.h (stp_dyninst_dtor): Don't shutdown the
  transport here, because this is called by every process that has the
  module loaded.

diff --git a/runtime/dyninst/runtime.h b/runtime/dyninst/runtime.h
index ca643444672ce52e10be517f25b454f75bb8bec8..e0b5d8aac88b9880d350d04236b87eabf04c03fa 100644 (file)
--- a/runtime/dyninst/runtime.h
+++ b/runtime/dyninst/runtime.h
@@ -94,7 +94,6 @@ static inline int pseudo_atomic_cmpxchg(atomic_t *v, int oldval, int newval)
 
 /* Semi-forward declarations from runtime_context.h, needed by stat.c/shm.c. */
 static int _stp_runtime_num_contexts;
-static void __stp_runtime_contexts_free(void);
 
 /* Semi-forward declarations from this file, needed by stat.c/transport.c. */
 static int stp_pthread_mutex_init_shared(pthread_mutex_t *mutex);
@@ -401,7 +400,6 @@ static void stp_dyninst_dtor(void)
     stp_dyninst_session_exit();
 
     _stp_print_cleanup();
-    _stp_dyninst_transport_shutdown();
     _stp_shm_destroy();
 
     if (_stp_mem_fd != -1) {

diff --git a/runtime/dyninst/runtime_context.h b/runtime/dyninst/runtime_context.h
index 0c06574591e5a597dea8beeb10bceedd5d34f130..31f88adae4fd1a04a77dcd129f43f160acc7d47d 100644 (file)
--- a/runtime/dyninst/runtime_context.h
+++ b/runtime/dyninst/runtime_context.h
@@ -56,22 +56,13 @@ static int _stp_runtime_contexts_alloc(void)
     return 0;
 }
 
+/* Free the context resources.
+ *
+ * NB: This should *not* be called by every process which has mmaped the shared
+ * memory.  Only the main process which created shm and originally called
+ * _stp_runtime_contexts_alloc should be the one to free it.
+ */
 static void _stp_runtime_contexts_free(void)
-{
-    /*
-     * On shutdown, _stp_runtime_context_free() gets called. However,
-     * there is no guarentee that even though _stp_print_flush() has
-     * been called at this point, that the data has been actually
-     * flushed. So, we still need the context structure's mutexes
-     * alive.
-     *
-     * We'll destroy the context structure's mutexes using
-     * __stp_runtime_contexts_free() (below) when we destroy the
-     * shared memory over in _stp_shm_destroy().
-     */
-}
-
-static void __stp_runtime_contexts_free(void)
 {
     int i;
 

diff --git a/runtime/dyninst/shm.c b/runtime/dyninst/shm.c
index d219b4068721e83ec5a44a49c53b3de3963e687b..44ff87f217ef63ad840de9f3b20986d1bb4fd13b 100644 (file)
--- a/runtime/dyninst/shm.c
+++ b/runtime/dyninst/shm.c
@@ -220,12 +220,10 @@ static void _stp_shm_finalize(void)
 // Tear down everything we created... *sniff*
 // NB: Make sure not to reference any memory within after this!
 // (Other processes may still have their own mmap reference though.)
+// Don't destroy things in shm that may still be used by other processes!
 static void _stp_shm_destroy(void)
 {
        if (_stp_shm_base) {
-               // Tear down the contexts themselves.
-               __stp_runtime_contexts_free();
-
                munmap(_stp_shm_base, _stp_shm_size);
                _stp_shm_base = NULL;
                _stp_shm_size = 0;

diff --git a/tapsets.cxx b/tapsets.cxx
index b598bdc69004717afa16fc666a043ae7c7cfb706..81fa4d8c3341959991fd83684a52a09933e3741c 100644 (file)
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -97,7 +97,7 @@ common_probe_entryfn_prologue (systemtap_session& s,
   s.op->newline() << "atomic_inc(probe_alibi(" << probe << "->index));";
   s.op->newline() << "#else";
 
-  s.op->newline() << "struct context* __restrict__ c;";
+  s.op->newline() << "struct context* __restrict__ c = NULL;";
   s.op->newline() << "#if !INTERRUPTIBLE";
   s.op->newline() << "unsigned long flags;";
   s.op->newline() << "#endif";
@@ -360,7 +360,7 @@ common_probe_entryfn_epilogue (systemtap_session& s,
 
   if (s.runtime_usermode_p())
     {
-      s.op->newline() << "_stp_runtime_entryfn_put_context();";
+      s.op->newline() << "if (c) _stp_runtime_entryfn_put_context();";
       s.op->newline() << "errno = _stp_saved_errno;";
       s.op->newline(-1) << "}";
     }

diff --git a/testsuite/systemtap.base/be_loaded.exp b/testsuite/systemtap.base/be_loaded.exp
new file mode 100644 (file)
index 0000000..82dbfa2
--- /dev/null
+++ b/testsuite/systemtap.base/be_loaded.exp
@@ -0,0 +1,16 @@
+# Simple test that begin/end probes work with a command loaded
+
+set test "be_loaded"
+
+set ::result_string {begin
+end}
+
+set script $srcdir/$subdir/$test.stp
+
+foreach runtime [get_runtime_list] {
+    if {$runtime != ""} {
+        stap_run3 "$test ($runtime)" $script --runtime=$runtime -c true
+    } else {
+        stap_run3 $test $script -c true
+    }
+}

diff --git a/testsuite/systemtap.base/be_loaded.stp b/testsuite/systemtap.base/be_loaded.stp
new file mode 100644 (file)
index 0000000..c9a82b5
--- /dev/null
+++ b/testsuite/systemtap.base/be_loaded.stp
@@ -0,0 +1,7 @@
+/*
+ * be_loaded.stp
+ *
+ * Simple test that begin/end probes work with a command loaded
+ */
+
+probe begin, end { println(pp()) }

diff --git a/translate.cxx b/translate.cxx
index d8f9b08aca0c7075362191c26bd70ada13d3c49a..9a5e6e02684ecee3e30facbdbdd74f735921b0b7 100644 (file)
--- a/translate.cxx
+++ b/translate.cxx
@@ -2096,10 +2096,11 @@ c_unparser::emit_module_exit ()
   // NB: PR13386 needs to restore preemption-blocking counts
   o->newline() << "preempt_enable_no_resched();";
 
-  // In dyninst mode, now we're done with the contexts.
+  // In dyninst mode, now we're done with the contexts, transport, everything!
   if (session->runtime_usermode_p())
     {
       o->newline() << "_stp_runtime_entryfn_put_context();";
+      o->newline() << "_stp_dyninst_transport_shutdown();";
       o->newline() << "_stp_runtime_contexts_free();";
     }