Message-ID: <ZTGa0Ukt7QyxWcVy@kernel.org>
Date:   Thu, 19 Oct 2023 18:08:33 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Manu Bretelle <chantr4@...il.com>
Cc:     Ian Rogers <irogers@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Rix <trix@...hat.com>, Fangrui Song <maskray@...gle.com>,
        Anshuman Khandual <anshuman.khandual@....com>,
        Andi Kleen <ak@...ux.intel.com>, Leo Yan <leo.yan@...aro.org>,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        Carsten Haitzler <carsten.haitzler@....com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Yang Jihong <yangjihong1@...wei.com>,
        James Clark <james.clark@....com>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>,
        Eduard Zingerman <eddyz87@...il.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Yonghong Song <yhs@...com>, Rob Herring <robh@...nel.org>,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        bpf@...r.kernel.org, llvm@...ts.linux.dev,
        Wang Nan <wangnan0@...wei.com>,
        Wang ShaoBo <bobo.shaobowang@...wei.com>,
        YueHaibing <yuehaibing@...wei.com>,
        He Kuang <hekuang@...wei.com>,
        Brendan Gregg <brendan.d.gregg@...il.com>,
        Quentin Monnet <quentin@...valent.com>
Subject: Re: [PATCH v1 1/4] perf parse-events: Remove BPF event support

Em Thu, Oct 19, 2023 at 12:45:08PM -0700, Manu Bretelle escreveu:
> cc @quentin
> 
> On Fri, Aug 11, 2023 at 12:41:36PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Aug 11, 2023 at 11:43:22AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Right now it is not applying due to some clash with other changes and
> > > when I tried to apply it manually there were some formatting issues:
> > > 
> > > ⬢[acme@...lbox perf-tools-next]$ head ~/wb/1.patch
> > > From SRS0=EALy=D3=flex--irogers.bounces.google.com=3IDHVZAcKBAUnwtljwxlttlqj.htrfhrjpjwsjq.twl@...nel.org Thu Aug 10 17:53:46 2023
> > > Delivered-To: arnaldo.melo@...il.com
> > > Received: from imap.gmail.com [64.233.186.109]
> > > 	by quaco with IMAP (fetchmail-6.4.37)
> > > 	for <acme@...alhost> (single-drop); Thu, 10 Aug 2023 17:53:46 -0300 (-03)
> > > Received: by 2002:a0c:ab03:0:b0:63d:780e:9480 with SMTP id h3csp908198qvb;
> > >  Thu, 10 Aug 2023 11:49:52 -0700 (PDT)
> > > X-Google-Smtp-Source: AGHT+IH9N/knUCyQ0tQ2Q0XBH0gqf8A8DB8/37YHWAJDKBmz7AGSV9CvCKYDuE3EwxriZFBwtZMs
> > > X-Received: by 2002:a4a:6b4f:0:b0:56c:b2ab:9820 with SMTP id
> > >  h15-20020a4a6b4f000000b0056cb2ab9820mr2695332oof.8.1691693392493; Thu, 10 Aug
> > > ⬢[acme@...lbox perf-tools-next]$ patch -p1 < ~/wb/1.patch
> > > patching file tools/perf/Documentation/perf-config.txt
> > > patch: **** malformed patch at line 234: ith
> > > 
> > > ⬢[acme@...lbox perf-tools-next]$
> > > 
> > > I'm trying to apply it manually.
> > 
> > I have this extracted from this patch as the first patch in the series:
> > 
> > >From adc61b5774a9de62f34d593f164ca02daa6fb44c Mon Sep 17 00:00:00 2001
> > From: Ian Rogers <irogers@...gle.com>
> > Date: Fri, 11 Aug 2023 12:19:48 -0300
> > Subject: [PATCH 1/1] perf bpf: Remove support for embedding clang for
> >  compiling BPF events (-e foo.c)
> > 
> > This never was in the default build for perf, is difficult to maintain
> > as it uses clang/llvm internals so ditch it, keeping, for now, the
> > external compilation of .c BPF into .o bytecode and its subsequent
> > loading, that is also going to be removed, do it separately to help
> > bisection and to properly document what is being removed and why.
> > 
> > Committer notes:
> > 
> > Extracted from a larger patch and removed some leftovers, namely
> > deleting these now unused feature tests:
> > 
> >     tools/build/feature/test-clang.cpp
> >     tools/build/feature/test-cxx.cpp
> >     tools/build/feature/test-llvm-version.cpp
> >     tools/build/feature/test-llvm.cpp
> > 
> 
> This seem to have broken `llvm` feature detection for `bpftool`.
> 
> The feature detections are still available in `tools/build/Makefile.feature` [0]
> but the .cpp files are gone.
> 
> `bpftool` still rely on the `llvm` feature:
> 
>     $ git --no-pager grep 'feature-llvm'
>     tools/bpf/bpftool/Makefile:ifeq ($(feature-llvm),1)
> 
> The result of testing llvm feature is:
> 
>     $ cat tools/build/feature/test-llvm.make.output
>     cc1plus: fatal error: test-llvm.cpp: No such file or directory
>     compilation terminated.
> 
> With current head:
> 
>     make -j $((4*$(nproc))) -C tools/bpf/bpftool && ./tools/bpf/bpftool/bpftool --version
>     ...
>     Auto-detecting system features:
>     ...                         clang-bpf-co-re: [ on  ]
>     ...                                    llvm: [ OFF ]
>     ...                                  libcap: [ on  ]
>     ...                                  libbfd: [ on  ]
>     ...
>     ...
>     ...
>     bpftool v7.3.0
>     using libbpf v1.3
>     features: libbfd, skeletons
> 
> After applying
> 
>     git show 56b11a2126bf2f422831ecf6112b87a4485b221b  tools/build/feature | \
>         patch -p1 -R


Ouch, so probably we need just to reintroduce that one
tools/build/feature/test-llvm.cpp file.

Building perf these days ends up using bpftool, and the end result as
noticed with me testing perf, perf trace with bpf, etc didn't change, so
I didn't notice :-\

And:

ifeq ($(feature-llvm),1)
  # If LLVM is available, use it for JIT disassembly
  CFLAGS  += -DHAVE_LLVM_SUPPORT
  LLVM_CONFIG_LIB_COMPONENTS := mcdisassembler all-targets
  CFLAGS  += $(shell $(LLVM_CONFIG) --cflags --libs $(LLVM_CONFIG_LIB_COMPONENTS))
  LIBS    += $(shell $(LLVM_CONFIG) --libs $(LLVM_CONFIG_LIB_COMPONENTS))
  ifeq ($(shell $(LLVM_CONFIG) --shared-mode),static)
    LIBS += $(shell $(LLVM_CONFIG) --system-libs $(LLVM_CONFIG_LIB_COMPONENTS))
    LIBS += -lstdc++
  endif
  LDFLAGS += $(shell $(LLVM_CONFIG) --ldflags)
else
  # Fall back on libbfd
  ifeq ($(feature-libbfd),1)
    LIBS += -lbfd -ldl -lopcodes
  else ifeq ($(feature-libbfd-liberty),1)
    LIBS += -lbfd -ldl -lopcodes -liberty
  else ifeq ($(feature-libbfd-liberty-z),1)
    LIBS += -lbfd -ldl -lopcodes -liberty -lz
  endif

  # If one of the above feature combinations is set, we support libbfd
  ifneq ($(filter -lbfd,$(LIBS)),)
    CFLAGS += -DHAVE_LIBBFD_SUPPORT

    # Libbfd interface changed over time, figure out what we need
    ifeq ($(feature-disassembler-four-args), 1)
      CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE
    endif
    ifeq ($(feature-disassembler-init-styled), 1)
      CFLAGS += -DDISASM_INIT_STYLED
    endif
  endif
endif

And there is a fallback to using binutils, so most people ended up not
noticing.

I wonder how to improve the current situation to detect these kinds of
problems in the future, i.e. how to notice that some file needed by some
Makefile, etc got removed or that some feature test fails because some
change in the test .c files makes them fail and thus activates fallbacks
like the one above :-\


So if I just get this back:

⬢[acme@...lbox 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@...lbox perf-tools-next]$

And install the llvm-devel package then it back working:

⬢[acme@...lbox perf-tools-next]$ make -C tools/bpf/bpftool
make: Entering directory '/home/acme/git/perf-tools-next/tools/bpf/bpftool'

Auto-detecting system features:
...                         clang-bpf-co-re: [ on  ]
...                                    llvm: [ on  ]
...                                  libcap: [ on  ]
...                                  libbfd: [ on  ]
<SNIP>
⬢[acme@...lbox perf-tools-next]$ cat tools/build/feature/test-llvm.make.output
⬢[acme@...lbox perf-tools-next]$ ls -la tools/build/feature/test-llvm.
test-llvm.bin          test-llvm.cpp          test-llvm.d            test-llvm.make.output
⬢[acme@...lbox perf-tools-next]$ ls -la tools/build/feature/test-llvm.bin
-rwxr-xr-x. 1 acme acme 17712 Oct 19 18:04 tools/build/feature/test-llvm.bin
⬢[acme@...lbox perf-tools-next]$ ldd tools/build/feature/test-llvm.bin
	linux-vdso.so.1 (0x00007ffcaf5d9000)
	libLLVM-16.so => /lib64/libLLVM-16.so (0x00007fc4faefa000)
	libstdc++.so.6 => /lib64/libstdc++.so.6 (0x00007fc4faca6000)
	libm.so.6 => /lib64/libm.so.6 (0x00007fc4fabc5000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007fc4faba1000)
	libc.so.6 => /lib64/libc.so.6 (0x00007fc4fa9c3000)
	libffi.so.8 => /lib64/libffi.so.8 (0x00007fc4fa9b7000)
	libedit.so.0 => /lib64/libedit.so.0 (0x00007fc4fa978000)
	libz.so.1 => /lib64/libz.so.1 (0x00007fc4fa95e000)
	libtinfo.so.6 => /lib64/libtinfo.so.6 (0x00007fc4fa92b000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fc502404000)
⬢[acme@...lbox perf-tools-next]$ sudo dnf install llvm-devel

I'll get this merged in my perf-tools-fixes-for-v6.6 that I'll submit
tomorrow to Linus, thanks for reporting!

I'll add your:

Reported-by: Manu Bretelle <chantr4@...il.com>

And:

Fixes: 56b11a2126bf2f42 ("perf bpf: Remove support for embedding clang for compiling BPF events (-e foo.c)")

Ok?

- Arnaldo

Powered by blists - more mailing lists