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

[Josh Stone <jistone at redhat dot com>]

On 06/24/2013 12:51 AM, Yichun Zhang (agentzh) wrote:
> From: "Yichun Zhang (agentzh)" <agentzh@gmail.com>
> 
> The notation @var("varname@cuname", "module") is now supported and @var
> can thus effectively be used almost everywhere like the context of stap
> functions, probe timer.profile, and probe kernel.trace().
> 
> Just like @cast, multiple module names can be specified by seperating
> them with ":", for example, "module1:module2:module3". And they will be
> attempted in turn until a match is found.

*separating

> 
> Refactored the code by introducing atvar_op as suggested by Josh Stone
> to make the implementation cleaner. The fields "target_name" and
> "cu_name" have been moved from target_symbol to its subclassses atvar_op
> and/or cast_op.

*subclasses

> 
> @var now searches all the CUs that matches the "cuname" specified for the
> variable. And when "cuname" is missing, @var just searches all the CUs.
> 
> Added quite a few test cases for @var. The at_var_lvalue.exp and
> at_var_timer_profile.exp are currently failing with the dyninst runtime
> because that runtime lacks proper support for using @var as lvalue and
> probe timer.profile. Also, the at_var_pie.exp test file for testing
> accessing globals in PIE executables via @var is also failing because
> accessing globals in PIE or DSO via @var does not work for @var on git
> master either.

You can use setup_kfail right before running the part that is known to
fail.  Reference PR 15540 for at_var_timer_profile, 15052 for PIE/DSO on
dyninst.  The kernel-mode PIE/DSO should be fixed by my suggestion below
for unwind symbols and vma tracking.  The lvalue should be better with
my PR15673 fix.


> @@ -288,11 +286,18 @@ std::ostream& operator << (std::ostream& o, const target_symbol::component& c);
>  struct cast_op: public target_symbol
>  {
>    expression *operand;
> -  std::string type_name, module;
> +  std::string type_name, cu_name, module;
>    void print (std::ostream& o) const;
>    void visit (visitor* u);
>  };

I can't find anywhere that @cast needs cu_name.  You agreed on IRC, so
let's leave this one out.


>  void
> +dwarf_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;
> +    }
> +
> +  // Fill in our current module context if needed
> +  if (e->module.empty())
> +    e->module = q.dw.module_name;

Slight tweak, I think actually the module should be set before the
visit_target_symbol call too.  That way if it doesn't get resolved
locally, the atvar visitor will still know what module context to use,
where it can walk over all CUs.


> +  catch (const semantic_error& er)
> +    {
> +      // NB: we can have multiple errors, since a @var
> +      // may be attempted using several different modules
> +      // or CUs.
> +      q->e.chain (er);
> +      return DWARF_CB_OK;
> +    }

While this is usually how we handle errors, it gets crazy if there are a
lot of CUs.  For example, try:

  stap -ve 'probe begin { println(@var("foo")) }'

I think we may have to just squash this specific "unable to find" error,
and instead create a more generic error at the module level.


> +      dwarf_atvar_query q (*dw, module, *e, userspace_p, lvalue, result, tick);
> +      dw->iterate_over_modules(&query_module, &q);
> +
> +      if (result)

If we squash the CU-level errors above, here with !result is where we
can create a module-level error.

> +        {
> +          try
> +            {
> +              if (lvalue)
> +                {
> +                  // Provide the functioncall to our parent, so that it can be
> +                  // used to substitute for the assignment node immediately above
> +                  // us.
> +                  assert(!target_symbol_setter_functioncalls.empty());
> +                  *(target_symbol_setter_functioncalls.top()) = result;
> +                }
> +
> +              result->visit(this);
> +            }
> +          catch (const semantic_error& er)
> +            {
> +              e->chain (er);
> +              provide (e);
> +            }

I don't think this needs a try-catch.  The lvalue trick won't throw, and
result is only visited to reach things like array indexes we didn't
touch yet.  If any of those throw (and they shouldn't) it wouldn't make
sense to chain into e.


> @@ -10037,6 +10197,8 @@ tracepoint_builder::build(systemtap_session& s,
>    string tracepoint;
>    assert(get_param (parameters, TOK_TRACE, tracepoint));
>  
> +  s.unwindsym_modules.insert ("kernel");  // for @var
> +
>    tracepoint_query q(*dw, tracepoint, base, location, finished_results);
>    dw->iterate_over_modules(&query_module, &q);
>  }

While this may have solved your particular testcase, I believe what it
actually needs is the @var itself to insert whatever module its using.

This should solve @var in PIE and DSO too - they just need the relevant
module added to this set.  And for userspace there's also one more
requirement to enable the vma tracker.

So remove that kernel hack, and then adding this seems to work:

@@ -4277,6 +4276,10 @@ dwarf_atvar_expanding_visitor::visit_atvar_op

       if (result)
         {
+          s.unwindsym_modules.insert(module);
+          if (userspace_p && !s.runtime_usermode_p())
+            enable_vma_tracker(s);
+