This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH v3 1/1] PR11096: Add support for the "module" argument to @var
- From: Josh Stone <jistone at redhat dot com>
- To: "Yichun Zhang (agentzh)" <agentzh at gmail dot com>
- Cc: systemtap at sourceware dot org
- Date: Wed, 26 Jun 2013 15:40:27 -0700
- Subject: Re: [PATCH v3 1/1] PR11096: Add support for the "module" argument to @var
- References: <51C8EF57 dot 2050801 at redhat dot com> <1372225343-3618-1-git-send-email-agentzh at gmail dot com> <1372225343-3618-2-git-send-email-agentzh at gmail dot com>
I have just two small comments, which you can probably just fix without
further review, then I think this is ready to push!
> 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;
Add a similar e->module update in sdt_uprobe_var_expanding_visitor.
That's superfluous when an sdt module does have debuginfo, since it will
end up in this dwarf visitor eventually. But SDT can technically get
resolved without debuginfo, and we don't want any @var from this context
to default to "kernel" in that case. (It will still fail without
debuginfo, but it should fail with a correct message.)
> + /* 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",
> + e->sym_name().c_str(), module.c_str(),
> + e->cu_name.empty() ? "" : _("in"),
> + e->cu_name.c_str()));
This message looks awkward for the empty-cu case, e.g.
[...]unable to find global 'foo' in kernel, : operator '@var'[...]
I suggest changing "%s, %s %s" to "%s%s%s", and "in" to ", in ".
Thanks,
Josh