Re: Accessing user-space global variables in timer.profile?

[Josh Stone <jistone@redhat.com>]

On 05/29/2013 09:00 PM, agentzh wrote:
> Hello!
> 
> On Mon, Apr 22, 2013 at 6:23 PM, Josh Stone wrote:
>> So we'll need a @var which at least hints which module holds it, as
>> described in http://sourceware.org/bugzilla/show_bug.cgi?id=11096#c5
>>
>> That's talking about functions, but timer.profile is similarly context-free.
>>
> 
> Okay, I've come up with a (naive) patch to implement
> @var("somevar@somefile", "somemodule") for stap user functions and
> context-free probe handlers (see below). It works for me on Fedora 17,
> both in a function or in timer.profile.
> 
> There's some minor things that I'm not very sure though:
> 
> 1. It may not be a good idea to expand such @var in
> dwarf_cast_expanding_visitor. Maybe I should add another ad-hoc
> visitor like dwarf_var_early_expanding_visitor?

Yeah, it will be small, just as dwarf_cast_expanding_visitor is small,
but let's keep them separate.  The session::code_filters is made for
this, even though @cast is the only one there so far.

I'm not sure what's "early" about this though.  I do see we have an
unfortunate collision against existing dwarf_var_expanding_visitor --
maybe call this as atvar, e.g. dwarf_atvar_expanding_visitor and
dwarf_atvar_query.  (FWIW even I don't think "atvar" is a *great* name,
but "var" is just too general.)

> 2. There's some code duplication between this early @var expansion and
> the existing dwarf_var_expanding_visitor. Maybe we could merge them
> two or just abstract out some common logic?

I think that could be helped a little by deferring the current @var
expansion into your new visitor.  This would be much like how the
dwarf_var_expanding_visitor::visit_cast_op() only sets the module name,
so dwarf_cast_expanding_visitor has the right context later.

> Because I'm very new to the systemtap code base, I'd highly appreciate
> any suggestions or other comments!

On the note of deferring as @cast does, I also think that @var should
really be its own target_symbol subclass, rather than being a weird
special-case flavor of target_symbol as it is now.  I know this is not
your invention, but "target_name", "cu_name", and now "module" are all
specific to @var needs.  It's a bigger change to make a new atvar_op, as
many tree visitors will need to be expanded, but I think it may be
cleaner overall.

...
> +void
> +dwarf_var_query::getscopes(target_symbol *e)

I can see this is copied from dwarf_var_expanding_visitor::getcuscope.
If we let dwarf_var_expanding_visitor defer @var processing until your
visitor, then yours becomes the only copy needed.  (Or equivalently,
just rename the existing function into your visitor class

...
> +void
> +dwarf_cast_expanding_visitor::visit_target_symbol(target_symbol* e)
> +{

I see this similar to dwarf_cast_expanding_visitor::visit_cast_op.  So
first it could be dwarf_atvar_expanding_visitor::visit_atvar_op as I
said.  The duplication here is not too bad, but a few tweaks:

> +  //cerr << "accessing target_symbol " << e->sym_name() << endl;
> +
> +  if (is_active_lvalue(e)
> +      || e->name != "@var"
> +      || e->target_name.empty()
> +      || e->module.empty()
> +      || (!e->components.empty()
> +          && e->components.back().type == target_symbol::comp_pretty_print))
> +    {
> +      provide(e);
> +      return;
> +    }

I'm not sure why you're skipping lvalues - does the existing @var
prevent writes?  And why punt on pretty_print?

Also, module.empty() might as well default to "kernel", the same way
that cast_op does.

> +
> +  functioncall* result = NULL;
> +
> +  // split the module string by ':' for alternatives
> +  vector<string> modules;
> +  tokenize(e->module, modules, ":");
> +  bool userspace_p = false;
> +  for (unsigned i = 0; !result && i < modules.size(); ++i)
> +    {
> +      string& module = modules[i];
> +      filter_special_modules(module);

The "special" modules don't apply here.  This is where @cast actually
compiles new modules from headers to get type debuginfo.  That doesn't
make sense for @var, as we'd never ever see that module loaded to get
access to any of its variables.