Message-ID: <CAP-5=fW3ZVfTU-=3WJny6KuD+DC7fzNzAehUscztr8Zhhfwrmw@mail.gmail.com>
Date: Fri, 5 Dec 2025 09:16:32 -0800
From: Ian Rogers <irogers@...gle.com>
To: Leo Yan <leo.yan@....com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, James Clark <james.clark@...aro.org>, 
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH 2/2] Revert "perf tools: Fix arm64 build by generating unistd_64.h"

On Fri, Dec 5, 2025 at 8:21 AM Leo Yan <leo.yan@....com> wrote:
>
> On Thu, Dec 04, 2025 at 10:19:59AM -0800, Ian Rogers wrote:
> > On Thu, Dec 4, 2025 at 8:53 AM Leo Yan <leo.yan@....com> wrote:
> > >
> > > This reverts:
> > >
> > > commit 8988c4b91945 ("perf tools: Fix in-source libperf build")
> > > commit bfb713ea53c7 ("perf tools: Fix arm64 build by generating unistd_64.h")
> > >
> > > Since we now have a static unistd_64.h for the arm64 build, there is no
> > > need to generate unistd_64.h in libperf.  Revert all patches related to
> > > generating unistd_64.h.
> >
> > Could we generate the file and then compare the two? We do something
> > like this for empty-pmu-events.c here:
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/Build?h=perf-tools-next#n49
> >
> > Similarly, could we get rid of the unistd_64.h generation and just add
> > a build time validation test? Generating the file is painful in many
> > ways, as this series testifies.
>
> Thanks for suggestions!
>
> I had a question: why we need to maintain unistd.h particually in
> ./tools folder?  This header is a standard C header and should be
> provided by toolchain instead.  Due to multiple copies, it is a bit mess
> the programs might include unpurposed one.
>
> I went through the history and found:
>
>   commit 34b009cfde2b ("tools include: Grab copies of arm64 dependent unistd.h files")
>
> It introduces the header unistd.h for generating system call table.
>
> Afterwards,
>
>   commit cb8197db8c09 ("perf tools arm64: Use syscall table")
>
> unistd.h headers are not used to generate syscall table anymore.  Since
> then, syscall tables are maintained in
> tools/perf/arch/*/entry/syscalls/syscall*.tbl for each arch.
>
> To verify this, I did a quick try with removing x86 and arm64 headers:
>
>   tools/arch/arm64/include/uapi/asm/unistd.h
>   tools/arch/x86/include/uapi/asm/unistd.h
>   tools/arch/x86/include/uapi/asm/unistd_32.h
>   tools/arch/x86/include/uapi/asm/unistd_64.h
>
> I run the linux-tools-container-builds and confirmed all x86 and arm64
> building tests pass and did not see any failure.
>
> For a neat fix, I think we can remove all unistd.h headers:
>
>   $ ls tools/arch/*/include/uapi/asm/unistd*
>   tools/arch/arc/include/uapi/asm/unistd.h
>   tools/arch/hexagon/include/uapi/asm/unistd.h
>   tools/arch/riscv/include/uapi/asm/unistd.h
>   tools/arch/x86/include/uapi/asm/unistd_64.h
>   tools/arch/arm64/include/uapi/asm/unistd.h
>   tools/arch/loongarch/include/uapi/asm/unistd.h
>   tools/arch/x86/include/uapi/asm/unistd_32.h
>   tools/arch/x86/include/uapi/asm/unistd.h
>
> Any concern?  I would get maintainers's confirmation before proceeding.

Thanks Leo! The tools/include directory is a concern for me as the use
of it is pretty unstructured. For example, what does <linux/types.h>
refer to? In the perf build it is actually the kernel version of
types.h rather than the UAPI version, despite perf being a user land
tool. In general this is necessary to get other kernel headers to work
(say list.h). I think it brings about a number of concerns, one being
around licensing. My preference would be to make tools/include an
explicit dependency, like libperf and libsubcmd. It would have
associated header files you could install and depend upon, placing
those header files onto your include path from the install directory.
Things in tools/include with different licenses can have different lib
variants so that it is clear what license is being linked against -
generally GPLv2 vs GPLv2+linux syscall exception.

Given that we're mixing kernel and UAPI headers there is a need for
the headers to be hermetic. We can't just use parts of headers from
the kernel, some from UAPI, some from the latest kernel tree and some
from whatever libc is on the system. There are differences in the use
of things like u64 vs __u64 that will all too easily give build
errors, especially when building against an older libc.

Arnaldo did make some moves to clean up tools/include with patches like:
https://lore.kernel.org/lkml/Ze93VPYpegMMt5kk@x1/
That mention a breakage I caused ARM64/James around tools/include (I
mention the library idea above again, sorry I'm a broken record):
https://lore.kernel.org/lkml/CAP-5=fWZVrpRufO4w-S4EcSi9STXcTAN2ERLwTSN7yrSSA-otQ@mail.gmail.com/
I think the breakage shows things need to be hermetic, which may mean
we need unistd_64.h. I think Arnaldo's moving of fs.h shows that less
in the tools/include directory is better and supported by the
maintainers.

Thanks for doing the build testing! Doing a simpler grep:
```
$ grep -r "asm/unistd" tools/include tools/perf tools/lib
tools/perf/check-headers.sh:  "arch/x86/include/uapi/asm/unistd.h"
tools/perf/check-headers.sh:  "arch/arm64/include/uapi/asm/unistd.h"
tools/lib/bpf/bpf.c:#include <asm/unistd.h>
tools/lib/bpf/libbpf.c:#include <asm/unistd.h>
```
I think unistd.h is needed to make things hermetic for libbpf :-( If
we can make libbpf use just the regular unistd.h then life would be
good. My experience is that libbpf tends to like the kernel way of
doing things a bit too much - <rant>they have the IS_ERR/ERR_PTR
antipattern rather than just using errno, as because you know this is
user land and that's how stuff normally gets done and we can make
pointers type safe. Also don't try adding comments to their syscall
APIs unless you enjoy being called "wrong" a lot with no constructive
feedback. </rant>. Anyway, I think getting rid of unistd.h is a good
thing but it will probably break perf's build that invokes libbpf's
build because libbpf will be mixing kernel and old libc headers on one
of the maintainers build test platforms (likely an old one) where the
type collisions yield compile time errors. To avoid that I'd suggest
making libbpf not use asm/unistd.h as a first step. We could get lucky
with something non-hermetic, but it wouldn't be my preference.

Thanks,
Ian

> Thanks,
> Leo

Powered by blists - more mailing lists