Re: [PATCH v11 1/4] perf check: introduce check subcommand

[Aditya Gupta <adityag@linux.ibm.com>]

Hi Arnaldo,


On 29/06/24 00:58, Arnaldo Carvalho de Melo wrote:
> On Fri, Jun 28, 2024 at 11:28:14AM -0700, Namhyung Kim wrote:
>> On Fri, Jun 28, 2024 at 11:24:53AM -0300, Arnaldo Carvalho de Melo wrote:
>>
>>> And while looking at it:
>>>
>>>               get_cpuid: [ on  ]  # HAVE_AUXTRACE_SUPPORT
>>>
>>> This looks wrong, no? Or at least confusing, have to check the source
>>> code...
>> We have this in Makefile.config
>>
>> ifndef NO_AUXTRACE
>>    ifeq ($(SRCARCH),x86)
>>      ifeq ($(feature-get_cpuid), 0)
>>        $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
>>        NO_AUXTRACE := 1
>>      endif
>>    endif
> The complete sequence is:
>
> ifndef NO_AUXTRACE
>    ifeq ($(SRCARCH),x86)
>      ifeq ($(feature-get_cpuid), 0)
>        $(warning Your gcc lacks the __get_cpuid() builtin, disables support for auxtrace/Intel PT, please install a newer gcc)
>        NO_AUXTRACE := 1
>      endif
>    endif
>    ifndef NO_AUXTRACE
>      $(call detected,CONFIG_AUXTRACE)
>      CFLAGS += -DHAVE_AUXTRACE_SUPPORT
>      ifeq ($(feature-reallocarray), 0)
>        CFLAGS += -DCOMPAT_NEED_REALLOCARRAY
>      endif
>    endif
> endif
>
> The most descriptive would be to HAVE_GET_CPUID_SUPPORT and have it used
> in the source code.
>
> That or have:
>
> diff --git a/tools/perf/builtin-check.c b/tools/perf/builtin-check.c
> index 44ffde6f8dbe51f3..ae4a686ff4f265be 100644
> --- a/tools/perf/builtin-check.c
> +++ b/tools/perf/builtin-check.c
> @@ -33,7 +33,7 @@ struct feature_status supported_features[] = {
>   	FEATURE_STATUS("dwarf", HAVE_DWARF_SUPPORT),
>   	FEATURE_STATUS("dwarf_getlocations", HAVE_DWARF_GETLOCATIONS_SUPPORT),
>   	FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
> -	FEATURE_STATUS("get_cpuid", HAVE_AUXTRACE_SUPPORT),
> +	FEATURE_STATUS("auxtrace", HAVE_AUXTRACE_SUPPORT),
>   	FEATURE_STATUS("libaudit", HAVE_LIBAUDIT_SUPPORT),
>   	FEATURE_STATUS("libbfd", HAVE_LIBBFD_SUPPORT),
>   	FEATURE_STATUS("libcapstone", HAVE_LIBCAPSTONE_SUPPORT),


Looks better. Went through all instances of 'HAVE_AUXTRACE_SUPPORT', 
it's mostly been used to conditionally define perf_*_auxtrace functions.

No one seems to depend on the feature 'name' 'get_cpuid'.

Any comments ?

>
> That:
>
>          FEATURE_STATUS("dwarf-unwind-support", HAVE_DWARF_UNWIND_SUPPORT),
>
> Should also really be:
>
>          FEATURE_STATUS("dwarf-unwind", HAVE_DWARF_UNWIND_SUPPORT),

Will do it in v13.


Thanks,

Aditya Gupta

> For consistency, the get_cpuid/auxtrace also for consistency, I think.
>
> - Arnaldo