Re: [RFC][PATCH 4/5] objtool: Enable compilation of objtool for all architectures

[Matt Helsley]

On Tue, May 12, 2020 at 06:04:56PM +0100, Julien Thierry wrote:
> Hi Matt,
> 
> On 5/11/20 6:35 PM, Matt Helsley wrote:
> > objtool currently only compiles for x86 architectures. This is
> > fine as it presently does not support tooling for other
> > architectures. However, we would like to be able to convert other
> > kernel tools to run as objtool sub commands because they too
> > process ELF object files. This will allow us to convert tools
> > such as recordmcount to use objtool's ELF code.
> > 
> > Since much of recordmcount's ELF code is copy-paste code to/from
> > a variety of other kernel tools (look at modpost for example) this
> > means that if we can convert recordmcount we can convert more.
> > 
> > We define a "missing" architecture which contains weak definitions
> > for tools that do not exist on all architectures. In this case the
> > "check" and "orc" tools do not exist on all architectures.
> > 
> > To test building for other architectures ($arch below):
> > 
> > 	cd tools/objtool/arch
> > 	ln -s missing $arch
> > 	make O=build-$arch ARCH=$arch tools/objtool
> > 
> 
> Since the stuff under arch/missing is only weak symbols to make up for
> missing subcmd implementations, can we put everything in a file
> subcmd_defaults.c (name up for debate!) that would be always be compiled an
> linked. And some SUBCMD_XXX is set to "y", the corresponding object file
> gets compiled and overrides the weak symbols from subcmd_defaults.c .

Hmm, I like keeping them separated along similar lines to the other
code because it makes it easier to see the intended correspondence and
likely will keep the files more readable / smaller. I could
just move them out of arch/missing and into missing_check.c and so forth.

What do you think of that?

> > diff --git a/tools/objtool/Build b/tools/objtool/Build
> > index 66f44f5cd2a6..fb6e6faf6f10 100644
> > --- a/tools/objtool/Build
> > +++ b/tools/objtool/Build
> > @@ -1,11 +1,12 @@
> >   objtool-y += arch/$(SRCARCH)/
> > +
> > +objtool-$(SUBCMD_CHECK) += check.o
> > +objtool-$(SUBCMD_ORC) += orc_gen.o
> > +objtool-$(SUBCMD_ORC) += orc_dump.o
> > +
> >   objtool-y += builtin-check.o
> >   objtool-y += builtin-orc.o
> > -objtool-y += check.o
> > -objtool-y += orc_gen.o
> > -objtool-y += orc_dump.o
> >   objtool-y += elf.o
> > -objtool-y += special.o
> 
> I'm not convinced by the moving of special under arch/x86 and I didn't
> understand it at first.
> 
> I guess you did it because it is only used by the check subcmd, which is
> currently only implemented by x86. Is that the reason?

Yeah, that was the original reasoning and this is an artifact of the
previous patch set.
 
> I feel that the proper way to do it would be to leave special.c/h where they
> are and have "objtool-$(SUBCMD_CHECK) += special.o". Unless there was some
> other motivation for it.

This makes sense. I'll incorporate that in the next posting.

> >   objtool-y += objtool.o
> >   objtool-y += libstring.o
> > diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> > index f591c4d1b6fe..8aac9e133188 100644
> > --- a/tools/objtool/Makefile
> > +++ b/tools/objtool/Makefile
> > @@ -45,7 +45,16 @@ elfshdr := $(shell echo '$(pound)include <libelf.h>' | $(CC) $(CFLAGS) -x c -E -
> >   CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
> >   AWK = awk
> > -export srctree OUTPUT CFLAGS SRCARCH AWK
> > +
> > +ifeq ($(SRCARCH),x86)
> > +	SUBCMD_CHECK := y
> > +	SUBCMD_ORC := y
> > +else
> > +	SUBCMD_CHECK := n
> > +	SUBCMD_ORC := n
> > +endif
> > +
> 
> Nit: Can we default all SUBCMD_* variables to 'n' outside a branch and then
> individual arches override the relevant variables to 'y' in their "ifeq
> ($SRCARCH, <arch>)" branch?

Definitely -- I'll set them all to n above then we can just have one
ifeq block per arch.

Thanks for the Review!

Cheers,
	-Matt Helsley