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

[netdev-bot+sashiko]

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

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

- [Medium] Incomplete LLVM cross-compilation support: the commit claims
  to "account for the cross-compiling options such as CROSS_COMPILE or…
- [Medium] Include ordering bug in tools/net/ynl/Makefile: `include
  ../../scripts/Makefile.include` is added on line 4, i.e. *after*…
- [Low] Verbosity-control mismatch in tools/net/ynl/ynltool/Makefile: the
  patch adds `include ../../../scripts/Makefile.include` (line 4), which…

--

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

The ynl Makefiles stop hardcoding CC=gcc and instead include
tools/scripts/Makefile.include so that CC/AR are resolved from
CROSS_COMPILE and the LLVM variables, and the ynltool CFLAGS assignment is
switched from := to += so it no longer clobbers the flags coming from
Makefile.include.

> diff --git a/tools/net/ynl/Makefile b/tools/net/ynl/Makefile
> index 3cefe4ed96cb..8bf72c063e86 100644
> --- a/tools/net/ynl/Makefile
> +++ b/tools/net/ynl/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  include ../../scripts/Makefile.arch
> +include ../../scripts/Makefile.include
>  
>  INSTALL	?= install
>  prefix  ?= /usr

[Severity: Medium]
Should the new include come before Makefile.arch rather than after it?

Makefile.arch probes the compiler with a simply-expanded assignment, so
${CC} and ${CFLAGS} are expanded at include time:

tools/scripts/Makefile.arch:
  LP64 := $(shell if command -v $(firstword ${CC}) >/dev/null 2>&1; then echo __LP64__ | ${CC} ${CFLAGS} -E -x c -; fi | tail -n 1)

At that point CC is still make's builtin cc and CFLAGS is empty, because
the allow-override of CC from CROSS_COMPILE and the CFLAGS += $(CLANG_CROSS_FLAGS)
in Makefile.include only run on the following line.

LP64 then feeds libdir_relative and libdir in this same Makefile:

  ifeq ($(LP64), 1)
    libdir_relative = lib64
  else
    libdir_relative = lib
  endif
  libdir  ?= $(prefix)/$(libdir_relative)

and libdir is used by the install target:

  @$(INSTALL) -d $(DESTDIR)$(libdir)
  @$(INSTALL) -m 0644 libynl.a $(DESTDIR)$(libdir)/libynl.a

So does make CROSS_COMPILE=arm-linux-gnueabihf- install on an x86_64 host
end up installing the 32-bit libynl.a under $(prefix)/lib64, with the
reverse happening on a 32-bit host cross-building for aarch64?

Other in-tree users of the same LP64/libdir_relative pattern include
Makefile.include first, for example tools/lib/perf/Makefile:

  include $(srctree)/tools/scripts/Makefile.include
  include $(srctree)/tools/scripts/Makefile.arch

Would simply swapping the two includes here be enough?

> diff --git a/tools/net/ynl/generated/Makefile b/tools/net/ynl/generated/Makefile
> index ea4128f612d6..5a186349b5a8 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 9b98c0599600..7b3eae89982f 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
> @@ -8,7 +9,6 @@ endif

[Severity: Medium]
The commit message says the change accounts for "the cross-compiling options
such as CROSS_COMPILE or the LLVM-specific variables". Does this also cover
the documented LLVM form described in Documentation/kbuild/llvm.rst, that is
make LLVM=1 ARCH=<arch> with no CROSS_COMPILE?

In tools/scripts/Makefile.include, the target triple is only appended inside
the CROSS_COMPILE branch:

  else ifneq ($(CROSS_COMPILE),)
  ifeq ($(CLANG_CROSS_FLAGS),)
  CLANG_TARGET := $(notdir $(CROSS_COMPILE:%-=%))
  ...
  CFLAGS += $(CLANG_CROSS_FLAGS)

With LLVM=1 and an empty CROSS_COMPILE, CC resolves to plain clang with no
--target, so lib/ynl.o and generated/*-user.o would be built for the host.

For the selftests path, tools/testing/selftests/lib.mk computes a
target-aware compiler from ARCH:

  CC := $(CLANG) $(CLANG_FLAGS) -fintegrated-as

but tools/testing/selftests/net/ynl.mk only forwards GENS and RSTS to the
sub-make:

  $(Q)$(MAKE) -C $(top_srcdir)/tools/net/ynl \
  	GENS="$(YNL_GENS)" RSTS="" libynl.a

Since ynl.mk forces the YNL_GEN_FILES targets (ncdevmem, toeplitz in
tools/testing/selftests/drivers/net/hw/Makefile) to link $(OUTPUT)/libynl.a,
does make LLVM=1 ARCH=arm64 -C tools/testing/selftests/drivers/net/hw fail at
link time with host objects in libynl.a against aarch64 objects?

If that configuration is out of scope for now, could the commit message say
so? The later patch "selftests: drv-net: Use cross-compilation environment
for the io_uring check" states the limitation explicitly ("this doesn't cover
the LLVM cross-compiling case though"), while this one reads as if the LLVM
case is handled.

[ ... ]

> diff --git a/tools/net/ynl/ynltool/Makefile b/tools/net/ynl/ynltool/Makefile
> index 48b0f32050f0..85cc0840b403 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
>  
>  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

[Severity: Low]
This isn't a bug, but now that Makefile.include is pulled in here, the
existing Q assignment later in this same file disagrees with it:

tools/scripts/Makefile.include:
  ifeq ($(V),1)
    quiet =
    Q =
  else
    quiet = quiet_
    Q = @
  endif
  ...
  export quiet Q

tools/net/ynl/ynltool/Makefile, after the include:
  Q = @

  $(YNLTOOL): ../libynl.a $(OBJS)
  	$(Q)echo -e "\tLINK $@"
  	$(Q)$(CC) $(CFLAGS) -o $@ $(OBJS) ../libynl.a -lm

Since the local assignment comes after the include, does make V=1 -C
tools/net/ynl/ynltool still hide the CC and LINK command lines, which is
what one would want to look at when debugging a cross build? The same
overridden Q is also exported into the $(MAKE) -C ../ recursion for
../libynl.a. Could the local Q = @ line be dropped now?

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