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@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?

>
> ```
> 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