Re: [PATCH net-next v3 1/2] tools: ynl: Allow cross-compiling ynl and associated tools

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] tools/net/ynl/ynltool/Makefile now inherits the tools/ `O=` ->
  `OUTPUT` contract from tools/scripts/Makefile.include but only half-…
- [Medium] Removing the `CC=gcc` pin while letting Makefile.include
  select clang makes the GCC-only `-static-libasan` flag reach clang,…

--

tools: ynl: Allow cross-compiling ynl and associated tools

The four ynl Makefiles now include tools/scripts/Makefile.include so that
CC and AR honour CROSS_COMPILE and LLVM, the ynltool CFLAGS assignment is
switched from := to += and the local Q definition is dropped in favour of
the one from Makefile.include.

No Link: tags in the commit header.

Two questions below, one about O= handling in ynltool and one about
-static-libasan now that CC is no longer pinned to gcc.

> diff --git a/tools/net/ynl/Makefile b/tools/net/ynl/Makefile
> index 3cefe4ed96cb9..838c465d65aa8 100644
> --- a/tools/net/ynl/Makefile
> +++ b/tools/net/ynl/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +include ../../scripts/Makefile.include
>  include ../../scripts/Makefile.arch
>  
>  INSTALL	?= install
> diff --git a/tools/net/ynl/generated/Makefile b/tools/net/ynl/generated/Makefile
> index ea4128f612d6c..5a186349b5a86 100644
> --- a/tools/net/ynl/generated/Makefile
> +++ b/tools/net/ynl/generated/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -CC=gcc
> +include ../../../scripts/Makefile.include
> +
>  CFLAGS += -std=gnu11 -O2 -W -Wall -Wextra -Wno-unused-parameter -Wshadow \
>  	-I../lib/ -idirafter $(UAPI_PATH)
>  ifeq ("$(DEBUG)","1")
> diff --git a/tools/net/ynl/lib/Makefile b/tools/net/ynl/lib/Makefile
> index 9b98c05996000..7b3eae89982f8 100644
> --- a/tools/net/ynl/lib/Makefile
> +++ b/tools/net/ynl/lib/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -CC=gcc
> +include ../../../scripts/Makefile.include
> +
>  CFLAGS += -std=gnu11 -O2 -W -Wall -Wextra -Wno-unused-parameter -Wshadow
>  ifeq ("$(DEBUG)","1")
>    CFLAGS += -g -fsanitize=address -fsanitize=leak -static-libasan
> diff --git a/tools/net/ynl/tests/Makefile b/tools/net/ynl/tests/Makefile
> index 40827ca8e579d..99ae7dcd63484 100644
> --- a/tools/net/ynl/tests/Makefile
> +++ b/tools/net/ynl/tests/Makefile
> @@ -2,8 +2,8 @@
>  # Makefile for YNL tests
>  
>  include ../Makefile.deps
> +include ../../../scripts/Makefile.include
>  
> -CC=gcc

[Severity: Medium]

With the CC=gcc pin removed here, does a "make LLVM=1 -C tools/net/ynl"
build still complete?  Makefile.include picks clang for LLVM builds:

  ifneq ($(LLVM),)
  ...
    $(call allow-override,CC,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))

and tests/Makefile turns the sanitizer flags on unless NDEBUG=1:

  ifneq ("$(NDEBUG)","1")
    CFLAGS += -g -fsanitize=address -fsanitize=leak -static-libasan
  endif

clang's driver does not accept -static-libasan.  Other selftests Makefiles
guard exactly this flag, for example
tools/testing/selftests/filesystems/openat2/Makefile:

  # gcc requires -static-libasan in order to ensure that Address Sanitizer's
  # library is the first one loaded. However, clang already statically links the
  # Address Sanitizer if -fsanitize is specified. Therefore, simply omit
  # -static-libasan for clang builds.
  ifeq ($(LLVM),)
      CFLAGS += -static-libasan
  endif

tests is listed in SUBDIRS of tools/net/ynl/Makefile and the default goal
is "all: $(SUBDIRS) libynl.a", so this is hit by a plain build with LLVM=1
(with or without CROSS_COMPILE, which is the configuration named in the
commit message).

The same unguarded flag sits on the DEBUG=1 paths of lib/Makefile,
generated/Makefile and ynltool/Makefile.  Should all four get the
ifeq ($(LLVM),) guard as part of this change?

>  CFLAGS += -std=gnu11 -O2 -W -Wall -Wextra -Wno-unused-parameter -Wshadow \
>  	-I../lib/ -I../generated/ -I../../../testing/selftests/ \
>  	-idirafter $(UAPI_PATH)
> diff --git a/tools/net/ynl/ynltool/Makefile b/tools/net/ynl/ynltool/Makefile
> index 48b0f32050f0e..b8c67cdb4fdf0 100644
> --- a/tools/net/ynl/ynltool/Makefile
> +++ b/tools/net/ynl/ynltool/Makefile
> @@ -1,12 +1,12 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
>  include ../Makefile.deps
> +include ../../../scripts/Makefile.include

[Severity: Medium]

Does pulling Makefile.include in here break "make O=<dir>" for ynltool?
Makefile.include translates a command line O= into OUTPUT:

  ifneq ($(O),)
  ifeq ($(origin O), command line)
          $(if $(shell cd $(PWD); test -d $(O) || echo $(O)),$(error O=$(O) does not exist),)
	  ABSOLUTE_O := $(shell cd $(PWD); cd $(O) ; pwd)
	  OUTPUT := $(ABSOLUTE_O)/$(if $(subdir),$(subdir)/)

ynltool/Makefile already names its targets with that prefix:

  OBJS := $(patsubst %.c,$(OUTPUT)%.o,$(SRCS))

  YNLTOOL := $(OUTPUT)ynltool

but the only object rule carries no $(OUTPUT) and does not create the
directory:

  %.o: %.c ../libynl.a
	$(Q)echo -e "\tCC $@"
	$(Q)$(COMPILE.c) -MMD -c -o $@ $<

For a target such as /tmp/out/ynl.o and a target pattern containing no
slash, make strips the directory, matches the stem, and re-prepends the
directory to the derived prerequisite, so it looks for /tmp/out/ynl.c.
There is no vpath and nothing creates /tmp/out, so does
"make -C tools/net/ynl/ynltool O=/tmp/out" stop with "No rule to make
target '/tmp/out/<name>.o', needed by '/tmp/out/ynltool'"?

Command line variables propagate through MAKEFLAGS, and the top level
recipe runs the sub-makes with

  $(SUBDIRS):
	@if [ -f "$@/Makefile" ] ; then \
		$(MAKE) -C $@ ; \
	fi

so "make -C tools/net/ynl O=/tmp/out" reaches the same path through the
default goal.  Other tools/ Makefiles that honour this interface spell the
rule as $(OUTPUT)%.o: %.c - would that plus an mkdir be the fix here?

The install rule interpolates the prefixed name too:

  install: $(YNLTOOL)
	$(INSTALL) -m 0755 $(YNLTOOL) $(DESTDIR)$(bindir)/$(YNLTOOL)

With OUTPUT set, does the destination become
$(DESTDIR)/usr/bin//tmp/out/ynltool rather than $(DESTDIR)/usr/bin/ynltool?

Related: lib/Makefile, generated/Makefile and tests/Makefile now include
Makefile.include but ignore OUTPUT entirely, so an O= build would keep
writing their objects into the source tree while only ynltool tries to
honour it.  Was leaving the four sub-makefiles inconsistent about O=
intentional, or should O= be rejected until they all handle it?

>  
>  INSTALL	?= install
>  prefix  ?= /usr
>  
> -CC := gcc
> -CFLAGS := -Wall -Wextra -Werror -O2
> +CFLAGS += -Wall -Wextra -Werror -O2
>  ifeq ("$(DEBUG)","1")
>    CFLAGS += -g -fsanitize=address -fsanitize=leak -static-libasan
>  endif

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907161438.755125-1-maxime.chevallier%40bootlin.com