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

[Josh Stone <jistone at redhat dot com>]

On 06/14/2013 07:16 PM, 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 or probe timer.profile.
> 
> 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.
> 
> Refactored the code by introducing atvar_op as suggested by Josh Stone
> to make the implementation cleaner. The field "target_name" has been
> moved from target_symbol to atvar_op but not cu_name because moving the
> latter requires too many modifications in dwflpp.

Thanks, the patch looks very good!  I have just a few comments.

Too bad about cu_name -- perhaps that's a cleanup for another day.  I do
have a few suggestions for that though.


> diff --git a/.gitignore b/.gitignore
> index 95de57b..f0d11e3 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -37,3 +37,7 @@ lib-elfutils
>  stamp-elfutils
>  dtrace
>  stappaths.7
> +testsuite/at_var
> +testsuite/at_var_func
> +testsuite/at_var_timer_profile
> +testsuite/at_var_lvalue

If these are really needed, they should go in testsuite/.gitignore.  But
generally, most of our testcases clean up after themselves.  Some tests
leave the files around in verbose mode, which is fine.


> +void
> +dwarf_atvar_query::handle_query_module ()
> +{
> +  string code;
> +  exp_type type;
> +
> +  try
> +    {
> +      get_cu_scope (&e);

It seems inflexible to me that get_cu_scope() is trying to find just one
best-matching CU, which is the only one that will get searched for the
variable.

I think it would be better to try *all* CUs that look like a match.
This should be pretty easy by pushing the code generation into a deeper
loop through dw.iterate_over_cus.

Take a look at how tracepoint_query_cu() is used, for example.  That one
doesn't care about the actual cu_name, but that's easy enough to add for
@var.

Consider also that we could allow @var("foo", "module"), having no
cu_name specified by the user, so it would iterate over them all.

> +
> +      this->dw.focus_on_cu (&(scopes->at(0)));

This is a hint to me how dwflpp can stop using e->cu_name.  With this
focus, dw.cu_name() will let dwflpp tell itself the right filename.

The other dwflpp uses I see of e->cu_name are trying to differentiate
globals vs. locals.  I think this is better served by the fact that
you're passing pc==0 for the global case.

> +
> +      if (! e.components.empty() &&
> +          e.components.back().type == target_symbol::comp_pretty_print)
> +        {
> +          dwarf_pretty_print dpp (this->dw, *scopes, 0,
> +                                  e.sym_name(),
> +                                  userspace_p, e);
> +          result = dpp.expand();
> +          return;
> +        }
> +
> +      type = pe_long;
> +      code = this->dw.literal_stmt_for_local (*scopes, 0, e.sym_name(),
> +                                              &e, lvalue, type);
> +    }
> +  catch (const semantic_error& er)
> +    {
> +      // NB: we can have multiple errors, since a @var
> +      // may be attempted using several different modules:
> +      //     @var(ptr, "type", "module1:module2:...")
> +      e.chain (er);
> +    }

Basically, this whole try-catch should move within the cu iteration.


> +void
> +tracepoint_var_expanding_visitor::visit_atvar_op (atvar_op* e)
> +{
> +  assert(e->name == "@var" && e->target_name != "");
> +  throw semantic_error(_("cannot use @var DWARF variables in tracepoints"),
> +                       e->tok);
> +}

This doesn't need to be an error anymore, as a tracepoint probe should
now be fine reading a global from a module.  So I don't think
tracepoints need any visit_atvar_op - dwarf_atvar_visitor will reach
this and do the right thing, just as for timer.profile, etc.


> +# Only run on make installcheck and uprobes present.
> +if {! [installtest_p]} { untested "$test"; return }
> +if {! [uprobes_p]} { untested "$test"; return }

First, I'm really glad to see that you've provided tests!

I think all your tests should work in --runtime=dyninst mode too.  We
don't have full module-tracking there (e.g. PR15052), but as long as the
variable is in a non-PIE executable, I think it should get absolute
addresses that will work fine.  See for instance how at_var_mark.exp
loops over the available runtimes.

Using @var on PIE and/or DSO might be another good test, to make sure
relocation-tracking works with @var-module too.


Finally, while you were pretty thorough with visitor coverage, I found a
couple that should have visit_atvar_op:

- void_statement_reducer: this is an optimizer, and visit_atvar_op can
probably just call up to visit_target_op.

- varuse_collecting_visitor: this needs to care for lvalue @var, so that
unresolved cases don't get optimized away.  I see that your
at_var_lvalue.exp works fine, because it's resolved, but consider:

> $ stap -g -e 'probe begin { @var("foo") = 1; println(pp()); exit() }'
> WARNING: Eliding side-effect-free expression : operator '@var' at <input>:1:15
>  source: probe begin { @var("foo") = 1; println(pp()); exit() }
>                        ^
> begin

That's not actually side-effect-free, and should not be elided.  This
should get a semko test to make sure it fails.


Thanks,
Josh