Re: [PATCH 1/1 next] tools build: Remove the libunwind feature tests from the ones detected when test-all.o builds

Re: [PATCH 1/1 next] tools build: Remove the libunwind feature tests from the ones detected when test-all.o builds

[Namhyung Kim]

Hello,

On Mon, Mar 17, 2025 at 09:10:29AM -0700, Ian Rogers wrote:
> On Wed, Dec 11, 2024 at 7:45 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > We have a tools/build/feature/test-all.c that has the most common set of
> > features that perf uses and are expected to have its development files
> > available when building perf.
> >
> > When we made libwunwind opt-in we forgot to remove them from the list of
> > features that are assumed to be available when test-all.c builds, remove
> > them.
> >
> > Before this patch:
> >
> >   $ rm -rf /tmp/b ; mkdir /tmp/b ; make -C tools/perf O=/tmp/b feature-dump ; grep feature-libunwind-aarch64= /tmp/b/FEATURE-DUMP
> >   feature-libunwind-aarch64=1
> >   $
> >
> > Even tho this not being test built and those header files being
> > available:
> >
> >   $ head -5 tools/build/feature/test-libunwind-aarch64.c
> >   // SPDX-License-Identifier: GPL-2.0
> >   #include <libunwind-aarch64.h>
> >   #include <stdlib.h>
> >
> >   extern int UNW_OBJ(dwarf_search_unwind_table) (unw_addr_space_t as,
> >   $
> >
> > After this patch:
> >
> >   $ grep feature-libunwind- /tmp/b/FEATURE-DUMP
> >   $
> >
> > Now an audit on what is being enabled when test-all.c builds will be
> > performed.
> >
> > Fixes: 176c9d1e6a06f2fa ("tools features: Don't check for libunwind devel files by default")
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Ian Rogers <irogers@google.com>
> > Cc: James Clark <james.clark@linaro.org>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Kan Liang <kan.liang@linux.intel.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Sorry for the delay on this.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>

Thanks for the review, but I think this part is used by other tools like
BPF and tracing.  It'd be nice to get reviews from them.

Thanks,
Namhyung

> 
> > ---
> >  tools/build/Makefile.feature | 7 -------
> >  1 file changed, 7 deletions(-)
> >
> > diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
> > index b2884bc23775e986..9cde51104c2d70ec 100644
> > --- a/tools/build/Makefile.feature
> > +++ b/tools/build/Makefile.feature
> > @@ -90,13 +90,6 @@ FEATURE_TESTS_EXTRA :=                  \
> >           libbfd-liberty                 \
> >           libbfd-liberty-z               \
> >           libopencsd                     \
> > -         libunwind-x86                  \
> > -         libunwind-x86_64               \
> > -         libunwind-arm                  \
> > -         libunwind-aarch64              \
> > -         libunwind-debug-frame          \
> > -         libunwind-debug-frame-arm      \
> > -         libunwind-debug-frame-aarch64  \
> >           cxx                            \
> >           llvm                           \
> >           clang                          \
> > --
> > 2.47.0
> >

Re: [PATCH 1/1 next] tools build: Remove the libunwind feature tests from the ones detected when test-all.o builds

[Ian Rogers]

On Mon, Mar 17, 2025 at 10:06 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Mon, Mar 17, 2025 at 09:10:29AM -0700, Ian Rogers wrote:
> > On Wed, Dec 11, 2024 at 7:45 AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > We have a tools/build/feature/test-all.c that has the most common set of
> > > features that perf uses and are expected to have its development files
> > > available when building perf.
> > >
> > > When we made libwunwind opt-in we forgot to remove them from the list of
> > > features that are assumed to be available when test-all.c builds, remove
> > > them.
> > >
> > > Before this patch:
> > >
> > >   $ rm -rf /tmp/b ; mkdir /tmp/b ; make -C tools/perf O=/tmp/b feature-dump ; grep feature-libunwind-aarch64= /tmp/b/FEATURE-DUMP
> > >   feature-libunwind-aarch64=1
> > >   $
> > >
> > > Even tho this not being test built and those header files being
> > > available:
> > >
> > >   $ head -5 tools/build/feature/test-libunwind-aarch64.c
> > >   // SPDX-License-Identifier: GPL-2.0
> > >   #include <libunwind-aarch64.h>
> > >   #include <stdlib.h>
> > >
> > >   extern int UNW_OBJ(dwarf_search_unwind_table) (unw_addr_space_t as,
> > >   $
> > >
> > > After this patch:
> > >
> > >   $ grep feature-libunwind- /tmp/b/FEATURE-DUMP
> > >   $
> > >
> > > Now an audit on what is being enabled when test-all.c builds will be
> > > performed.
> > >
> > > Fixes: 176c9d1e6a06f2fa ("tools features: Don't check for libunwind devel files by default")
> > > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > > Cc: Ian Rogers <irogers@google.com>
> > > Cc: James Clark <james.clark@linaro.org>
> > > Cc: Jiri Olsa <jolsa@kernel.org>
> > > Cc: Kan Liang <kan.liang@linux.intel.com>
> > > Cc: Namhyung Kim <namhyung@kernel.org>
> > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> >
> > Sorry for the delay on this.
> >
> > Reviewed-by: Ian Rogers <irogers@google.com>
>
> Thanks for the review, but I think this part is used by other tools like
> BPF and tracing.  It'd be nice to get reviews from them.

Sgtm. The patch hasn't had attention for 3 months. A quick grep for
"unwind" and "UNW_" shows only use in perf and the feature tests.

Thanks,
Ian

Re: [PATCH 1/1 next] tools build: Remove the libunwind feature tests from the ones detected when test-all.o builds

[Quentin Monnet]

2025-03-17 10:16 UTC-0700 ~ Ian Rogers <irogers@google.com>
> On Mon, Mar 17, 2025 at 10:06 AM Namhyung Kim <namhyung@kernel.org> wrote:
>>
>> Hello,
>>
>> On Mon, Mar 17, 2025 at 09:10:29AM -0700, Ian Rogers wrote:
>>> On Wed, Dec 11, 2024 at 7:45 AM Arnaldo Carvalho de Melo
>>> <acme@kernel.org> wrote:
>>>>
>>>> We have a tools/build/feature/test-all.c that has the most common set of
>>>> features that perf uses and are expected to have its development files
>>>> available when building perf.
>>>>
>>>> When we made libwunwind opt-in we forgot to remove them from the list of
>>>> features that are assumed to be available when test-all.c builds, remove
>>>> them.
>>>>
>>>> Before this patch:
>>>>
>>>>   $ rm -rf /tmp/b ; mkdir /tmp/b ; make -C tools/perf O=/tmp/b feature-dump ; grep feature-libunwind-aarch64= /tmp/b/FEATURE-DUMP
>>>>   feature-libunwind-aarch64=1
>>>>   $
>>>>
>>>> Even tho this not being test built and those header files being
>>>> available:
>>>>
>>>>   $ head -5 tools/build/feature/test-libunwind-aarch64.c
>>>>   // SPDX-License-Identifier: GPL-2.0
>>>>   #include <libunwind-aarch64.h>
>>>>   #include <stdlib.h>
>>>>
>>>>   extern int UNW_OBJ(dwarf_search_unwind_table) (unw_addr_space_t as,
>>>>   $
>>>>
>>>> After this patch:
>>>>
>>>>   $ grep feature-libunwind- /tmp/b/FEATURE-DUMP
>>>>   $
>>>>
>>>> Now an audit on what is being enabled when test-all.c builds will be
>>>> performed.
>>>>
>>>> Fixes: 176c9d1e6a06f2fa ("tools features: Don't check for libunwind devel files by default")
>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>>> Cc: Ian Rogers <irogers@google.com>
>>>> Cc: James Clark <james.clark@linaro.org>
>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>> Cc: Kan Liang <kan.liang@linux.intel.com>
>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>>>
>>> Sorry for the delay on this.
>>>
>>> Reviewed-by: Ian Rogers <irogers@google.com>
>>
>> Thanks for the review, but I think this part is used by other tools like
>> BPF and tracing.  It'd be nice to get reviews from them.
> 
> Sgtm. The patch hasn't had attention for 3 months. A quick grep for
> "unwind" and "UNW_" shows only use in perf and the feature tests.
> 
> Thanks,
> Ian


Indeed, bpftool does not rely on libunwind, and I don't remember other
BPF components doing so, either.

Quentin

Re: [PATCH 1/1 next] tools build: Remove the libunwind feature tests from the ones detected when test-all.o builds

[Namhyung Kim]

Hello,

On Mon, Mar 17, 2025 at 09:19:22PM +0000, Quentin Monnet wrote:
> 2025-03-17 10:16 UTC-0700 ~ Ian Rogers <irogers@google.com>
> > On Mon, Mar 17, 2025 at 10:06 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >>
> >> Hello,
> >>
> >> On Mon, Mar 17, 2025 at 09:10:29AM -0700, Ian Rogers wrote:
> >>> On Wed, Dec 11, 2024 at 7:45 AM Arnaldo Carvalho de Melo
> >>> <acme@kernel.org> wrote:
> >>>>
> >>>> We have a tools/build/feature/test-all.c that has the most common set of
> >>>> features that perf uses and are expected to have its development files
> >>>> available when building perf.
> >>>>
> >>>> When we made libwunwind opt-in we forgot to remove them from the list of
> >>>> features that are assumed to be available when test-all.c builds, remove
> >>>> them.
> >>>>
> >>>> Before this patch:
> >>>>
> >>>>   $ rm -rf /tmp/b ; mkdir /tmp/b ; make -C tools/perf O=/tmp/b feature-dump ; grep feature-libunwind-aarch64= /tmp/b/FEATURE-DUMP
> >>>>   feature-libunwind-aarch64=1
> >>>>   $
> >>>>
> >>>> Even tho this not being test built and those header files being
> >>>> available:
> >>>>
> >>>>   $ head -5 tools/build/feature/test-libunwind-aarch64.c
> >>>>   // SPDX-License-Identifier: GPL-2.0
> >>>>   #include <libunwind-aarch64.h>
> >>>>   #include <stdlib.h>
> >>>>
> >>>>   extern int UNW_OBJ(dwarf_search_unwind_table) (unw_addr_space_t as,
> >>>>   $
> >>>>
> >>>> After this patch:
> >>>>
> >>>>   $ grep feature-libunwind- /tmp/b/FEATURE-DUMP
> >>>>   $
> >>>>
> >>>> Now an audit on what is being enabled when test-all.c builds will be
> >>>> performed.
> >>>>
> >>>> Fixes: 176c9d1e6a06f2fa ("tools features: Don't check for libunwind devel files by default")
> >>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
> >>>> Cc: Ian Rogers <irogers@google.com>
> >>>> Cc: James Clark <james.clark@linaro.org>
> >>>> Cc: Jiri Olsa <jolsa@kernel.org>
> >>>> Cc: Kan Liang <kan.liang@linux.intel.com>
> >>>> Cc: Namhyung Kim <namhyung@kernel.org>
> >>>> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> >>>
> >>> Sorry for the delay on this.
> >>>
> >>> Reviewed-by: Ian Rogers <irogers@google.com>
> >>
> >> Thanks for the review, but I think this part is used by other tools like
> >> BPF and tracing.  It'd be nice to get reviews from them.
> > 
> > Sgtm. The patch hasn't had attention for 3 months. A quick grep for
> > "unwind" and "UNW_" shows only use in perf and the feature tests.
> > 
> > Thanks,
> > Ian
> 
> 
> Indeed, bpftool does not rely on libunwind, and I don't remember other
> BPF components doing so, either.

Right, but my concern was about the feature test itself and the related
changes in the build files.

Can I get your Acked-by then?

Thanks,
Namhyung

Re: [PATCH 1/1 next] tools build: Remove the libunwind feature tests from the ones detected when test-all.o builds

[Jiri Olsa]

On Tue, Mar 18, 2025 at 04:51:53PM -0700, Namhyung Kim wrote:
> Hello,
> 
> On Mon, Mar 17, 2025 at 09:19:22PM +0000, Quentin Monnet wrote:
> > 2025-03-17 10:16 UTC-0700 ~ Ian Rogers <irogers@google.com>
> > > On Mon, Mar 17, 2025 at 10:06 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >>
> > >> Hello,
> > >>
> > >> On Mon, Mar 17, 2025 at 09:10:29AM -0700, Ian Rogers wrote:
> > >>> On Wed, Dec 11, 2024 at 7:45 AM Arnaldo Carvalho de Melo
> > >>> <acme@kernel.org> wrote:
> > >>>>
> > >>>> We have a tools/build/feature/test-all.c that has the most common set of
> > >>>> features that perf uses and are expected to have its development files
> > >>>> available when building perf.
> > >>>>
> > >>>> When we made libwunwind opt-in we forgot to remove them from the list of
> > >>>> features that are assumed to be available when test-all.c builds, remove
> > >>>> them.
> > >>>>
> > >>>> Before this patch:
> > >>>>
> > >>>>   $ rm -rf /tmp/b ; mkdir /tmp/b ; make -C tools/perf O=/tmp/b feature-dump ; grep feature-libunwind-aarch64= /tmp/b/FEATURE-DUMP
> > >>>>   feature-libunwind-aarch64=1
> > >>>>   $
> > >>>>
> > >>>> Even tho this not being test built and those header files being
> > >>>> available:
> > >>>>
> > >>>>   $ head -5 tools/build/feature/test-libunwind-aarch64.c
> > >>>>   // SPDX-License-Identifier: GPL-2.0
> > >>>>   #include <libunwind-aarch64.h>
> > >>>>   #include <stdlib.h>
> > >>>>
> > >>>>   extern int UNW_OBJ(dwarf_search_unwind_table) (unw_addr_space_t as,
> > >>>>   $
> > >>>>
> > >>>> After this patch:
> > >>>>
> > >>>>   $ grep feature-libunwind- /tmp/b/FEATURE-DUMP
> > >>>>   $
> > >>>>
> > >>>> Now an audit on what is being enabled when test-all.c builds will be
> > >>>> performed.
> > >>>>
> > >>>> Fixes: 176c9d1e6a06f2fa ("tools features: Don't check for libunwind devel files by default")
> > >>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
> > >>>> Cc: Ian Rogers <irogers@google.com>
> > >>>> Cc: James Clark <james.clark@linaro.org>
> > >>>> Cc: Jiri Olsa <jolsa@kernel.org>
> > >>>> Cc: Kan Liang <kan.liang@linux.intel.com>
> > >>>> Cc: Namhyung Kim <namhyung@kernel.org>
> > >>>> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > >>>
> > >>> Sorry for the delay on this.
> > >>>
> > >>> Reviewed-by: Ian Rogers <irogers@google.com>
> > >>
> > >> Thanks for the review, but I think this part is used by other tools like
> > >> BPF and tracing.  It'd be nice to get reviews from them.
> > > 
> > > Sgtm. The patch hasn't had attention for 3 months. A quick grep for
> > > "unwind" and "UNW_" shows only use in perf and the feature tests.
> > > 
> > > Thanks,
> > > Ian
> > 
> > 
> > Indeed, bpftool does not rely on libunwind, and I don't remember other
> > BPF components doing so, either.
> 
> Right, but my concern was about the feature test itself and the related
> changes in the build files.
> 
> Can I get your Acked-by then?

hi,
I might be missing something, but I see following commit in git already:
  b40fbeb0b1cd tools build: Remove the libunwind feature tests from the ones detected when test-all.o builds

jirka

Re: [PATCH 1/1 next] tools build: Remove the libunwind feature tests from the ones detected when test-all.o builds

[Namhyung Kim]

Hi Jiri,

On Wed, Mar 19, 2025 at 10:47:40AM +0100, Jiri Olsa wrote:
> On Tue, Mar 18, 2025 at 04:51:53PM -0700, Namhyung Kim wrote:
> > Hello,
> > 
> > On Mon, Mar 17, 2025 at 09:19:22PM +0000, Quentin Monnet wrote:
> > > 2025-03-17 10:16 UTC-0700 ~ Ian Rogers <irogers@google.com>
> > > > On Mon, Mar 17, 2025 at 10:06 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >>
> > > >> Hello,
> > > >>
> > > >> On Mon, Mar 17, 2025 at 09:10:29AM -0700, Ian Rogers wrote:
> > > >>> On Wed, Dec 11, 2024 at 7:45 AM Arnaldo Carvalho de Melo
> > > >>> <acme@kernel.org> wrote:
> > > >>>>
> > > >>>> We have a tools/build/feature/test-all.c that has the most common set of
> > > >>>> features that perf uses and are expected to have its development files
> > > >>>> available when building perf.
> > > >>>>
> > > >>>> When we made libwunwind opt-in we forgot to remove them from the list of
> > > >>>> features that are assumed to be available when test-all.c builds, remove
> > > >>>> them.
> > > >>>>
> > > >>>> Before this patch:
> > > >>>>
> > > >>>>   $ rm -rf /tmp/b ; mkdir /tmp/b ; make -C tools/perf O=/tmp/b feature-dump ; grep feature-libunwind-aarch64= /tmp/b/FEATURE-DUMP
> > > >>>>   feature-libunwind-aarch64=1
> > > >>>>   $
> > > >>>>
> > > >>>> Even tho this not being test built and those header files being
> > > >>>> available:
> > > >>>>
> > > >>>>   $ head -5 tools/build/feature/test-libunwind-aarch64.c
> > > >>>>   // SPDX-License-Identifier: GPL-2.0
> > > >>>>   #include <libunwind-aarch64.h>
> > > >>>>   #include <stdlib.h>
> > > >>>>
> > > >>>>   extern int UNW_OBJ(dwarf_search_unwind_table) (unw_addr_space_t as,
> > > >>>>   $
> > > >>>>
> > > >>>> After this patch:
> > > >>>>
> > > >>>>   $ grep feature-libunwind- /tmp/b/FEATURE-DUMP
> > > >>>>   $
> > > >>>>
> > > >>>> Now an audit on what is being enabled when test-all.c builds will be
> > > >>>> performed.
> > > >>>>
> > > >>>> Fixes: 176c9d1e6a06f2fa ("tools features: Don't check for libunwind devel files by default")
> > > >>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
> > > >>>> Cc: Ian Rogers <irogers@google.com>
> > > >>>> Cc: James Clark <james.clark@linaro.org>
> > > >>>> Cc: Jiri Olsa <jolsa@kernel.org>
> > > >>>> Cc: Kan Liang <kan.liang@linux.intel.com>
> > > >>>> Cc: Namhyung Kim <namhyung@kernel.org>
> > > >>>> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > >>>
> > > >>> Sorry for the delay on this.
> > > >>>
> > > >>> Reviewed-by: Ian Rogers <irogers@google.com>
> > > >>
> > > >> Thanks for the review, but I think this part is used by other tools like
> > > >> BPF and tracing.  It'd be nice to get reviews from them.
> > > > 
> > > > Sgtm. The patch hasn't had attention for 3 months. A quick grep for
> > > > "unwind" and "UNW_" shows only use in perf and the feature tests.
> > > > 
> > > > Thanks,
> > > > Ian
> > > 
> > > 
> > > Indeed, bpftool does not rely on libunwind, and I don't remember other
> > > BPF components doing so, either.
> > 
> > Right, but my concern was about the feature test itself and the related
> > changes in the build files.
> > 
> > Can I get your Acked-by then?
> 
> hi,
> I might be missing something, but I see following commit in git already:
>   b40fbeb0b1cd tools build: Remove the libunwind feature tests from the ones detected when test-all.o builds

Oops, thanks for checking this.

I was confused by Ian's late reply and thought it belongs to this
cycle. :)  Yep, it's already merged in the previous cycle.

Sorry for the noise.
Namhyung