[PATCH v2 0/4] objtool build improvements

[PATCH v2 0/4] objtool build improvements

[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.

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 (4):
  tools lib subcmd: Add install target
  objtool: Install libsubcmd in build
  objtool: Properly support make V=1
  objtool: Alter how HOSTCC is forced

 tools/lib/subcmd/Makefile | 49 ++++++++++++++++++++++++++++
 tools/objtool/Build       |  2 --
 tools/objtool/Makefile    | 68 +++++++++++++++++++++++++++------------
 3 files changed, 97 insertions(+), 22 deletions(-)

-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH v2 1/4] tools lib subcmd: Add install target

[Ian Rogers]

This allows libsubcmd to be installed as a dependency.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/subcmd/Makefile | 49 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
index 8f1a09cdfd17..e96566f8991c 100644
--- a/tools/lib/subcmd/Makefile
+++ b/tools/lib/subcmd/Makefile
@@ -17,6 +17,15 @@ RM = rm -f
 
 MAKEFLAGS += --no-print-directory
 
+INSTALL = install
+
+# Use DESTDIR for installing into a different root directory.
+# This is useful for building a package. The program will be
+# installed in this directory as if it was the root directory.
+# Then the build tool can move it later.
+DESTDIR ?=
+DESTDIR_SQ = '$(subst ','\'',$(DESTDIR))'
+
 LIBFILE = $(OUTPUT)libsubcmd.a
 
 CFLAGS := -ggdb3 -Wall -Wextra -std=gnu99 -fPIC
@@ -48,6 +57,18 @@ CFLAGS += $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
 
 SUBCMD_IN := $(OUTPUT)libsubcmd-in.o
 
+ifeq ($(LP64), 1)
+  libdir_relative = lib64
+else
+  libdir_relative = lib
+endif
+
+prefix ?=
+libdir = $(prefix)/$(libdir_relative)
+
+# Shell quotes
+libdir_SQ = $(subst ','\'',$(libdir))
+
 all:
 
 export srctree OUTPUT CC LD CFLAGS V
@@ -61,6 +82,34 @@ $(SUBCMD_IN): FORCE
 $(LIBFILE): $(SUBCMD_IN)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(SUBCMD_IN)
 
+define do_install_mkdir
+	if [ ! -d '$(DESTDIR_SQ)$1' ]; then             \
+		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$1'; \
+	fi
+endef
+
+define do_install
+	if [ ! -d '$(DESTDIR_SQ)$2' ]; then             \
+		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
+	fi;                                             \
+	$(INSTALL) $1 $(if $3,-m $3,) '$(DESTDIR_SQ)$2'
+endef
+
+install_lib: $(LIBFILE)
+	$(call QUIET_INSTALL, $(LIBFILE)) \
+		$(call do_install_mkdir,$(libdir_SQ)); \
+		cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
+
+install_headers:
+	$(call QUIET_INSTALL, headers) \
+		$(call do_install,exec-cmd.h,$(prefix)/include/subcmd,644); \
+		$(call do_install,help.h,$(prefix)/include/subcmd,644); \
+		$(call do_install,pager.h,$(prefix)/include/subcmd,644); \
+		$(call do_install,parse-options.h,$(prefix)/include/subcmd,644); \
+		$(call do_install,run-command.h,$(prefix)/include/subcmd,644);
+
+install: install_lib install_headers
+
 clean:
 	$(call QUIET_CLEAN, libsubcmd) $(RM) $(LIBFILE); \
 	find $(or $(OUTPUT),.) -name \*.o -or -name \*.o.cmd -or -name \*.o.d | xargs $(RM)
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH v2 2/4] objtool: Install libsubcmd in build

[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>
---
 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.38.1.584.g0f3c55d4c2-goog

[PATCH v2 3/4] objtool: Properly support make V=1

[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>
---
 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.38.1.584.g0f3c55d4c2-goog

[PATCH v2 4/4] objtool: Alter how HOSTCC is forced

[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. 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 | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 61a00b7acae9..e550a98e2dd9 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
@@ -36,13 +32,20 @@ INCLUDES := -I$(srctree)/tools/include \
 	    -I$(srctree)/tools/objtool/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)
+WARNINGS := -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
+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.38.1.584.g0f3c55d4c2-goog

Re: [PATCH v2 1/4] tools lib subcmd: Add install target

[Nicolas Schier]

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

On Mon 21 Nov 2022 16:11:22 GMT, Ian Rogers wrote:
> This allows libsubcmd to be installed as a dependency.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/subcmd/Makefile | 49 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
> index 8f1a09cdfd17..e96566f8991c 100644
> --- a/tools/lib/subcmd/Makefile
> +++ b/tools/lib/subcmd/Makefile
> @@ -17,6 +17,15 @@ RM = rm -f
>  
>  MAKEFLAGS += --no-print-directory
>  
> +INSTALL = install
> +
> +# Use DESTDIR for installing into a different root directory.
> +# This is useful for building a package. The program will be
> +# installed in this directory as if it was the root directory.
> +# Then the build tool can move it later.
> +DESTDIR ?=
> +DESTDIR_SQ = '$(subst ','\'',$(DESTDIR))'
> +
>  LIBFILE = $(OUTPUT)libsubcmd.a
>  
>  CFLAGS := -ggdb3 -Wall -Wextra -std=gnu99 -fPIC
> @@ -48,6 +57,18 @@ CFLAGS += $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
>  
>  SUBCMD_IN := $(OUTPUT)libsubcmd-in.o
>  
> +ifeq ($(LP64), 1)
> +  libdir_relative = lib64
> +else
> +  libdir_relative = lib
> +endif
> +
> +prefix ?=
> +libdir = $(prefix)/$(libdir_relative)
> +
> +# Shell quotes
> +libdir_SQ = $(subst ','\'',$(libdir))
> +
>  all:
>  
>  export srctree OUTPUT CC LD CFLAGS V
> @@ -61,6 +82,34 @@ $(SUBCMD_IN): FORCE
>  $(LIBFILE): $(SUBCMD_IN)
>  	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(SUBCMD_IN)
>  
> +define do_install_mkdir
> +	if [ ! -d '$(DESTDIR_SQ)$1' ]; then             \
> +		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$1'; \
> +	fi
> +endef
> +
> +define do_install
> +	if [ ! -d '$(DESTDIR_SQ)$2' ]; then             \
> +		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
> +	fi;                                             \
> +	$(INSTALL) $1 $(if $3,-m $3,) '$(DESTDIR_SQ)$2'
> +endef
> +
> +install_lib: $(LIBFILE)
> +	$(call QUIET_INSTALL, $(LIBFILE)) \
> +		$(call do_install_mkdir,$(libdir_SQ)); \
> +		cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
> +

Sorry for being late.

Is there a specific reason why you do not use 'mkdir -p 
$(DESTDIR)/$(2); cp $(1) $(2)' or 'install $(addprefix -m,$(3)) -D $(1) 
$(2)' for the install rules (cp.  scripts/Makefile.{dtb,mod}inst)?

I think you could get rid of mkdir calls and the ..._SQ handling when 
using one of them.

> +install_headers:
> +	$(call QUIET_INSTALL, headers) \

Unlikely, but if one of the install commands fails, you probably want 
make to exit with an error.  Might you want to add 'set -e; \' here?

Kind regards,
Nicolas


> +		$(call do_install,exec-cmd.h,$(prefix)/include/subcmd,644); \
> +		$(call do_install,help.h,$(prefix)/include/subcmd,644); \
> +		$(call do_install,pager.h,$(prefix)/include/subcmd,644); \
> +		$(call do_install,parse-options.h,$(prefix)/include/subcmd,644); \
> +		$(call do_install,run-command.h,$(prefix)/include/subcmd,644);
> +
> +install: install_lib install_headers
> +
>  clean:
>  	$(call QUIET_CLEAN, libsubcmd) $(RM) $(LIBFILE); \
>  	find $(or $(OUTPUT),.) -name \*.o -or -name \*.o.cmd -or -name \*.o.d | xargs $(RM)
> -- 
> 2.38.1.584.g0f3c55d4c2-goog

-- 
epost|xmpp: nicolas@fjasle.eu          irc://oftc.net/nsc
↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
     -- frykten for herren er opphav til kunnskap --

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

Re: [PATCH v2 1/4] tools lib subcmd: Add install target

[Ian Rogers]

On Mon, Nov 21, 2022 at 10:48 PM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Mon 21 Nov 2022 16:11:22 GMT, Ian Rogers wrote:
> > This allows libsubcmd to be installed as a dependency.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/lib/subcmd/Makefile | 49 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
> > index 8f1a09cdfd17..e96566f8991c 100644
> > --- a/tools/lib/subcmd/Makefile
> > +++ b/tools/lib/subcmd/Makefile
> > @@ -17,6 +17,15 @@ RM = rm -f
> >
> >  MAKEFLAGS += --no-print-directory
> >
> > +INSTALL = install
> > +
> > +# Use DESTDIR for installing into a different root directory.
> > +# This is useful for building a package. The program will be
> > +# installed in this directory as if it was the root directory.
> > +# Then the build tool can move it later.
> > +DESTDIR ?=
> > +DESTDIR_SQ = '$(subst ','\'',$(DESTDIR))'
> > +
> >  LIBFILE = $(OUTPUT)libsubcmd.a
> >
> >  CFLAGS := -ggdb3 -Wall -Wextra -std=gnu99 -fPIC
> > @@ -48,6 +57,18 @@ CFLAGS += $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
> >
> >  SUBCMD_IN := $(OUTPUT)libsubcmd-in.o
> >
> > +ifeq ($(LP64), 1)
> > +  libdir_relative = lib64
> > +else
> > +  libdir_relative = lib
> > +endif
> > +
> > +prefix ?=
> > +libdir = $(prefix)/$(libdir_relative)
> > +
> > +# Shell quotes
> > +libdir_SQ = $(subst ','\'',$(libdir))
> > +
> >  all:
> >
> >  export srctree OUTPUT CC LD CFLAGS V
> > @@ -61,6 +82,34 @@ $(SUBCMD_IN): FORCE
> >  $(LIBFILE): $(SUBCMD_IN)
> >       $(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(SUBCMD_IN)
> >
> > +define do_install_mkdir
> > +     if [ ! -d '$(DESTDIR_SQ)$1' ]; then             \
> > +             $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$1'; \
> > +     fi
> > +endef
> > +
> > +define do_install
> > +     if [ ! -d '$(DESTDIR_SQ)$2' ]; then             \
> > +             $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
> > +     fi;                                             \
> > +     $(INSTALL) $1 $(if $3,-m $3,) '$(DESTDIR_SQ)$2'
> > +endef
> > +
> > +install_lib: $(LIBFILE)
> > +     $(call QUIET_INSTALL, $(LIBFILE)) \
> > +             $(call do_install_mkdir,$(libdir_SQ)); \
> > +             cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
> > +
>
> Sorry for being late.
>
> Is there a specific reason why you do not use 'mkdir -p
> $(DESTDIR)/$(2); cp $(1) $(2)' or 'install $(addprefix -m,$(3)) -D $(1)
> $(2)' for the install rules (cp.  scripts/Makefile.{dtb,mod}inst)?
>
> I think you could get rid of mkdir calls and the ..._SQ handling when
> using one of them.

Thanks for the feedback! Perhaps we can merge this as being consistent
with existing approaches:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/perf/Makefile#n185
and then refactor as you say.

> > +install_headers:
> > +     $(call QUIET_INSTALL, headers) \
>
> Unlikely, but if one of the install commands fails, you probably want
> make to exit with an error.  Might you want to add 'set -e; \' here?

Possibly. Again, I think we should aim for consistency and do this as
a follow up.

Thanks,
Ian

> Kind regards,
> Nicolas
>
>
> > +             $(call do_install,exec-cmd.h,$(prefix)/include/subcmd,644); \
> > +             $(call do_install,help.h,$(prefix)/include/subcmd,644); \
> > +             $(call do_install,pager.h,$(prefix)/include/subcmd,644); \
> > +             $(call do_install,parse-options.h,$(prefix)/include/subcmd,644); \
> > +             $(call do_install,run-command.h,$(prefix)/include/subcmd,644);
> > +
> > +install: install_lib install_headers
> > +
> >  clean:
> >       $(call QUIET_CLEAN, libsubcmd) $(RM) $(LIBFILE); \
> >       find $(or $(OUTPUT),.) -name \*.o -or -name \*.o.cmd -or -name \*.o.d | xargs $(RM)
> > --
> > 2.38.1.584.g0f3c55d4c2-goog
>
> --
> epost|xmpp: nicolas@fjasle.eu          irc://oftc.net/nsc
> ↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
>      -- frykten for herren er opphav til kunnskap --

Re: [PATCH v2 0/4] objtool build improvements

[Nick Desaulniers]

On Mon, Nov 21, 2022 at 4:11 PM 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.

Ian, I'm terribly sorry about the delay in reviewing this; usually my
turnaround time is much lower on code review...

Anyways, the patchset LGTM; thanks for the work that went into this.

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

>
> 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 (4):
>   tools lib subcmd: Add install target
>   objtool: Install libsubcmd in build
>   objtool: Properly support make V=1
>   objtool: Alter how HOSTCC is forced
>
>  tools/lib/subcmd/Makefile | 49 ++++++++++++++++++++++++++++
>  tools/objtool/Build       |  2 --
>  tools/objtool/Makefile    | 68 +++++++++++++++++++++++++++------------
>  3 files changed, 97 insertions(+), 22 deletions(-)
>
> --
> 2.38.1.584.g0f3c55d4c2-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 2/4] objtool: Install libsubcmd in build

[Nick Desaulniers]

On Mon, Nov 21, 2022 at 4:11 PM Ian Rogers <irogers@google.com> wrote:
>
> +ifneq ($(OUTPUT),)
> +  LIBSUBCMD_OUTPUT = $(abspath $(OUTPUT))/libsubcmd
> +else
> +  LIBSUBCMD_OUTPUT = $(CURDIR)/libsubcmd
> +endif

Seeing an else clause when the predicate is negated makes this read as:

if !foo:
  baz()
else:
  bar()

Consider using:

if foo:
  bar()
else
  baz()

in the future.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 4/4] objtool: Alter how HOSTCC is forced

[Nick Desaulniers]

On Mon, Nov 21, 2022 at 4:12 PM 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. 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 | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> index 61a00b7acae9..e550a98e2dd9 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
> @@ -36,13 +32,20 @@ INCLUDES := -I$(srctree)/tools/include \
>             -I$(srctree)/tools/objtool/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

Looking closer at the V=1 diff in meld, I think this is dropping
EXTRA_WARNINGS.  I think you want to add those back to OBJTOOL_CFLAGS.

> -CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> -LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> +WARNINGS := -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> +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.38.1.584.g0f3c55d4c2-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/4] objtool build improvements

[Nick Desaulniers]

On Fri, Dec 9, 2022 at 10:17 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Nov 21, 2022 at 4:11 PM 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.
>
> Ian, I'm terribly sorry about the delay in reviewing this; usually my
> turnaround time is much lower on code review...
>
> Anyways, the patchset LGTM; thanks for the work that went into this.
>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>

Sorry, I just spotted a potential issue in patch 4/4.  You may retain
my above tags for the first three patches if you send a 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 (4):
> >   tools lib subcmd: Add install target
> >   objtool: Install libsubcmd in build
> >   objtool: Properly support make V=1
> >   objtool: Alter how HOSTCC is forced
> >
> >  tools/lib/subcmd/Makefile | 49 ++++++++++++++++++++++++++++
> >  tools/objtool/Build       |  2 --
> >  tools/objtool/Makefile    | 68 +++++++++++++++++++++++++++------------
> >  3 files changed, 97 insertions(+), 22 deletions(-)
> >
> > --
> > 2.38.1.584.g0f3c55d4c2-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 4/4] objtool: Alter how HOSTCC is forced

[Ian Rogers]

On Fri, Dec 9, 2022 at 10:26 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Nov 21, 2022 at 4:12 PM 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. 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 | 27 ++++++++++++++++-----------
> >  1 file changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> > index 61a00b7acae9..e550a98e2dd9 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
> > @@ -36,13 +32,20 @@ INCLUDES := -I$(srctree)/tools/include \
> >             -I$(srctree)/tools/objtool/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
>
> Looking closer at the V=1 diff in meld, I think this is dropping
> EXTRA_WARNINGS.  I think you want to add those back to OBJTOOL_CFLAGS.

Thanks Nick! When I try this it causes new build failures.
Specifically EXTRA_WARNINGS includes things like -Wswitch-enum that
trigger:

```
check.c:1312:10: error: 14 enumeration values not explicitly handled
in switch: 'INSN_JUMP_DYNAMIC',
'INSN_JUMP_DYNAMIC_CONDITIONAL', 'INSN_CALL_DYNAMIC'... [-Werror,-Wswitch-enum]
       switch (insn->type) {
               ^~~~~~~~~~
check.c:2620:11: error: enumeration value 'OP_SRC_CONST' not
explicitly handled in switch [-Werror,-
Wswitch-enum]
               switch (op->src.type) {
                       ^~~~~~~~~~~~
check.c:3447:11: error: 5 enumeration values not explicitly handled in
switch: 'INSN_BUG', 'INSN_NOP
', 'INSN_TRAP'... [-Werror,-Wswitch-enum]
               switch (insn->type) {
                       ^~~~~~~~~~
check.c:3647:11: error: 9 enumeration values not explicitly handled in
switch: 'INSN_CONTEXT_SWITCH'
, 'INSN_BUG', 'INSN_STAC'... [-Werror,-Wswitch-enum]
               switch (insn->type) {
                       ^~~~~~~~~~
check.c:4008:10: error: 9 enumeration values not explicitly handled in
switch: 'INSN_CONTEXT_SWITCH'
, 'INSN_BUG', 'INSN_STAC'... [-Werror,-Wswitch-enum]
       switch (insn->type) {
               ^~~~~~~~~~
check.c:4173:11: error: 15 enumeration values not explicitly handled
in switch: 'INSN_JUMP_CONDITION
AL', 'INSN_JUMP_UNCONDITIONAL', 'INSN_JUMP_DYNAMIC_CONDITIONAL'...
[-Werror,-Wswitch-enum]
               switch (insn->type) {
                       ^~~~~~~~~~
6 errors generated.
```

What should the next step be? Land this or add EXTRA_WARNINGS and fix
all the issues?

Thanks,
Ian

> > -CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> > -LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> > +WARNINGS := -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> > +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.38.1.584.g0f3c55d4c2-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers

Re: [PATCH v2 4/4] objtool: Alter how HOSTCC is forced

[Nick Desaulniers]

On Wed, Dec 14, 2022 at 10:25 AM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Dec 9, 2022 at 10:26 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Mon, Nov 21, 2022 at 4:12 PM 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. 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 | 27 ++++++++++++++++-----------
> > >  1 file changed, 16 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> > > index 61a00b7acae9..e550a98e2dd9 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
> > > @@ -36,13 +32,20 @@ INCLUDES := -I$(srctree)/tools/include \
> > >             -I$(srctree)/tools/objtool/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
> >
> > Looking closer at the V=1 diff in meld, I think this is dropping
> > EXTRA_WARNINGS.  I think you want to add those back to OBJTOOL_CFLAGS.
>
> Thanks Nick! When I try this it causes new build failures.
> Specifically EXTRA_WARNINGS includes things like -Wswitch-enum that
> trigger:

That's...unexpected. These warnings didn't exist when EXTRA_WARNINGS
was being passed to the compiler for these...unless that wasn't the
case?

>
> ```
> check.c:1312:10: error: 14 enumeration values not explicitly handled
> in switch: 'INSN_JUMP_DYNAMIC',
> 'INSN_JUMP_DYNAMIC_CONDITIONAL', 'INSN_CALL_DYNAMIC'... [-Werror,-Wswitch-enum]
>        switch (insn->type) {
>                ^~~~~~~~~~
> check.c:2620:11: error: enumeration value 'OP_SRC_CONST' not
> explicitly handled in switch [-Werror,-
> Wswitch-enum]
>                switch (op->src.type) {
>                        ^~~~~~~~~~~~
> check.c:3447:11: error: 5 enumeration values not explicitly handled in
> switch: 'INSN_BUG', 'INSN_NOP
> ', 'INSN_TRAP'... [-Werror,-Wswitch-enum]
>                switch (insn->type) {
>                        ^~~~~~~~~~
> check.c:3647:11: error: 9 enumeration values not explicitly handled in
> switch: 'INSN_CONTEXT_SWITCH'
> , 'INSN_BUG', 'INSN_STAC'... [-Werror,-Wswitch-enum]
>                switch (insn->type) {
>                        ^~~~~~~~~~
> check.c:4008:10: error: 9 enumeration values not explicitly handled in
> switch: 'INSN_CONTEXT_SWITCH'
> , 'INSN_BUG', 'INSN_STAC'... [-Werror,-Wswitch-enum]
>        switch (insn->type) {
>                ^~~~~~~~~~
> check.c:4173:11: error: 15 enumeration values not explicitly handled
> in switch: 'INSN_JUMP_CONDITION
> AL', 'INSN_JUMP_UNCONDITIONAL', 'INSN_JUMP_DYNAMIC_CONDITIONAL'...
> [-Werror,-Wswitch-enum]
>                switch (insn->type) {
>                        ^~~~~~~~~~
> 6 errors generated.
> ```
>
> What should the next step be? Land this or add EXTRA_WARNINGS and fix
> all the issues?

Up to Josh. I think the first 3 patches are ready to go.

>
> Thanks,
> Ian
>
> > > -CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> > > -LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> > > +WARNINGS := -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> > > +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.38.1.584.g0f3c55d4c2-goog
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 4/4] objtool: Alter how HOSTCC is forced

[Ian Rogers]

On Wed, Dec 14, 2022 at 10:34 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Dec 14, 2022 at 10:25 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Fri, Dec 9, 2022 at 10:26 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > >
> > > On Mon, Nov 21, 2022 at 4:12 PM 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. 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 | 27 ++++++++++++++++-----------
> > > >  1 file changed, 16 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> > > > index 61a00b7acae9..e550a98e2dd9 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
> > > > @@ -36,13 +32,20 @@ INCLUDES := -I$(srctree)/tools/include \
> > > >             -I$(srctree)/tools/objtool/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
> > >
> > > Looking closer at the V=1 diff in meld, I think this is dropping
> > > EXTRA_WARNINGS.  I think you want to add those back to OBJTOOL_CFLAGS.
> >
> > Thanks Nick! When I try this it causes new build failures.
> > Specifically EXTRA_WARNINGS includes things like -Wswitch-enum that
> > trigger:
>
> That's...unexpected. These warnings didn't exist when EXTRA_WARNINGS
> was being passed to the compiler for these...unless that wasn't the
> case?

I wasn't adding EXTRA_WARNINGS but they are set up here:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/tools/scripts/Makefile.include?id=630ae80ea1dd253609cb50cff87f3248f901aca3#n23

I've avoided in these changes passing through values from
Makefile.include to the subprocesses, like Makefile.build, as
Makefile.include makes choices based on the current flags like clang
and llvm, not those we override.

> >
> > ```
> > check.c:1312:10: error: 14 enumeration values not explicitly handled
> > in switch: 'INSN_JUMP_DYNAMIC',
> > 'INSN_JUMP_DYNAMIC_CONDITIONAL', 'INSN_CALL_DYNAMIC'... [-Werror,-Wswitch-enum]
> >        switch (insn->type) {
> >                ^~~~~~~~~~
> > check.c:2620:11: error: enumeration value 'OP_SRC_CONST' not
> > explicitly handled in switch [-Werror,-
> > Wswitch-enum]
> >                switch (op->src.type) {
> >                        ^~~~~~~~~~~~
> > check.c:3447:11: error: 5 enumeration values not explicitly handled in
> > switch: 'INSN_BUG', 'INSN_NOP
> > ', 'INSN_TRAP'... [-Werror,-Wswitch-enum]
> >                switch (insn->type) {
> >                        ^~~~~~~~~~
> > check.c:3647:11: error: 9 enumeration values not explicitly handled in
> > switch: 'INSN_CONTEXT_SWITCH'
> > , 'INSN_BUG', 'INSN_STAC'... [-Werror,-Wswitch-enum]
> >                switch (insn->type) {
> >                        ^~~~~~~~~~
> > check.c:4008:10: error: 9 enumeration values not explicitly handled in
> > switch: 'INSN_CONTEXT_SWITCH'
> > , 'INSN_BUG', 'INSN_STAC'... [-Werror,-Wswitch-enum]
> >        switch (insn->type) {
> >                ^~~~~~~~~~
> > check.c:4173:11: error: 15 enumeration values not explicitly handled
> > in switch: 'INSN_JUMP_CONDITION
> > AL', 'INSN_JUMP_UNCONDITIONAL', 'INSN_JUMP_DYNAMIC_CONDITIONAL'...
> > [-Werror,-Wswitch-enum]
> >                switch (insn->type) {
> >                        ^~~~~~~~~~
> > 6 errors generated.
> > ```
> >
> > What should the next step be? Land this or add EXTRA_WARNINGS and fix
> > all the issues?
>
> Up to Josh. I think the first 3 patches are ready to go.

Note, the first patch is already in Arnaldo's tree:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=630ae80ea1dd253609cb50cff87f3248f901aca3
and shows up in linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/tools/lib/subcmd/Makefile?id=630ae80ea1dd253609cb50cff87f3248f901aca3

Thanks,
Ian



> >
> > Thanks,
> > Ian
> >
> > > > -CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> > > > -LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> > > > +WARNINGS := -Wno-switch-default -Wno-switch-enum -Wno-packed -Wno-nested-externs
> > > > +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.38.1.584.g0f3c55d4c2-goog
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > > ~Nick Desaulniers
>
>
>
> --
> Thanks,
> ~Nick Desaulniers