Re: [PATCH v1 0/3] Support perf -vv

Re: [PATCH v1 0/3] Support perf -vv

[Andi Kleen]

On Tue, Mar 27, 2018 at 12:07:01AM +0800, Jin Yao wrote:
> We keep having bug reports that when users build perf on their own,
> but they don't install some needed libraries such as libelf,
> libbfd/libibery.
> 
> The perf can build, but it is missing important functionality. And
> users may complain that perf has issue or bug. 
> 
> This patch-set support 'perf -vv' which will print the compiled-in
> status of libraries. Once users think perf missing some functionality,
> it should be very easy for them to check the libraries status.

I don't think this solves the problem. How should the user know
that they need to run perf -vv. Also normal users don't know
that libelf is needed for symbols for example.

We need a warning that is visible together with the symbols
and that clearly describes the problem.

-Andi

> 
> For example:
> 
> $ ./perf -vv
> perf version 4.13.rc5.g9b7a81b
>                  dwarf: [  on ]
>     dwarf_getlocations: [  on ]
>                  glibc: [  on ]
>                   gtk2: [  on ]
>               libaudit: [ off ]
>                 libbfd: [  on ]
>                 libelf: [  on ]
>                libnuma: [  on ]
> numa_num_possible_cpus: [  on ]
>                libperl: [  on ]
>              libpython: [  on ]
>               libslang: [  on ]
>              libcrypto: [  on ]
>              libunwind: [  on ]
>     libdw-dwarf-unwind: [  on ]
>                   zlib: [  on ]
>                   lzma: [  on ]
>              get_cpuid: [  on ]
>                    bpf: [  on ]
> 
> Jin Yao (3):
>   perf config: Add -DNO_GLIBC to CFLAGS
>   perf version: Print the status of compiled-in libraries
>   perf: Support perf -vv
> 
>  tools/perf/Makefile.config   |   2 +
>  tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/builtin.h         |   1 +
>  tools/perf/perf.c            |   6 +++
>  4 files changed, 134 insertions(+)
> 
> -- 
> 2.7.4
> 

Re: [PATCH v1 0/3] Support perf -vv

[Jiri Olsa]

On Mon, Mar 26, 2018 at 02:00:31AM -0700, Andi Kleen wrote:
> On Tue, Mar 27, 2018 at 12:07:01AM +0800, Jin Yao wrote:
> > We keep having bug reports that when users build perf on their own,
> > but they don't install some needed libraries such as libelf,
> > libbfd/libibery.
> > 
> > The perf can build, but it is missing important functionality. And
> > users may complain that perf has issue or bug. 
> > 
> > This patch-set support 'perf -vv' which will print the compiled-in
> > status of libraries. Once users think perf missing some functionality,
> > it should be very easy for them to check the libraries status.
> 
> I don't think this solves the problem. How should the user know
> that they need to run perf -vv. Also normal users don't know
> that libelf is needed for symbols for example.
> 
> We need a warning that is visible together with the symbols
> and that clearly describes the problem.

IIRC we decided to go *with* the message in the perf report
or other affected command that would suggest to run 'perf -vv'
for more details

jirka

> 
> -Andi
> 
> > 
> > For example:
> > 
> > $ ./perf -vv
> > perf version 4.13.rc5.g9b7a81b
> >                  dwarf: [  on ]
> >     dwarf_getlocations: [  on ]
> >                  glibc: [  on ]
> >                   gtk2: [  on ]
> >               libaudit: [ off ]
> >                 libbfd: [  on ]
> >                 libelf: [  on ]
> >                libnuma: [  on ]
> > numa_num_possible_cpus: [  on ]
> >                libperl: [  on ]
> >              libpython: [  on ]
> >               libslang: [  on ]
> >              libcrypto: [  on ]
> >              libunwind: [  on ]
> >     libdw-dwarf-unwind: [  on ]
> >                   zlib: [  on ]
> >                   lzma: [  on ]
> >              get_cpuid: [  on ]
> >                    bpf: [  on ]
> > 
> > Jin Yao (3):
> >   perf config: Add -DNO_GLIBC to CFLAGS
> >   perf version: Print the status of compiled-in libraries
> >   perf: Support perf -vv
> > 
> >  tools/perf/Makefile.config   |   2 +
> >  tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
> >  tools/perf/builtin.h         |   1 +
> >  tools/perf/perf.c            |   6 +++
> >  4 files changed, 134 insertions(+)
> > 
> > -- 
> > 2.7.4
> > 

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jiri Olsa]

On Tue, Mar 27, 2018 at 12:07:03AM +0800, Jin Yao wrote:
> This patch checks the values passed by CFLAGS (-DXXX) and then
> print the status of libraries.
> 
> For example, if HAVE_DWARF_SUPPORT is defined, that means the
> library "dwarf" is compiled-in. The patch will print the status
> "on" for this library.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/builtin.h         |   1 +
>  2 files changed, 126 insertions(+)
> 
> diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
> index 37019c5..90a0a7f 100644
> --- a/tools/perf/builtin-version.c
> +++ b/tools/perf/builtin-version.c
> @@ -9,3 +9,128 @@ int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
>  	printf("perf version %s\n", perf_version_string);
>  	return 0;
>  }
> +
> +static void status_print(const char *name, const char *status)
> +{
> +	printf("%22s: [ %3s ]\n", name, status);
> +}
> +
> +static void library_status(void)
> +{
> +#ifdef HAVE_DWARF_SUPPORT
> +	status_print("dwarf", "on");
> +#else
> +	status_print("dwarf", "off");
> +#endif

could this and all those below be in some generic macro?

#define STATUS(__d, __m)		\
#ifdef __d 				\
	status_print(#__m, "on");	\
#else					\
	status_print(#__m, "OFF");	\
#endif

STATUS(HAVE_DWARF_SUPPORT, dwarf)


also please print the 'OFF' and use colors as in the build message

> +
> +#ifdef HAVE_DWARF_GETLOCATIONS
> +	status_print("dwarf_getlocations", "on");
> +#else
> +	status_print("dwarf_getlocations", "off");
> +#endif
> +
> +#ifdef NO_GLIBC
> +	status_print("glibc", "off");
> +#else
> +	status_print("glibc", "on");
> +#endif
> +
> +#ifdef HAVE_GTK2_SUPPORT
> +	status_print("gtk2", "on");
> +#else
> +	status_print("gtk2", "off");
> +#endif
> +

SNIP

> +}
> +
> +int cmd_version2(int argc, const char **argv)
> +{
> +	cmd_version(argc, argv);
> +	library_status();
> +	return 0;
> +}
> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> index 05745f3..c7508ee 100644
> --- a/tools/perf/builtin.h
> +++ b/tools/perf/builtin.h
> @@ -29,6 +29,7 @@ int cmd_timechart(int argc, const char **argv);
>  int cmd_top(int argc, const char **argv);
>  int cmd_script(int argc, const char **argv);
>  int cmd_version(int argc, const char **argv);
> +int cmd_version2(int argc, const char **argv);

please don't add new command for this, handle this in cmd_version

thanks,
jirka

Re: [PATCH v1 0/3] Support perf -vv

[Jin, Yao]

On 3/26/2018 5:07 PM, Jiri Olsa wrote:
> On Mon, Mar 26, 2018 at 02:00:31AM -0700, Andi Kleen wrote:
>> On Tue, Mar 27, 2018 at 12:07:01AM +0800, Jin Yao wrote:
>>> We keep having bug reports that when users build perf on their own,
>>> but they don't install some needed libraries such as libelf,
>>> libbfd/libibery.
>>>
>>> The perf can build, but it is missing important functionality. And
>>> users may complain that perf has issue or bug.
>>>
>>> This patch-set support 'perf -vv' which will print the compiled-in
>>> status of libraries. Once users think perf missing some functionality,
>>> it should be very easy for them to check the libraries status.
>>
>> I don't think this solves the problem. How should the user know
>> that they need to run perf -vv. Also normal users don't know
>> that libelf is needed for symbols for example.
>>
>> We need a warning that is visible together with the symbols
>> and that clearly describes the problem.
> 
> IIRC we decided to go *with* the message in the perf report
> or other affected command that would suggest to run 'perf -vv'
> for more details
> 
> jirka
> 

Hi Andi,

For how the users know they should run 'perf -vv', as the discussion 
with Jiri and Arnaldo, the message will be displayed if necessary when 
users perform perf report or other perf commands. That could be 
implemented in a follow-up patch (not in this patch-set).

For let user know what the library they need, maybe we can add more 
information in 'perf -vv', for example, add something like,

"Please install libelf-dev, libelf-devel or elfutils-libelf-devel before 
building perf"

Thanks
Jin Yao

>>
>> -Andi
>>
>>>
>>> For example:
>>>
>>> $ ./perf -vv
>>> perf version 4.13.rc5.g9b7a81b
>>>                   dwarf: [  on ]
>>>      dwarf_getlocations: [  on ]
>>>                   glibc: [  on ]
>>>                    gtk2: [  on ]
>>>                libaudit: [ off ]
>>>                  libbfd: [  on ]
>>>                  libelf: [  on ]
>>>                 libnuma: [  on ]
>>> numa_num_possible_cpus: [  on ]
>>>                 libperl: [  on ]
>>>               libpython: [  on ]
>>>                libslang: [  on ]
>>>               libcrypto: [  on ]
>>>               libunwind: [  on ]
>>>      libdw-dwarf-unwind: [  on ]
>>>                    zlib: [  on ]
>>>                    lzma: [  on ]
>>>               get_cpuid: [  on ]
>>>                     bpf: [  on ]
>>>
>>> Jin Yao (3):
>>>    perf config: Add -DNO_GLIBC to CFLAGS
>>>    perf version: Print the status of compiled-in libraries
>>>    perf: Support perf -vv
>>>
>>>   tools/perf/Makefile.config   |   2 +
>>>   tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
>>>   tools/perf/builtin.h         |   1 +
>>>   tools/perf/perf.c            |   6 +++
>>>   4 files changed, 134 insertions(+)
>>>
>>> -- 
>>> 2.7.4
>>>

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jin, Yao]

On 3/26/2018 5:39 PM, Jiri Olsa wrote:
> On Tue, Mar 27, 2018 at 12:07:03AM +0800, Jin Yao wrote:
>> This patch checks the values passed by CFLAGS (-DXXX) and then
>> print the status of libraries.
>>
>> For example, if HAVE_DWARF_SUPPORT is defined, that means the
>> library "dwarf" is compiled-in. The patch will print the status
>> "on" for this library.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
>>   tools/perf/builtin.h         |   1 +
>>   2 files changed, 126 insertions(+)
>>
>> diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
>> index 37019c5..90a0a7f 100644
>> --- a/tools/perf/builtin-version.c
>> +++ b/tools/perf/builtin-version.c
>> @@ -9,3 +9,128 @@ int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
>>   	printf("perf version %s\n", perf_version_string);
>>   	return 0;
>>   }
>> +
>> +static void status_print(const char *name, const char *status)
>> +{
>> +	printf("%22s: [ %3s ]\n", name, status);
>> +}
>> +
>> +static void library_status(void)
>> +{
>> +#ifdef HAVE_DWARF_SUPPORT
>> +	status_print("dwarf", "on");
>> +#else
>> +	status_print("dwarf", "off");
>> +#endif
> 
> could this and all those below be in some generic macro?
> 
> #define STATUS(__d, __m)		\
> #ifdef __d 				\
> 	status_print(#__m, "on");	\
> #else					\
> 	status_print(#__m, "OFF");	\
> #endif
> 
> STATUS(HAVE_DWARF_SUPPORT, dwarf)
> 
> 
> also please print the 'OFF' and use colors as in the build message
> 

Fine, thanks. I will update the code.

>> +
>> +#ifdef HAVE_DWARF_GETLOCATIONS
>> +	status_print("dwarf_getlocations", "on");
>> +#else
>> +	status_print("dwarf_getlocations", "off");
>> +#endif
>> +
>> +#ifdef NO_GLIBC
>> +	status_print("glibc", "off");
>> +#else
>> +	status_print("glibc", "on");
>> +#endif
>> +
>> +#ifdef HAVE_GTK2_SUPPORT
>> +	status_print("gtk2", "on");
>> +#else
>> +	status_print("gtk2", "off");
>> +#endif
>> +
> 
> SNIP
> 
>> +}
>> +
>> +int cmd_version2(int argc, const char **argv)
>> +{
>> +	cmd_version(argc, argv);
>> +	library_status();
>> +	return 0;
>> +}
>> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
>> index 05745f3..c7508ee 100644
>> --- a/tools/perf/builtin.h
>> +++ b/tools/perf/builtin.h
>> @@ -29,6 +29,7 @@ int cmd_timechart(int argc, const char **argv);
>>   int cmd_top(int argc, const char **argv);
>>   int cmd_script(int argc, const char **argv);
>>   int cmd_version(int argc, const char **argv);
>> +int cmd_version2(int argc, const char **argv);
> 
> please don't add new command for this, handle this in cmd_version
>

Yeah, I know that, adding a new command is not a good idea.

But how can I pass a new parameter to cmd_version to indicate it's the 
-vv case? Use argv[1]? But looks not good since argc is 1.

Any idea about that?

Thanks
Jin Yao

> thanks,
> jirka
> 

[PATCH v1 0/3] Support perf -vv

[Jin Yao]

We keep having bug reports that when users build perf on their own,
but they don't install some needed libraries such as libelf,
libbfd/libibery.

The perf can build, but it is missing important functionality. And
users may complain that perf has issue or bug. 

This patch-set support 'perf -vv' which will print the compiled-in
status of libraries. Once users think perf missing some functionality,
it should be very easy for them to check the libraries status.

For example:

$ ./perf -vv
perf version 4.13.rc5.g9b7a81b
                 dwarf: [  on ]
    dwarf_getlocations: [  on ]
                 glibc: [  on ]
                  gtk2: [  on ]
              libaudit: [ off ]
                libbfd: [  on ]
                libelf: [  on ]
               libnuma: [  on ]
numa_num_possible_cpus: [  on ]
               libperl: [  on ]
             libpython: [  on ]
              libslang: [  on ]
             libcrypto: [  on ]
             libunwind: [  on ]
    libdw-dwarf-unwind: [  on ]
                  zlib: [  on ]
                  lzma: [  on ]
             get_cpuid: [  on ]
                   bpf: [  on ]

Jin Yao (3):
  perf config: Add -DNO_GLIBC to CFLAGS
  perf version: Print the status of compiled-in libraries
  perf: Support perf -vv

 tools/perf/Makefile.config   |   2 +
 tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
 tools/perf/builtin.h         |   1 +
 tools/perf/perf.c            |   6 +++
 4 files changed, 134 insertions(+)

-- 
2.7.4

[PATCH v1 1/3] perf config: Add -DNO_GLIBC to CFLAGS

[Jin Yao]

For most of libraries, in perf.config, they can be recorded with
-DHAVE_XXX or -DNO_XXX in CFLAGS according to if they are compiled-in.
Then C code could know if the library is compiled-in or not.

While for glibc, no existing -DHAVE_XXX or -DNO_XXX.

This patch adds -DNO_GLIBC to CFLAGS.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/Makefile.config | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index 98ff736..5883dd6 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -324,6 +324,8 @@ else
       NO_LIBBPF := 1
       NO_JVMTI := 1
     else
+      CFLAGS += -DNO_GLIBC
+
       ifneq ($(filter s% -static%,$(LDFLAGS),),)
         msg := $(error No static glibc found, please install glibc-static);
       else
-- 
2.7.4

[PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jin Yao]

This patch checks the values passed by CFLAGS (-DXXX) and then
print the status of libraries.

For example, if HAVE_DWARF_SUPPORT is defined, that means the
library "dwarf" is compiled-in. The patch will print the status
"on" for this library.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
 tools/perf/builtin.h         |   1 +
 2 files changed, 126 insertions(+)

diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
index 37019c5..90a0a7f 100644
--- a/tools/perf/builtin-version.c
+++ b/tools/perf/builtin-version.c
@@ -9,3 +9,128 @@ int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
 	printf("perf version %s\n", perf_version_string);
 	return 0;
 }
+
+static void status_print(const char *name, const char *status)
+{
+	printf("%22s: [ %3s ]\n", name, status);
+}
+
+static void library_status(void)
+{
+#ifdef HAVE_DWARF_SUPPORT
+	status_print("dwarf", "on");
+#else
+	status_print("dwarf", "off");
+#endif
+
+#ifdef HAVE_DWARF_GETLOCATIONS
+	status_print("dwarf_getlocations", "on");
+#else
+	status_print("dwarf_getlocations", "off");
+#endif
+
+#ifdef NO_GLIBC
+	status_print("glibc", "off");
+#else
+	status_print("glibc", "on");
+#endif
+
+#ifdef HAVE_GTK2_SUPPORT
+	status_print("gtk2", "on");
+#else
+	status_print("gtk2", "off");
+#endif
+
+#ifdef HAVE_LIBAUDIT_SUPPORT
+	status_print("libaudit", "on");
+#else
+	status_print("libaudit", "off");
+#endif
+
+#ifdef HAVE_LIBBFD_SUPPORT
+	status_print("libbfd", "on");
+#else
+	status_print("libbfd", "off");
+#endif
+
+#ifdef HAVE_LIBELF_SUPPORT
+	status_print("libelf", "on");
+#else
+	status_print("libelf", "off");
+#endif
+
+#ifdef HAVE_LIBNUMA_SUPPORT
+	status_print("libnuma", "on");
+	status_print("numa_num_possible_cpus", "on");
+#else
+	status_print("libnuma", "off");
+	status_print("numa_num_possible_cpus", "off");
+#endif
+
+#ifdef NO_LIBPERL
+	status_print("libperl", "off");
+#else
+	status_print("libperl", "on");
+#endif
+
+#ifdef NO_LIBPYTHON
+	status_print("libpython", "off");
+#else
+	status_print("libpython", "on");
+#endif
+
+#ifdef HAVE_SLANG_SUPPORT
+	status_print("libslang", "on");
+#else
+	status_print("libslang", "off");
+#endif
+
+#ifdef HAVE_LIBCRYPTO_SUPPORT
+	status_print("libcrypto", "on");
+#else
+	status_print("libcrypto", "off");
+#endif
+
+#ifdef HAVE_LIBUNWIND_SUPPORT
+	status_print("libunwind", "on");
+#else
+	status_print("libunwind", "off");
+#endif
+
+#ifdef HAVE_DWARF_SUPPORT
+	status_print("libdw-dwarf-unwind", "on");
+#else
+	status_print("libdw-dwarf-unwind", "off");
+#endif
+
+#ifdef HAVE_ZLIB_SUPPORT
+	status_print("zlib", "on");
+#else
+	status_print("zlib", "off");
+#endif
+
+#ifdef HAVE_LZMA_SUPPORT
+	status_print("lzma", "on");
+#else
+	status_print("lzma", "off");
+#endif
+
+#ifdef HAVE_AUXTRACE_SUPPORT
+	status_print("get_cpuid", "on");
+#else
+	status_print("get_cpuid", "off");
+#endif
+
+#ifdef HAVE_LIBBPF_SUPPORT
+	status_print("bpf", "on");
+#else
+	status_print("bpf", "off");
+#endif
+}
+
+int cmd_version2(int argc, const char **argv)
+{
+	cmd_version(argc, argv);
+	library_status();
+	return 0;
+}
diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
index 05745f3..c7508ee 100644
--- a/tools/perf/builtin.h
+++ b/tools/perf/builtin.h
@@ -29,6 +29,7 @@ int cmd_timechart(int argc, const char **argv);
 int cmd_top(int argc, const char **argv);
 int cmd_script(int argc, const char **argv);
 int cmd_version(int argc, const char **argv);
+int cmd_version2(int argc, const char **argv);
 int cmd_probe(int argc, const char **argv);
 int cmd_kmem(int argc, const char **argv);
 int cmd_lock(int argc, const char **argv);
-- 
2.7.4

[PATCH v1 3/3] perf: Support perf -vv

[Jin Yao]

We keep having bug reports that when users build perf on their own,
but they don't install some needed libraries such as libelf,
libbfd/libibery.

The perf can build, but it is missing important functionality.

This patch provides a new option '-vv' which will print the
compiled-in status of libraries.

For example:

$ ./perf -vv
perf version 4.13.rc5.g9b7a81b
                 dwarf: [  on ]
    dwarf_getlocations: [  on ]
                 glibc: [  on ]
                  gtk2: [  on ]
              libaudit: [ off ]
                libbfd: [  on ]
                libelf: [  on ]
               libnuma: [  on ]
numa_num_possible_cpus: [  on ]
               libperl: [  on ]
             libpython: [  on ]
              libslang: [  on ]
             libcrypto: [  on ]
             libunwind: [  on ]
    libdw-dwarf-unwind: [  on ]
                  zlib: [  on ]
                  lzma: [  on ]
             get_cpuid: [  on ]
                   bpf: [  on ]

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/perf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 1b3fc8e..300c83d 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -64,6 +64,7 @@ static struct cmd_struct commands[] = {
 	{ "top",	cmd_top,	0 },
 	{ "annotate",	cmd_annotate,	0 },
 	{ "version",	cmd_version,	0 },
+	{ "version2",	cmd_version2,	0 },
 	{ "script",	cmd_script,	0 },
 	{ "sched",	cmd_sched,	0 },
 #ifdef HAVE_LIBELF_SUPPORT
@@ -190,6 +191,11 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			break;
 		}
 
+		if (!strcmp(cmd, "-vv")) {
+			(*argv)[0] = "--version2";
+			break;
+		}
+
 		/*
 		 * Check remaining flags.
 		 */
-- 
2.7.4

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jin, Yao]

On 3/26/2018 5:39 PM, Jiri Olsa wrote:
> On Tue, Mar 27, 2018 at 12:07:03AM +0800, Jin Yao wrote:
>> This patch checks the values passed by CFLAGS (-DXXX) and then
>> print the status of libraries.
>>
>> For example, if HAVE_DWARF_SUPPORT is defined, that means the
>> library "dwarf" is compiled-in. The patch will print the status
>> "on" for this library.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
>>   tools/perf/builtin.h         |   1 +
>>   2 files changed, 126 insertions(+)
>>
>> diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
>> index 37019c5..90a0a7f 100644
>> --- a/tools/perf/builtin-version.c
>> +++ b/tools/perf/builtin-version.c
>> @@ -9,3 +9,128 @@ int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
>>   	printf("perf version %s\n", perf_version_string);
>>   	return 0;
>>   }
>> +
>> +static void status_print(const char *name, const char *status)
>> +{
>> +	printf("%22s: [ %3s ]\n", name, status);
>> +}
>> +
>> +static void library_status(void)
>> +{
>> +#ifdef HAVE_DWARF_SUPPORT
>> +	status_print("dwarf", "on");
>> +#else
>> +	status_print("dwarf", "off");
>> +#endif
> 
> could this and all those below be in some generic macro?
> 
> #define STATUS(__d, __m)		\
> #ifdef __d 				\
> 	status_print(#__m, "on");	\
> #else					\
> 	status_print(#__m, "OFF");	\
> #endif
> 
> STATUS(HAVE_DWARF_SUPPORT, dwarf)
> 
> 

Hi Jiri,

I have tried this macro definition, but unfortunately the compilation is 
failed.

error: '#' is not followed by a macro parameter
  #define STATUS(__d, __m)  \

I just guess the '#' in #ifdef confuses the gcc. Looks we can't define 
#ifdef/#endif block in a macro.

Thanks
Jin Yao

> also please print the 'OFF' and use colors as in the build message
> 
>> +
>> +#ifdef HAVE_DWARF_GETLOCATIONS
>> +	status_print("dwarf_getlocations", "on");
>> +#else
>> +	status_print("dwarf_getlocations", "off");
>> +#endif
>> +
>> +#ifdef NO_GLIBC
>> +	status_print("glibc", "off");
>> +#else
>> +	status_print("glibc", "on");
>> +#endif
>> +
>> +#ifdef HAVE_GTK2_SUPPORT
>> +	status_print("gtk2", "on");
>> +#else
>> +	status_print("gtk2", "off");
>> +#endif
>> +
> 
> SNIP
> 
>> +}
>> +
>> +int cmd_version2(int argc, const char **argv)
>> +{
>> +	cmd_version(argc, argv);
>> +	library_status();
>> +	return 0;
>> +}
>> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
>> index 05745f3..c7508ee 100644
>> --- a/tools/perf/builtin.h
>> +++ b/tools/perf/builtin.h
>> @@ -29,6 +29,7 @@ int cmd_timechart(int argc, const char **argv);
>>   int cmd_top(int argc, const char **argv);
>>   int cmd_script(int argc, const char **argv);
>>   int cmd_version(int argc, const char **argv);
>> +int cmd_version2(int argc, const char **argv);
> 
> please don't add new command for this, handle this in cmd_version
> 
> thanks,
> jirka
> 

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jin, Yao]

On 3/26/2018 9:51 PM, Jin, Yao wrote:
> 
> 
> On 3/26/2018 5:39 PM, Jiri Olsa wrote:
>> On Tue, Mar 27, 2018 at 12:07:03AM +0800, Jin Yao wrote:
>>> This patch checks the values passed by CFLAGS (-DXXX) and then
>>> print the status of libraries.
>>>
>>> For example, if HAVE_DWARF_SUPPORT is defined, that means the
>>> library "dwarf" is compiled-in. The patch will print the status
>>> "on" for this library.
>>>
>>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>>> ---
>>>   tools/perf/builtin-version.c | 125 
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>   tools/perf/builtin.h         |   1 +
>>>   2 files changed, 126 insertions(+)
>>>
>>> diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
>>> index 37019c5..90a0a7f 100644
>>> --- a/tools/perf/builtin-version.c
>>> +++ b/tools/perf/builtin-version.c
>>> @@ -9,3 +9,128 @@ int cmd_version(int argc __maybe_unused, const char 
>>> **argv __maybe_unused)
>>>       printf("perf version %s\n", perf_version_string);
>>>       return 0;
>>>   }
>>> +
>>> +static void status_print(const char *name, const char *status)
>>> +{
>>> +    printf("%22s: [ %3s ]\n", name, status);
>>> +}
>>> +
>>> +static void library_status(void)
>>> +{
>>> +#ifdef HAVE_DWARF_SUPPORT
>>> +    status_print("dwarf", "on");
>>> +#else
>>> +    status_print("dwarf", "off");
>>> +#endif
>>
>> could this and all those below be in some generic macro?
>>
>> #define STATUS(__d, __m)        \
>> #ifdef __d                 \
>>     status_print(#__m, "on");    \
>> #else                    \
>>     status_print(#__m, "OFF");    \
>> #endif
>>
>> STATUS(HAVE_DWARF_SUPPORT, dwarf)
>>
>>
>> also please print the 'OFF' and use colors as in the build message
>>
> 
> Fine, thanks. I will update the code.
> 
>>> +
>>> +#ifdef HAVE_DWARF_GETLOCATIONS
>>> +    status_print("dwarf_getlocations", "on");
>>> +#else
>>> +    status_print("dwarf_getlocations", "off");
>>> +#endif
>>> +
>>> +#ifdef NO_GLIBC
>>> +    status_print("glibc", "off");
>>> +#else
>>> +    status_print("glibc", "on");
>>> +#endif
>>> +
>>> +#ifdef HAVE_GTK2_SUPPORT
>>> +    status_print("gtk2", "on");
>>> +#else
>>> +    status_print("gtk2", "off");
>>> +#endif
>>> +
>>
>> SNIP
>>
>>> +}
>>> +
>>> +int cmd_version2(int argc, const char **argv)
>>> +{
>>> +    cmd_version(argc, argv);
>>> +    library_status();
>>> +    return 0;
>>> +}
>>> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
>>> index 05745f3..c7508ee 100644
>>> --- a/tools/perf/builtin.h
>>> +++ b/tools/perf/builtin.h
>>> @@ -29,6 +29,7 @@ int cmd_timechart(int argc, const char **argv);
>>>   int cmd_top(int argc, const char **argv);
>>>   int cmd_script(int argc, const char **argv);
>>>   int cmd_version(int argc, const char **argv);
>>> +int cmd_version2(int argc, const char **argv);
>>
>> please don't add new command for this, handle this in cmd_version
>>
> 
> Yeah, I know that, adding a new command is not a good idea.
> 
> But how can I pass a new parameter to cmd_version to indicate it's the 
> -vv case? Use argv[1]? But looks not good since argc is 1.
> 
> Any idea about that?
> 
> Thanks
> Jin Yao
> 

Hi Jiri,

Maybe could we just add the information to 'perf -v'? I mean we don't 
need the 'perf -vv', directly add the information to 'perf -v'. Is it OK?

Thanks
Jin Yao

>> thanks,
>> jirka
>>

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Ingo Molnar]

* Jin Yao <yao.jin@linux.intel.com> wrote:

> +#ifdef HAVE_DWARF_SUPPORT
> +#ifdef HAVE_DWARF_GETLOCATIONS
> +#ifdef NO_GLIBC
> +#ifdef HAVE_GTK2_SUPPORT
> +#ifdef HAVE_LIBAUDIT_SUPPORT
> +#ifdef HAVE_LIBBFD_SUPPORT
> +#ifdef HAVE_LIBELF_SUPPORT
> +#ifdef HAVE_LIBNUMA_SUPPORT
> +#ifdef NO_LIBPERL
> +#ifdef NO_LIBPYTHON
> +#ifdef HAVE_SLANG_SUPPORT
> +#ifdef HAVE_LIBCRYPTO_SUPPORT
> +#ifdef HAVE_LIBUNWIND_SUPPORT
> +#ifdef HAVE_DWARF_SUPPORT
> +#ifdef HAVE_ZLIB_SUPPORT
> +#ifdef HAVE_LZMA_SUPPORT
> +#ifdef HAVE_AUXTRACE_SUPPORT
> +#ifdef HAVE_LIBBPF_SUPPORT

BTW., it would be nice at this point to fix those 3 outliers that have a negation 
in their library support status macro:

> +#ifdef NO_GLIBC
> +#ifdef NO_LIBPERL
> +#ifdef NO_LIBPYTHON

... and invert them back to the HAVE_* side of the logic:

> +#ifdef HAVE_GLIBC
> +#ifdef HAVE_LIBPERL
> +#ifdef HAVE_LIBPYTHON

That should make all related code more consistent and more readable.

Thanks,

	Ingo

Re: [PATCH v1 3/3] perf: Support perf -vv

[Ingo Molnar]

* Jin Yao <yao.jin@linux.intel.com> wrote:

> +++ b/tools/perf/perf.c
> @@ -64,6 +64,7 @@ static struct cmd_struct commands[] = {
>  	{ "top",	cmd_top,	0 },
>  	{ "annotate",	cmd_annotate,	0 },
>  	{ "version",	cmd_version,	0 },
> +	{ "version2",	cmd_version2,	0 },
>  	{ "script",	cmd_script,	0 },
>  	{ "sched",	cmd_sched,	0 },
>  #ifdef HAVE_LIBELF_SUPPORT
> @@ -190,6 +191,11 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			break;
>  		}
>  
> +		if (!strcmp(cmd, "-vv")) {
> +			(*argv)[0] = "--version2";
> +			break;
> +		}

That is just lazy hackery, please use the regular option library to add options 
and document them!

Also, '--version2' is a sloppy name, the right solution is to do what Git does, it 
has a --build-options sub-option to 'perf version':

  triton:~/tip> git version -h
  usage: git version [<options>]

      --build-options       also print build options


  triton:~/tip> git version --build-options
  git version 2.14.1
  sizeof-long: 8

This should be done for perf as well, and _then_ maybe can we add a helper alias 
that maps '-vv' to 'version --build-options'.

Thanks,

	Ingo

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jin, Yao]

On 3/27/2018 1:58 PM, Ingo Molnar wrote:
> 
> * Jin Yao <yao.jin@linux.intel.com> wrote:
> 
>> +#ifdef HAVE_DWARF_SUPPORT
>> +#ifdef HAVE_DWARF_GETLOCATIONS
>> +#ifdef NO_GLIBC
>> +#ifdef HAVE_GTK2_SUPPORT
>> +#ifdef HAVE_LIBAUDIT_SUPPORT
>> +#ifdef HAVE_LIBBFD_SUPPORT
>> +#ifdef HAVE_LIBELF_SUPPORT
>> +#ifdef HAVE_LIBNUMA_SUPPORT
>> +#ifdef NO_LIBPERL
>> +#ifdef NO_LIBPYTHON
>> +#ifdef HAVE_SLANG_SUPPORT
>> +#ifdef HAVE_LIBCRYPTO_SUPPORT
>> +#ifdef HAVE_LIBUNWIND_SUPPORT
>> +#ifdef HAVE_DWARF_SUPPORT
>> +#ifdef HAVE_ZLIB_SUPPORT
>> +#ifdef HAVE_LZMA_SUPPORT
>> +#ifdef HAVE_AUXTRACE_SUPPORT
>> +#ifdef HAVE_LIBBPF_SUPPORT
> 
> BTW., it would be nice at this point to fix those 3 outliers that have a negation
> in their library support status macro:
> 
>> +#ifdef NO_GLIBC
>> +#ifdef NO_LIBPERL
>> +#ifdef NO_LIBPYTHON
> 
> ... and invert them back to the HAVE_* side of the logic:
> 
>> +#ifdef HAVE_GLIBC
>> +#ifdef HAVE_LIBPERL
>> +#ifdef HAVE_LIBPYTHON
> 
> That should make all related code more consistent and more readable.
> 
> Thanks,
> 
> 	Ingo
> 

Yes, it's better, thanks!
I will try to convert them to the HAVE_*.

Thanks
Jin Yao

Re: [PATCH v1 3/3] perf: Support perf -vv

[Jin, Yao]

On 3/27/2018 2:03 PM, Ingo Molnar wrote:
> 
> * Jin Yao <yao.jin@linux.intel.com> wrote:
> 
>> +++ b/tools/perf/perf.c
>> @@ -64,6 +64,7 @@ static struct cmd_struct commands[] = {
>>   	{ "top",	cmd_top,	0 },
>>   	{ "annotate",	cmd_annotate,	0 },
>>   	{ "version",	cmd_version,	0 },
>> +	{ "version2",	cmd_version2,	0 },
>>   	{ "script",	cmd_script,	0 },
>>   	{ "sched",	cmd_sched,	0 },
>>   #ifdef HAVE_LIBELF_SUPPORT
>> @@ -190,6 +191,11 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>>   			break;
>>   		}
>>   
>> +		if (!strcmp(cmd, "-vv")) {
>> +			(*argv)[0] = "--version2";
>> +			break;
>> +		}
> 
> That is just lazy hackery, please use the regular option library to add options
> and document them!
> 
> Also, '--version2' is a sloppy name, the right solution is to do what Git does, it
> has a --build-options sub-option to 'perf version':
> 
>    triton:~/tip> git version -h
>    usage: git version [<options>]
> 
>        --build-options       also print build options
> 
> 
>    triton:~/tip> git version --build-options
>    git version 2.14.1
>    sizeof-long: 8
> 
> This should be done for perf as well, and _then_ maybe can we add a helper alias
> that maps '-vv' to 'version --build-options'.
> 
> Thanks,
> 
> 	Ingo
> 

Something like 'perf version --build-options' and that will be mapped to 
'-vv'.

It's a good idea, thanks!

Thanks
Jin Yao

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jiri Olsa]

On Mon, Mar 26, 2018 at 09:51:03PM +0800, Jin, Yao wrote:
> 
> 
> On 3/26/2018 5:39 PM, Jiri Olsa wrote:
> > On Tue, Mar 27, 2018 at 12:07:03AM +0800, Jin Yao wrote:
> > > This patch checks the values passed by CFLAGS (-DXXX) and then
> > > print the status of libraries.
> > > 
> > > For example, if HAVE_DWARF_SUPPORT is defined, that means the
> > > library "dwarf" is compiled-in. The patch will print the status
> > > "on" for this library.
> > > 
> > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> > > ---
> > >   tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
> > >   tools/perf/builtin.h         |   1 +
> > >   2 files changed, 126 insertions(+)
> > > 
> > > diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
> > > index 37019c5..90a0a7f 100644
> > > --- a/tools/perf/builtin-version.c
> > > +++ b/tools/perf/builtin-version.c
> > > @@ -9,3 +9,128 @@ int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
> > >   	printf("perf version %s\n", perf_version_string);
> > >   	return 0;
> > >   }
> > > +
> > > +static void status_print(const char *name, const char *status)
> > > +{
> > > +	printf("%22s: [ %3s ]\n", name, status);
> > > +}
> > > +
> > > +static void library_status(void)
> > > +{
> > > +#ifdef HAVE_DWARF_SUPPORT
> > > +	status_print("dwarf", "on");
> > > +#else
> > > +	status_print("dwarf", "off");
> > > +#endif
> > 
> > could this and all those below be in some generic macro?
> > 
> > #define STATUS(__d, __m)		\
> > #ifdef __d 				\
> > 	status_print(#__m, "on");	\
> > #else					\
> > 	status_print(#__m, "OFF");	\
> > #endif
> > 
> > STATUS(HAVE_DWARF_SUPPORT, dwarf)
> > 
> > 
> > also please print the 'OFF' and use colors as in the build message
> > 
> 
> Fine, thanks. I will update the code.
> 
> > > +
> > > +#ifdef HAVE_DWARF_GETLOCATIONS
> > > +	status_print("dwarf_getlocations", "on");
> > > +#else
> > > +	status_print("dwarf_getlocations", "off");
> > > +#endif
> > > +
> > > +#ifdef NO_GLIBC
> > > +	status_print("glibc", "off");
> > > +#else
> > > +	status_print("glibc", "on");
> > > +#endif
> > > +
> > > +#ifdef HAVE_GTK2_SUPPORT
> > > +	status_print("gtk2", "on");
> > > +#else
> > > +	status_print("gtk2", "off");
> > > +#endif
> > > +
> > 
> > SNIP
> > 
> > > +}
> > > +
> > > +int cmd_version2(int argc, const char **argv)
> > > +{
> > > +	cmd_version(argc, argv);
> > > +	library_status();
> > > +	return 0;
> > > +}
> > > diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> > > index 05745f3..c7508ee 100644
> > > --- a/tools/perf/builtin.h
> > > +++ b/tools/perf/builtin.h
> > > @@ -29,6 +29,7 @@ int cmd_timechart(int argc, const char **argv);
> > >   int cmd_top(int argc, const char **argv);
> > >   int cmd_script(int argc, const char **argv);
> > >   int cmd_version(int argc, const char **argv);
> > > +int cmd_version2(int argc, const char **argv);
> > 
> > please don't add new command for this, handle this in cmd_version
> > 
> 
> Yeah, I know that, adding a new command is not a good idea.
> 
> But how can I pass a new parameter to cmd_version to indicate it's the -vv
> case? Use argv[1]? But looks not good since argc is 1.
> 
> Any idea about that?

I guess the based would be to have 'perf version -v', where
in cmd_version you'd call standard option processing..

the 'perf -vv' shortcut could for example be done through
some global var like in attached patch... untested

jirka


---
diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
index 37019c5d675f..1fe458792cc1 100644
--- a/tools/perf/builtin-version.c
+++ b/tools/perf/builtin-version.c
@@ -4,6 +4,8 @@
 #include <linux/compiler.h>
 #include <stdio.h>
 
+int version_verbose;
+
 int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
 {
 	printf("perf version %s\n", perf_version_string);
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 1b3fc8ec0fa2..a5c95c6c0de7 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -185,7 +185,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			break;
 		}
 
-		if (!strcmp(cmd, "-v")) {
+		if (strstarts(cmd, "-v")) {
+			int i;
+
+			for (i = 2; cmd[i]; i++) {
+				if (cmd[i] == 'v')
+					version_verbose++;
+			}
+
 			(*argv)[0] = "--version";
 			break;
 		}
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 8fec1abd0f1f..9c45ba179635 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -13,6 +13,8 @@ void test_attr__init(void);
 void test_attr__open(struct perf_event_attr *attr, pid_t pid, int cpu,
 		     int fd, int group_fd, unsigned long flags);
 
+extern int version_verbose;
+
 #define HAVE_ATTR_TEST
 #include "perf-sys.h"
 

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jiri Olsa]

On Tue, Mar 27, 2018 at 09:44:23AM +0800, Jin, Yao wrote:
> 
> 
> On 3/26/2018 5:39 PM, Jiri Olsa wrote:
> > On Tue, Mar 27, 2018 at 12:07:03AM +0800, Jin Yao wrote:
> > > This patch checks the values passed by CFLAGS (-DXXX) and then
> > > print the status of libraries.
> > > 
> > > For example, if HAVE_DWARF_SUPPORT is defined, that means the
> > > library "dwarf" is compiled-in. The patch will print the status
> > > "on" for this library.
> > > 
> > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> > > ---
> > >   tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
> > >   tools/perf/builtin.h         |   1 +
> > >   2 files changed, 126 insertions(+)
> > > 
> > > diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
> > > index 37019c5..90a0a7f 100644
> > > --- a/tools/perf/builtin-version.c
> > > +++ b/tools/perf/builtin-version.c
> > > @@ -9,3 +9,128 @@ int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
> > >   	printf("perf version %s\n", perf_version_string);
> > >   	return 0;
> > >   }
> > > +
> > > +static void status_print(const char *name, const char *status)
> > > +{
> > > +	printf("%22s: [ %3s ]\n", name, status);
> > > +}
> > > +
> > > +static void library_status(void)
> > > +{
> > > +#ifdef HAVE_DWARF_SUPPORT
> > > +	status_print("dwarf", "on");
> > > +#else
> > > +	status_print("dwarf", "off");
> > > +#endif
> > 
> > could this and all those below be in some generic macro?
> > 
> > #define STATUS(__d, __m)		\
> > #ifdef __d 				\
> > 	status_print(#__m, "on");	\
> > #else					\
> > 	status_print(#__m, "OFF");	\
> > #endif
> > 
> > STATUS(HAVE_DWARF_SUPPORT, dwarf)
> > 
> > 
> 
> Hi Jiri,
> 
> I have tried this macro definition, but unfortunately the compilation is
> failed.
> 
> error: '#' is not followed by a macro parameter
>  #define STATUS(__d, __m)  \
> 
> I just guess the '#' in #ifdef confuses the gcc. Looks we can't define
> #ifdef/#endif block in a macro.
> 

ah crap.. right ;-) how about we take the IS_BUILTIN thingie
from include/linux/kconfig.h and use it as in attached test
change.. it gives me:

	[jolsa@krava perf]$ ./perf version
	perf version 4.16.rc6.g1ee9a60
			 dwarf: [  on ]
			 krava: [ OFF ]

we could put those macros into tools/include/tools/config.h, Arnaldo?
together with the comments from kconfig.h that I cut out..

thanks,
jirka


---
diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
index 1fe458792cc1..b392f9e80fd0 100644
--- a/tools/perf/builtin-version.c
+++ b/tools/perf/builtin-version.c
@@ -6,8 +6,33 @@
 
 int version_verbose;
 
+
+#define __ARG_PLACEHOLDER_1 0,
+#define __take_second_arg(__ignored, val, ...) val
+
+#define __is_defined(x)                 ___is_defined(x)
+#define ___is_defined(val)              ____is_defined(__ARG_PLACEHOLDER_##val) 
+#define ____is_defined(arg1_or_junk)    __take_second_arg(arg1_or_junk 1, 0)
+
+#define IS_BUILTIN(option) __is_defined(option)
+
+
+static void status_print(const char *name, const char *status)
+{
+	printf("%22s: [ %3s ]\n", name, status);
+}
+
 int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
 {
 	printf("perf version %s\n", perf_version_string);
+
+#define STATUS(__d, __m)			\
+	if (IS_BUILTIN(__d))			\
+		status_print(#__m, "on");	\
+	else					\
+		status_print(#__m, "OFF");
+
+	STATUS(HAVE_DWARF_SUPPORT, dwarf)
+	STATUS(HAVE_KRAVA_SUPPORT, krava)
 	return 0;
 }

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jin, Yao]

On 3/27/2018 8:56 PM, Jiri Olsa wrote:
> On Tue, Mar 27, 2018 at 09:44:23AM +0800, Jin, Yao wrote:
>>
>>
>> On 3/26/2018 5:39 PM, Jiri Olsa wrote:
>>> On Tue, Mar 27, 2018 at 12:07:03AM +0800, Jin Yao wrote:
>>>> This patch checks the values passed by CFLAGS (-DXXX) and then
>>>> print the status of libraries.
>>>>
>>>> For example, if HAVE_DWARF_SUPPORT is defined, that means the
>>>> library "dwarf" is compiled-in. The patch will print the status
>>>> "on" for this library.
>>>>
>>>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>>>> ---
>>>>    tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
>>>>    tools/perf/builtin.h         |   1 +
>>>>    2 files changed, 126 insertions(+)
>>>>
>>>> diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
>>>> index 37019c5..90a0a7f 100644
>>>> --- a/tools/perf/builtin-version.c
>>>> +++ b/tools/perf/builtin-version.c
>>>> @@ -9,3 +9,128 @@ int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
>>>>    	printf("perf version %s\n", perf_version_string);
>>>>    	return 0;
>>>>    }
>>>> +
>>>> +static void status_print(const char *name, const char *status)
>>>> +{
>>>> +	printf("%22s: [ %3s ]\n", name, status);
>>>> +}
>>>> +
>>>> +static void library_status(void)
>>>> +{
>>>> +#ifdef HAVE_DWARF_SUPPORT
>>>> +	status_print("dwarf", "on");
>>>> +#else
>>>> +	status_print("dwarf", "off");
>>>> +#endif
>>>
>>> could this and all those below be in some generic macro?
>>>
>>> #define STATUS(__d, __m)		\
>>> #ifdef __d 				\
>>> 	status_print(#__m, "on");	\
>>> #else					\
>>> 	status_print(#__m, "OFF");	\
>>> #endif
>>>
>>> STATUS(HAVE_DWARF_SUPPORT, dwarf)
>>>
>>>
>>
>> Hi Jiri,
>>
>> I have tried this macro definition, but unfortunately the compilation is
>> failed.
>>
>> error: '#' is not followed by a macro parameter
>>   #define STATUS(__d, __m)  \
>>
>> I just guess the '#' in #ifdef confuses the gcc. Looks we can't define
>> #ifdef/#endif block in a macro.
>>
> 
> ah crap.. right ;-) how about we take the IS_BUILTIN thingie
> from include/linux/kconfig.h and use it as in attached test
> change.. it gives me:
> 
> 	[jolsa@krava perf]$ ./perf version
> 	perf version 4.16.rc6.g1ee9a60
> 			 dwarf: [  on ]
> 			 krava: [ OFF ]
> 
> we could put those macros into tools/include/tools/config.h, Arnaldo?
> together with the comments from kconfig.h that I cut out..
> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
> index 1fe458792cc1..b392f9e80fd0 100644
> --- a/tools/perf/builtin-version.c
> +++ b/tools/perf/builtin-version.c
> @@ -6,8 +6,33 @@
>   
>   int version_verbose;
>   
> +
> +#define __ARG_PLACEHOLDER_1 0,
> +#define __take_second_arg(__ignored, val, ...) val
> +
> +#define __is_defined(x)                 ___is_defined(x)
> +#define ___is_defined(val)              ____is_defined(__ARG_PLACEHOLDER_##val)
> +#define ____is_defined(arg1_or_junk)    __take_second_arg(arg1_or_junk 1, 0)
> +
> +#define IS_BUILTIN(option) __is_defined(option)
> +
> +
> +static void status_print(const char *name, const char *status)
> +{
> +	printf("%22s: [ %3s ]\n", name, status);
> +}
> +
>   int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
>   {
>   	printf("perf version %s\n", perf_version_string);
> +
> +#define STATUS(__d, __m)			\
> +	if (IS_BUILTIN(__d))			\
> +		status_print(#__m, "on");	\
> +	else					\
> +		status_print(#__m, "OFF");
> +
> +	STATUS(HAVE_DWARF_SUPPORT, dwarf)
> +	STATUS(HAVE_KRAVA_SUPPORT, krava)
>   	return 0;
>   }
> 

Hi Jiri,

It looks we need to copy the IS_BUILTIN and associated macro definitions 
to a common header file.

I guess you will post this patch if Arnaldo agree with this idea, right?

I plan to post v2 of this patch series after this 'IS_BUILTIN' patch 
gets merged.

Thanks
Jin Yao

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jin, Yao]

On 3/27/2018 8:38 PM, Jiri Olsa wrote:
> On Mon, Mar 26, 2018 at 09:51:03PM +0800, Jin, Yao wrote:
>>
>>
>> On 3/26/2018 5:39 PM, Jiri Olsa wrote:
>>> On Tue, Mar 27, 2018 at 12:07:03AM +0800, Jin Yao wrote:
>>>> This patch checks the values passed by CFLAGS (-DXXX) and then
>>>> print the status of libraries.
>>>>
>>>> For example, if HAVE_DWARF_SUPPORT is defined, that means the
>>>> library "dwarf" is compiled-in. The patch will print the status
>>>> "on" for this library.
>>>>
>>>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>>>> ---
>>>>    tools/perf/builtin-version.c | 125 +++++++++++++++++++++++++++++++++++++++++++
>>>>    tools/perf/builtin.h         |   1 +
>>>>    2 files changed, 126 insertions(+)
>>>>
>>>> diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
>>>> index 37019c5..90a0a7f 100644
>>>> --- a/tools/perf/builtin-version.c
>>>> +++ b/tools/perf/builtin-version.c
>>>> @@ -9,3 +9,128 @@ int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
>>>>    	printf("perf version %s\n", perf_version_string);
>>>>    	return 0;
>>>>    }
>>>> +
>>>> +static void status_print(const char *name, const char *status)
>>>> +{
>>>> +	printf("%22s: [ %3s ]\n", name, status);
>>>> +}
>>>> +
>>>> +static void library_status(void)
>>>> +{
>>>> +#ifdef HAVE_DWARF_SUPPORT
>>>> +	status_print("dwarf", "on");
>>>> +#else
>>>> +	status_print("dwarf", "off");
>>>> +#endif
>>>
>>> could this and all those below be in some generic macro?
>>>
>>> #define STATUS(__d, __m)		\
>>> #ifdef __d 				\
>>> 	status_print(#__m, "on");	\
>>> #else					\
>>> 	status_print(#__m, "OFF");	\
>>> #endif
>>>
>>> STATUS(HAVE_DWARF_SUPPORT, dwarf)
>>>
>>>
>>> also please print the 'OFF' and use colors as in the build message
>>>
>>
>> Fine, thanks. I will update the code.
>>
>>>> +
>>>> +#ifdef HAVE_DWARF_GETLOCATIONS
>>>> +	status_print("dwarf_getlocations", "on");
>>>> +#else
>>>> +	status_print("dwarf_getlocations", "off");
>>>> +#endif
>>>> +
>>>> +#ifdef NO_GLIBC
>>>> +	status_print("glibc", "off");
>>>> +#else
>>>> +	status_print("glibc", "on");
>>>> +#endif
>>>> +
>>>> +#ifdef HAVE_GTK2_SUPPORT
>>>> +	status_print("gtk2", "on");
>>>> +#else
>>>> +	status_print("gtk2", "off");
>>>> +#endif
>>>> +
>>>
>>> SNIP
>>>
>>>> +}
>>>> +
>>>> +int cmd_version2(int argc, const char **argv)
>>>> +{
>>>> +	cmd_version(argc, argv);
>>>> +	library_status();
>>>> +	return 0;
>>>> +}
>>>> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
>>>> index 05745f3..c7508ee 100644
>>>> --- a/tools/perf/builtin.h
>>>> +++ b/tools/perf/builtin.h
>>>> @@ -29,6 +29,7 @@ int cmd_timechart(int argc, const char **argv);
>>>>    int cmd_top(int argc, const char **argv);
>>>>    int cmd_script(int argc, const char **argv);
>>>>    int cmd_version(int argc, const char **argv);
>>>> +int cmd_version2(int argc, const char **argv);
>>>
>>> please don't add new command for this, handle this in cmd_version
>>>
>>
>> Yeah, I know that, adding a new command is not a good idea.
>>
>> But how can I pass a new parameter to cmd_version to indicate it's the -vv
>> case? Use argv[1]? But looks not good since argc is 1.
>>
>> Any idea about that?
> 
> I guess the based would be to have 'perf version -v', where
> in cmd_version you'd call standard option processing..
> 
> the 'perf -vv' shortcut could for example be done through
> some global var like in attached patch... untested
> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-version.c b/tools/perf/builtin-version.c
> index 37019c5d675f..1fe458792cc1 100644
> --- a/tools/perf/builtin-version.c
> +++ b/tools/perf/builtin-version.c
> @@ -4,6 +4,8 @@
>   #include <linux/compiler.h>
>   #include <stdio.h>
>   
> +int version_verbose;
> +
>   int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
>   {
>   	printf("perf version %s\n", perf_version_string);
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index 1b3fc8ec0fa2..a5c95c6c0de7 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -185,7 +185,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>   			break;
>   		}
>   
> -		if (!strcmp(cmd, "-v")) {
> +		if (strstarts(cmd, "-v")) {
> +			int i;
> +
> +			for (i = 2; cmd[i]; i++) {
> +				if (cmd[i] == 'v')
> +					version_verbose++;
> +			}
> +
>   			(*argv)[0] = "--version";
>   			break;
>   		}
> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> index 8fec1abd0f1f..9c45ba179635 100644
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -13,6 +13,8 @@ void test_attr__init(void);
>   void test_attr__open(struct perf_event_attr *attr, pid_t pid, int cpu,
>   		     int fd, int group_fd, unsigned long flags);
>   
> +extern int version_verbose;
> +
>   #define HAVE_ATTR_TEST
>   #include "perf-sys.h"
>   
> 

Using global variable, hmm, that's fine, looks no better way. :)

I will follow Ingo's suggestion. Use 'perf -v --build-options' and map 
it to 'perf -vv'.

Thanks
Jin Yao

Re: [PATCH v1 2/3] perf version: Print the status of compiled-in libraries

[Jiri Olsa]

On Tue, Mar 27, 2018 at 09:17:57PM +0800, Jin, Yao wrote:

SNIP

> > +#define IS_BUILTIN(option) __is_defined(option)
> > +
> > +
> > +static void status_print(const char *name, const char *status)
> > +{
> > +	printf("%22s: [ %3s ]\n", name, status);
> > +}
> > +
> >   int cmd_version(int argc __maybe_unused, const char **argv __maybe_unused)
> >   {
> >   	printf("perf version %s\n", perf_version_string);
> > +
> > +#define STATUS(__d, __m)			\
> > +	if (IS_BUILTIN(__d))			\
> > +		status_print(#__m, "on");	\
> > +	else					\
> > +		status_print(#__m, "OFF");
> > +
> > +	STATUS(HAVE_DWARF_SUPPORT, dwarf)
> > +	STATUS(HAVE_KRAVA_SUPPORT, krava)
> >   	return 0;
> >   }
> > 
> 
> Hi Jiri,
> 
> It looks we need to copy the IS_BUILTIN and associated macro definitions to
> a common header file.
> 
> I guess you will post this patch if Arnaldo agree with this idea, right?
> 
> I plan to post v2 of this patch series after this 'IS_BUILTIN' patch gets
> merged.


please post patch below with your patchset
and feel free to change it if needed

thanks,
jirka


---
>From bdd36e208bde1576bba54679734174c9a2064822 Mon Sep 17 00:00:00 2001
From: Jiri Olsa <jolsa@kernel.org>
Date: Tue, 27 Mar 2018 15:30:26 +0200
Subject: [PATCH] tools include: Add config.h header file

Adding IS_BUILTIN macro and its dependencies into
tools world.

It's taken from kernel's include/linux/kconfig.h,
which can't be taken completely due to its kconfig
dependencies.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/include/tools/config.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 tools/include/tools/config.h

diff --git a/tools/include/tools/config.h b/tools/include/tools/config.h
new file mode 100644
index 000000000000..08ade7df8132
--- /dev/null
+++ b/tools/include/tools/config.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _TOOLS_CONFIG_H
+#define _TOOLS_CONFIG_H
+
+/* Subset of include/linux/kconfig.h */
+
+#define __ARG_PLACEHOLDER_1 0,
+#define __take_second_arg(__ignored, val, ...) val
+
+/*
+ * Helper macros to use CONFIG_ options in C/CPP expressions. Note that
+ * these only work with boolean and tristate options.
+ */
+
+/*
+ * Getting something that works in C and CPP for an arg that may or may
+ * not be defined is tricky.  Here, if we have "#define CONFIG_BOOGER 1"
+ * we match on the placeholder define, insert the "0," for arg1 and generate
+ * the triplet (0, 1, 0).  Then the last step cherry picks the 2nd arg (a one).
+ * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and when
+ * the last step cherry picks the 2nd arg, we get a zero.
+ */
+#define __is_defined(x)			___is_defined(x)
+#define ___is_defined(val)		____is_defined(__ARG_PLACEHOLDER_##val)
+#define ____is_defined(arg1_or_junk)	__take_second_arg(arg1_or_junk 1, 0)
+
+/*
+ * IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0
+ * otherwise. For boolean options, this is equivalent to
+ * IS_ENABLED(CONFIG_FOO).
+ */
+#define IS_BUILTIN(option) __is_defined(option)
+
+#endif /* _TOOLS_CONFIG_H */
-- 
2.13.6