Re: [PATCH v2 4/4] objtool: Alter how HOSTCC is forced
From: Ian Rogers
Date: Wed Dec 14 2022 - 14:14:30 EST
On Wed, Dec 14, 2022 at 10:34 AM Nick Desaulniers
<ndesaulniers@xxxxxxxxxx> wrote:
>
> On Wed, Dec 14, 2022 at 10:25 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >
> > On Fri, Dec 9, 2022 at 10:26 AM Nick Desaulniers
> > <ndesaulniers@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Nov 21, 2022 at 4:12 PM Ian Rogers <irogers@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > > 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