Re: [PATCH v3 1/1] PR11096: Add support for the "module" argument to @var

["Yichun Zhang (agentzh)" <agentzh at gmail dot com>]

Hello Josh!

On Wed, Jun 26, 2013 at 3:40 PM, Josh Stone wrote:
> I have just two small comments, which you can probably just fix without
> further review, then I think this is ready to push!
>

Thank you for your suggestions! I've fixed these and also added some
tests for them. Below is the diff against my local branch with my v3
patch applied. Please have a look :)

BTW, the newly added sdt_global_var.exp test file also uncovers PR15688:

    http://sourceware.org/bugzilla/show_bug.cgi?id=15688

I've marked the corresponding subtests with the PR number.

Thanks!
-agentzh

diff --git a/tapsets.cxx b/tapsets.cxx
index c67beaf..05dbd9a 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -3688,23 +3688,18 @@
dwarf_var_expanding_visitor::visit_target_symbol_context
(target_symbol* e)
 void
 dwarf_var_expanding_visitor::visit_atvar_op (atvar_op *e)
 {
-  if (e->module.empty() && e->cu_name.empty())
-    {
-      // Fill in the module name so that even if there is no match among
-      // local variables, dwarf_atvar_expanding_visitor can still scan
-      // all the CUs in the current module for a global variable match.
-      e->module = q.dw.module_name;
+  // Fill in our current module context if needed
+  if (e->module.empty())
+    e->module = q.dw.module_name;

+  if (e->module == q.dw.module_name && e->cu_name.empty())
+    {
       // process like any other local
       // e->sym_name() will do the right thing
       visit_target_symbol(e);
       return;
     }

-  // Fill in our current module context if needed
-  if (e->module.empty())
-    e->module = q.dw.module_name;
-
   var_expanding_visitor::visit_atvar_op(e);
 }

@@ -4299,9 +4294,9 @@ dwarf_atvar_expanding_visitor::visit_atvar_op
(atvar_op* e)

       /* Unable to find the variable in the current module, so we chain
        * an error in atvar_op */
-      semantic_error  er(_F("unable to find global '%s' in %s, %s %s",
+      semantic_error  er(_F("unable to find global '%s' in %s%s%s",
                             e->sym_name().c_str(), module.c_str(),
-                            e->cu_name.empty() ? "" : _("in"),
+                            e->cu_name.empty() ? "" : _(", in "),
                             e->cu_name.c_str()));
       e->chain (er);
     }
@@ -6129,13 +6124,7 @@
sdt_uprobe_var_expanding_visitor::visit_target_symbol (target_symbol*
e)
 void
 sdt_uprobe_var_expanding_visitor::visit_atvar_op (atvar_op* e)
 {
-  if (e->module.empty() && e->cu_name.empty())
-    {
-      // process like any other local
-      // e->sym_name() will do the right thing
-      visit_target_symbol(e);
-      return;
-    }
+  need_debug_info = true;

   // Fill in our current module context if needed
   if (e->module.empty())
diff --git a/testsuite/systemtap.base/at_var_func.exp
b/testsuite/systemtap.base/at_var_func.exp
index f515bab..cf12d6e 100644
--- a/testsuite/systemtap.base/at_var_func.exp
+++ b/testsuite/systemtap.base/at_var_func.exp
@@ -16,7 +16,8 @@ set ::result_string {@var("foo", @1)->bar: 42
 @var("foo", @1)$$: {.bar=42}
 @defined(@var("foo", "badmodle")->bar): NO
 @defined(@var("foo", @3)->bar): NO
-@defined(@var("foo@blah.c", @1)->bar): NO}
+@defined(@var("foo@blah.c", @1)->bar): NO
+@var("foo")->bar: 42}

 # Only run on make installcheck and uprobes present.
 if {! [installtest_p]} { untested "$test"; return }
diff --git a/testsuite/systemtap.base/at_var_func.stp
b/testsuite/systemtap.base/at_var_func.stp
index 3da6536..5d12656 100644
--- a/testsuite/systemtap.base/at_var_func.stp
+++ b/testsuite/systemtap.base/at_var_func.stp
@@ -18,4 +18,5 @@ function f()
 probe process.function("sub")
 {
   f()
+  printf("@var(\"foo\")->bar: %d\n", @var("foo")->bar);
 }
diff --git a/testsuite/systemtap.base/at_var_mark.exp
b/testsuite/systemtap.base/at_var_mark.exp
index a3b1199..7b2074f 100644
--- a/testsuite/systemtap.base/at_var_mark.exp
+++ b/testsuite/systemtap.base/at_var_mark.exp
@@ -4,7 +4,7 @@ set testpath "$srcdir/$subdir"
 set stap_path $env(SYSTEMTAP_PATH)/stap

 # Check @var, even when var doesn't exist, works in process.mark probes.
-set output_string "pass:yes:0\r\n"
+set output_string "s = \\d+\r\n.*pass:yes:0\r\n"
 set invoke "$stap_path -e 'probe begin \{ exit() \}'"

 # Only run on make installcheck and uprobes present.
diff --git a/testsuite/systemtap.base/at_var_mark.stp
b/testsuite/systemtap.base/at_var_mark.stp
index fe47bf9..0481cb0 100644
--- a/testsuite/systemtap.base/at_var_mark.stp
+++ b/testsuite/systemtap.base/at_var_mark.stp
@@ -5,11 +5,12 @@ probe process.mark("pass*")
   if (more_addr == 0)
     {
       log("systemtap starting probe");
+      log("systemtap ending probe");
       more_addr = @var("morehelp@session.cxx");
+      printf("s = %d\n", @var("s"))
     }
   else
     {
-      log("systemtap ending probe");
       name = substr($$name, 0, 4);
       correct = @defined(@var("no_such_var_really_not")) ? "no" : "yes";
       diff = more_addr - @var("morehelp@session.cxx");
diff --git a/testsuite/systemtap.base/at_var_pie.exp
b/testsuite/systemtap.base/at_var_pie.exp
index 2cabb8f..9433571 100644
--- a/testsuite/systemtap.base/at_var_pie.exp
+++ b/testsuite/systemtap.base/at_var_pie.exp
@@ -13,7 +13,8 @@ set ::result_string {@var("foo", @1)->bar: 42
 @var("foo", @1)$$: {.bar=42}
 @defined(@var("foo", "badmodle")->bar): NO
 @defined(@var("foo", @3)->bar): NO
-@defined(@var("foo@blah.c", @1)->bar): NO}
+@defined(@var("foo@blah.c", @1)->bar): NO
+@var("foo")->bar: 42}

 # Only run on make installcheck and uprobes present.
 if {! [installtest_p]} { untested "$test"; return }
diff --git a/testsuite/systemtap.base/at_var_unresolved.exp
b/testsuite/systemtap.base/at_var_unresolved.exp
index 10db59d..8247224 100644
--- a/testsuite/systemtap.base/at_var_unresolved.exp
+++ b/testsuite/systemtap.base/at_var_unresolved.exp
@@ -13,7 +13,7 @@ set errs 0
 expect {
     -timeout 180

-    -re {^semantic error: unable to find global
'var_really_not_exist'.*? in kernel} {
+    -re {^semantic error: unable to find global
'var_really_not_exist'.*? in kernel: operator '@var'} {
         incr errs; exp_continue
     }

diff --git a/testsuite/systemtap.base/sdt.c b/testsuite/systemtap.base/sdt.c
index 39f4e88..58ea705 100644
--- a/testsuite/systemtap.base/sdt.c
+++ b/testsuite/systemtap.base/sdt.c
@@ -1,8 +1,11 @@
 #include "sys/sdt.h"

+int my_global_var = 56;
+
 static void call0(void)
 {
   STAP_PROBE(test, mark_z);
+  my_global_var++;
 }

 static void call1(int a)
diff --git a/testsuite/systemtap.base/sdt_global_var.exp
b/testsuite/systemtap.base/sdt_global_var.exp
new file mode 100644
index 0000000..e29148c
--- /dev/null
+++ b/testsuite/systemtap.base/sdt_global_var.exp
@@ -0,0 +1,122 @@
+set test "sdt_global_var"
+
+# Test accessing global variables in SDT probes.
+set result_string10 "z: @(\"my_global_var\") = 56
+z: \$my_global_var = 56
+j: @(\"my_global_var\") = 57
+j: \$my_global_var = 57"
+set result_string12 "
+l: @(\"my_global_var\") = 57
+l: \$my_global_var = 57"
+
+set extra_flags {{additional_flags=-O2} {additional_flags=-fPIE
additional_flags=-pie additional_flags=-O2}}
+set extra_mssgs {-O2}
+
+set pbtype_flags {{""} {additional_flags=-DSTAP_SDT_V2}}
+set pbtype_mssgs {{uprobe} {V2_uprobe}}
+
+proc sdt_stap_run { TEST_NAME TEST_FILE FAIL args } {
+    foreach runtime [get_runtime_list] {
+ set full_test_name $TEST_NAME
+ if {$runtime != ""} {
+            if {$runtime == "dyninst" && [regexp "\-pie" $TEST_NAME]} {
+                setup_kfail 15052 "*-*-*"
+            }
+
+    lappend full_test_name "($runtime)"
+    set cmd [concat stap --runtime=$runtime $TEST_FILE $args]
+ } elseif {[uprobes_p]} {
+            if {[regexp "\-pie" $TEST_NAME]} {
+                setup_kfail 15688 "*-*-*"
+            }
+
+    set cmd [concat stap $TEST_FILE $args]
+ } else {
+    untested $full_test_name
+    continue
+ }
+ send_log "executing: $cmd\n"
+ catch {eval exec $cmd} res
+
+ set skip 0
+ set n 0
+ set expected [split $::result_string "\n"]
+ foreach line [split $res "\n"] {
+    if {![string equal $line [lindex $expected $n]]} {
+ $FAIL "$full_test_name"
+ send_log "line [expr $n + 1]: expected \"[lindex $expected $n]\"\n"
+ send_log "Got \"$line\"\n"
+ set skip 1
+ break
+    }
+    incr n
+ }
+ if {$skip != 0} {
+    continue
+ }
+ if {[expr $n == [llength $expected]]} {
+    pass "$full_test_name"
+ } else {
+    $FAIL "$full_test_name"
+    send_log "too few lines of output, got $n, expected [llength $expected]\n"
+ }
+    }
+}
+
+# Iterate pbtype_flags
+for {set p 0} {$p < [llength $pbtype_flags]} {incr p} {
+set pbtype_flag [lindex $pbtype_flags $p]
+set pbtype_mssg [lindex $pbtype_mssgs $p]
+
+# Iterate extra_flags, trying the cases with and without PIE
+for {set i 0} {$i < [llength $extra_flags]} {incr i} {
+set extra_flag [lindex $extra_flags $i]
+set extra_mssg [lindex $extra_mssgs $i]
+set testprog "sdt.c.exe.$i"
+
+set test_flags "additional_flags=-g"
+set test_flags "$test_flags [sdt_includes]"
+set test_flags "$test_flags additional_flags=-Wall"
+set test_flags "$test_flags additional_flags=-Wextra"
+set test_flags "$test_flags additional_flags=-Werror $pbtype_flag"
+
+set saveidx 0
+
+set res [target_compile $srcdir/$subdir/sdt.c $testprog executable
"$test_flags $extra_flag"]
+if { $res != "" } {
+    verbose "target_compile failed: $res" 2
+    if {[string first "unrecognized command line option" $res] == -1} {
+      fail "compiling sdt.c $extra_mssg $pbtype_mssg"
+    } else {
+      untested "compiling sdt.c $extra_mssg $pbtype_mssg"
+    }
+    untested "$test $extra_mssg $pbtype_mssg"
+    continue
+} else {
+    pass "compiling sdt.c $extra_mssg $pbtype_mssg"
+}
+
+if {[installtest_p]} {
+    if {[regexp "^(ppc64|s390x)$" $::tcl_platform(machine)] \
+    && [regexp "^(-O)" $extra_mssg]} {
+ # register name and constant is ambiguous in ppc/s390 asm syntax
+ # thus constant folding can throw the result off
+ set FAIL xfail
+    } else {
+ set FAIL fail
+    }
+    set ::result_string $result_string10
+    if { $pbtype_mssg == "uprobe" } {
+ append ::result_string $result_string12
+    }
+    sdt_stap_run "$test $extra_mssg $pbtype_mssg $extra_flag" \
+ -w $FAIL $srcdir/$subdir/$test.stp $testprog -c ./$testprog
+} else {
+    untested "$test $extra_mssg $pbtype_mssg"
+}
+catch {exec rm -f $testprog}
+
+} ; # for {set i 0} {$i < [llength $extra_flags]}
+
+} ; # for {set i 0} {$i < [llength $pbtype_flags]}
+
diff --git a/testsuite/systemtap.base/sdt_global_var.stp
b/testsuite/systemtap.base/sdt_global_var.stp
new file mode 100644
index 0000000..c259f34
--- /dev/null
+++ b/testsuite/systemtap.base/sdt_global_var.stp
@@ -0,0 +1,17 @@
+probe process(@1).mark("mark_z")
+{
+  printf("z: @(\"my_global_var\") = %d\n", @var("my_global_var"));
+  printf("z: $my_global_var = %d\n", $my_global_var);
+}
+
+probe process(@1).mark("mark_j")
+{
+  printf("j: @(\"my_global_var\") = %d\n", @var("my_global_var"));
+  printf("j: $my_global_var = %d\n", $my_global_var);
+}
+
+probe process(@1).mark("mark_l") ?
+{
+  printf("l: @(\"my_global_var\") = %d\n", @var("my_global_var"));
+  printf("l: $my_global_var = %d\n", $my_global_var);
+}