[PATCH 1/2] perf build: Autodetect minimum required llvm-dev version

[PATCH 1/2] perf build: Autodetect minimum required llvm-dev version

[James Clark]

The new LLVM addr2line feature requires a minimum version of 13 to
compile. Add a feature check for the version so that NO_LLVM=1 doesn't
need to be explicitly added. Leave the existing llvm feature check
intact because it's used by tools other than Perf.

This fixes the following compilation error when the llvm-dev version
doesn't match:

  util/llvm-c-helpers.cpp: In function 'char* llvm_name_for_code(dso*, const char*, u64)':
  util/llvm-c-helpers.cpp:178:21: error: 'std::remove_reference_t<llvm::DILineInfo>' {aka 'struct llvm::DILineInfo'} has no member named 'StartAddress'
    178 |   addr, res_or_err->StartAddress ? *res_or_err->StartAddress : 0);

Fixes: c3f8644c21df ("perf report: Support LLVM for addr2line()")
Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/build/Makefile.feature           |  2 +-
 tools/build/feature/Makefile           |  9 +++++++++
 tools/build/feature/test-llvm-perf.cpp | 14 ++++++++++++++
 tools/perf/Makefile.config             |  6 +++---
 4 files changed, 27 insertions(+), 4 deletions(-)
 create mode 100644 tools/build/feature/test-llvm-perf.cpp

diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 0717e96d6a0e..427a9389e26c 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -136,7 +136,7 @@ FEATURE_DISPLAY ?=              \
          libunwind              \
          libdw-dwarf-unwind     \
          libcapstone            \
-         llvm                   \
+         llvm-perf              \
          zlib                   \
          lzma                   \
          get_cpuid              \
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index 12796808f07a..d6a98b3854f8 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -73,6 +73,7 @@ FILES=                                          \
          test-libopencsd.bin			\
          test-clang.bin				\
          test-llvm.bin				\
+         test-llvm-perf.bin   \
          test-llvm-version.bin			\
          test-libaio.bin			\
          test-libzstd.bin			\
@@ -388,6 +389,14 @@ $(OUTPUT)test-llvm.bin:
 		$(shell $(LLVM_CONFIG) --system-libs)		\
 		> $(@:.bin=.make.output) 2>&1
 
+$(OUTPUT)test-llvm-perf.bin:
+	$(BUILDXX) -std=gnu++17 				\
+		-I$(shell $(LLVM_CONFIG) --includedir) 		\
+		-L$(shell $(LLVM_CONFIG) --libdir)		\
+		$(shell $(LLVM_CONFIG) --libs Core BPF)		\
+		$(shell $(LLVM_CONFIG) --system-libs)		\
+		> $(@:.bin=.make.output) 2>&1
+
 $(OUTPUT)test-llvm-version.bin:
 	$(BUILDXX) -std=gnu++17					\
 		-I$(shell $(LLVM_CONFIG) --includedir)		\
diff --git a/tools/build/feature/test-llvm-perf.cpp b/tools/build/feature/test-llvm-perf.cpp
new file mode 100644
index 000000000000..a8cbb67e335e
--- /dev/null
+++ b/tools/build/feature/test-llvm-perf.cpp
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/raw_ostream.h"
+
+#if LLVM_VERSION_MAJOR < 13
+# error "Perf requires llvm-devel/llvm-dev version 13 or greater"
+#endif
+
+int main()
+{
+	llvm::errs() << "Hello World!\n";
+	llvm::llvm_shutdown();
+	return 0;
+}
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index 7888c932b1b4..37e3eee2986e 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -981,8 +981,8 @@ ifdef BUILD_NONDISTRO
 endif
 
 ifndef NO_LIBLLVM
-  $(call feature_check,llvm)
-  ifeq ($(feature-llvm), 1)
+  $(call feature_check,llvm-perf)
+  ifeq ($(feature-llvm-perf), 1)
     CFLAGS += -DHAVE_LIBLLVM_SUPPORT
     CFLAGS += $(shell $(LLVM_CONFIG) --cflags)
     CXXFLAGS += -DHAVE_LIBLLVM_SUPPORT
@@ -992,7 +992,7 @@ ifndef NO_LIBLLVM
     EXTLIBS += -lstdc++
     $(call detected,CONFIG_LIBLLVM)
   else
-    $(warning No libllvm found, slower source file resolution, please install llvm-devel/llvm-dev)
+    $(warning No libllvm 13+ found, slower source file resolution, please install llvm-devel/llvm-dev)
     NO_LIBLLVM := 1
   endif
 endif
-- 
2.34.1

[PATCH 2/2] perf build: Remove unused feature test target

[James Clark]

llvm-version was removed in commit 56b11a2126bf ("perf bpf: Remove
support for embedding clang for compiling BPF events (-e foo.c)") but
some parts were left in the Makefile so finish removing them.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/build/feature/Makefile | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index d6a98b3854f8..5938cf799dc6 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -74,7 +74,6 @@ FILES=                                          \
          test-clang.bin				\
          test-llvm.bin				\
          test-llvm-perf.bin   \
-         test-llvm-version.bin			\
          test-libaio.bin			\
          test-libzstd.bin			\
          test-clang-bpf-co-re.bin		\
@@ -397,11 +396,6 @@ $(OUTPUT)test-llvm-perf.bin:
 		$(shell $(LLVM_CONFIG) --system-libs)		\
 		> $(@:.bin=.make.output) 2>&1
 
-$(OUTPUT)test-llvm-version.bin:
-	$(BUILDXX) -std=gnu++17					\
-		-I$(shell $(LLVM_CONFIG) --includedir)		\
-		> $(@:.bin=.make.output) 2>&1
-
 $(OUTPUT)test-clang.bin:
 	$(BUILDXX) -std=gnu++17					\
 		-I$(shell $(LLVM_CONFIG) --includedir) 		\
-- 
2.34.1

Re: [PATCH 1/2] perf build: Autodetect minimum required llvm-dev version

[Quentin Monnet]

2024-09-10 15:04 UTC+0100 ~ James Clark <james.clark@linaro.org>
> The new LLVM addr2line feature requires a minimum version of 13 to
> compile. Add a feature check for the version so that NO_LLVM=1 doesn't
> need to be explicitly added. Leave the existing llvm feature check
> intact because it's used by tools other than Perf.
> 
> This fixes the following compilation error when the llvm-dev version
> doesn't match:
> 
>    util/llvm-c-helpers.cpp: In function 'char* llvm_name_for_code(dso*, const char*, u64)':
>    util/llvm-c-helpers.cpp:178:21: error: 'std::remove_reference_t<llvm::DILineInfo>' {aka 'struct llvm::DILineInfo'} has no member named 'StartAddress'
>      178 |   addr, res_or_err->StartAddress ? *res_or_err->StartAddress : 0);
> 
> Fixes: c3f8644c21df ("perf report: Support LLVM for addr2line()")
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>   tools/build/Makefile.feature           |  2 +-
>   tools/build/feature/Makefile           |  9 +++++++++
>   tools/build/feature/test-llvm-perf.cpp | 14 ++++++++++++++
>   tools/perf/Makefile.config             |  6 +++---
>   4 files changed, 27 insertions(+), 4 deletions(-)
>   create mode 100644 tools/build/feature/test-llvm-perf.cpp
> 
> diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
> index 0717e96d6a0e..427a9389e26c 100644
> --- a/tools/build/Makefile.feature
> +++ b/tools/build/Makefile.feature
> @@ -136,7 +136,7 @@ FEATURE_DISPLAY ?=              \
>            libunwind              \
>            libdw-dwarf-unwind     \
>            libcapstone            \
> -         llvm                   \
> +         llvm-perf              \

Hi! Just a quick question, why remove "llvm" from the list, here?

Quentin

Re: [PATCH 1/2] perf build: Autodetect minimum required llvm-dev version

[Arnaldo Carvalho de Melo]

On Tue, Sep 10, 2024 at 03:04:00PM +0100, James Clark wrote:
> The new LLVM addr2line feature requires a minimum version of 13 to
> compile. Add a feature check for the version so that NO_LLVM=1 doesn't
> need to be explicitly added. Leave the existing llvm feature check
> intact because it's used by tools other than Perf.
> 
> This fixes the following compilation error when the llvm-dev version
> doesn't match:
> 
>   util/llvm-c-helpers.cpp: In function 'char* llvm_name_for_code(dso*, const char*, u64)':
>   util/llvm-c-helpers.cpp:178:21: error: 'std::remove_reference_t<llvm::DILineInfo>' {aka 'struct llvm::DILineInfo'} has no member named 'StartAddress'
>     178 |   addr, res_or_err->StartAddress ? *res_or_err->StartAddress : 0);
> 
> Fixes: c3f8644c21df ("perf report: Support LLVM for addr2line()")
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  tools/build/Makefile.feature           |  2 +-
>  tools/build/feature/Makefile           |  9 +++++++++
>  tools/build/feature/test-llvm-perf.cpp | 14 ++++++++++++++
>  tools/perf/Makefile.config             |  6 +++---
>  4 files changed, 27 insertions(+), 4 deletions(-)
>  create mode 100644 tools/build/feature/test-llvm-perf.cpp
> 
> diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
> index 0717e96d6a0e..427a9389e26c 100644
> --- a/tools/build/Makefile.feature
> +++ b/tools/build/Makefile.feature
> @@ -136,7 +136,7 @@ FEATURE_DISPLAY ?=              \
>           libunwind              \
>           libdw-dwarf-unwind     \
>           libcapstone            \
> -         llvm                   \
> +         llvm-perf              \
>           zlib                   \
>           lzma                   \
>           get_cpuid              \

There was one leftover on the other patch, I added it here:

⬢[acme@toolbox perf-tools-next]$ git diff
diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 427a9389e26cd203..ffd117135094cc68 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -100,7 +100,6 @@ FEATURE_TESTS_EXTRA :=                  \
          libunwind-debug-frame-aarch64  \
          cxx                            \
          llvm                           \
-         llvm-version                   \
          clang                          \
          libbpf                         \
          libbpf-btf__load_from_kernel_by_id \
⬢[acme@toolbox perf-tools-next]$

Re: [PATCH 1/2] perf build: Autodetect minimum required llvm-dev version

[Arnaldo Carvalho de Melo]

On Tue, Sep 10, 2024 at 03:27:16PM +0100, Quentin Monnet wrote:
> 2024-09-10 15:04 UTC+0100 ~ James Clark <james.clark@linaro.org>
> > The new LLVM addr2line feature requires a minimum version of 13 to
> > compile. Add a feature check for the version so that NO_LLVM=1 doesn't
> > need to be explicitly added. Leave the existing llvm feature check
> > intact because it's used by tools other than Perf.
> > 
> > This fixes the following compilation error when the llvm-dev version
> > doesn't match:
> > 
> >    util/llvm-c-helpers.cpp: In function 'char* llvm_name_for_code(dso*, const char*, u64)':
> >    util/llvm-c-helpers.cpp:178:21: error: 'std::remove_reference_t<llvm::DILineInfo>' {aka 'struct llvm::DILineInfo'} has no member named 'StartAddress'
> >      178 |   addr, res_or_err->StartAddress ? *res_or_err->StartAddress : 0);
> > 
> > Fixes: c3f8644c21df ("perf report: Support LLVM for addr2line()")
> > Signed-off-by: James Clark <james.clark@linaro.org>
> > ---
> >   tools/build/Makefile.feature           |  2 +-
> >   tools/build/feature/Makefile           |  9 +++++++++
> >   tools/build/feature/test-llvm-perf.cpp | 14 ++++++++++++++
> >   tools/perf/Makefile.config             |  6 +++---
> >   4 files changed, 27 insertions(+), 4 deletions(-)
> >   create mode 100644 tools/build/feature/test-llvm-perf.cpp
> > 
> > diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
> > index 0717e96d6a0e..427a9389e26c 100644
> > --- a/tools/build/Makefile.feature
> > +++ b/tools/build/Makefile.feature
> > @@ -136,7 +136,7 @@ FEATURE_DISPLAY ?=              \
> >            libunwind              \
> >            libdw-dwarf-unwind     \
> >            libcapstone            \
> > -         llvm                   \
> > +         llvm-perf              \
 
> Hi! Just a quick question, why remove "llvm" from the list, here?

Right, having it here is still interesting, if not for perf, for some
other tool in tools/ that uses this:

⬢[acme@toolbox perf-tools-next]$ cat tools/build/feature/test-llvm.cpp
// SPDX-License-Identifier: GPL-2.0
#include "llvm/Support/ManagedStatic.h"
#include "llvm/Support/raw_ostream.h"
#define NUM_VERSION (((LLVM_VERSION_MAJOR) << 16) + (LLVM_VERSION_MINOR << 8) + LLVM_VERSION_PATCH)

#if NUM_VERSION < 0x030900
# error "LLVM version too low"
#endif
int main()
{
	llvm::errs() << "Hello World!\n";
	llvm::llvm_shutdown();
	return 0;
}
⬢[acme@toolbox perf-tools-next]$

My understanding about James intention is that for perf we need at least
llvm-dev 13, but he kept the other feature test for other projects.

From Quentin, since this is in tools/build/Makefile.feature, so not perf
specific, maybe it should be somewhere else?

But keeping both in FEATURE_DISPLAY at tools/build/Makefile.feature
may be confusing?

- Arnaldo

Re: [PATCH 1/2] perf build: Autodetect minimum required llvm-dev version

[James Clark]

On 9/10/24 15:27, Quentin Monnet wrote:
> 2024-09-10 15:04 UTC+0100 ~ James Clark <james.clark@linaro.org>
>> The new LLVM addr2line feature requires a minimum version of 13 to
>> compile. Add a feature check for the version so that NO_LLVM=1 doesn't
>> need to be explicitly added. Leave the existing llvm feature check
>> intact because it's used by tools other than Perf.
>>
>> This fixes the following compilation error when the llvm-dev version
>> doesn't match:
>>
>>    util/llvm-c-helpers.cpp: In function 'char* 
>> llvm_name_for_code(dso*, const char*, u64)':
>>    util/llvm-c-helpers.cpp:178:21: error: 
>> 'std::remove_reference_t<llvm::DILineInfo>' {aka 'struct 
>> llvm::DILineInfo'} has no member named 'StartAddress'
>>      178 |   addr, res_or_err->StartAddress ? 
>> *res_or_err->StartAddress : 0);
>>
>> Fixes: c3f8644c21df ("perf report: Support LLVM for addr2line()")
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   tools/build/Makefile.feature           |  2 +-
>>   tools/build/feature/Makefile           |  9 +++++++++
>>   tools/build/feature/test-llvm-perf.cpp | 14 ++++++++++++++
>>   tools/perf/Makefile.config             |  6 +++---
>>   4 files changed, 27 insertions(+), 4 deletions(-)
>>   create mode 100644 tools/build/feature/test-llvm-perf.cpp
>>
>> diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
>> index 0717e96d6a0e..427a9389e26c 100644
>> --- a/tools/build/Makefile.feature
>> +++ b/tools/build/Makefile.feature
>> @@ -136,7 +136,7 @@ FEATURE_DISPLAY ?=              \
>>            libunwind              \
>>            libdw-dwarf-unwind     \
>>            libcapstone            \
>> -         llvm                   \
>> +         llvm-perf              \
> 
> Hi! Just a quick question, why remove "llvm" from the list, here?
> 
> Quentin

Just because with respect to the linked fixes: commit, it wasn't 
actually there before. It was added just for addr2line so it should 
probably be llvm-perf rather than the generic one.

But yes we can add llvm output if it's useful, but could probably be a 
separate commit.

Re: [PATCH 1/2] perf build: Autodetect minimum required llvm-dev version

[Quentin Monnet]

2024-09-10 16:11 UTC+0100 ~ James Clark <james.clark@linaro.org>
> 
> 
> On 9/10/24 15:27, Quentin Monnet wrote:
>> 2024-09-10 15:04 UTC+0100 ~ James Clark <james.clark@linaro.org>
>>> The new LLVM addr2line feature requires a minimum version of 13 to
>>> compile. Add a feature check for the version so that NO_LLVM=1 doesn't
>>> need to be explicitly added. Leave the existing llvm feature check
>>> intact because it's used by tools other than Perf.
>>>
>>> This fixes the following compilation error when the llvm-dev version
>>> doesn't match:
>>>
>>>    util/llvm-c-helpers.cpp: In function 'char* 
>>> llvm_name_for_code(dso*, const char*, u64)':
>>>    util/llvm-c-helpers.cpp:178:21: error: 
>>> 'std::remove_reference_t<llvm::DILineInfo>' {aka 'struct 
>>> llvm::DILineInfo'} has no member named 'StartAddress'
>>>      178 |   addr, res_or_err->StartAddress ? *res_or_err- 
>>> >StartAddress : 0);
>>>
>>> Fixes: c3f8644c21df ("perf report: Support LLVM for addr2line()")
>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>> ---
>>>   tools/build/Makefile.feature           |  2 +-
>>>   tools/build/feature/Makefile           |  9 +++++++++
>>>   tools/build/feature/test-llvm-perf.cpp | 14 ++++++++++++++
>>>   tools/perf/Makefile.config             |  6 +++---
>>>   4 files changed, 27 insertions(+), 4 deletions(-)
>>>   create mode 100644 tools/build/feature/test-llvm-perf.cpp
>>>
>>> diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
>>> index 0717e96d6a0e..427a9389e26c 100644
>>> --- a/tools/build/Makefile.feature
>>> +++ b/tools/build/Makefile.feature
>>> @@ -136,7 +136,7 @@ FEATURE_DISPLAY ?=              \
>>>            libunwind              \
>>>            libdw-dwarf-unwind     \
>>>            libcapstone            \
>>> -         llvm                   \
>>> +         llvm-perf              \
>>
>> Hi! Just a quick question, why remove "llvm" from the list, here?
>>
>> Quentin
> 
> Just because with respect to the linked fixes: commit, it wasn't 
> actually there before. It was added just for addr2line so it should 
> probably be llvm-perf rather than the generic one.
> 
> But yes we can add llvm output if it's useful, but could probably be a 
> separate commit.
> 


It wasn't there before, but you're not removing the rest of the "llvm" 
feature, so I'd expect that part to stay as well? But I don't mind much. 
We use the "llvm" feature in bpftool, but beyond that, I don't 
personally need it to be displayed in tools/build/Makefile.feature, so 
no need to respin for that :)

Thanks,
Quentin

Re: [PATCH 1/2] perf build: Autodetect minimum required llvm-dev version

[Arnaldo Carvalho de Melo]

On Tue, Sep 10, 2024 at 05:53:16PM +0100, Quentin Monnet wrote:
> 2024-09-10 16:11 UTC+0100 ~ James Clark <james.clark@linaro.org>
> > 
> > 
> > On 9/10/24 15:27, Quentin Monnet wrote:
> > > 2024-09-10 15:04 UTC+0100 ~ James Clark <james.clark@linaro.org>
> > > > The new LLVM addr2line feature requires a minimum version of 13 to
> > > > compile. Add a feature check for the version so that NO_LLVM=1 doesn't
> > > > need to be explicitly added. Leave the existing llvm feature check
> > > > intact because it's used by tools other than Perf.
> > > > 
> > > > This fixes the following compilation error when the llvm-dev version
> > > > doesn't match:
> > > > 
> > > >    util/llvm-c-helpers.cpp: In function 'char*
> > > > llvm_name_for_code(dso*, const char*, u64)':
> > > >    util/llvm-c-helpers.cpp:178:21: error:
> > > > 'std::remove_reference_t<llvm::DILineInfo>' {aka 'struct
> > > > llvm::DILineInfo'} has no member named 'StartAddress'
> > > >      178 |   addr, res_or_err->StartAddress ? *res_or_err-
> > > > >StartAddress : 0);
> > > > 
> > > > Fixes: c3f8644c21df ("perf report: Support LLVM for addr2line()")
> > > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > > ---
> > > >   tools/build/Makefile.feature           |  2 +-
> > > >   tools/build/feature/Makefile           |  9 +++++++++
> > > >   tools/build/feature/test-llvm-perf.cpp | 14 ++++++++++++++
> > > >   tools/perf/Makefile.config             |  6 +++---
> > > >   4 files changed, 27 insertions(+), 4 deletions(-)
> > > >   create mode 100644 tools/build/feature/test-llvm-perf.cpp
> > > > 
> > > > diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
> > > > index 0717e96d6a0e..427a9389e26c 100644
> > > > --- a/tools/build/Makefile.feature
> > > > +++ b/tools/build/Makefile.feature
> > > > @@ -136,7 +136,7 @@ FEATURE_DISPLAY ?=              \
> > > >            libunwind              \
> > > >            libdw-dwarf-unwind     \
> > > >            libcapstone            \
> > > > -         llvm                   \
> > > > +         llvm-perf              \
> > > 
> > > Hi! Just a quick question, why remove "llvm" from the list, here?
> > > 
> > > Quentin
> > 
> > Just because with respect to the linked fixes: commit, it wasn't
> > actually there before. It was added just for addr2line so it should
> > probably be llvm-perf rather than the generic one.
> > 
> > But yes we can add llvm output if it's useful, but could probably be a
> > separate commit.
 
> It wasn't there before, but you're not removing the rest of the "llvm"
> feature, so I'd expect that part to stay as well? But I don't mind much. We
> use the "llvm" feature in bpftool, but beyond that, I don't personally need
> it to be displayed in tools/build/Makefile.feature, so no need to respin for
> that :)

My worry was that something were being removed because it wasn't being
used in tools/perf/ only to realize later that that was being used
somewhere else in tools/.

That is not the case as you reported, so going back to what we had
before the addr2line llvm code being introduced, i.e.:

commit c3f8644c21df9b7db97eb70e08e2826368aaafa0
Author: Steinar H. Gunderson <sesse@google.com>
Date:   Sat Aug 3 17:20:06 2024 +0200

    perf report: Support LLVM for addr2line()

Just did:

+++ b/tools/build/Makefile.feature
@@ -136,6 +136,7 @@ FEATURE_DISPLAY ?=              \
          libunwind              \
          libdw-dwarf-unwind     \
          libcapstone            \
+         llvm                   \
          zlib                   \
          lzma                   \
          get_cpuid              \

So just displaying whatever was detected, and now we display the new
llvm-perf, so that should be ok, probably even for bpftool, its just
requires a newer version of llvm-dev, right?

- Arnaldo