linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] objtool build improvements
@ 2023-01-05  9:01 Ian Rogers
  2023-01-05  9:01 ` [PATCH v3 1/3] objtool: Install libsubcmd in build Ian Rogers
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Ian Rogers @ 2023-01-05  9:01 UTC (permalink / raw)
  To: Josh Poimboeuf, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, Nicolas Schier,
	linux-kernel, llvm
  Cc: Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers

Install libsubcmd and then get headers from there, this avoids
inadvertent dependencies on things in tools/lib. Fix V=1
support. Clean up how HOSTCC is used to override CC to avoid CFLAGS
being set for say gcc, and then CC being overridden to clang. Support
HOSTCFLAGS as a make option.

v3. Is a rebase that removes the merged "tools lib subcmd: Add install
    target" patch. In:
https://lore.kernel.org/lkml/CAKwvOd=kgXmpfbVa1wiEvwL0tX3gu+dDTGi-HEiRXSojwCLRrg@mail.gmail.com/
    Nick rightly points out that:
WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
    became:
WARNINGS := -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
    losing the EXTRA_WARNINGS which v3 now adds back in. Previous
    testing had added the warnings to the end rather than the
    beginning, thereby causing unexpected build issues that aren't present in v3.
v2. Include required "tools lib subcmd: Add install target" that is
    already in Arnaldo's tree:
https://lore.kernel.org/lkml/20221109184914.1357295-3-irogers@google.com/
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=630ae80ea1dd253609cb50cff87f3248f901aca3
    When building libsubcmd.a from objtool's Makefile, clear the
    subdir to avoid it being appended onto OUTPUT and breaking the
    build.

Ian Rogers (3):
  objtool: Install libsubcmd in build
  objtool: Properly support make V=1
  objtool: Alter how HOSTCC is forced

 tools/objtool/Build    |  2 --
 tools/objtool/Makefile | 66 ++++++++++++++++++++++++++++++------------
 2 files changed, 47 insertions(+), 21 deletions(-)

-- 
2.39.0.314.g84b9a713c41-goog


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3 1/3] objtool: Install libsubcmd in build
  2023-01-05  9:01 [PATCH v3 0/3] objtool build improvements Ian Rogers
@ 2023-01-05  9:01 ` Ian Rogers
  2023-01-12 20:25   ` Nicolas Schier
  2023-01-26  1:46   ` Josh Poimboeuf
  2023-01-05  9:01 ` [PATCH v3 2/3] objtool: Properly support make V=1 Ian Rogers
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 15+ messages in thread
From: Ian Rogers @ 2023-01-05  9:01 UTC (permalink / raw)
  To: Josh Poimboeuf, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, Nicolas Schier,
	linux-kernel, llvm
  Cc: Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers

Including from tools/lib can create inadvertent dependencies. Install
libsubcmd in the objtool build and then include the headers from
there.

Signed-off-by: Ian Rogers <irogers@google.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
 tools/objtool/Build    |  2 --
 tools/objtool/Makefile | 33 +++++++++++++++++++++++++--------
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/tools/objtool/Build b/tools/objtool/Build
index 33f2ee5a46d3..a3cdf8af6635 100644
--- a/tools/objtool/Build
+++ b/tools/objtool/Build
@@ -16,8 +16,6 @@ objtool-y += libctype.o
 objtool-y += str_error_r.o
 objtool-y += librbtree.o
 
-CFLAGS += -I$(srctree)/tools/lib
-
 $(OUTPUT)libstring.o: ../lib/string.c FORCE
 	$(call rule_mkdir)
 	$(call if_changed_dep,cc_o_c)
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index a3a9cc24e0e3..fd9b3e3113c6 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -12,9 +12,15 @@ srctree := $(patsubst %/,%,$(dir $(CURDIR)))
 srctree := $(patsubst %/,%,$(dir $(srctree)))
 endif
 
-SUBCMD_SRCDIR		= $(srctree)/tools/lib/subcmd/
-LIBSUBCMD_OUTPUT	= $(or $(OUTPUT),$(CURDIR)/)
-LIBSUBCMD		= $(LIBSUBCMD_OUTPUT)libsubcmd.a
+LIBSUBCMD_DIR = $(srctree)/tools/lib/subcmd/
+ifneq ($(OUTPUT),)
+  LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
+else
+  LIBSUBCMD_OUTPUT = $(CURDIR)/libsubcmd
+endif
+LIBSUBCMD_DESTDIR = $(LIBSUBCMD_OUTPUT)
+LIBSUBCMD = $(LIBSUBCMD_OUTPUT)/libsubcmd.a
+CFLAGS += -I$(LIBSUBCMD_OUTPUT)/include
 
 OBJTOOL    := $(OUTPUT)objtool
 OBJTOOL_IN := $(OBJTOOL)-in.o
@@ -28,7 +34,8 @@ INCLUDES := -I$(srctree)/tools/include \
 	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
 	    -I$(srctree)/tools/arch/$(SRCARCH)/include	\
 	    -I$(srctree)/tools/objtool/include \
-	    -I$(srctree)/tools/objtool/arch/$(SRCARCH)/include
+	    -I$(srctree)/tools/objtool/arch/$(SRCARCH)/include \
+	    -I$(LIBSUBCMD_OUTPUT)/include
 WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
 CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
 LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
@@ -38,6 +45,7 @@ elfshdr := $(shell echo '$(pound)include <libelf.h>' | $(CC) $(CFLAGS) -x c -E -
 CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
 
 AWK = awk
+MKDIR = mkdir
 
 BUILD_ORC := n
 
@@ -57,13 +65,22 @@ $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
 	$(QUIET_LINK)$(CC) $(OBJTOOL_IN) $(LDFLAGS) -o $@
 
 
-$(LIBSUBCMD): fixdep FORCE
-	$(Q)$(MAKE) -C $(SUBCMD_SRCDIR) OUTPUT=$(LIBSUBCMD_OUTPUT)
+$(LIBSUBCMD_OUTPUT):
+	@$(MKDIR) -p $@
+
+$(LIBSUBCMD): fixdep FORCE $(LIBSUBCMD_OUTPUT)
+	@$(MAKE) -C $(LIBSUBCMD_DIR) O=$(LIBSUBCMD_OUTPUT) \
+		DESTDIR=$(LIBSUBCMD_DESTDIR) prefix= subdir= \
+		$@ install_headers
+
+$(LIBSUBCMD)-clean:
+	$(call QUIET_CLEAN, libsubcmd)
+	$(Q)$(RM) -r -- $(LIBSUBCMD_OUTPUT)
 
-clean:
+clean: $(LIBSUBCMD)-clean
 	$(call QUIET_CLEAN, objtool) $(RM) $(OBJTOOL)
 	$(Q)find $(OUTPUT) -name '*.o' -delete -o -name '\.*.cmd' -delete -o -name '\.*.d' -delete
-	$(Q)$(RM) $(OUTPUT)arch/x86/lib/inat-tables.c $(OUTPUT)fixdep $(LIBSUBCMD)
+	$(Q)$(RM) $(OUTPUT)arch/x86/lib/inat-tables.c $(OUTPUT)fixdep
 
 FORCE:
 
-- 
2.39.0.314.g84b9a713c41-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 2/3] objtool: Properly support make V=1
  2023-01-05  9:01 [PATCH v3 0/3] objtool build improvements Ian Rogers
  2023-01-05  9:01 ` [PATCH v3 1/3] objtool: Install libsubcmd in build Ian Rogers
@ 2023-01-05  9:01 ` Ian Rogers
  2023-01-12 20:31   ` Nicolas Schier
  2023-01-05  9:01 ` [PATCH v3 3/3] objtool: Alter how HOSTCC is forced Ian Rogers
  2023-01-12 17:41 ` [PATCH v3 0/3] objtool build improvements Ian Rogers
  3 siblings, 1 reply; 15+ messages in thread
From: Ian Rogers @ 2023-01-05  9:01 UTC (permalink / raw)
  To: Josh Poimboeuf, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, Nicolas Schier,
	linux-kernel, llvm
  Cc: Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers

The Q variable was being used but never correctly set up. Add the
setting up and use in place of @.

Signed-off-by: Ian Rogers <irogers@google.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
 tools/objtool/Makefile | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index fd9b3e3113c6..61a00b7acae9 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -47,6 +47,12 @@ CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
 AWK = awk
 MKDIR = mkdir
 
+ifeq ($(V),1)
+  Q =
+else
+  Q = @
+endif
+
 BUILD_ORC := n
 
 ifeq ($(SRCARCH),x86)
@@ -58,18 +64,18 @@ export srctree OUTPUT CFLAGS SRCARCH AWK
 include $(srctree)/tools/build/Makefile.include
 
 $(OBJTOOL_IN): fixdep FORCE
-	@$(CONFIG_SHELL) ./sync-check.sh
-	@$(MAKE) $(build)=objtool
+	$(Q)$(CONFIG_SHELL) ./sync-check.sh
+	$(Q)$(MAKE) $(build)=objtool
 
 $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
 	$(QUIET_LINK)$(CC) $(OBJTOOL_IN) $(LDFLAGS) -o $@
 
 
 $(LIBSUBCMD_OUTPUT):
-	@$(MKDIR) -p $@
+	$(Q)$(MKDIR) -p $@
 
 $(LIBSUBCMD): fixdep FORCE $(LIBSUBCMD_OUTPUT)
-	@$(MAKE) -C $(LIBSUBCMD_DIR) O=$(LIBSUBCMD_OUTPUT) \
+	$(Q)$(MAKE) -C $(LIBSUBCMD_DIR) O=$(LIBSUBCMD_OUTPUT) \
 		DESTDIR=$(LIBSUBCMD_DESTDIR) prefix= subdir= \
 		$@ install_headers
 
-- 
2.39.0.314.g84b9a713c41-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 3/3] objtool: Alter how HOSTCC is forced
  2023-01-05  9:01 [PATCH v3 0/3] objtool build improvements Ian Rogers
  2023-01-05  9:01 ` [PATCH v3 1/3] objtool: Install libsubcmd in build Ian Rogers
  2023-01-05  9:01 ` [PATCH v3 2/3] objtool: Properly support make V=1 Ian Rogers
@ 2023-01-05  9:01 ` Ian Rogers
  2023-01-09 23:05   ` Nick Desaulniers
                     ` (2 more replies)
  2023-01-12 17:41 ` [PATCH v3 0/3] objtool build improvements Ian Rogers
  3 siblings, 3 replies; 15+ messages in thread
From: Ian Rogers @ 2023-01-05  9:01 UTC (permalink / raw)
  To: Josh Poimboeuf, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, Nicolas Schier,
	linux-kernel, llvm
  Cc: Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers

HOSTCC is always wanted when building objtool. Setting CC to HOSTCC
happens after tools/scripts/Makefile.include is included, meaning
flags are set assuming say CC is gcc, but then it can be later set to
HOSTCC which may be clang. tools/scripts/Makefile.include is needed
for host set up and common macros in objtool's Makefile. Rather than
override CC to HOSTCC, just pass CC as HOSTCC to Makefile.build, the
libsubcmd builds and the linkage step. This means the Makefiles don't
see things like CC changing and tool flag determination, and similar,
work properly. To avoid mixing CFLAGS from different compilers just
the objtool CFLAGS are determined with the exception of
EXTRA_WARNINGS. HOSTCFLAGS is added to these so that command line
flags can add to the CFLAGS.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/objtool/Makefile | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 61a00b7acae9..49956f4f58b9 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -2,16 +2,12 @@
 include ../scripts/Makefile.include
 include ../scripts/Makefile.arch
 
-# always use the host compiler
-AR	 = $(HOSTAR)
-CC	 = $(HOSTCC)
-LD	 = $(HOSTLD)
-
 ifeq ($(srctree),)
 srctree := $(patsubst %/,%,$(dir $(CURDIR)))
 srctree := $(patsubst %/,%,$(dir $(srctree)))
 endif
 
+MAKE = make -S
 LIBSUBCMD_DIR = $(srctree)/tools/lib/subcmd/
 ifneq ($(OUTPUT),)
   LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
@@ -37,12 +33,19 @@ INCLUDES := -I$(srctree)/tools/include \
 	    -I$(srctree)/tools/objtool/arch/$(SRCARCH)/include \
 	    -I$(LIBSUBCMD_OUTPUT)/include
 WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
-CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
-LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
+OBJTOOL_CFLAGS := -Werror $(WARNINGS) -g $(INCLUDES) $(LIBELF_FLAGS) $(HOSTCFLAGS)
+OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD)
 
 # Allow old libelf to be used:
 elfshdr := $(shell echo '$(pound)include <libelf.h>' | $(CC) $(CFLAGS) -x c -E - | grep elf_getshdr)
-CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
+OBJTOOL_CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
+
+# Always want host compilation.
+HOST_OVERRIDES := CC="$(HOSTCC)" EXTRA_CFLAGS="$(OBJTOOL_CFLAGS)" \
+		  LD="$(HOSTLD)" AR="$(HOSTAR)"
+BUILD_HOST_OVERRIDES := CC="$(HOSTCC)" CFLAGS="$(OBJTOOL_CFLAGS)" \
+			LD="$(HOSTLD)" LDFLAGS="$(OBJTOOL_LDFLAGS)" \
+			AR="$(HOSTAR)"
 
 AWK = awk
 MKDIR = mkdir
@@ -65,10 +68,11 @@ include $(srctree)/tools/build/Makefile.include
 
 $(OBJTOOL_IN): fixdep FORCE
 	$(Q)$(CONFIG_SHELL) ./sync-check.sh
-	$(Q)$(MAKE) $(build)=objtool
+	$(Q)$(MAKE) $(build)=objtool $(BUILD_HOST_OVERRIDES)
+
 
 $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
-	$(QUIET_LINK)$(CC) $(OBJTOOL_IN) $(LDFLAGS) -o $@
+	$(QUIET_LINK)$(HOSTCC) $(OBJTOOL_IN) $(KBUILD_HOSTLDFLAGS) $(OBJTOOL_LDFLAGS) -o $@
 
 
 $(LIBSUBCMD_OUTPUT):
@@ -77,6 +81,7 @@ $(LIBSUBCMD_OUTPUT):
 $(LIBSUBCMD): fixdep FORCE $(LIBSUBCMD_OUTPUT)
 	$(Q)$(MAKE) -C $(LIBSUBCMD_DIR) O=$(LIBSUBCMD_OUTPUT) \
 		DESTDIR=$(LIBSUBCMD_DESTDIR) prefix= subdir= \
+		$(HOST_OVERRIDES) \
 		$@ install_headers
 
 $(LIBSUBCMD)-clean:
-- 
2.39.0.314.g84b9a713c41-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 3/3] objtool: Alter how HOSTCC is forced
  2023-01-05  9:01 ` [PATCH v3 3/3] objtool: Alter how HOSTCC is forced Ian Rogers
@ 2023-01-09 23:05   ` Nick Desaulniers
  2023-01-12 20:40   ` Nicolas Schier
  2023-01-26  1:49   ` Josh Poimboeuf
  2 siblings, 0 replies; 15+ messages in thread
From: Nick Desaulniers @ 2023-01-09 23:05 UTC (permalink / raw)
  To: Ian Rogers, Josh Poimboeuf, Peter Zijlstra
  Cc: Nathan Chancellor, Tom Rix, Masahiro Yamada, Nicolas Schier,
	linux-kernel, llvm, Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

On Thu, Jan 5, 2023 at 1:02 AM Ian Rogers <irogers@google.com> wrote:
>
> HOSTCC is always wanted when building objtool. Setting CC to HOSTCC
> happens after tools/scripts/Makefile.include is included, meaning
> flags are set assuming say CC is gcc, but then it can be later set to
> HOSTCC which may be clang. tools/scripts/Makefile.include is needed
> for host set up and common macros in objtool's Makefile. Rather than
> override CC to HOSTCC, just pass CC as HOSTCC to Makefile.build, the
> libsubcmd builds and the linkage step. This means the Makefiles don't
> see things like CC changing and tool flag determination, and similar,
> work properly. To avoid mixing CFLAGS from different compilers just
> the objtool CFLAGS are determined with the exception of
> EXTRA_WARNINGS. HOSTCFLAGS is added to these so that command line
> flags can add to the CFLAGS.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Thanks Ian, and happy new year! Sorry for delays reviewing this; I'm
just getting back online today.  Assuming Peter and Josh may be too,
otherwise can Peter or Josh PTAL?

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>

(I'm happy that this allows me to now pass HOSTCFLAGS on to objtool
successfully)

> ---
>  tools/objtool/Makefile | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> index 61a00b7acae9..49956f4f58b9 100644
> --- a/tools/objtool/Makefile
> +++ b/tools/objtool/Makefile
> @@ -2,16 +2,12 @@
>  include ../scripts/Makefile.include
>  include ../scripts/Makefile.arch
>
> -# always use the host compiler
> -AR      = $(HOSTAR)
> -CC      = $(HOSTCC)
> -LD      = $(HOSTLD)
> -
>  ifeq ($(srctree),)
>  srctree := $(patsubst %/,%,$(dir $(CURDIR)))
>  srctree := $(patsubst %/,%,$(dir $(srctree)))
>  endif
>
> +MAKE = make -S
>  LIBSUBCMD_DIR = $(srctree)/tools/lib/subcmd/
>  ifneq ($(OUTPUT),)
>    LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
> @@ -37,12 +33,19 @@ INCLUDES := -I$(srctree)/tools/include \
>             -I$(srctree)/tools/objtool/arch/$(SRCARCH)/include \
>             -I$(LIBSUBCMD_OUTPUT)/include
>  WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> -CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> -LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> +OBJTOOL_CFLAGS := -Werror $(WARNINGS) -g $(INCLUDES) $(LIBELF_FLAGS) $(HOSTCFLAGS)
> +OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD)
>
>  # Allow old libelf to be used:
>  elfshdr := $(shell echo '$(pound)include <libelf.h>' | $(CC) $(CFLAGS) -x c -E - | grep elf_getshdr)
> -CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
> +OBJTOOL_CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
> +
> +# Always want host compilation.
> +HOST_OVERRIDES := CC="$(HOSTCC)" EXTRA_CFLAGS="$(OBJTOOL_CFLAGS)" \
> +                 LD="$(HOSTLD)" AR="$(HOSTAR)"
> +BUILD_HOST_OVERRIDES := CC="$(HOSTCC)" CFLAGS="$(OBJTOOL_CFLAGS)" \
> +                       LD="$(HOSTLD)" LDFLAGS="$(OBJTOOL_LDFLAGS)" \
> +                       AR="$(HOSTAR)"
>
>  AWK = awk
>  MKDIR = mkdir
> @@ -65,10 +68,11 @@ include $(srctree)/tools/build/Makefile.include
>
>  $(OBJTOOL_IN): fixdep FORCE
>         $(Q)$(CONFIG_SHELL) ./sync-check.sh
> -       $(Q)$(MAKE) $(build)=objtool
> +       $(Q)$(MAKE) $(build)=objtool $(BUILD_HOST_OVERRIDES)
> +
>
>  $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
> -       $(QUIET_LINK)$(CC) $(OBJTOOL_IN) $(LDFLAGS) -o $@
> +       $(QUIET_LINK)$(HOSTCC) $(OBJTOOL_IN) $(KBUILD_HOSTLDFLAGS) $(OBJTOOL_LDFLAGS) -o $@
>
>
>  $(LIBSUBCMD_OUTPUT):
> @@ -77,6 +81,7 @@ $(LIBSUBCMD_OUTPUT):
>  $(LIBSUBCMD): fixdep FORCE $(LIBSUBCMD_OUTPUT)
>         $(Q)$(MAKE) -C $(LIBSUBCMD_DIR) O=$(LIBSUBCMD_OUTPUT) \
>                 DESTDIR=$(LIBSUBCMD_DESTDIR) prefix= subdir= \
> +               $(HOST_OVERRIDES) \
>                 $@ install_headers
>
>  $(LIBSUBCMD)-clean:
> --
> 2.39.0.314.g84b9a713c41-goog
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 0/3] objtool build improvements
  2023-01-05  9:01 [PATCH v3 0/3] objtool build improvements Ian Rogers
                   ` (2 preceding siblings ...)
  2023-01-05  9:01 ` [PATCH v3 3/3] objtool: Alter how HOSTCC is forced Ian Rogers
@ 2023-01-12 17:41 ` Ian Rogers
  2023-01-19 16:12   ` Ian Rogers
  3 siblings, 1 reply; 15+ messages in thread
From: Ian Rogers @ 2023-01-12 17:41 UTC (permalink / raw)
  To: Josh Poimboeuf, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, Nicolas Schier,
	linux-kernel, llvm
  Cc: Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

On Thu, Jan 5, 2023 at 1:02 AM Ian Rogers <irogers@google.com> wrote:
>
> Install libsubcmd and then get headers from there, this avoids
> inadvertent dependencies on things in tools/lib. Fix V=1
> support. Clean up how HOSTCC is used to override CC to avoid CFLAGS
> being set for say gcc, and then CC being overridden to clang. Support
> HOSTCFLAGS as a make option.
>
> v3. Is a rebase that removes the merged "tools lib subcmd: Add install
>     target" patch. In:
> https://lore.kernel.org/lkml/CAKwvOd=kgXmpfbVa1wiEvwL0tX3gu+dDTGi-HEiRXSojwCLRrg@mail.gmail.com/
>     Nick rightly points out that:
> WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
>     became:
> WARNINGS := -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
>     losing the EXTRA_WARNINGS which v3 now adds back in. Previous
>     testing had added the warnings to the end rather than the
>     beginning, thereby causing unexpected build issues that aren't present in v3.
> v2. Include required "tools lib subcmd: Add install target" that is
>     already in Arnaldo's tree:
> https://lore.kernel.org/lkml/20221109184914.1357295-3-irogers@google.com/
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=630ae80ea1dd253609cb50cff87f3248f901aca3
>     When building libsubcmd.a from objtool's Makefile, clear the
>     subdir to avoid it being appended onto OUTPUT and breaking the
>     build.
>
> Ian Rogers (3):
>   objtool: Install libsubcmd in build
>   objtool: Properly support make V=1
>   objtool: Alter how HOSTCC is forced
>
>  tools/objtool/Build    |  2 --
>  tools/objtool/Makefile | 66 ++++++++++++++++++++++++++++++------------
>  2 files changed, 47 insertions(+), 21 deletions(-)

Ping. Relatively small set of patches, with Reviewed-by and Tested-by,
would be nice to land. Thanks!

Ian

> --
> 2.39.0.314.g84b9a713c41-goog
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 1/3] objtool: Install libsubcmd in build
  2023-01-05  9:01 ` [PATCH v3 1/3] objtool: Install libsubcmd in build Ian Rogers
@ 2023-01-12 20:25   ` Nicolas Schier
  2023-01-12 20:54     ` Ian Rogers
  2023-01-26  1:46   ` Josh Poimboeuf
  1 sibling, 1 reply; 15+ messages in thread
From: Nicolas Schier @ 2023-01-12 20:25 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Josh Poimboeuf, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, linux-kernel, llvm,
	Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

[-- Attachment #1: Type: text/plain, Size: 1399 bytes --]

On Thu, Jan 05, 2023 at 01:01:53AM -0800 Ian Rogers wrote:
> Including from tools/lib can create inadvertent dependencies. Install
> libsubcmd in the objtool build and then include the headers from
> there.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  tools/objtool/Build    |  2 --
>  tools/objtool/Makefile | 33 +++++++++++++++++++++++++--------
>  2 files changed, 25 insertions(+), 10 deletions(-)
> 
[...]
> diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> index a3a9cc24e0e3..fd9b3e3113c6 100644
> --- a/tools/objtool/Makefile
> +++ b/tools/objtool/Makefile
> @@ -12,9 +12,15 @@ srctree := $(patsubst %/,%,$(dir $(CURDIR)))
>  srctree := $(patsubst %/,%,$(dir $(srctree)))
>  endif
>  
> -SUBCMD_SRCDIR		= $(srctree)/tools/lib/subcmd/
> -LIBSUBCMD_OUTPUT	= $(or $(OUTPUT),$(CURDIR)/)
> -LIBSUBCMD		= $(LIBSUBCMD_OUTPUT)libsubcmd.a
> +LIBSUBCMD_DIR = $(srctree)/tools/lib/subcmd/
> +ifneq ($(OUTPUT),)
> +  LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
> +else
> +  LIBSUBCMD_OUTPUT = $(CURDIR)/libsubcmd
> +endif
> +LIBSUBCMD_DESTDIR = $(LIBSUBCMD_OUTPUT)

Hi Ian,

Is there a reason for distinguishing between $(LIBSUBCMD_DESTDIR) and
$(LIBSUBCMD_OUTPUT)?

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/3] objtool: Properly support make V=1
  2023-01-05  9:01 ` [PATCH v3 2/3] objtool: Properly support make V=1 Ian Rogers
@ 2023-01-12 20:31   ` Nicolas Schier
  0 siblings, 0 replies; 15+ messages in thread
From: Nicolas Schier @ 2023-01-12 20:31 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Josh Poimboeuf, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, linux-kernel, llvm,
	Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On Thu, Jan 05, 2023 at 01:01:54AM -0800 Ian Rogers wrote:
> The Q variable was being used but never correctly set up. Add the
> setting up and use in place of @.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  tools/objtool/Makefile | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 3/3] objtool: Alter how HOSTCC is forced
  2023-01-05  9:01 ` [PATCH v3 3/3] objtool: Alter how HOSTCC is forced Ian Rogers
  2023-01-09 23:05   ` Nick Desaulniers
@ 2023-01-12 20:40   ` Nicolas Schier
  2023-01-26  1:49   ` Josh Poimboeuf
  2 siblings, 0 replies; 15+ messages in thread
From: Nicolas Schier @ 2023-01-12 20:40 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Josh Poimboeuf, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, linux-kernel, llvm,
	Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

On Thu, Jan 05, 2023 at 01:01:55AM -0800 Ian Rogers wrote:
> HOSTCC is always wanted when building objtool. Setting CC to HOSTCC
> happens after tools/scripts/Makefile.include is included, meaning
> flags are set assuming say CC is gcc, but then it can be later set to
> HOSTCC which may be clang. tools/scripts/Makefile.include is needed
> for host set up and common macros in objtool's Makefile. Rather than
> override CC to HOSTCC, just pass CC as HOSTCC to Makefile.build, the
> libsubcmd builds and the linkage step. This means the Makefiles don't
> see things like CC changing and tool flag determination, and similar,
> work properly. To avoid mixing CFLAGS from different compilers just
> the objtool CFLAGS are determined with the exception of
> EXTRA_WARNINGS. HOSTCFLAGS is added to these so that command line
> flags can add to the CFLAGS.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/objtool/Makefile | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 1/3] objtool: Install libsubcmd in build
  2023-01-12 20:25   ` Nicolas Schier
@ 2023-01-12 20:54     ` Ian Rogers
  0 siblings, 0 replies; 15+ messages in thread
From: Ian Rogers @ 2023-01-12 20:54 UTC (permalink / raw)
  To: Nicolas Schier
  Cc: Josh Poimboeuf, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, linux-kernel, llvm,
	Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

On Thu, Jan 12, 2023 at 12:32 PM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Thu, Jan 05, 2023 at 01:01:53AM -0800 Ian Rogers wrote:
> > Including from tools/lib can create inadvertent dependencies. Install
> > libsubcmd in the objtool build and then include the headers from
> > there.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> > Tested-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> >  tools/objtool/Build    |  2 --
> >  tools/objtool/Makefile | 33 +++++++++++++++++++++++++--------
> >  2 files changed, 25 insertions(+), 10 deletions(-)
> >
> [...]
> > diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> > index a3a9cc24e0e3..fd9b3e3113c6 100644
> > --- a/tools/objtool/Makefile
> > +++ b/tools/objtool/Makefile
> > @@ -12,9 +12,15 @@ srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> >  srctree := $(patsubst %/,%,$(dir $(srctree)))
> >  endif
> >
> > -SUBCMD_SRCDIR                = $(srctree)/tools/lib/subcmd/
> > -LIBSUBCMD_OUTPUT     = $(or $(OUTPUT),$(CURDIR)/)
> > -LIBSUBCMD            = $(LIBSUBCMD_OUTPUT)libsubcmd.a
> > +LIBSUBCMD_DIR = $(srctree)/tools/lib/subcmd/
> > +ifneq ($(OUTPUT),)
> > +  LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
> > +else
> > +  LIBSUBCMD_OUTPUT = $(CURDIR)/libsubcmd
> > +endif
> > +LIBSUBCMD_DESTDIR = $(LIBSUBCMD_OUTPUT)
>
> Hi Ian,
>
> Is there a reason for distinguishing between $(LIBSUBCMD_DESTDIR) and
> $(LIBSUBCMD_OUTPUT)?

I believe the naming aligns with their use when passed to the
corresponding sub-make variable, so "DESTDIR=$(LIBSUBCMD_DESTDIR)". It
is possible to do other this, a slightly different convention is in
some BPF related code:
https://lore.kernel.org/lkml/20230112004024.1934601-1-irogers@google.com/

Thanks,
Ian

> Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 0/3] objtool build improvements
  2023-01-12 17:41 ` [PATCH v3 0/3] objtool build improvements Ian Rogers
@ 2023-01-19 16:12   ` Ian Rogers
  0 siblings, 0 replies; 15+ messages in thread
From: Ian Rogers @ 2023-01-19 16:12 UTC (permalink / raw)
  To: Josh Poimboeuf, Peter Zijlstra, Nathan Chancellor,
	Nick Desaulniers, Tom Rix, Masahiro Yamada, Nicolas Schier,
	linux-kernel, llvm
  Cc: Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

On Thu, Jan 12, 2023 at 9:41 AM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Jan 5, 2023 at 1:02 AM Ian Rogers <irogers@google.com> wrote:
> >
> > Install libsubcmd and then get headers from there, this avoids
> > inadvertent dependencies on things in tools/lib. Fix V=1
> > support. Clean up how HOSTCC is used to override CC to avoid CFLAGS
> > being set for say gcc, and then CC being overridden to clang. Support
> > HOSTCFLAGS as a make option.
> >
> > v3. Is a rebase that removes the merged "tools lib subcmd: Add install
> >     target" patch. In:
> > https://lore.kernel.org/lkml/CAKwvOd=kgXmpfbVa1wiEvwL0tX3gu+dDTGi-HEiRXSojwCLRrg@mail.gmail.com/
> >     Nick rightly points out that:
> > WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> >     became:
> > WARNINGS := -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> >     losing the EXTRA_WARNINGS which v3 now adds back in. Previous
> >     testing had added the warnings to the end rather than the
> >     beginning, thereby causing unexpected build issues that aren't present in v3.
> > v2. Include required "tools lib subcmd: Add install target" that is
> >     already in Arnaldo's tree:
> > https://lore.kernel.org/lkml/20221109184914.1357295-3-irogers@google.com/
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=630ae80ea1dd253609cb50cff87f3248f901aca3
> >     When building libsubcmd.a from objtool's Makefile, clear the
> >     subdir to avoid it being appended onto OUTPUT and breaking the
> >     build.
> >
> > Ian Rogers (3):
> >   objtool: Install libsubcmd in build
> >   objtool: Properly support make V=1
> >   objtool: Alter how HOSTCC is forced
> >
> >  tools/objtool/Build    |  2 --
> >  tools/objtool/Makefile | 66 ++++++++++++++++++++++++++++++------------
> >  2 files changed, 47 insertions(+), 21 deletions(-)
>
> Ping. Relatively small set of patches, with Reviewed-by and Tested-by,
> would be nice to land. Thanks!
>
> Ian

Ping2. Small set of patches, 2x Reviewed-by and 1x Tested-by.

Thanks,
Ian

>
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 1/3] objtool: Install libsubcmd in build
  2023-01-05  9:01 ` [PATCH v3 1/3] objtool: Install libsubcmd in build Ian Rogers
  2023-01-12 20:25   ` Nicolas Schier
@ 2023-01-26  1:46   ` Josh Poimboeuf
  2023-01-26 17:50     ` Ian Rogers
  1 sibling, 1 reply; 15+ messages in thread
From: Josh Poimboeuf @ 2023-01-26  1:46 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, llvm,
	Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

On Thu, Jan 05, 2023 at 01:01:53AM -0800, Ian Rogers wrote:
> Including from tools/lib can create inadvertent dependencies. Install
> libsubcmd in the objtool build and then include the headers from
> there.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>

After a make, "git status" shows:

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	tools/objtool/libsubcmd/

So tools/objtool/.gitignore needs an update.

> --- a/tools/objtool/Makefile
> +++ b/tools/objtool/Makefile
> @@ -12,9 +12,15 @@ srctree := $(patsubst %/,%,$(dir $(CURDIR)))
>  srctree := $(patsubst %/,%,$(dir $(srctree)))
>  endif
>  
> -SUBCMD_SRCDIR		= $(srctree)/tools/lib/subcmd/
> -LIBSUBCMD_OUTPUT	= $(or $(OUTPUT),$(CURDIR)/)
> -LIBSUBCMD		= $(LIBSUBCMD_OUTPUT)libsubcmd.a
> +LIBSUBCMD_DIR = $(srctree)/tools/lib/subcmd/
> +ifneq ($(OUTPUT),)
> +  LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
> +else
> +  LIBSUBCMD_OUTPUT = $(CURDIR)/libsubcmd
> +endif
> +LIBSUBCMD_DESTDIR = $(LIBSUBCMD_OUTPUT)

Similar to Nicolas' comment, it's confusing to have two variables with
the same value.  Please s/LIBSUBCMD_DESTDIR/LIBSUBCMD_OUTPUT/

> +LIBSUBCMD = $(LIBSUBCMD_OUTPUT)/libsubcmd.a
> +CFLAGS += -I$(LIBSUBCMD_OUTPUT)/include

As far as I can tell, this CFLAGS addition is both ineffective (it's
overwritten later) and unnecessary (it's made redundant by the same
addition to the INCLUDES variable).

-- 
Josh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 3/3] objtool: Alter how HOSTCC is forced
  2023-01-05  9:01 ` [PATCH v3 3/3] objtool: Alter how HOSTCC is forced Ian Rogers
  2023-01-09 23:05   ` Nick Desaulniers
  2023-01-12 20:40   ` Nicolas Schier
@ 2023-01-26  1:49   ` Josh Poimboeuf
  2023-01-26 18:30     ` Ian Rogers
  2 siblings, 1 reply; 15+ messages in thread
From: Josh Poimboeuf @ 2023-01-26  1:49 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, llvm,
	Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

On Thu, Jan 05, 2023 at 01:01:55AM -0800, Ian Rogers wrote:
> HOSTCC is always wanted when building objtool. Setting CC to HOSTCC
> happens after tools/scripts/Makefile.include is included, meaning
> flags are set assuming say CC is gcc, but then it can be later set to
> HOSTCC which may be clang. tools/scripts/Makefile.include is needed
> for host set up and common macros in objtool's Makefile. Rather than
> override CC to HOSTCC, just pass CC as HOSTCC to Makefile.build, the
> libsubcmd builds and the linkage step. This means the Makefiles don't
> see things like CC changing and tool flag determination, and similar,
> work properly.

I'm having trouble parsing this last sentence, can you rephrase or
restructure it?

> To avoid mixing CFLAGS from different compilers just
> the objtool CFLAGS are determined with the exception of
> EXTRA_WARNINGS.

I'm not really sure what this one means either.

> HOSTCFLAGS is added to these so that command line
> flags can add to the CFLAGS.

Overall this patch description is a big wall of dense text which I found
hard to decipher.  Please split it up into paragraphs and make it more
legible and logical.

For example, un-jumble the ordering, with the background first, then the
problem(s), then the fix(es).  (At least three paragraphs)

If possible, the subject should describe the end result for the user,
something like

  objtool: Fix HOSTCFLAGS cmdline support

... unless I'm misunderstanding the point of the patch.

HOSTCFLAGS from the cmdline *used* to work, a git bisect shows it
stopped working with

  96f14fe738b6 ("kbuild: Rename HOSTCFLAGS to KBUILD_HOSTCFLAGS")

... so please add a "Fixes" tag for that commit.

> +MAKE = make -S

Why?

>  LIBSUBCMD_DIR = $(srctree)/tools/lib/subcmd/
>  ifneq ($(OUTPUT),)
>    LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
> @@ -37,12 +33,19 @@ INCLUDES := -I$(srctree)/tools/include \
>  	    -I$(srctree)/tools/objtool/arch/$(SRCARCH)/include \
>  	    -I$(LIBSUBCMD_OUTPUT)/include
>  WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> -CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> -LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> +OBJTOOL_CFLAGS := -Werror $(WARNINGS) -g $(INCLUDES) $(LIBELF_FLAGS) $(HOSTCFLAGS)

I *think* I understand the reason for the switch from KBUILD_HOSTCFLAGS
to HOSTCFLAGS -- because KBUILD_HOSTCFLAGS is defined in the top-level
kernel Makefile which isn't included here so it's undefined? -- but
regardless that should be called out more explicitly as a problem being
fixed in the commit log.

This issue really has me scratching my head, as I could have sworn
objtool was being built with -O2.

> +OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD)

Is there a reason to not include $(HOSTLDFLAGS) here?

>  # Allow old libelf to be used:
>  elfshdr := $(shell echo '$(pound)include <libelf.h>' | $(CC) $(CFLAGS) -x c -E - | grep elf_getshdr)
> -CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
> +OBJTOOL_CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
> +
> +# Always want host compilation.
> +HOST_OVERRIDES := CC="$(HOSTCC)" EXTRA_CFLAGS="$(OBJTOOL_CFLAGS)" \
> +		  LD="$(HOSTLD)" AR="$(HOSTAR)"
> +BUILD_HOST_OVERRIDES := CC="$(HOSTCC)" CFLAGS="$(OBJTOOL_CFLAGS)" \
> +			LD="$(HOSTLD)" LDFLAGS="$(OBJTOOL_LDFLAGS)" \
> +			AR="$(HOSTAR)"

Maybe it depends on your perspective, but I'm thinking that some of
these aren't really overrides, but rather normal expected flags.  And
the distinction between these two variables is muddy: it's not only
Build vs non-Build, but also objtool vs libsubcmd.

How about just

  HOST_OVERRIDES := CC="$(HOSTCC) "LD="$(HOSTLD)" AR="$(HOSTAR)"

And then specifying the {EXTRA_}CFLAGS and LDFLAGS below where needed.

>  AWK = awk
>  MKDIR = mkdir
> @@ -65,10 +68,11 @@ include $(srctree)/tools/build/Makefile.include
>  
>  $(OBJTOOL_IN): fixdep FORCE
>  	$(Q)$(CONFIG_SHELL) ./sync-check.sh
> -	$(Q)$(MAKE) $(build)=objtool
> +	$(Q)$(MAKE) $(build)=objtool $(BUILD_HOST_OVERRIDES)
> +
>  
>  $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
> -	$(QUIET_LINK)$(CC) $(OBJTOOL_IN) $(LDFLAGS) -o $@
> +	$(QUIET_LINK)$(HOSTCC) $(OBJTOOL_IN) $(KBUILD_HOSTLDFLAGS) $(OBJTOOL_LDFLAGS) -o $@

Does KBUILD_HOSTLDFLAGS even work here?

-- 
Josh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 1/3] objtool: Install libsubcmd in build
  2023-01-26  1:46   ` Josh Poimboeuf
@ 2023-01-26 17:50     ` Ian Rogers
  0 siblings, 0 replies; 15+ messages in thread
From: Ian Rogers @ 2023-01-26 17:50 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Peter Zijlstra, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, llvm,
	Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

On Wed, Jan 25, 2023 at 5:46 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Thu, Jan 05, 2023 at 01:01:53AM -0800, Ian Rogers wrote:
> > Including from tools/lib can create inadvertent dependencies. Install
> > libsubcmd in the objtool build and then include the headers from
> > there.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> > Tested-by: Nick Desaulniers <ndesaulniers@google.com>
>
> After a make, "git status" shows:
>
> Untracked files:
>   (use "git add <file>..." to include in what will be committed)
>         tools/objtool/libsubcmd/
>
> So tools/objtool/.gitignore needs an update.

Done.

> > --- a/tools/objtool/Makefile
> > +++ b/tools/objtool/Makefile
> > @@ -12,9 +12,15 @@ srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> >  srctree := $(patsubst %/,%,$(dir $(srctree)))
> >  endif
> >
> > -SUBCMD_SRCDIR                = $(srctree)/tools/lib/subcmd/
> > -LIBSUBCMD_OUTPUT     = $(or $(OUTPUT),$(CURDIR)/)
> > -LIBSUBCMD            = $(LIBSUBCMD_OUTPUT)libsubcmd.a
> > +LIBSUBCMD_DIR = $(srctree)/tools/lib/subcmd/
> > +ifneq ($(OUTPUT),)
> > +  LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
> > +else
> > +  LIBSUBCMD_OUTPUT = $(CURDIR)/libsubcmd
> > +endif
> > +LIBSUBCMD_DESTDIR = $(LIBSUBCMD_OUTPUT)
>
> Similar to Nicolas' comment, it's confusing to have two variables with
> the same value.  Please s/LIBSUBCMD_DESTDIR/LIBSUBCMD_OUTPUT/

Done. I feel this is more of a meta question for the standard in the
current build. DESTDIR is related to installation, in this case header
files, OUTPUT is a build directory. Anyway, LIBSUBCMD_DESTDIR is gone
in v4.

> > +LIBSUBCMD = $(LIBSUBCMD_OUTPUT)/libsubcmd.a
> > +CFLAGS += -I$(LIBSUBCMD_OUTPUT)/include
>
> As far as I can tell, this CFLAGS addition is both ineffective (it's
> overwritten later) and unnecessary (it's made redundant by the same
> addition to the INCLUDES variable).

Agreed, removed in v4.

> --
> Josh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 3/3] objtool: Alter how HOSTCC is forced
  2023-01-26  1:49   ` Josh Poimboeuf
@ 2023-01-26 18:30     ` Ian Rogers
  0 siblings, 0 replies; 15+ messages in thread
From: Ian Rogers @ 2023-01-26 18:30 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Peter Zijlstra, Nathan Chancellor, Nick Desaulniers, Tom Rix,
	Masahiro Yamada, Nicolas Schier, linux-kernel, llvm,
	Stephane Eranian, Andrii Nakryiko, Jiri Olsa,
	Arnaldo Carvalho de Melo, Namhyung Kim

On Wed, Jan 25, 2023 at 5:49 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Thu, Jan 05, 2023 at 01:01:55AM -0800, Ian Rogers wrote:
> > HOSTCC is always wanted when building objtool. Setting CC to HOSTCC
> > happens after tools/scripts/Makefile.include is included, meaning
> > flags are set assuming say CC is gcc, but then it can be later set to
> > HOSTCC which may be clang. tools/scripts/Makefile.include is needed
> > for host set up and common macros in objtool's Makefile. Rather than
> > override CC to HOSTCC, just pass CC as HOSTCC to Makefile.build, the
> > libsubcmd builds and the linkage step. This means the Makefiles don't
> > see things like CC changing and tool flag determination, and similar,
> > work properly.
>
> I'm having trouble parsing this last sentence, can you rephrase or
> restructure it?

It was restating what was happening, deleted.

> > To avoid mixing CFLAGS from different compilers just
> > the objtool CFLAGS are determined with the exception of
> > EXTRA_WARNINGS.
>
> I'm not really sure what this one means either.

Moved to an inline comment.

> > HOSTCFLAGS is added to these so that command line
> > flags can add to the CFLAGS.
>
> Overall this patch description is a big wall of dense text which I found
> hard to decipher.  Please split it up into paragraphs and make it more
> legible and logical.

Yes, I've deleted most of the text now. I'd been adding to it as v1
went to v2 and so on.

> For example, un-jumble the ordering, with the background first, then the
> problem(s), then the fix(es).  (At least three paragraphs)
>
> If possible, the subject should describe the end result for the user,
> something like
>
>   objtool: Fix HOSTCFLAGS cmdline support
>
> ... unless I'm misunderstanding the point of the patch.

The patch is trying to fix the Makefile. Setting "CC=$(HOSTCC)" was
just wrong as apparent from looking at the behavior of
tools/scripts/Makefile.include. Some of that persists after this
change with WARNINGS, as now noted in a comment. A side effect of
fixing the Makefile is to make HOSTCFLAGS work, but I suspect with the
variable renames this may not be working. I'll leave that for yourself
and Nick. I told Nick I'd take a look at this as I saw the wrong use
of libsubcmd's headers and that was something I wanted to clean up
having done similar in tools/perf, along with the whole removal of
tools/lib/traceevent from Linux 6.2.

> HOSTCFLAGS from the cmdline *used* to work, a git bisect shows it
> stopped working with
>
>   96f14fe738b6 ("kbuild: Rename HOSTCFLAGS to KBUILD_HOSTCFLAGS")
>
> ... so please add a "Fixes" tag for that commit.

Left off, see later.

> > +MAKE = make -S
>
> Why?

Cruft, removed.

> >  LIBSUBCMD_DIR = $(srctree)/tools/lib/subcmd/
> >  ifneq ($(OUTPUT),)
> >    LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
> > @@ -37,12 +33,19 @@ INCLUDES := -I$(srctree)/tools/include \
> >           -I$(srctree)/tools/objtool/arch/$(SRCARCH)/include \
> >           -I$(LIBSUBCMD_OUTPUT)/include
> >  WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> > -CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> > -LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> > +OBJTOOL_CFLAGS := -Werror $(WARNINGS) -g $(INCLUDES) $(LIBELF_FLAGS) $(HOSTCFLAGS)
>
> I *think* I understand the reason for the switch from KBUILD_HOSTCFLAGS
> to HOSTCFLAGS -- because KBUILD_HOSTCFLAGS is defined in the top-level
> kernel Makefile which isn't included here so it's undefined? -- but
> regardless that should be called out more explicitly as a problem being
> fixed in the commit log.

I was matching tools/perf, I've switched back to KBUILD_HOSTCFLAGS in
v4. There's some higher logic at play with these variable names and
I'm not aware of it so I'll leave it to be fixed if necessary later.

> This issue really has me scratching my head, as I could have sworn
> objtool was being built with -O2.
>
> > +OBJTOOL_LDFLAGS := $(LIBELF_LIBS) $(LIBSUBCMD)
>
> Is there a reason to not include $(HOSTLDFLAGS) here?

Done as KBUILD_HOSTLDFLAGS as I don't see HOSTLDFLAGS set anywhere.

> >  # Allow old libelf to be used:
> >  elfshdr := $(shell echo '$(pound)include <libelf.h>' | $(CC) $(CFLAGS) -x c -E - | grep elf_getshdr)
> > -CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
> > +OBJTOOL_CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
> > +
> > +# Always want host compilation.
> > +HOST_OVERRIDES := CC="$(HOSTCC)" EXTRA_CFLAGS="$(OBJTOOL_CFLAGS)" \
> > +               LD="$(HOSTLD)" AR="$(HOSTAR)"
> > +BUILD_HOST_OVERRIDES := CC="$(HOSTCC)" CFLAGS="$(OBJTOOL_CFLAGS)" \
> > +                     LD="$(HOSTLD)" LDFLAGS="$(OBJTOOL_LDFLAGS)" \
> > +                     AR="$(HOSTAR)"
>
> Maybe it depends on your perspective, but I'm thinking that some of
> these aren't really overrides, but rather normal expected flags.  And
> the distinction between these two variables is muddy: it's not only
> Build vs non-Build, but also objtool vs libsubcmd.
>
> How about just
>
>   HOST_OVERRIDES := CC="$(HOSTCC) "LD="$(HOSTLD)" AR="$(HOSTAR)"
>
> And then specifying the {EXTRA_}CFLAGS and LDFLAGS below where needed.

Done.

> >  AWK = awk
> >  MKDIR = mkdir
> > @@ -65,10 +68,11 @@ include $(srctree)/tools/build/Makefile.include
> >
> >  $(OBJTOOL_IN): fixdep FORCE
> >       $(Q)$(CONFIG_SHELL) ./sync-check.sh
> > -     $(Q)$(MAKE) $(build)=objtool
> > +     $(Q)$(MAKE) $(build)=objtool $(BUILD_HOST_OVERRIDES)
> > +
> >
> >  $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
> > -     $(QUIET_LINK)$(CC) $(OBJTOOL_IN) $(LDFLAGS) -o $@
> > +     $(QUIET_LINK)$(HOSTCC) $(OBJTOOL_IN) $(KBUILD_HOSTLDFLAGS) $(OBJTOOL_LDFLAGS) -o $@
>
> Does KBUILD_HOSTLDFLAGS even work here?

Removed, albeit now to be part of OBJTOOL_LDFLAGS as your earlier
comment requested.

Thanks,
Ian

> --
> Josh

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-01-26 18:30 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-05  9:01 [PATCH v3 0/3] objtool build improvements Ian Rogers
2023-01-05  9:01 ` [PATCH v3 1/3] objtool: Install libsubcmd in build Ian Rogers
2023-01-12 20:25   ` Nicolas Schier
2023-01-12 20:54     ` Ian Rogers
2023-01-26  1:46   ` Josh Poimboeuf
2023-01-26 17:50     ` Ian Rogers
2023-01-05  9:01 ` [PATCH v3 2/3] objtool: Properly support make V=1 Ian Rogers
2023-01-12 20:31   ` Nicolas Schier
2023-01-05  9:01 ` [PATCH v3 3/3] objtool: Alter how HOSTCC is forced Ian Rogers
2023-01-09 23:05   ` Nick Desaulniers
2023-01-12 20:40   ` Nicolas Schier
2023-01-26  1:49   ` Josh Poimboeuf
2023-01-26 18:30     ` Ian Rogers
2023-01-12 17:41 ` [PATCH v3 0/3] objtool build improvements Ian Rogers
2023-01-19 16:12   ` Ian Rogers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).