Re: [PATCH 3/3] objtool: Enable compilation of objtool for all architectures

[Matt Helsley <mhelsley@vmware.com>]

On Wed, May 20, 2020 at 09:31:46AM +0100, Julien Thierry wrote:
> 
> 
> On 5/19/20 9:55 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 "missing" weak definitions for subcommand entry functions
> > and other weak definitions for shared functions critical to
> > building existing subcommands. These return 127 when the command is
> > missing which signify tools that do not exist on all architectures.
> > In this case the "check" and "orc" tools do not exist on all
> > architectures so we only add them for x86. Future changes adding
> > support for "check", to arm64 for example, can then modify the
> > SUBCMD_CHECK variable when building for arm64.
> > 
> > objtool is not currently wired in to KConfig to be built for other
> > architectures because it's not needed for those architectures and
> > there are no commands it supports other than those for x86. As more
> > command support is enabled on various architectures the necessary
> > KConfig changes can be made (e.g. adding "STACK_VALIDATION") to
> > trigger building objtool.
> > 
> > Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> > Cc: Julien Thierry <jthierry@redhat.com>
> > ---
> >   tools/objtool/Build           | 13 +++++++++----
> >   tools/objtool/Makefile        | 11 ++++++++++-
> >   tools/objtool/arch.h          |  4 +++-
> >   tools/objtool/builtin-check.c |  2 +-
> >   tools/objtool/builtin-orc.c   |  3 +--
> >   tools/objtool/check.c         |  4 ++--
> >   tools/objtool/check.h         |  4 ----
> >   tools/objtool/objtool.h       | 14 ++++++++++++++
> >   tools/objtool/orc.h           | 18 ------------------
> >   tools/objtool/orc_dump.c      |  3 ++-
> >   tools/objtool/orc_gen.c       |  1 -
> >   tools/objtool/weak.c          | 35 +++++++++++++++++++++++++++++++++++
> >   12 files changed, 77 insertions(+), 35 deletions(-)
> >   delete mode 100644 tools/objtool/orc.h
> >   create mode 100644 tools/objtool/weak.c
> > 
> > diff --git a/tools/objtool/Build b/tools/objtool/Build
> > index 66f44f5cd2a6..b7222d5cc7bc 100644
> > --- a/tools/objtool/Build
> > +++ b/tools/objtool/Build
> > @@ -1,11 +1,16 @@
> >   objtool-y += arch/$(SRCARCH)/
> > +
> > +objtool-y += weak.o
> > +
> > +objtool-$(SUBCMD_CHECK) += check.o
> > +objtool-$(SUBCMD_CHECK) += special.o
> > +objtool-$(SUBCMD_ORC) += 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
> >   objtool-y += objtool.o
> >   objtool-y += libstring.o
> > diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> > index 6b91388aecbb..12686e2f1a56 100644
> > --- a/tools/objtool/Makefile
> > +++ b/tools/objtool/Makefile
> > @@ -46,7 +46,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
> > +
> > +SUBCMD_CHECK := n
> > +SUBCMD_ORC := n
> > +
> > +ifeq ($(SRCARCH),x86)
> > +	SUBCMD_CHECK := y
> > +	SUBCMD_ORC := y
> > +endif
> > +
> > +export srctree OUTPUT CFLAGS SRCARCH AWK SUBCMD_CHECK SUBCMD_ORC
> 
> Nit: I was thinking, since the list of SUBCMD_* is only going to grow
> maybe it would be nicer to have a single export line for the SUBCMD_*
> variables and leave the export line of [srctree..AWK] untouched.

That's a really good idea actually. Glad to see Josh included it in the
changed patch.

> 
> Just a suggestion, and only in case you respin this taking into account
> Josh's comment.
> 
> Otherwise things look good to me.

Thanks for all the reviews and good ideas!

Cheers,
    -Matt Helsley