Re: [PATCH v4] tools/perf: Allow user probes on versioned symbols

[Masami Hiramatsu <mhiramat@kernel.org>]

On Mon, 10 Apr 2017 09:53:48 -0500
Paul Clarke <pc@us.ibm.com> wrote:
> >> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> >> index 5245d2f..bde2100 100644
> >> --- a/tools/perf/util/symbol.h
> >> +++ b/tools/perf/util/symbol.h
> >> @@ -348,8 +348,17 @@ void arch__sym_update(struct symbol *s, GElf_Sym *sym);
> >>   #define SYMBOL_A 0
> >>   #define SYMBOL_B 1
> >>
> >> +int arch__compare_symbol_names(const char *namea, const char *nameb);
> >> +int arch__compare_symbol_names_n(const char *namea, const char *nameb,
> >> +				 unsigned int n);
> >>   int arch__choose_best_symbol(struct symbol *syma, struct symbol *symb);
> >>
> >> +#define SYMBOLS__TAG_INCLUDE_NONE 0
> >> +#define SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY 1
> >
> > Hmm, even if you like to pass an obvious flag, these should be
> > defined by enum, and please choose simpler and clear flag name.
> >
> > E.g.
> > enum {
> > 	SYMBOLS__COMP_WITH_TAG,
> > 	SYMBOLS__COMP_WITHOUT_TAG,
> > };
> 
> Earlier in this thread, we discussed supporting non-default tagged symbols (those tagged with "@version").  To allow for that, I was thinking of these three cases:
> - SYMBOLS__TAG_INCLUDE_NONE: ignore all tagged symbols (the current behavior)
> - SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY:  match tagged symbols when tagged as default ("@@"; added with this patch)
> - SYMBOLS__TAG_INCLUDE_ALL:  match all tagged symbols ("@" and "@@"; future)

Ah, I see now.

> For the names of the enums, I chose "SYMBOLS__TAG..." to put it first in the SYMBOLS namespace, and second in a "TAG" sub-namespace.

For the namespace, we should use same namespase as enum's name.

> I'm happy to fix the discrepancy with signed/unsigned by changing to enum.
> 
> Let me know what enum names are preferred, given the above discussion, and I'll post a new patch.

OK, then what about enum symbol_tag and use "SYMBOL_TAG__" for namespace?

Thank you,



-- 
Masami Hiramatsu <mhiramat@kernel.org>