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
This uses the weak, no-op definitions of the "check" and "orc"
commands for the newly-supported architecture. Presently these
exit with 127 to indicate that the subcommands are missing.
Subsequent patches can then be made to determine if the weak
definitions are used and explicitly report a missing command,
and even to list which subcommands are supported (perhaps if
no subcommand is specified it can list the supported subcommands).
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.
This commit allows us to demonstrate the pattern of adding
architecture support and isolates moving the various files from
adding support for more objtool subcommands.
Signed-off-by: Matt Helsley <[email protected]>
---
tools/objtool/Build | 9 +++---
tools/objtool/Makefile | 11 ++++++-
tools/objtool/arch.h | 40 ++++++++++++++++++++++++++
tools/objtool/arch/missing/Build | 3 ++
tools/objtool/arch/missing/check.c | 14 +++++++++
tools/objtool/arch/missing/orc_dump.c | 11 +++++++
tools/objtool/arch/missing/orc_gen.c | 16 +++++++++++
tools/objtool/arch/x86/Build | 1 +
tools/objtool/{ => arch/x86}/special.c | 4 +--
tools/objtool/{ => arch/x86}/special.h | 2 +-
tools/objtool/builtin-orc.c | 2 +-
tools/objtool/check.c | 5 ++--
tools/objtool/check.h | 37 ------------------------
tools/objtool/objtool.h | 2 +-
tools/objtool/orc.h | 2 --
tools/objtool/orc_dump.c | 1 +
tools/objtool/orc_gen.c | 3 ++
17 files changed, 112 insertions(+), 51 deletions(-)
create mode 100644 tools/objtool/arch/missing/Build
create mode 100644 tools/objtool/arch/missing/check.c
create mode 100644 tools/objtool/arch/missing/orc_dump.c
create mode 100644 tools/objtool/arch/missing/orc_gen.c
rename tools/objtool/{ => arch/x86}/special.c (98%)
rename tools/objtool/{ => arch/x86}/special.h (95%)
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
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
+
+export srctree OUTPUT CFLAGS SRCARCH AWK SUBCMD_CHECK SUBCMD_ORC
include $(srctree)/tools/build/Makefile.include
$(OBJTOOL_IN): fixdep FORCE
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index ced3765c4f44..4ec9686f4898 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -11,6 +11,9 @@
#include "elf.h"
#include "cfi.h"
+#include <asm/orc_types.h>
+#include "orc.h"
+
enum insn_type {
INSN_JUMP_CONDITIONAL,
INSN_JUMP_UNCONDITIONAL,
@@ -66,6 +69,43 @@ struct stack_op {
struct op_src src;
};
+struct insn_state {
+ struct cfi_reg cfa;
+ struct cfi_reg regs[CFI_NUM_REGS];
+ int stack_size;
+ unsigned char type;
+ bool bp_scratch;
+ bool drap, end, uaccess, df;
+ bool noinstr;
+ s8 instr;
+ unsigned int uaccess_stack;
+ int drap_reg, drap_offset;
+ struct cfi_reg vals[CFI_NUM_REGS];
+};
+
+struct instruction {
+ struct list_head list;
+ struct hlist_node hash;
+ struct section *sec;
+ unsigned long offset;
+ unsigned int len;
+ enum insn_type type;
+ unsigned long immediate;
+ bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+ bool retpoline_safe;
+ s8 instr;
+ u8 visited;
+ struct symbol *call_dest;
+ struct instruction *jump_dest;
+ struct instruction *first_jump_src;
+ struct rela *jump_table;
+ struct list_head alts;
+ struct symbol *func;
+ struct stack_op stack_op;
+ struct insn_state state;
+ struct orc_entry orc;
+};
+
void arch_initial_func_cfi_state(struct cfi_state *state);
int arch_decode_instruction(struct elf *elf, struct section *sec,
diff --git a/tools/objtool/arch/missing/Build b/tools/objtool/arch/missing/Build
new file mode 100644
index 000000000000..a2478db59484
--- /dev/null
+++ b/tools/objtool/arch/missing/Build
@@ -0,0 +1,3 @@
+objtool-y += check.o
+objtool-y += orc_gen.o
+objtool-y += orc_dump.o
diff --git a/tools/objtool/arch/missing/check.c b/tools/objtool/arch/missing/check.c
new file mode 100644
index 000000000000..5eac14c8c6a5
--- /dev/null
+++ b/tools/objtool/arch/missing/check.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2020 Matt Helsley <[email protected]>
+ */
+
+#include <stdbool.h>
+#include "../../check.h"
+
+const char *objname;
+
+int __attribute__ ((weak)) check(const char *_objname, bool orc)
+{
+ return 127;
+}
diff --git a/tools/objtool/arch/missing/orc_dump.c b/tools/objtool/arch/missing/orc_dump.c
new file mode 100644
index 000000000000..f7ebb3a2ce9e
--- /dev/null
+++ b/tools/objtool/arch/missing/orc_dump.c
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2017 Josh Poimboeuf <[email protected]>
+ */
+
+#include "../../orc.h"
+
+int __attribute__ ((weak)) orc_dump(const char *_objname)
+{
+ return 127;
+}
diff --git a/tools/objtool/arch/missing/orc_gen.c b/tools/objtool/arch/missing/orc_gen.c
new file mode 100644
index 000000000000..4bf62ddf58a4
--- /dev/null
+++ b/tools/objtool/arch/missing/orc_gen.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2017 Josh Poimboeuf <[email protected]>
+ */
+
+#include "../../orc.h"
+
+int __attribute__ ((weak)) create_orc(struct objtool_file *file)
+{
+ return 127;
+}
+
+int __attribute__ ((weak)) create_orc_sections(struct objtool_file *file)
+{
+ return 127;
+}
diff --git a/tools/objtool/arch/x86/Build b/tools/objtool/arch/x86/Build
index 7c5004008e97..de01c67dea14 100644
--- a/tools/objtool/arch/x86/Build
+++ b/tools/objtool/arch/x86/Build
@@ -1,4 +1,5 @@
objtool-y += decode.o
+objtool-y += special.o
inat_tables_script = ../arch/x86/tools/gen-insn-attr-x86.awk
inat_tables_maps = ../arch/x86/lib/x86-opcode-map.txt
diff --git a/tools/objtool/special.c b/tools/objtool/arch/x86/special.c
similarity index 98%
rename from tools/objtool/special.c
rename to tools/objtool/arch/x86/special.c
index e74e0189de22..677b6efe1446 100644
--- a/tools/objtool/special.c
+++ b/tools/objtool/arch/x86/special.c
@@ -11,9 +11,9 @@
#include <stdlib.h>
#include <string.h>
-#include "builtin.h"
+#include "../../builtin.h"
#include "special.h"
-#include "warn.h"
+#include "../../warn.h"
#define EX_ENTRY_SIZE 12
#define EX_ORIG_OFFSET 0
diff --git a/tools/objtool/special.h b/tools/objtool/arch/x86/special.h
similarity index 95%
rename from tools/objtool/special.h
rename to tools/objtool/arch/x86/special.h
index 35061530e46e..beba41950725 100644
--- a/tools/objtool/special.h
+++ b/tools/objtool/arch/x86/special.h
@@ -7,7 +7,7 @@
#define _SPECIAL_H
#include <stdbool.h>
-#include "elf.h"
+#include "../../elf.h"
struct special_alt {
struct list_head list;
diff --git a/tools/objtool/builtin-orc.c b/tools/objtool/builtin-orc.c
index 5f7cc6157edd..e6e54ae4ab75 100644
--- a/tools/objtool/builtin-orc.c
+++ b/tools/objtool/builtin-orc.c
@@ -15,7 +15,7 @@
#include <string.h>
#include "builtin.h"
#include "check.h"
-
+#include "orc.h"
static const char *orc_usage[] = {
"objtool orc generate [<options>] file.o",
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 4b170fd08a28..d8a10961fe2c 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -7,10 +7,11 @@
#include <stdlib.h>
#include "builtin.h"
+#include "cfi.h"
+#include "arch.h"
#include "check.h"
#include "elf.h"
-#include "special.h"
-#include "arch.h"
+#include "arch/x86/special.h"
#include "warn.h"
#include <linux/hashtable.h>
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index ec6ff7f0970c..4d34bfc84dc7 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -8,43 +8,6 @@
#include <stdbool.h>
#include "objtool.h"
-#include "cfi.h"
-#include "arch.h"
-#include "orc.h"
-
-struct insn_state {
- struct cfi_reg cfa;
- struct cfi_reg regs[CFI_NUM_REGS];
- int stack_size;
- unsigned char type;
- bool bp_scratch;
- bool drap, end, uaccess, df;
- unsigned int uaccess_stack;
- int drap_reg, drap_offset;
- struct cfi_reg vals[CFI_NUM_REGS];
-};
-
-struct instruction {
- struct list_head list;
- struct hlist_node hash;
- struct section *sec;
- unsigned long offset;
- unsigned int len;
- enum insn_type type;
- unsigned long immediate;
- bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
- bool retpoline_safe;
- u8 visited;
- struct symbol *call_dest;
- struct instruction *jump_dest;
- struct instruction *first_jump_src;
- struct rela *jump_table;
- struct list_head alts;
- struct symbol *func;
- struct stack_op stack_op;
- struct insn_state state;
- struct orc_entry orc;
-};
int check(const char *objname, bool orc);
diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
index afa52fe6f644..5ff352083056 100644
--- a/tools/objtool/objtool.h
+++ b/tools/objtool/objtool.h
@@ -17,4 +17,4 @@ struct objtool_file {
DECLARE_HASHTABLE(insn_hash, 20);
bool ignore_unreachables, c_file, hints, rodata;
};
-#endif
+#endif /* _OBJTOOL_H */
diff --git a/tools/objtool/orc.h b/tools/objtool/orc.h
index ee2832221e62..c67f451d7610 100644
--- a/tools/objtool/orc.h
+++ b/tools/objtool/orc.h
@@ -6,8 +6,6 @@
#ifndef _ORC_H
#define _ORC_H
-#include <asm/orc_types.h>
-
struct objtool_file;
int create_orc(struct objtool_file *file);
diff --git a/tools/objtool/orc_dump.c b/tools/objtool/orc_dump.c
index ba4cbb1cdd63..73c8beb6e402 100644
--- a/tools/objtool/orc_dump.c
+++ b/tools/objtool/orc_dump.c
@@ -4,6 +4,7 @@
*/
#include <unistd.h>
+#include <asm/orc_types.h>
#include "orc.h"
#include "warn.h"
diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index 4c0dabd28000..7693f7f31293 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -5,8 +5,11 @@
#include <stdlib.h>
#include <string.h>
+#include <asm/orc_types.h>
#include "orc.h"
+#include "cfi.h"
+#include "arch.h"
#include "check.h"
#include "warn.h"
--
2.20.1
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 .
> This uses the weak, no-op definitions of the "check" and "orc"
> commands for the newly-supported architecture. Presently these
> exit with 127 to indicate that the subcommands are missing.
> Subsequent patches can then be made to determine if the weak
> definitions are used and explicitly report a missing command,
> and even to list which subcommands are supported (perhaps if
> no subcommand is specified it can list the supported subcommands).
>
> 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.
>
> This commit allows us to demonstrate the pattern of adding
> architecture support and isolates moving the various files from
> adding support for more objtool subcommands.
>
> Signed-off-by: Matt Helsley <[email protected]>
> ---
> tools/objtool/Build | 9 +++---
> tools/objtool/Makefile | 11 ++++++-
> tools/objtool/arch.h | 40 ++++++++++++++++++++++++++
> tools/objtool/arch/missing/Build | 3 ++
> tools/objtool/arch/missing/check.c | 14 +++++++++
> tools/objtool/arch/missing/orc_dump.c | 11 +++++++
> tools/objtool/arch/missing/orc_gen.c | 16 +++++++++++
> tools/objtool/arch/x86/Build | 1 +
> tools/objtool/{ => arch/x86}/special.c | 4 +--
> tools/objtool/{ => arch/x86}/special.h | 2 +-
> tools/objtool/builtin-orc.c | 2 +-
> tools/objtool/check.c | 5 ++--
> tools/objtool/check.h | 37 ------------------------
> tools/objtool/objtool.h | 2 +-
> tools/objtool/orc.h | 2 --
> tools/objtool/orc_dump.c | 1 +
> tools/objtool/orc_gen.c | 3 ++
> 17 files changed, 112 insertions(+), 51 deletions(-)
> create mode 100644 tools/objtool/arch/missing/Build
> create mode 100644 tools/objtool/arch/missing/check.c
> create mode 100644 tools/objtool/arch/missing/orc_dump.c
> create mode 100644 tools/objtool/arch/missing/orc_gen.c
> rename tools/objtool/{ => arch/x86}/special.c (98%)
> rename tools/objtool/{ => arch/x86}/special.h (95%)
>
> 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?
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.
> 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?
> +export srctree OUTPUT CFLAGS SRCARCH AWK SUBCMD_CHECK SUBCMD_ORC
> include $(srctree)/tools/build/Makefile.include
>
> $(OBJTOOL_IN): fixdep FORCE
> diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
> index ced3765c4f44..4ec9686f4898 100644
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -11,6 +11,9 @@
> #include "elf.h"
> #include "cfi.h"
>
> +#include <asm/orc_types.h>
> +#include "orc.h"
> +
> enum insn_type {
> INSN_JUMP_CONDITIONAL,
> INSN_JUMP_UNCONDITIONAL,
> @@ -66,6 +69,43 @@ struct stack_op {
> struct op_src src;
> };
>
> +struct insn_state {
> + struct cfi_reg cfa;
> + struct cfi_reg regs[CFI_NUM_REGS];
> + int stack_size;
> + unsigned char type;
> + bool bp_scratch;
> + bool drap, end, uaccess, df;
> + bool noinstr;
> + s8 instr;
> + unsigned int uaccess_stack;
> + int drap_reg, drap_offset;
> + struct cfi_reg vals[CFI_NUM_REGS];
> +};
> +
> +struct instruction {
> + struct list_head list;
> + struct hlist_node hash;
> + struct section *sec;
> + unsigned long offset;
> + unsigned int len;
> + enum insn_type type;
> + unsigned long immediate;
> + bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> + bool retpoline_safe;
> + s8 instr;
> + u8 visited;
> + struct symbol *call_dest;
> + struct instruction *jump_dest;
> + struct instruction *first_jump_src;
> + struct rela *jump_table;
> + struct list_head alts;
> + struct symbol *func;
> + struct stack_op stack_op;
> + struct insn_state state;
> + struct orc_entry orc;
> +};
> +
> void arch_initial_func_cfi_state(struct cfi_state *state);
>
> int arch_decode_instruction(struct elf *elf, struct section *sec,
> diff --git a/tools/objtool/arch/missing/Build b/tools/objtool/arch/missing/Build
> new file mode 100644
> index 000000000000..a2478db59484
> --- /dev/null
> +++ b/tools/objtool/arch/missing/Build
> @@ -0,0 +1,3 @@
> +objtool-y += check.o
> +objtool-y += orc_gen.o
> +objtool-y += orc_dump.o
> diff --git a/tools/objtool/arch/missing/check.c b/tools/objtool/arch/missing/check.c
> new file mode 100644
> index 000000000000..5eac14c8c6a5
> --- /dev/null
> +++ b/tools/objtool/arch/missing/check.c
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Matt Helsley <[email protected]>
> + */
> +
> +#include <stdbool.h>
> +#include "../../check.h"
> +
> +const char *objname;
> +
> +int __attribute__ ((weak)) check(const char *_objname, bool orc)
> +{
> + return 127;
> +}
> diff --git a/tools/objtool/arch/missing/orc_dump.c b/tools/objtool/arch/missing/orc_dump.c
> new file mode 100644
> index 000000000000..f7ebb3a2ce9e
> --- /dev/null
> +++ b/tools/objtool/arch/missing/orc_dump.c
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017 Josh Poimboeuf <[email protected]>
> + */
> +
> +#include "../../orc.h"
> +
> +int __attribute__ ((weak)) orc_dump(const char *_objname)
> +{
> + return 127;
> +}
> diff --git a/tools/objtool/arch/missing/orc_gen.c b/tools/objtool/arch/missing/orc_gen.c
> new file mode 100644
> index 000000000000..4bf62ddf58a4
> --- /dev/null
> +++ b/tools/objtool/arch/missing/orc_gen.c
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2017 Josh Poimboeuf <[email protected]>
> + */
> +
> +#include "../../orc.h"
> +
> +int __attribute__ ((weak)) create_orc(struct objtool_file *file)
> +{
> + return 127;
> +}
> +
> +int __attribute__ ((weak)) create_orc_sections(struct objtool_file *file)
> +{
> + return 127;
> +}
> diff --git a/tools/objtool/arch/x86/Build b/tools/objtool/arch/x86/Build
> index 7c5004008e97..de01c67dea14 100644
> --- a/tools/objtool/arch/x86/Build
> +++ b/tools/objtool/arch/x86/Build
> @@ -1,4 +1,5 @@
> objtool-y += decode.o
> +objtool-y += special.o
>
> inat_tables_script = ../arch/x86/tools/gen-insn-attr-x86.awk
> inat_tables_maps = ../arch/x86/lib/x86-opcode-map.txt
> diff --git a/tools/objtool/special.c b/tools/objtool/arch/x86/special.c
> similarity index 98%
> rename from tools/objtool/special.c
> rename to tools/objtool/arch/x86/special.c
> index e74e0189de22..677b6efe1446 100644
> --- a/tools/objtool/special.c
> +++ b/tools/objtool/arch/x86/special.c
> @@ -11,9 +11,9 @@
> #include <stdlib.h>
> #include <string.h>
>
> -#include "builtin.h"
> +#include "../../builtin.h"
> #include "special.h"
> -#include "warn.h"
> +#include "../../warn.h"
>
> #define EX_ENTRY_SIZE 12
> #define EX_ORIG_OFFSET 0
> diff --git a/tools/objtool/special.h b/tools/objtool/arch/x86/special.h
> similarity index 95%
> rename from tools/objtool/special.h
> rename to tools/objtool/arch/x86/special.h
> index 35061530e46e..beba41950725 100644
> --- a/tools/objtool/special.h
> +++ b/tools/objtool/arch/x86/special.h
> @@ -7,7 +7,7 @@
> #define _SPECIAL_H
>
> #include <stdbool.h>
> -#include "elf.h"
> +#include "../../elf.h"
>
> struct special_alt {
> struct list_head list;
> diff --git a/tools/objtool/builtin-orc.c b/tools/objtool/builtin-orc.c
> index 5f7cc6157edd..e6e54ae4ab75 100644
> --- a/tools/objtool/builtin-orc.c
> +++ b/tools/objtool/builtin-orc.c
> @@ -15,7 +15,7 @@
> #include <string.h>
> #include "builtin.h"
> #include "check.h"
> -
> +#include "orc.h"
>
> static const char *orc_usage[] = {
> "objtool orc generate [<options>] file.o",
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 4b170fd08a28..d8a10961fe2c 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -7,10 +7,11 @@
> #include <stdlib.h>
>
> #include "builtin.h"
> +#include "cfi.h"
> +#include "arch.h"
> #include "check.h"
> #include "elf.h"
> -#include "special.h"
> -#include "arch.h"
> +#include "arch/x86/special.h"
> #include "warn.h"
>
> #include <linux/hashtable.h>
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index ec6ff7f0970c..4d34bfc84dc7 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -8,43 +8,6 @@
>
> #include <stdbool.h>
> #include "objtool.h"
> -#include "cfi.h"
> -#include "arch.h"
> -#include "orc.h"
> -
> -struct insn_state {
> - struct cfi_reg cfa;
> - struct cfi_reg regs[CFI_NUM_REGS];
> - int stack_size;
> - unsigned char type;
> - bool bp_scratch;
> - bool drap, end, uaccess, df;
> - unsigned int uaccess_stack;
> - int drap_reg, drap_offset;
> - struct cfi_reg vals[CFI_NUM_REGS];
> -};
> -
> -struct instruction {
> - struct list_head list;
> - struct hlist_node hash;
> - struct section *sec;
> - unsigned long offset;
> - unsigned int len;
> - enum insn_type type;
> - unsigned long immediate;
> - bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> - bool retpoline_safe;
> - u8 visited;
> - struct symbol *call_dest;
> - struct instruction *jump_dest;
> - struct instruction *first_jump_src;
> - struct rela *jump_table;
> - struct list_head alts;
> - struct symbol *func;
> - struct stack_op stack_op;
> - struct insn_state state;
> - struct orc_entry orc;
> -};
>
> int check(const char *objname, bool orc);
>
> diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
> index afa52fe6f644..5ff352083056 100644
> --- a/tools/objtool/objtool.h
> +++ b/tools/objtool/objtool.h
> @@ -17,4 +17,4 @@ struct objtool_file {
> DECLARE_HASHTABLE(insn_hash, 20);
> bool ignore_unreachables, c_file, hints, rodata;
> };
> -#endif
> +#endif /* _OBJTOOL_H */
> diff --git a/tools/objtool/orc.h b/tools/objtool/orc.h
> index ee2832221e62..c67f451d7610 100644
> --- a/tools/objtool/orc.h
> +++ b/tools/objtool/orc.h
> @@ -6,8 +6,6 @@
> #ifndef _ORC_H
> #define _ORC_H
>
> -#include <asm/orc_types.h>
> -
> struct objtool_file;
>
> int create_orc(struct objtool_file *file);
> diff --git a/tools/objtool/orc_dump.c b/tools/objtool/orc_dump.c
> index ba4cbb1cdd63..73c8beb6e402 100644
> --- a/tools/objtool/orc_dump.c
> +++ b/tools/objtool/orc_dump.c
> @@ -4,6 +4,7 @@
> */
>
> #include <unistd.h>
> +#include <asm/orc_types.h>
> #include "orc.h"
> #include "warn.h"
>
> diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
> index 4c0dabd28000..7693f7f31293 100644
> --- a/tools/objtool/orc_gen.c
> +++ b/tools/objtool/orc_gen.c
> @@ -5,8 +5,11 @@
>
> #include <stdlib.h>
> #include <string.h>
> +#include <asm/orc_types.h>
>
> #include "orc.h"
> +#include "cfi.h"
> +#include "arch.h"
> #include "check.h"
> #include "warn.h"
>
>
--
Julien Thierry
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
On 5/13/20 4:59 PM, Matt Helsley wrote:
> 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?
>
I do prefer that to the introduction of an arch/missing.
Still, I'm not sure I see much benefit in splitting those small
implementations in separate files, but it's not a problem either. This
seems more a matter of taste rather than one approach working better
than the other. So it's more up to what the maintainer prefer! :)
>>> 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.
>
Great, thanks!
--
Julien Thierry
On Wed, May 13, 2020 at 05:55:31PM +0100, Julien Thierry wrote:
> > > 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?
> >
>
> I do prefer that to the introduction of an arch/missing.
>
> Still, I'm not sure I see much benefit in splitting those small
> implementations in separate files, but it's not a problem either. This seems
> more a matter of taste rather than one approach working better than the
> other. So it's more up to what the maintainer prefer! :)
For now I'd prefer getting rid of the 'missing' arch and just having a
single top-level weak.c which has all the weak functions in it. Keeps
the clutter down :-)
Down the road, if the number of weak functions got out of hand then we
could look at splitting them up into multiple files.
--
Josh
On Mon, May 11, 2020 at 10:35:12AM -0700, Matt Helsley wrote:
> +struct insn_state {
> + struct cfi_reg cfa;
> + struct cfi_reg regs[CFI_NUM_REGS];
> + int stack_size;
> + unsigned char type;
> + bool bp_scratch;
> + bool drap, end, uaccess, df;
> + bool noinstr;
> + s8 instr;
> + unsigned int uaccess_stack;
> + int drap_reg, drap_offset;
> + struct cfi_reg vals[CFI_NUM_REGS];
> +};
> +
> +struct instruction {
> + struct list_head list;
> + struct hlist_node hash;
> + struct section *sec;
> + unsigned long offset;
> + unsigned int len;
> + enum insn_type type;
> + unsigned long immediate;
> + bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> + bool retpoline_safe;
> + s8 instr;
> + u8 visited;
> + struct symbol *call_dest;
> + struct instruction *jump_dest;
> + struct instruction *first_jump_src;
> + struct rela *jump_table;
> + struct list_head alts;
> + struct symbol *func;
> + struct stack_op stack_op;
> + struct insn_state state;
> + struct orc_entry orc;
> +};
Why were these moved to arch.h? They're not necessarily arch-specific,
but rather "check"-specific, so I think they still belong in check.h, if
possible.
--
Josh
On Fri, May 15, 2020 at 03:51:35PM -0500, Josh Poimboeuf wrote:
> On Wed, May 13, 2020 at 05:55:31PM +0100, Julien Thierry wrote:
> > > > 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?
> > >
> >
> > I do prefer that to the introduction of an arch/missing.
> >
> > Still, I'm not sure I see much benefit in splitting those small
> > implementations in separate files, but it's not a problem either. This seems
> > more a matter of taste rather than one approach working better than the
> > other. So it's more up to what the maintainer prefer! :)
>
> For now I'd prefer getting rid of the 'missing' arch and just having a
> single top-level weak.c which has all the weak functions in it. Keeps
> the clutter down :-)
>
> Down the road, if the number of weak functions got out of hand then we
> could look at splitting them up into multiple files.
OK, I'll merge them all into weak.c
Thanks!
Cheers,
-Matt Helsley
On Fri, May 15, 2020 at 03:56:10PM -0500, Josh Poimboeuf wrote:
> On Mon, May 11, 2020 at 10:35:12AM -0700, Matt Helsley wrote:
> > +struct insn_state {
> > + struct cfi_reg cfa;
> > + struct cfi_reg regs[CFI_NUM_REGS];
> > + int stack_size;
> > + unsigned char type;
> > + bool bp_scratch;
> > + bool drap, end, uaccess, df;
> > + bool noinstr;
> > + s8 instr;
> > + unsigned int uaccess_stack;
> > + int drap_reg, drap_offset;
> > + struct cfi_reg vals[CFI_NUM_REGS];
> > +};
> > +
> > +struct instruction {
> > + struct list_head list;
> > + struct hlist_node hash;
> > + struct section *sec;
> > + unsigned long offset;
> > + unsigned int len;
> > + enum insn_type type;
> > + unsigned long immediate;
> > + bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> > + bool retpoline_safe;
> > + s8 instr;
> > + u8 visited;
> > + struct symbol *call_dest;
> > + struct instruction *jump_dest;
> > + struct instruction *first_jump_src;
> > + struct rela *jump_table;
> > + struct list_head alts;
> > + struct symbol *func;
> > + struct stack_op stack_op;
> > + struct insn_state state;
> > + struct orc_entry orc;
> > +};
>
> Why were these moved to arch.h? They're not necessarily arch-specific,
> but rather "check"-specific, so I think they still belong in check.h, if
> possible.
Hmm, good question. I thought it had something to do with the CFI_NUM_REGS
definition but even if that's the case I think you're right that it should
stay in "check" -- I can verify and if that's the case just just define it
to some dummy value (1?) when it's missing.
Cheers,
-Matt Helsley
On Fri, May 15, 2020 at 03:56:10PM -0500, Josh Poimboeuf wrote:
> On Mon, May 11, 2020 at 10:35:12AM -0700, Matt Helsley wrote:
> > +struct insn_state {
> > + struct cfi_reg cfa;
> > + struct cfi_reg regs[CFI_NUM_REGS];
> > + int stack_size;
> > + unsigned char type;
> > + bool bp_scratch;
> > + bool drap, end, uaccess, df;
> > + bool noinstr;
> > + s8 instr;
> > + unsigned int uaccess_stack;
> > + int drap_reg, drap_offset;
> > + struct cfi_reg vals[CFI_NUM_REGS];
> > +};
> > +
> > +struct instruction {
> > + struct list_head list;
> > + struct hlist_node hash;
> > + struct section *sec;
> > + unsigned long offset;
> > + unsigned int len;
> > + enum insn_type type;
> > + unsigned long immediate;
> > + bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> > + bool retpoline_safe;
> > + s8 instr;
> > + u8 visited;
> > + struct symbol *call_dest;
> > + struct instruction *jump_dest;
> > + struct instruction *first_jump_src;
> > + struct rela *jump_table;
> > + struct list_head alts;
> > + struct symbol *func;
> > + struct stack_op stack_op;
> > + struct insn_state state;
> > + struct orc_entry orc;
> > +};
>
> Why were these moved to arch.h? They're not necessarily arch-specific,
> but rather "check"-specific, so I think they still belong in check.h, if
> possible.
Ah, found it. They are arch specific due to struct orc_entry, which is
presently not defined for any archs besides x86.
Prior to the patch (-> means "includes"):
check.h -> asm/orc_types.h (defines struct orc_entry)
orc_gen.c -> check,h
After patch:
check.c -> asm/orc_types.h
orc_gen.c -> asm/orc_types.h
orc_gen.c -> check.h
orc_gen.c -> arch.h
{ now weak.c } -> check.h
So this prevents the headers, which help us keep the weak definitions
consistent with the strong definitions, from breaking compiles on archs
that lack struct orc_entry.
I'm not sure what the best way to remove this dependency is without
a nasty void * for the orc entry, or some #ifdef games related to
checking for cpp defines from asm/orc_types.h. This approach neatly
avoids conditional preprocessor games and type casting though I do
agree it's surprising.
Do you have any advice here?
Cheers,
-Matt Helsley
On Mon, May 18, 2020 at 12:50:45PM -0700, Matt Helsley wrote:
> > Why were these moved to arch.h? They're not necessarily arch-specific,
> > but rather "check"-specific, so I think they still belong in check.h, if
> > possible.
>
> Ah, found it. They are arch specific due to struct orc_entry, which is
> presently not defined for any archs besides x86.
>
> Prior to the patch (-> means "includes"):
> check.h -> asm/orc_types.h (defines struct orc_entry)
> orc_gen.c -> check,h
>
> After patch:
> check.c -> asm/orc_types.h
> orc_gen.c -> asm/orc_types.h
> orc_gen.c -> check.h
> orc_gen.c -> arch.h
> { now weak.c } -> check.h
>
> So this prevents the headers, which help us keep the weak definitions
> consistent with the strong definitions, from breaking compiles on archs
> that lack struct orc_entry.
>
> I'm not sure what the best way to remove this dependency is without
> a nasty void * for the orc entry, or some #ifdef games related to
> checking for cpp defines from asm/orc_types.h. This approach neatly
> avoids conditional preprocessor games and type casting though I do
> agree it's surprising.
>
> Do you have any advice here?
Would it work if we just move the check() and orc_dump() prototypes to
objtool.h? Then weak.c can just include objtool.h. And check.h and
orc.h would only need to be included for arches which implement them.
--
Josh
On Mon, May 18, 2020 at 05:27:54PM -0500, Josh Poimboeuf wrote:
> On Mon, May 18, 2020 at 12:50:45PM -0700, Matt Helsley wrote:
> > > Why were these moved to arch.h? They're not necessarily arch-specific,
> > > but rather "check"-specific, so I think they still belong in check.h, if
> > > possible.
> >
> > Ah, found it. They are arch specific due to struct orc_entry, which is
> > presently not defined for any archs besides x86.
> >
> > Prior to the patch (-> means "includes"):
> > check.h -> asm/orc_types.h (defines struct orc_entry)
> > orc_gen.c -> check,h
> >
> > After patch:
> > check.c -> asm/orc_types.h
> > orc_gen.c -> asm/orc_types.h
> > orc_gen.c -> check.h
> > orc_gen.c -> arch.h
> > { now weak.c } -> check.h
> >
> > So this prevents the headers, which help us keep the weak definitions
> > consistent with the strong definitions, from breaking compiles on archs
> > that lack struct orc_entry.
> >
> > I'm not sure what the best way to remove this dependency is without
> > a nasty void * for the orc entry, or some #ifdef games related to
> > checking for cpp defines from asm/orc_types.h. This approach neatly
> > avoids conditional preprocessor games and type casting though I do
> > agree it's surprising.
> >
> > Do you have any advice here?
>
> Would it work if we just move the check() and orc_dump() prototypes to
> objtool.h? Then weak.c can just include objtool.h. And check.h and
> orc.h would only need to be included for arches which implement them.
OK, I tested that and it works well. In fact it simplifies the includes
a bit so I think it might be the right move.
Cheers,
-Matt Helsley