| Message ID | 20240910140405.568791-1-james.clark@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] perf build: Autodetect minimum required llvm-dev version | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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
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]$
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
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.
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
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
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
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