[RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[Prerna Saxena]

[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]

Hi,
Here's a set of patches which enable the stap-translator to utilize 
kprobes for resolving function addresses.
( Similar to James Bottomley's patch sent out last july )
In place of resolving probe points in semantic pass (Pass 2 ) by 
consulting vmlinux/debuginfo, this approach defers symbol resolution to 
the generated kprobes module. The kprobes module is passed the name of 
the function to be probed, which gets resolved against the kernel symbol 
tables to insert probes.

This construct can be invoked using a new switch "--ksym" .

In its present form, it is capable of probing function entry & returns 
(non-inlines). It does /*not*/ support :
    *  a wildcard - based search for module/function names
    *  probing  select/all functions in a given source file
    *  probing inline functions.
    *  statement probes

Known issues :
1. Apparently systemtap modules pick up build-id from the debuginfo 
files. Since debuginfo lookup is completely bypassed here, the generated 
stap modules fail a consistency check later owing to incorrect build id. 
For now, patch 4 comments out this check and the stap modules run fine, 
but I'd appreciate some pointers on how to set it right. :-)

2. An incorrect indentation parameter passed to the translated output in 
pass 3 causes stap to abort due to assert failure. Patch 4 corrects this 
as well.

Patches :
1. kallsym_patch_1 , kallsym_patch_2 : introduce changes to session.h / 
main.cxx for the new switch "--ksym"
2. kallsym_patch_3 : changes to tapsets.cxx.
3. kallsym_patch_4 : workarounds for known problems.

I'm working on fine-tuning its capabilities.....looking fwd to 
suggestions... :-)

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India 


[-- Attachment #2: kallsym_patch_1 --]
[-- Type: text/plain, Size: 354 bytes --]

Index: git-02-mar/session.h
===================================================================
--- git-02-mar.orig/session.h
+++ git-02-mar/session.h
@@ -118,6 +118,7 @@ struct systemtap_session
 
   // dwarfless operation
   bool consult_symtab;
+  bool consult_kallsym;
   std::string kernel_symtab_path;
   bool ignore_vmlinux;
   bool ignore_dwarf;

Re: [RFC][PATCH 2/4] kprobe-based symbol resolution for stap-translator

[Prerna Saxena]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

Prerna Saxena wrote:
> Hi,
> Here's a set of patches which enable the stap-translator to utilize 
> kprobes for resolving function addresses.
> ( Similar to James Bottomley's patch sent out last july )
> In place of resolving probe points in semantic pass (Pass 2 ) by 
> consulting vmlinux/debuginfo, this approach defers symbol resolution 
> to the generated kprobes module. The kprobes module is passed the name 
> of the function to be probed, which gets resolved against the kernel 
> symbol tables to insert probes.
>
> This construct can be invoked using a new switch "--ksym" .
>
> In its present form, it is capable of probing function entry & returns 
> (non-inlines). It does /*not*/ support :
>    *  a wildcard - based search for module/function names
>    *  probing  select/all functions in a given source file
>    *  probing inline functions.
>    *  statement probes
>
> Known issues :
> 1. Apparently systemtap modules pick up build-id from the debuginfo 
> files. Since debuginfo lookup is completely bypassed here, the 
> generated stap modules fail a consistency check later owing to 
> incorrect build id. For now, patch 4 comments out this check and the 
> stap modules run fine, but I'd appreciate some pointers on how to set 
> it right. :-)
>
> 2. An incorrect indentation parameter passed to the translated output 
> in pass 3 causes stap to abort due to assert failure. Patch 4 corrects 
> this as well.
>
> Patches :
> 1. kallsym_patch_1 , kallsym_patch_2 : introduce changes to session.h 
> / main.cxx for the new switch "--ksym"
> 2. kallsym_patch_3 : changes to tapsets.cxx.
> 3. kallsym_patch_4 : workarounds for known problems.
>
> I'm working on fine-tuning its capabilities.....looking fwd to 
> suggestions... :-)
>

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India 


[-- Attachment #2: kallsym_patch_2 --]
[-- Type: text/plain, Size: 1708 bytes --]

Index: git-02-mar/main.cxx
===================================================================
--- git-02-mar.orig/main.cxx
+++ git-02-mar/main.cxx
@@ -131,6 +131,7 @@ usage (systemtap_session& s, int exitcod
     << "   --kelf     make do with symbol table from vmlinux" << endl
     << "   --kmap[=FILE]" << endl
     << "              make do with symbol table from nm listing" << endl
+    << "   --ksym     defer symbol resolution to kprobes " <<endl
 #endif
   // Formerly present --ignore-{vmlinux,dwarf} options are for testsuite use
   // only, and don't belong in the eyesight of a plain user.
@@ -428,6 +429,7 @@ main (int argc, char * const argv [])
 #define LONG_OPT_IGNORE_VMLINUX 3
 #define LONG_OPT_IGNORE_DWARF 4
 #define LONG_OPT_VERBOSE_PASS 5
+#define LONG_OPT_KALLSYMS 6
       // NB: also see find_hash(), usage(), switch stmt below, stap.1 man page
       static struct option long_options[] = {
         { "kelf", 0, &long_opt, LONG_OPT_KELF },
@@ -435,6 +437,7 @@ main (int argc, char * const argv [])
         { "ignore-vmlinux", 0, &long_opt, LONG_OPT_IGNORE_VMLINUX },
         { "ignore-dwarf", 0, &long_opt, LONG_OPT_IGNORE_DWARF },
         { "vp", 1, &long_opt, LONG_OPT_VERBOSE_PASS },
+        { "ksym", 0, &long_opt, LONG_OPT_KALLSYMS },
         { NULL, 0, NULL, 0 }
       };
       int grc = getopt_long (argc, argv, "hVMvtp:I:e:o:R:r:m:kgPc:x:D:bs:uqwl:d:L:F",
@@ -675,6 +678,9 @@ main (int argc, char * const argv [])
 	    case LONG_OPT_IGNORE_DWARF:
 	      s.ignore_dwarf = true;
 	      break;
+	    case LONG_OPT_KALLSYMS:
+	      s.consult_kallsym = true;
+              break;
 	    case LONG_OPT_VERBOSE_PASS:
               {
                 bool ok = true;

Re: [RFC][PATCH 3/4] kprobe-based symbol resolution for stap-translator

[Prerna Saxena]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

Prerna Saxena wrote:
> Hi,
> Here's a set of patches which enable the stap-translator to utilize 
> kprobes for resolving function addresses.
> ( Similar to James Bottomley's patch sent out last july )
> In place of resolving probe points in semantic pass (Pass 2 ) by 
> consulting vmlinux/debuginfo, this approach defers symbol resolution 
> to the generated kprobes module. The kprobes module is passed the name 
> of the function to be probed, which gets resolved against the kernel 
> symbol tables to insert probes.
>
> This construct can be invoked using a new switch "--ksym" .
>
> In its present form, it is capable of probing function entry & returns 
> (non-inlines). It does /*not*/ support :
>    *  a wildcard - based search for module/function names
>    *  probing  select/all functions in a given source file
>    *  probing inline functions.
>    *  statement probes
>
> Known issues :
> 1. Apparently systemtap modules pick up build-id from the debuginfo 
> files. Since debuginfo lookup is completely bypassed here, the 
> generated stap modules fail a consistency check later owing to 
> incorrect build id. For now, patch 4 comments out this check and the 
> stap modules run fine, but I'd appreciate some pointers on how to set 
> it right. :-)
>
> 2. An incorrect indentation parameter passed to the translated output 
> in pass 3 causes stap to abort due to assert failure. Patch 4 corrects 
> this as well.
>
> Patches :
> 1. kallsym_patch_1 , kallsym_patch_2 : introduce changes to session.h 
> / main.cxx for the new switch "--ksym"
> 2. kallsym_patch_3 : changes to tapsets.cxx.
> 3. kallsym_patch_4 : workarounds for known problems.
>
> I'm working on fine-tuning its capabilities.....looking fwd to 
> suggestions... :-)

> -- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India 


[-- Attachment #2: kallsym_patch_3 --]
[-- Type: text/plain, Size: 11962 bytes --]

Index: git-02-mar/tapsets.cxx
===================================================================
--- git-02-mar.orig/tapsets.cxx
+++ git-02-mar/tapsets.cxx
@@ -931,7 +931,7 @@ struct dwflpp
                                                elfutils_kernel_path.c_str(),
                                                NULL);
 
-    if (debuginfo_needed)
+    if (debuginfo_needed && !sess.consult_kallsym)
       dwfl_assert (string("missing ") + sess.architecture +
                    string(" kernel/module debuginfo under '") +
                    sess.kernel_build_tree + string("'"),
@@ -2572,6 +2572,7 @@ struct dwarf_derived_probe: public deriv
                        int line,
                        const string& module,
                        const string& section,
+                       const string& symbol_str,
 		       Dwarf_Addr dwfl_addr,
 		       Dwarf_Addr addr,
 		       dwarf_query & q,
@@ -2579,6 +2580,7 @@ struct dwarf_derived_probe: public deriv
 
   string module;
   string section;
+  string symbol_string;
   Dwarf_Addr addr;
   bool has_return;
   bool has_maxactive;
@@ -3633,7 +3635,9 @@ dwarf_query::add_probe_point(const strin
         {
           assert (has_kernel || has_module);
           results.push_back (new dwarf_derived_probe(funcname, filename, line,
-                                                     module, reloc_section, addr, reloc_addr,
+                                                     module, reloc_section,
+						     funcname,
+						     addr, reloc_addr,
                                                      *this, scope_die));
         }
     }
@@ -5120,7 +5124,7 @@ dwarf_derived_probe::printsig (ostream& 
   // function instances.  This is distinct from the verbose/clog
   // output, since this part goes into the cache hash calculations.
   sole_location()->print (o);
-  o << " /* pc=" << section << "+0x" << hex << addr << dec << " */";
+  o << " /* symbol: "<< symbol_string <<" pc=" << section << "+0x" << hex << addr << dec << " */";
   printsig_nested (o);
 }
 
@@ -5143,6 +5147,7 @@ dwarf_derived_probe::dwarf_derived_probe
                                          // (equivalently module=="kernel")
                                          const string& module,
                                          const string& section,
+					 const string& symbol_str,
                                          // NB: dwfl_addr is the virtualized
                                          // address for this symbol.
                                          Dwarf_Addr dwfl_addr,
@@ -5152,7 +5157,7 @@ dwarf_derived_probe::dwarf_derived_probe
                                          dwarf_query& q,
                                          Dwarf_Die* scope_die /* may be null */)
   : derived_probe (q.base_probe, new probe_point(*q.base_loc) /* .components soon rewritten */ ),
-    module (module), section (section), addr (addr),
+    module (module), section (section), symbol_string (symbol_str), addr (addr),
     has_return (q.has_return),
     has_maxactive (q.has_maxactive),
     maxactive_val (q.maxactive_val)
@@ -5353,6 +5358,8 @@ dwarf_derived_probe_group::emit_module_d
   s.op->newline() << "#error \"Need CONFIG_KPROBES!\"";
   s.op->newline() << "#endif";
   s.op->newline();
+  s.op->newline() << "#define KALLSYMS_USED " << s.consult_kallsym;
+  s.op->newline();
 
   // Forward declare the master entry functions
   s.op->newline() << "static int enter_kprobe_probe (struct kprobe *inst,";
@@ -5387,8 +5394,8 @@ dwarf_derived_probe_group::emit_module_d
   // Let's find some stats for the three embedded strings.  Maybe they
   // are small and uniform enough to justify putting char[MAX]'s into
   // the array instead of relocated char*'s.
-  size_t module_name_max = 0, section_name_max = 0, pp_name_max = 0;
-  size_t module_name_tot = 0, section_name_tot = 0, pp_name_tot = 0;
+  size_t module_name_max = 0, section_name_max = 0, pp_name_max = 0, symbol_string_name_max = 0;
+  size_t module_name_tot = 0, section_name_tot = 0, pp_name_tot = 0, symbol_string_name_tot = 0;
   size_t all_name_cnt = probes_by_module.size(); // for average
   for (p_b_m_iterator it = probes_by_module.begin(); it != probes_by_module.end(); it++)
     {
@@ -5400,6 +5407,7 @@ dwarf_derived_probe_group::emit_module_d
       DOIT(module_name, p->module.size());
       DOIT(section_name, p->section.size());
       DOIT(pp_name, lex_cast_qstring(*p->sole_location()).size());
+      DOIT(symbol_string_name,p->symbol_string.size());
 #undef DOIT
     }
 
@@ -5422,6 +5430,7 @@ dwarf_derived_probe_group::emit_module_d
   CALCIT(module);
   CALCIT(section);
   CALCIT(pp);
+  CALCIT(symbol_string);
 
   s.op->newline() << "const unsigned long address;";
   s.op->newline() << "void (* const ph) (struct context*);";
@@ -5444,7 +5453,8 @@ dwarf_derived_probe_group::emit_module_d
       s.op->line() << " .module=\"" << p->module << "\",";
       s.op->line() << " .section=\"" << p->section << "\",";
       s.op->line() << " .pp=" << lex_cast_qstring (*p->sole_location()) << ",";
-      s.op->line() << " .ph=&" << p->name;
+      s.op->line() << " .ph=&" << p->name << ",";
+      s.op->line() << " .symbol_string=\""<< p->symbol_string <<"\"";
       s.op->line() << " },";
     }
 
@@ -5502,11 +5512,19 @@ dwarf_derived_probe_group::emit_module_i
   s.op->newline() << "for (i=0; i<" << probes_by_module.size() << "; i++) {";
   s.op->newline(1) << "struct stap_dwarf_probe *sdp = & stap_dwarf_probes[i];";
   s.op->newline() << "struct stap_dwarf_kprobe *kp = & stap_dwarf_kprobes[i];";
-  s.op->newline() << "unsigned long relocated_addr = _stp_module_relocate (sdp->module, sdp->section, sdp->address);";
-  s.op->newline() << "if (relocated_addr == 0) continue;"; // quietly; assume module is absent
+  s.op->newline() << "unsigned long relocated_addr = 0;";
+  s.op->newline() << "if(!KALLSYMS_USED)";
+  s.op->newline() << "   {";
+  s.op->newline() << "     relocated_addr = _stp_module_relocate (sdp->module, sdp->section, sdp->address);";
+  s.op->newline() << "     if (relocated_addr == 0) continue;"; // quietly; assume module is absent
+  s.op->newline() << "   } ";
   s.op->newline() << "probe_point = sdp->pp;"; // for error messages
   s.op->newline() << "if (sdp->return_p) {";
-  s.op->newline(1) << "kp->u.krp.kp.addr = (void *) relocated_addr;";
+  s.op->newline() << "kp->u.krp.kp.addr = (void *) relocated_addr;";
+  s.op->newline() << "if(KALLSYMS_USED)";
+  s.op->newline() << "   {";
+  s.op->newline() << "      kp->u.krp.kp.symbol_name = sdp->symbol_string;";
+  s.op->newline() << "   }";
   s.op->newline() << "if (sdp->maxactive_p) {";
   s.op->newline(1) << "kp->u.krp.maxactive = sdp->maxactive_val;";
   s.op->newline(-1) << "} else {";
@@ -5517,6 +5535,10 @@ dwarf_derived_probe_group::emit_module_i
   s.op->newline() << "#ifdef __ia64__";
   s.op->newline() << "kp->dummy.addr = kp->u.krp.kp.addr;";
   s.op->newline() << "kp->dummy.pre_handler = NULL;";
+  s.op->newline() << "if(KALLSYMS_USED)";
+  s.op->newline() << "   {";
+  s.op->newline() << "      kp->dummy.symbol_name = sdp->symbol_string;";
+  s.op->newline() << "   }";
   s.op->newline() << "rc = register_kprobe (& kp->dummy);";
   s.op->newline() << "if (rc == 0) {";
   s.op->newline(1) << "rc = register_kretprobe (& kp->u.krp);";
@@ -5529,10 +5551,18 @@ dwarf_derived_probe_group::emit_module_i
   s.op->newline(-1) << "} else {";
   // to ensure safeness of bspcache, always use aggr_kprobe on ia64
   s.op->newline(1) << "kp->u.kp.addr = (void *) relocated_addr;";
+  s.op->newline() << "if(KALLSYMS_USED)";
+  s.op->newline() << "   {";
+  s.op->newline() << "      kp->u.kp.symbol_name = sdp->symbol_string;";
+  s.op->newline() << "   }";
   s.op->newline() << "kp->u.kp.pre_handler = &enter_kprobe_probe;";
   s.op->newline() << "#ifdef __ia64__";
   s.op->newline() << "kp->dummy.addr = kp->u.kp.addr;";
   s.op->newline() << "kp->dummy.pre_handler = NULL;";
+  s.op->newline() << "if(KALLSYMS_USED)";
+  s.op->newline() << "   {";
+  s.op->newline() << "      kp->dummy.symbol_name = sdp->symbol_string;";
+  s.op->newline() << "   }";
   s.op->newline() << "rc = register_kprobe (& kp->dummy);";
   s.op->newline() << "if (rc == 0) {";
   s.op->newline(1) << "rc = register_kprobe (& kp->u.kp);";
@@ -5545,7 +5575,16 @@ dwarf_derived_probe_group::emit_module_i
   s.op->newline(-1) << "}";
   s.op->newline() << "if (rc) {"; // PR6749: tolerate a failed register_*probe.
   s.op->newline(1) << "sdp->registered_p = 0;";
-  s.op->newline() << "_stp_warn (\"probe %s registration error (rc %d)\", probe_point, rc);";
+  s.op->newline() << " if(KALLSYMS_USED && rc == -EINVAL) ";
+  s.op->newline() << " {";
+  s.op->newline() << "   _stp_error(\"Error registering kprobe,possibly an incorrect name : %s\", sdp->symbol_string); ";
+  s.op->newline() << "   atomic_set (&session_state, STAP_SESSION_ERROR);";
+  s.op->newline() << "   goto out;";
+
+  s.op->newline() << " }";
+  s.op->newline() << " else";
+
+  s.op->newline() << "_stp_warn (\"probe %s for %s registration error (rc %d)\", probe_point, sdp->pp, rc);";
   s.op->newline() << "rc = 0;"; // continue with other probes
   // XXX: shall we increment numskipped?
   s.op->newline(-1) << "}";
@@ -5782,10 +5821,6 @@ dwarf_builder::build(systemtap_session &
   
   dwarf_query q(sess, base, location, *dw, parameters, finished_results);
 
-
-  // XXX: kernel.statement.absolute is a special case that requires no
-  // dwfl processing.  This code should be in a separate builder.
-
   if (q.has_kernel && q.has_absolute)
     {
       // assert guru mode for absolute probes
@@ -5799,13 +5834,52 @@ dwarf_builder::build(systemtap_session &
       // dwarf_derived_probe right here and now.
       dwarf_derived_probe* p =
         new dwarf_derived_probe ("", "", 0, "kernel", "",
-                                 q.statement_num_val, q.statement_num_val,
+                                 q.function_str_val,
+				 q.statement_num_val, q.statement_num_val,
                                  q, 0);
       finished_results.push_back (p);
       sess.unwindsym_modules.insert ("kernel");
       return;
     }
 
+  //consult_kallsym case : leave the name resolution to kprobes
+  if (sess.consult_kallsym )
+     {
+	   if (q.has_statement_str)
+		throw semantic_error("statement probes not supported with kprobes based symbol resolution");
+	   if (q.function_str_val == "")
+		throw semantic_error("empty function name string");
+	   string sym_funcname = q.function_str_val;
+	   unsigned int sym_length = sym_funcname.size();
+           if ( (sym_funcname.find_first_of('@',0) < sym_length ) || (sym_funcname.find_first_of(':',0) < sym_length ) || (sym_funcname.find_first_of('*',0)< sym_length) )
+		throw semantic_error("kprobe-based symbol resolution doesn not support wildcards or source-file based probing : " + sym_funcname + "\n");
+
+	    if (q.has_inline)
+		throw semantic_error("kprobe-based resolution does not support inlines \n");
+       	    if (q.has_kernel)
+	  	{
+ 	    	  dwarf_derived_probe* p =
+	          new dwarf_derived_probe (q.function_str_val, "", 0, "kernel", "",
+	               			   q.function_str_val, 0, 0, q, NULL );
+                  finished_results.push_back (p);
+                  sess.unwindsym_modules.insert ("kernel");
+                  return;
+                }
+            else if (q.has_module)
+	        {
+ 	          dwarf_derived_probe* p =
+	          new dwarf_derived_probe (q.function_str_val, "", 0, module_name, "",
+	               			   q.function_str_val, 0, 0, q, NULL );
+                  finished_results.push_back (p);
+                  sess.unwindsym_modules.insert (module_name);
+                  return;
+                 }
+      }
+  // XXX: kernel.statement.absolute is a special case that requires no
+  // dwfl processing.  This code should be in a separate builder.
+
+
+
   dw->query_modules(&q);
 }
 

Re: [RFC][PATCH 4/4] kprobe-based symbol resolution for stap-translator

[Prerna Saxena]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

Prerna Saxena wrote:
> Hi,
> Here's a set of patches which enable the stap-translator to utilize 
> kprobes for resolving function addresses.
> ( Similar to James Bottomley's patch sent out last july )
> In place of resolving probe points in semantic pass (Pass 2 ) by 
> consulting vmlinux/debuginfo, this approach defers symbol resolution 
> to the generated kprobes module. The kprobes module is passed the name 
> of the function to be probed, which gets resolved against the kernel 
> symbol tables to insert probes.
>
> This construct can be invoked using a new switch "--ksym" .
>
> In its present form, it is capable of probing function entry & returns 
> (non-inlines). It does /*not*/ support :
>    *  a wildcard - based search for module/function names
>    *  probing  select/all functions in a given source file
>    *  probing inline functions.
>    *  statement probes
>
> Known issues :
> 1. Apparently systemtap modules pick up build-id from the debuginfo 
> files. Since debuginfo lookup is completely bypassed here, the 
> generated stap modules fail a consistency check later owing to 
> incorrect build id. For now, patch 4 comments out this check and the 
> stap modules run fine, but I'd appreciate some pointers on how to set 
> it right. :-)
>
> 2. An incorrect indentation parameter passed to the translated output 
> in pass 3 causes stap to abort due to assert failure. Patch 4 corrects 
> this as well.
>
> Patches :
> 1. kallsym_patch_1 , kallsym_patch_2 : introduce changes to session.h 
> / main.cxx for the new switch "--ksym"
> 2. kallsym_patch_3 : changes to tapsets.cxx.
> 3. kallsym_patch_4 : workarounds for known problems.
>
> I'm working on fine-tuning its capabilities.....looking fwd to 
> suggestions... :-)
>

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India 


[-- Attachment #2: kallsym_patch_4 --]
[-- Type: text/plain, Size: 1153 bytes --]

---
 tapsets.cxx   |    2 +-
 translate.cxx |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Index: git-12-march/translate.cxx
===================================================================
--- git-12-march.orig/translate.cxx
+++ git-12-march/translate.cxx
@@ -1129,7 +1129,7 @@ c_unparser::emit_module_init ()
   o->newline(-1) << "}";
 
   // perform buildid-based checking if able
-  o->newline() << "if (_stp_module_check()) rc = -EINVAL;";
+//  o->newline() << "if (_stp_module_check()) rc = -EINVAL;";
 
   o->newline(-1) << "}";
   o->newline() << "if (rc) goto out;";
Index: git-12-march/tapsets.cxx
===================================================================
--- git-12-march.orig/tapsets.cxx
+++ git-12-march/tapsets.cxx
@@ -5434,7 +5434,7 @@ dwarf_derived_probe_group::emit_module_d
 
   s.op->newline() << "const unsigned long address;";
   s.op->newline() << "void (* const ph) (struct context*);";
-  s.op->newline(-1) << "} stap_dwarf_probes[] = {";
+  s.op->newline() << "} stap_dwarf_probes[] = {";
   s.op->indent(1);
 
   for (p_b_m_iterator it = probes_by_module.begin(); it != probes_by_module.end(); it++)

Re: [RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[Frank Ch. Eigler]

Prerna Saxena <prerna@linux.vnet.ibm.com> writes:

> Here's a set of patches which enable the stap-translator to utilize
> kprobes for resolving function addresses.  ( Similar to James
> Bottomley's patch sent out last july ) In place of resolving probe
> points in semantic pass (Pass 2 ) by consulting vmlinux/debuginfo,
> this approach defers symbol resolution to the generated kprobes
> module. [...]

I remain unfond of this approach for a variety of reasons.  Perhaps the
best thing is to add this as a separate probe point family entirely,
like    kernel.kprobe("name")   Then there will be no expectation
that wildcards or $-variables should work, nor would there be any
disruption to the dwarf-based code currently in the translator.


> In its present form, it is capable of probing function entry & returns
> (non-inlines). It does /*not*/ support :
>    *  a wildcard - based search for module/function names

The re-forthcoming symbol-table-based probing would enable this.

>    *  probing  select/all functions in a given source file
>    *  probing inline functions.
>    *  statement probes

And of course these are impossible without debuginfo


- FChE

Re: [RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[Ananth N Mavinakayanahalli]

On Fri, Mar 13, 2009 at 09:31:52AM -0400, Frank Ch. Eigler wrote:
> Prerna Saxena <prerna@linux.vnet.ibm.com> writes:
> 
> > Here's a set of patches which enable the stap-translator to utilize
> > kprobes for resolving function addresses.  ( Similar to James
> > Bottomley's patch sent out last july ) In place of resolving probe
> > points in semantic pass (Pass 2 ) by consulting vmlinux/debuginfo,
> > this approach defers symbol resolution to the generated kprobes
> > module. [...]
> 
> I remain unfond of this approach for a variety of reasons.  Perhaps the
> best thing is to add this as a separate probe point family entirely,
> like    kernel.kprobe("name")   Then there will be no expectation
> that wildcards or $-variables should work, nor would there be any
> disruption to the dwarf-based code currently in the translator.

I do see merit in having such a bare-bones option available to users.
Very simple cases as counting of number of calls to a particular
routine, etc., can very well be handled by this approach.

I don't see why we need a new construct. The changes to translator
seem fairly self contained with this patchset along with an independent
option. With just the command line switch, the benefit is that simple
scripts cited above would 'just work' and IMO, that's a big usability
win.

Further, the work Jim and I did for parameter access using arg*
(registers.stp) should just work with this approach too -- so we'll have
a feature that's fairly close to what we had earlier. Agreed,
restoration of the earlier dwarfless support does have its own benefits.

Ananth

Re: [RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[Frank Ch. Eigler]

Hi -

On Fri, Mar 13, 2009 at 07:20:05PM +0530, Ananth N Mavinakayanahalli wrote:

> [...]  I don't see why we need a new construct. The changes to
> translator seem fairly self contained with this patchset along with
> an independent option. [...]

But the option is not independent - it changes the treatment of
otherwise valid scripts and command line options (-l).  I'd rather see 

- no new command line option
- a separate namespace for these restricted sorts of probes
- and then some new intelligence in the translator that automatically
  downgrades "kernel.function("...")' probes to 'kernel.kprobe("...")'
  if the probe point & handler does not appear to require debuginfo

- FChE

Re: [RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[Josh Stone]

Frank Ch. Eigler wrote:
> - and then some new intelligence in the translator that automatically
>   downgrades "kernel.function("...")' probes to 'kernel.kprobe("...")'
>   if the probe point & handler does not appear to require debuginfo

This downgrade could only occur for .call variants, right?  A plain 
'kernel.function("...")' could exist as both a standalone function and 
as an inline instance if gcc is being clever...

I'm leery even of this though, because it seems there's no way to 
validate the function name until you actually run it, right?

Josh

Re: [RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[Frank Ch. Eigler]

Hi -

> >- and then some new intelligence in the translator that automatically
> >  downgrades "kernel.function("...")' probes to 'kernel.kprobe("...")'
> >  if the probe point & handler does not appear to require debuginfo
> 
> This downgrade could only occur for .call variants, right?  A plain 
> 'kernel.function("...")' could exist as both a standalone function and 
> as an inline instance if gcc is being clever...

Yup.

> I'm leery even of this though, because it seems there's no way to 
> validate the function name until you actually run it, right?

And yup.  Therefore we can't evaluate "?"/"!" type probe point flags
to let tapsets adapt either.

Just some more reasons why I'm unfond of this approach...

- FChE

Re: [RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[Ananth N Mavinakayanahalli]

On Fri, Mar 13, 2009 at 02:03:40PM -0700, Josh Stone wrote:
> Frank Ch. Eigler wrote:
>> - and then some new intelligence in the translator that automatically
>>   downgrades "kernel.function("...")' probes to 'kernel.kprobe("...")'
>>   if the probe point & handler does not appear to require debuginfo
>
> This downgrade could only occur for .call variants, right?  A plain 
> 'kernel.function("...")' could exist as both a standalone function and as 
> an inline instance if gcc is being clever...

Yes, kallsyms_lookup_name() works only with .call. It can and will return
only one address per symbol; it won't even resolve compiler inlined
instances (many of the signal tapset failures are a result of such
inlining).

> I'm leery even of this though, because it seems there's no way to validate 
> the function name until you actually run it, right?

I think that fear is an unfounded; as mentioned before, if there are
namespace clashes, the first 'found' instance is what gets returned. Sure,
there can be bugs in the kallsyms' function name resolution. That'd be a
kernel bug and can be fixed.

Ananth

Re: [RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[Josh Stone]

Ananth N Mavinakayanahalli wrote:
> On Fri, Mar 13, 2009 at 02:03:40PM -0700, Josh Stone wrote:
>> I'm leery even of this though, because it seems there's no way to validate 
>> the function name until you actually run it, right?
> 
> I think that fear is an unfounded; as mentioned before, if there are
> namespace clashes, the first 'found' instance is what gets returned. Sure,
> there can be bugs in the kallsyms' function name resolution. That'd be a
> kernel bug and can be fixed.

I understand how kprobes will work in this regard, but that doesn't help
the user, because we don't know until runtime if the function name is
valid.  Right now we can issue an error in the semantic pass if a name
is not found, or provide alternate implementations, as Frank pointed
out.  Consider your own changes in the syscall tapsets:

  probe syscall.read = kernel.function("SyS_read") !,
                       kernel.function("sys_read") { ... }


This could no longer be a compile-time decision with kprobes-based
symbol resolution.  We'd have to compile in both options and make the
decision at runtime.  That may not be too bad in this simple case, but
if it were even slightly more complex, the runtime code will end up a
lot more convoluted.

  probe _syscall_utrace.read =
        process.syscall { if ($syscall != __NR_read) next; }
  probe syscall.read = kernel.function("SyS_read") !,
                       kernel.function("sys_read") !,
                       _syscall_utrace.read { ... }

We allow scripts to do pretty interesting things, but I wouldn't want to
 try to resolve those at runtime...

Josh

Re: [RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[Frank Ch. Eigler]

Hi -

I wrote:
> [...] I'd rather see
> - no new command line option
> - a separate namespace for these restricted sorts of probes
> - and then some new intelligence in the translator that automatically
>   downgrades "kernel.function("...")' probes to 'kernel.kprobe("...")'
>   if the probe point & handler does not appear to require debuginfo

Have you thought more about this approach? 

It may be possible to approximate the kernel.function complications
such as wildcards by heuristics that search /proc/kallsyms (and
therefore assuming/warning that the instrumentation host == target).

- FChE

Re: [RFC][PATCH 1/4] kprobe-based symbol resolution for stap-translator

[Prerna Saxena]

Hi Frank,
Frank Ch. Eigler wrote:
> Hi -
>
> I wrote:
>   
>> [...] I'd rather see
>> - no new command line option
>> - a separate namespace for these restricted sorts of probes
>> - and then some new intelligence in the translator that automatically
>>   downgrades "kernel.function("...")' probes to 'kernel.kprobe("...")'
>>   if the probe point & handler does not appear to require debuginfo
>>     
>
> Have you thought more about this approach? 
>
> It may be possible to approximate the kernel.function complications
> such as wildcards by heuristics that search /proc/kallsyms (and
> therefore assuming/warning that the instrumentation host == target).
>
> - FChE
>   
I'm working on a prototype of "probe kprobe.function" type, which I'll 
be sending out in a few days. In its basic form, it would defer symbol 
resolution to runtime, depending on kernel symbol tables to resolve 
function names.
in its first draft this shall not be capable enough to support wildcards 
; but its capability might be subsequently extended to search 
/proc/kallsyms when the stap probe is instrumented for currently running 
kernel.

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India