Messages in this thread | | | From | Ian Rogers <> | Date | Thu, 26 Jan 2023 10:30:24 -0800 | Subject | Re: [PATCH v3 3/3] objtool: Alter how HOSTCC is forced |
| |
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
| |