[RFC][PATCH 0/5] Enable objtool multiarch build

[RFC][PATCH 0/5] Enable objtool multiarch build

[Matt Helsley]

My previous RFC[1] tried to add recordmcount as the mcount subcommand
of objtool. As a necessary first step that required enabling building
of objtool for more than the x86 architecture.

Some folks have been working on enabling objtool checking functionality
for arm64. Rather than repeat that work here I aim to show a minimal
set which ensures that objtool builds for any architecture. This
will allow for not only building the check and ORC subcommands
but also incorporating more subcommands -- such as recordmcount.

I changed the Makefile to use the SRCARCH to determine which subcommands
should be available and set the appropriate SUBCMD_ variables. Those
variables then get used in the Build file to conditionally build them into
objtool. When the files are missing suitable empty definitions are located
in arch/missing so that the compilation will succeed while also allowing
objtool to report that the command is unavailable on the architecture.

Since the series does not add support for stack validation or checking
to any new architectures there's no reason to make KConfig or Makefile
changes which would normally be used to test this. So I've been forcing
builds of objtool with:

make O=build-ARCH ARCH=foo CROSS_COMPILE=foo-linux-gnu- defconfig
make O=build-ARCH ARCH=foo CROSS_COMPILE=foo-linux-gnu- tools/objtool

And running the resulting binary in qemu-static to verify that it
shows all objtool subcommands are supported on x86 and unsupported on
another arch.

[1] https://lore.kernel.org/lkml/cover.1586468801.git.mhelsley@vmware.com/

Matt Helsley (5):
  objtool: Exit successfully when requesting help
  objtool: Move struct objtool_file into arch-independent header
  objtool: Add support for relocations without addends
  objtool: Enable compilation of objtool for all architectures
  objtool: Report missing support for subcommands

 tools/objtool/Build                    | 10 +++--
 tools/objtool/Makefile                 | 11 +++++-
 tools/objtool/arch.h                   | 40 +++++++++++++++++++
 tools/objtool/arch/missing/Build       |  3 ++
 tools/objtool/arch/missing/check.c     | 16 ++++++++
 tools/objtool/arch/missing/orc_dump.c  | 13 ++++++
 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-check.c          |  5 +++
 tools/objtool/builtin-orc.c            |  9 ++++-
 tools/objtool/builtin.h                |  2 +
 tools/objtool/check.c                  |  5 ++-
 tools/objtool/check.h                  | 48 +---------------------
 tools/objtool/elf.c                    | 55 +++++++++++++++++++++-----
 tools/objtool/elf.h                    |  5 ++-
 tools/objtool/objtool.c                | 39 +++++++++++++++---
 tools/objtool/objtool.h                | 20 ++++++++++
 tools/objtool/orc.h                    |  3 +-
 tools/objtool/orc_dump.c               |  1 +
 tools/objtool/orc_gen.c                |  3 ++
 22 files changed, 235 insertions(+), 76 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%)
 create mode 100644 tools/objtool/objtool.h


base-commit: 6e7f2eacf09811d092c1b41263108ac7fe0d089d
-- 
2.20.1

[RFC][PATCH 1/5] objtool: Exit successfully when requesting help

[Matt Helsley]

When the user requests help it's not an error so do not exit with
a non-zero exit code. This is not especially useful for a user but
any script that might wish to check that objtool --help is at least
available can't rely on the exit code to crudely check that, for
example building an objtool executable succeeds.

Signed-off-by: Matt Helsley <mhelsley@vmware.com>
---
 tools/objtool/objtool.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 0b3528f05053..593ec85915a9 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -58,7 +58,10 @@ static void cmd_usage(void)
 
 	printf("\n");
 
-	exit(129);
+	if (!help)
+		exit(129);
+	else
+		exit(0);
 }
 
 static void handle_options(int *argc, const char ***argv)
-- 
2.20.1

[RFC][PATCH 2/5] objtool: Move struct objtool_file into arch-independent header

[Matt Helsley]

The objtool_file structure describing the files objtool works on is
not architecture dependent -- it's not x86 only -- and it will be useful
for any future commands that might not be part of the check / orc
tooling. So we move it from the check.h header  into the objtool.h header.

Signed-off-by: Matt Helsley <mhelsley@vmware.com>
---
 tools/objtool/check.h   | 10 +---------
 tools/objtool/objtool.h | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 9 deletions(-)
 create mode 100644 tools/objtool/objtool.h

diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index f0ce8ffe7135..ec6ff7f0970c 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -7,11 +7,10 @@
 #define _CHECK_H
 
 #include <stdbool.h>
-#include "elf.h"
+#include "objtool.h"
 #include "cfi.h"
 #include "arch.h"
 #include "orc.h"
-#include <linux/hashtable.h>
 
 struct insn_state {
 	struct cfi_reg cfa;
@@ -47,13 +46,6 @@ struct instruction {
 	struct orc_entry orc;
 };
 
-struct objtool_file {
-	struct elf *elf;
-	struct list_head insn_list;
-	DECLARE_HASHTABLE(insn_hash, 20);
-	bool ignore_unreachables, c_file, hints, rodata;
-};
-
 int check(const char *objname, bool orc);
 
 struct instruction *find_insn(struct objtool_file *file,
diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
new file mode 100644
index 000000000000..afa52fe6f644
--- /dev/null
+++ b/tools/objtool/objtool.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2020 Matt Helsley <mhelsley@vmware.com>
+ */
+
+#ifndef _OBJTOOL_H
+#define _OBJTOOL_H
+#include <stdbool.h>
+#include <linux/list.h>
+#include <linux/hashtable.h>
+
+#include "elf.h"
+
+struct objtool_file {
+	struct elf *elf;
+	struct list_head insn_list;
+	DECLARE_HASHTABLE(insn_hash, 20);
+	bool ignore_unreachables, c_file, hints, rodata;
+};
+#endif
-- 
2.20.1

[RFC][PATCH 3/5] objtool: Add support for relocations without addends

[Matt Helsley]

Currently objtool only collects information about relocations with
addends. In recordmcount, which we are about to merge into objtool,
some supported architectures do not use rela relocations. Since
object files use one or the other the list can be reused.

Signed-off-by: Matt Helsley <mhelsley@vmware.com>
---
 tools/objtool/elf.c | 55 ++++++++++++++++++++++++++++++++++++---------
 tools/objtool/elf.h |  5 ++++-
 2 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index c4857fa3f1d1..cd841e3df87d 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -465,12 +465,14 @@ static int read_relas(struct elf *elf)
 	unsigned long nr_rela, max_rela = 0, tot_rela = 0;
 
 	list_for_each_entry(sec, &elf->sections, list) {
-		if (sec->sh.sh_type != SHT_RELA)
+		if ((sec->sh.sh_type != SHT_RELA) &&
+		     (sec->sh.sh_type != SHT_REL))
 			continue;
 
-		sec->base = find_section_by_name(elf, sec->name + 5);
+		sec->base = find_section_by_name(elf, sec->name +
+				((sec->sh.sh_type != SHT_REL) ? 5 : 4));
 		if (!sec->base) {
-			WARN("can't find base section for rela section %s",
+			WARN("can't find base section for relocation section %s",
 			     sec->name);
 			return -1;
 		}
@@ -486,13 +488,26 @@ static int read_relas(struct elf *elf)
 			}
 			memset(rela, 0, sizeof(*rela));
 
-			if (!gelf_getrela(sec->data, i, &rela->rela)) {
-				WARN_ELF("gelf_getrela");
-				return -1;
+			switch(sec->sh.sh_type) {
+			case SHT_REL:
+				if (!gelf_getrel(sec->data, i, &rela->rel)) {
+					WARN_ELF("gelf_getrel");
+					return -1;
+				}
+				rela->addend = 0;
+				break;
+			case SHT_RELA:
+				if (!gelf_getrela(sec->data, i, &rela->rela)) {
+					WARN_ELF("gelf_getrela");
+					return -1;
+				}
+				rela->addend = rela->rela.r_addend;
+				break;
+			default:
+				break;
 			}
 
 			rela->type = GELF_R_TYPE(rela->rela.r_info);
-			rela->addend = rela->rela.r_addend;
 			rela->offset = rela->rela.r_offset;
 			symndx = GELF_R_SYM(rela->rela.r_info);
 			rela->sym = find_symbol_by_index(elf, symndx);
@@ -717,17 +732,27 @@ int elf_rebuild_rela_section(struct section *sec)
 	struct rela *rela;
 	int nr, idx = 0, size;
 	GElf_Rela *relas;
+	GElf_Rel *rels;
 
 	nr = 0;
 	list_for_each_entry(rela, &sec->rela_list, list)
 		nr++;
 
+	/*
+	 * Allocate a buffer for relocations with addends but also use
+	 * it for other relocations too. The section type determines
+	 * the size of the section, the buffer used, and the entries.
+	 */
 	size = nr * sizeof(*relas);
 	relas = malloc(size);
 	if (!relas) {
 		perror("malloc");
 		return -1;
 	}
+	rels = (void *)relas;
+	if (sec->sh.sh_type == SHT_REL) {
+		size = nr * sizeof(*rels);
+	}
 
 	sec->data->d_buf = relas;
 	sec->data->d_size = size;
@@ -736,9 +761,19 @@ int elf_rebuild_rela_section(struct section *sec)
 
 	idx = 0;
 	list_for_each_entry(rela, &sec->rela_list, list) {
-		relas[idx].r_offset = rela->offset;
-		relas[idx].r_addend = rela->addend;
-		relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
+		switch(sec->sh.sh_type) {
+		case SHT_REL:
+			rels[idx].r_offset = rela->offset;
+			rels[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
+			break;
+		case SHT_RELA:
+			relas[idx].r_addend = rela->addend;
+			relas[idx].r_offset = rela->offset;
+			relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
+			break;
+		default:
+			break;
+		}
 		idx++;
 	}
 
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index 0b79c2353a21..71bd47055963 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -61,7 +61,10 @@ struct symbol {
 struct rela {
 	struct list_head list;
 	struct hlist_node hash;
-	GElf_Rela rela;
+	union {
+		GElf_Rela rela;
+		GElf_Rel  rel;
+	};
 	struct section *sec;
 	struct symbol *sym;
 	unsigned int type;
-- 
2.20.1

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

[Matt Helsley]

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 <mhelsley@vmware.com>
---
 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 <mhelsley@vmware.com>
+ */
+
+#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 <jpoimboe@redhat.com>
+ */
+
+#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 <jpoimboe@redhat.com>
+ */
+
+#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

[RFC][PATCH 5/5] objtool: Report missing support for subcommands

[Matt Helsley]

The check and orc-related subcommands for objtool are x86-specific.
To make this clear to anyone using the tool return a non-zero exit
code and indicate in the help and usage output which commands are
(and are not) available.

Signed-off-by: Matt Helsley <mhelsley@vmware.com>
---
 tools/objtool/Build                   |  1 +
 tools/objtool/arch/missing/check.c    |  4 +++-
 tools/objtool/arch/missing/orc_dump.c |  4 +++-
 tools/objtool/builtin-check.c         |  5 ++++
 tools/objtool/builtin-orc.c           |  7 ++++++
 tools/objtool/builtin.h               |  2 ++
 tools/objtool/check.h                 |  1 +
 tools/objtool/objtool.c               | 34 +++++++++++++++++++++++----
 tools/objtool/orc.h                   |  1 +
 9 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/tools/objtool/Build b/tools/objtool/Build
index fb6e6faf6f10..8e5ad5c238ba 100644
--- a/tools/objtool/Build
+++ b/tools/objtool/Build
@@ -1,3 +1,4 @@
+objtool-y += arch/missing/
 objtool-y += arch/$(SRCARCH)/
 
 objtool-$(SUBCMD_CHECK) += check.o
diff --git a/tools/objtool/arch/missing/check.c b/tools/objtool/arch/missing/check.c
index 5eac14c8c6a5..7f9deb0451bb 100644
--- a/tools/objtool/arch/missing/check.c
+++ b/tools/objtool/arch/missing/check.c
@@ -8,7 +8,9 @@
 
 const char *objname;
 
-int __attribute__ ((weak)) check(const char *_objname, bool orc)
+int missing_check(const char *_objname, bool orc)
 {
 	return 127;
 }
+
+int __attribute__ ((weak, alias("missing_check"))) check(const char *_objname, bool orc);
diff --git a/tools/objtool/arch/missing/orc_dump.c b/tools/objtool/arch/missing/orc_dump.c
index f7ebb3a2ce9e..45367ddd08a4 100644
--- a/tools/objtool/arch/missing/orc_dump.c
+++ b/tools/objtool/arch/missing/orc_dump.c
@@ -5,7 +5,9 @@
 
 #include "../../orc.h"
 
-int __attribute__ ((weak)) orc_dump(const char *_objname)
+int missing_orc_dump(const char *_objname)
 {
 	return 127;
 }
+
+int __attribute__ ((weak, alias("missing_orc_dump"))) orc_dump(const char *_objname);
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 10fbe75ab43d..7e7ad576ebc0 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -35,6 +35,11 @@ const struct option check_options[] = {
 	OPT_END(),
 };
 
+bool is_cmd_check_available(void)
+{
+	return check != missing_check;
+}
+
 int cmd_check(int argc, const char **argv)
 {
 	const char *objname;
diff --git a/tools/objtool/builtin-orc.c b/tools/objtool/builtin-orc.c
index e6e54ae4ab75..043b93f68e83 100644
--- a/tools/objtool/builtin-orc.c
+++ b/tools/objtool/builtin-orc.c
@@ -23,6 +23,13 @@ static const char *orc_usage[] = {
 	NULL,
 };
 
+bool is_cmd_orc_available(void)
+{
+	if (check == missing_check)
+		return false;
+	return orc_dump != missing_orc_dump;
+}
+
 int cmd_orc(int argc, const char **argv)
 {
 	const char *objname;
diff --git a/tools/objtool/builtin.h b/tools/objtool/builtin.h
index 0b907902ee79..478558d38007 100644
--- a/tools/objtool/builtin.h
+++ b/tools/objtool/builtin.h
@@ -10,7 +10,9 @@
 extern const struct option check_options[];
 extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats;
 
+extern bool is_cmd_check_available(void);
 extern int cmd_check(int argc, const char **argv);
+extern bool is_cmd_orc_available(void);
 extern int cmd_orc(int argc, const char **argv);
 
 #endif /* _BUILTIN_H */
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 4d34bfc84dc7..c54fb9f5c8e6 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -9,6 +9,7 @@
 #include <stdbool.h>
 #include "objtool.h"
 
+int missing_check(const char *objname, bool orc);
 int check(const char *objname, bool orc);
 
 struct instruction *find_insn(struct objtool_file *file,
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 593ec85915a9..e901c40c76ef 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -27,21 +27,22 @@ struct cmd_struct {
 	const char *name;
 	int (*fn)(int, const char **);
 	const char *help;
+	bool (*is_available)(void);
 };
 
 static const char objtool_usage_string[] =
 	"objtool COMMAND [ARGS]";
 
 static struct cmd_struct objtool_cmds[] = {
-	{"check",	cmd_check,	"Perform stack metadata validation on an object file" },
-	{"orc",		cmd_orc,	"Generate in-place ORC unwind tables for an object file" },
+	{"check",	cmd_check,	"Perform stack metadata validation on an object file", is_cmd_check_available },
+	{"orc",		cmd_orc,	"Generate in-place ORC unwind tables for an object file", is_cmd_orc_available },
 };
 
 bool help;
 
 static void cmd_usage(void)
 {
-	unsigned int i, longest = 0;
+	unsigned int i, longest = 0, num_unavail = 0;
 
 	printf("\n usage: %s\n\n", objtool_usage_string);
 
@@ -52,8 +53,26 @@ static void cmd_usage(void)
 
 	puts(" Commands:");
 	for (i = 0; i < ARRAY_SIZE(objtool_cmds); i++) {
-		printf("   %-*s   ", longest, objtool_cmds[i].name);
-		puts(objtool_cmds[i].help);
+		struct cmd_struct *p = objtool_cmds+i;
+
+		if (!p->is_available()) {
+			num_unavail++;
+			continue;
+		}
+		printf("   %-*s   ", longest, p->name);
+		puts(p->help);
+	}
+
+	if (num_unavail > 0) {
+		puts("\n Unavailable commands on this architecture:");
+		for (i = 0; i < ARRAY_SIZE(objtool_cmds); i++) {
+			struct cmd_struct *p = objtool_cmds+i;
+
+			if (p->is_available())
+				continue;
+			printf("   %-*s   ", longest, p->name);
+			puts(p->help);
+		}
 	}
 
 	printf("\n");
@@ -96,6 +115,11 @@ static void handle_internal_command(int argc, const char **argv)
 		if (strcmp(p->name, cmd))
 			continue;
 
+		if (!p->is_available()) {
+			fprintf(stderr, "command %s is not available on this architecture\n", cmd);
+			exit(127);
+		}
+
 		ret = p->fn(argc, argv);
 
 		exit(ret);
diff --git a/tools/objtool/orc.h b/tools/objtool/orc.h
index c67f451d7610..9174356ba0fa 100644
--- a/tools/objtool/orc.h
+++ b/tools/objtool/orc.h
@@ -11,6 +11,7 @@ struct objtool_file;
 int create_orc(struct objtool_file *file);
 int create_orc_sections(struct objtool_file *file);
 
+int missing_orc_dump(const char *objname);
 int orc_dump(const char *objname);
 
 #endif /* _ORC_H */
-- 
2.20.1

Re: [RFC][PATCH 0/5] Enable objtool multiarch build

[Julien Thierry]

Hi Matt,

On 5/11/20 6:35 PM, Matt Helsley wrote:
> My previous RFC[1] tried to add recordmcount as the mcount subcommand
> of objtool. As a necessary first step that required enabling building
> of objtool for more than the x86 architecture.
> 
> Some folks have been working on enabling objtool checking functionality
> for arm64. Rather than repeat that work here I aim to show a minimal
> set which ensures that objtool builds for any architecture. This
> will allow for not only building the check and ORC subcommands
> but also incorporating more subcommands -- such as recordmcount.
> 

Thanks for putting the effort to simplify adding support of the 
different command for new architectures.

I have a few some comments on some commits.

Cheers,

-- 
Julien Thierry

Re: [RFC][PATCH 2/5] objtool: Move struct objtool_file into arch-independent header

[Julien Thierry]

Hi Matt,

On 5/11/20 6:35 PM, Matt Helsley wrote:
> The objtool_file structure describing the files objtool works on is
> not architecture dependent -- it's not x86 only -- and it will be useful
> for any future commands that might not be part of the check / orc
> tooling. So we move it from the check.h header  into the objtool.h header.
> 

The change itself looks alright to me, however I'd say the justification 
is more about the fact the more subcommands dealing with object files 
are going to be added to objtool, and all those subcommand will likely 
use the objtool_file representation.

I think it doesn't have much to do with arch specificity. (But this is 
really about the commit message, otherwise the changes make sense)

> Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> ---
>   tools/objtool/check.h   | 10 +---------
>   tools/objtool/objtool.h | 20 ++++++++++++++++++++
>   2 files changed, 21 insertions(+), 9 deletions(-)
>   create mode 100644 tools/objtool/objtool.h
> 
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index f0ce8ffe7135..ec6ff7f0970c 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -7,11 +7,10 @@
>   #define _CHECK_H
>   
>   #include <stdbool.h>
> -#include "elf.h"
> +#include "objtool.h"
>   #include "cfi.h"
>   #include "arch.h"
>   #include "orc.h"
> -#include <linux/hashtable.h>
>   
>   struct insn_state {
>   	struct cfi_reg cfa;
> @@ -47,13 +46,6 @@ struct instruction {
>   	struct orc_entry orc;
>   };
>   
> -struct objtool_file {
> -	struct elf *elf;
> -	struct list_head insn_list;
> -	DECLARE_HASHTABLE(insn_hash, 20);
> -	bool ignore_unreachables, c_file, hints, rodata;
> -};
> -
>   int check(const char *objname, bool orc);
>   
>   struct instruction *find_insn(struct objtool_file *file,
> diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
> new file mode 100644
> index 000000000000..afa52fe6f644
> --- /dev/null
> +++ b/tools/objtool/objtool.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020 Matt Helsley <mhelsley@vmware.com>
> + */
> +
> +#ifndef _OBJTOOL_H
> +#define _OBJTOOL_H
> +#include <stdbool.h>
> +#include <linux/list.h>
> +#include <linux/hashtable.h>
> +
> +#include "elf.h"
> +
> +struct objtool_file {
> +	struct elf *elf;
> +	struct list_head insn_list;
> +	DECLARE_HASHTABLE(insn_hash, 20);
> +	bool ignore_unreachables, c_file, hints, rodata;
> +};
> +#endif
> 

-- 
Julien Thierry

Re: [RFC][PATCH 3/5] objtool: Add support for relocations without addends

[Julien Thierry]

Hi Matt,

On 5/11/20 6:35 PM, Matt Helsley wrote:
> Currently objtool only collects information about relocations with
> addends. In recordmcount, which we are about to merge into objtool,
> some supported architectures do not use rela relocations. Since
> object files use one or the other the list can be reused.
> 
> Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> ---
>   tools/objtool/elf.c | 55 ++++++++++++++++++++++++++++++++++++---------
>   tools/objtool/elf.h |  5 ++++-
>   2 files changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index c4857fa3f1d1..cd841e3df87d 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -465,12 +465,14 @@ static int read_relas(struct elf *elf)
>   	unsigned long nr_rela, max_rela = 0, tot_rela = 0;
>   
>   	list_for_each_entry(sec, &elf->sections, list) {
> -		if (sec->sh.sh_type != SHT_RELA)
> +		if ((sec->sh.sh_type != SHT_RELA) &&
> +		     (sec->sh.sh_type != SHT_REL))
>   			continue;
>   
> -		sec->base = find_section_by_name(elf, sec->name + 5);
> +		sec->base = find_section_by_name(elf, sec->name +
> +				((sec->sh.sh_type != SHT_REL) ? 5 : 4));
>   		if (!sec->base) {
> -			WARN("can't find base section for rela section %s",
> +			WARN("can't find base section for relocation section %s",
>   			     sec->name);
>   			return -1;
>   		}
> @@ -486,13 +488,26 @@ static int read_relas(struct elf *elf)
>   			}
>   			memset(rela, 0, sizeof(*rela));
>   
> -			if (!gelf_getrela(sec->data, i, &rela->rela)) {
> -				WARN_ELF("gelf_getrela");
> -				return -1;
> +			switch(sec->sh.sh_type) {
> +			case SHT_REL:
> +				if (!gelf_getrel(sec->data, i, &rela->rel)) {
> +					WARN_ELF("gelf_getrel");
> +					return -1;
> +				}
> +				rela->addend = 0;
> +				break;
> +			case SHT_RELA:
> +				if (!gelf_getrela(sec->data, i, &rela->rela)) {
> +					WARN_ELF("gelf_getrela");
> +					return -1;
> +				}
> +				rela->addend = rela->rela.r_addend;
> +				break;
> +			default:
> +				break;
>   			}
>   
>   			rela->type = GELF_R_TYPE(rela->rela.r_info);
> -			rela->addend = rela->rela.r_addend;
>   			rela->offset = rela->rela.r_offset;
>   			symndx = GELF_R_SYM(rela->rela.r_info);
>   			rela->sym = find_symbol_by_index(elf, symndx);
> @@ -717,17 +732,27 @@ int elf_rebuild_rela_section(struct section *sec)
>   	struct rela *rela;
>   	int nr, idx = 0, size;
>   	GElf_Rela *relas;
> +	GElf_Rel *rels;
>   
>   	nr = 0;
>   	list_for_each_entry(rela, &sec->rela_list, list)
>   		nr++;
>   
> +	/*
> +	 * Allocate a buffer for relocations with addends but also use
> +	 * it for other relocations too. The section type determines
> +	 * the size of the section, the buffer used, and the entries.
> +	 */
>   	size = nr * sizeof(*relas);
>   	relas = malloc(size);
>   	if (!relas) {
>   		perror("malloc");
>   		return -1;
>   	}
> +	rels = (void *)relas;
> +	if (sec->sh.sh_type == SHT_REL) {
> +		size = nr * sizeof(*rels);
> +	}

This looks a bit error prone to me.

What about having:

     void *rel_buf;
     [...]
     size = nr * (sec->sh.sh_type == SHT_REL ? sizeof(GElf_Rel) : 
sizeof(GElf_Rela));
     rel_buf = malloc(size);
     [...]

And then casting rel_buf to the correct pointer type in the fitting 
switch cases?

>   
>   	sec->data->d_buf = relas;
>   	sec->data->d_size = size;
> @@ -736,9 +761,19 @@ int elf_rebuild_rela_section(struct section *sec)
>   
>   	idx = 0;
>   	list_for_each_entry(rela, &sec->rela_list, list) {
> -		relas[idx].r_offset = rela->offset;
> -		relas[idx].r_addend = rela->addend;
> -		relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> +		switch(sec->sh.sh_type) {
> +		case SHT_REL:
> +			rels[idx].r_offset = rela->offset;
> +			rels[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> +			break;
> +		case SHT_RELA:
> +			relas[idx].r_addend = rela->addend;
> +			relas[idx].r_offset = rela->offset;
> +			relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> +			break;
> +		default:
> +			break;
> +		}
>   		idx++;
>   	}
>   
> diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
> index 0b79c2353a21..71bd47055963 100644
> --- a/tools/objtool/elf.h
> +++ b/tools/objtool/elf.h
> @@ -61,7 +61,10 @@ struct symbol {
>   struct rela {
>   	struct list_head list;
>   	struct hlist_node hash;
> -	GElf_Rela rela;
> +	union {
> +		GElf_Rela rela;
> +		GElf_Rel  rel;
> +	};
>   	struct section *sec;
>   	struct symbol *sym;
>   	unsigned int type;
> 

-- 
Julien Thierry

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

[Julien Thierry]

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 <mhelsley@vmware.com>
> ---
>   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 <mhelsley@vmware.com>
> + */
> +
> +#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 <jpoimboe@redhat.com>
> + */
> +
> +#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 <jpoimboe@redhat.com>
> + */
> +
> +#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

Re: [RFC][PATCH 2/5] objtool: Move struct objtool_file into arch-independent header

[Matt Helsley]

On Tue, May 12, 2020 at 06:04:43PM +0100, Julien Thierry wrote:
> Hi Matt,
> 
> On 5/11/20 6:35 PM, Matt Helsley wrote:
> > The objtool_file structure describing the files objtool works on is
> > not architecture dependent -- it's not x86 only -- and it will be useful
> > for any future commands that might not be part of the check / orc
> > tooling. So we move it from the check.h header  into the objtool.h header.
> > 
> 
> The change itself looks alright to me, however I'd say the justification is
> more about the fact the more subcommands dealing with object files are going
> to be added to objtool, and all those subcommand will likely use the
> objtool_file representation.
> 
> I think it doesn't have much to do with arch specificity. (But this is
> really about the commit message, otherwise the changes make sense)

Good point. Here's a rewrite of the commit message:

	The objtool_file structure describes the files objtool works on,
	is used by the check subcommand, and the check.h header is included
	by the orc subcommands so it's presently used by all subcommands.

	Since the structure will be useful in all subcommands besides check, 
	and some subcommands may not want to include check.h to get the
	definition, split the structure out into a new header meant for use
	by all objtool subcommands.

Thanks!

Cheers,
     -Matt Helsley

> 
> > Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> > ---
> >   tools/objtool/check.h   | 10 +---------
> >   tools/objtool/objtool.h | 20 ++++++++++++++++++++
> >   2 files changed, 21 insertions(+), 9 deletions(-)
> >   create mode 100644 tools/objtool/objtool.h
> > 
> > diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> > index f0ce8ffe7135..ec6ff7f0970c 100644
> > --- a/tools/objtool/check.h
> > +++ b/tools/objtool/check.h
> > @@ -7,11 +7,10 @@
> >   #define _CHECK_H
> >   #include <stdbool.h>
> > -#include "elf.h"
> > +#include "objtool.h"
> >   #include "cfi.h"
> >   #include "arch.h"
> >   #include "orc.h"
> > -#include <linux/hashtable.h>
> >   struct insn_state {
> >   	struct cfi_reg cfa;
> > @@ -47,13 +46,6 @@ struct instruction {
> >   	struct orc_entry orc;
> >   };
> > -struct objtool_file {
> > -	struct elf *elf;
> > -	struct list_head insn_list;
> > -	DECLARE_HASHTABLE(insn_hash, 20);
> > -	bool ignore_unreachables, c_file, hints, rodata;
> > -};
> > -
> >   int check(const char *objname, bool orc);
> >   struct instruction *find_insn(struct objtool_file *file,
> > diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
> > new file mode 100644
> > index 000000000000..afa52fe6f644
> > --- /dev/null
> > +++ b/tools/objtool/objtool.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020 Matt Helsley <mhelsley@vmware.com>
> > + */
> > +
> > +#ifndef _OBJTOOL_H
> > +#define _OBJTOOL_H
> > +#include <stdbool.h>
> > +#include <linux/list.h>
> > +#include <linux/hashtable.h>
> > +
> > +#include "elf.h"
> > +
> > +struct objtool_file {
> > +	struct elf *elf;
> > +	struct list_head insn_list;
> > +	DECLARE_HASHTABLE(insn_hash, 20);
> > +	bool ignore_unreachables, c_file, hints, rodata;
> > +};
> > +#endif
> > 
> 
> -- 
> Julien Thierry
> 

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

Re: [RFC][PATCH 3/5] objtool: Add support for relocations without addends

[Matt Helsley]

On Tue, May 12, 2020 at 06:04:50PM +0100, Julien Thierry wrote:
> Hi Matt,
> 
> On 5/11/20 6:35 PM, Matt Helsley wrote:
> > Currently objtool only collects information about relocations with
> > addends. In recordmcount, which we are about to merge into objtool,
> > some supported architectures do not use rela relocations. Since
> > object files use one or the other the list can be reused.
> > 
> > Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> > ---
> >   tools/objtool/elf.c | 55 ++++++++++++++++++++++++++++++++++++---------
> >   tools/objtool/elf.h |  5 ++++-
> >   2 files changed, 49 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> > index c4857fa3f1d1..cd841e3df87d 100644
> > --- a/tools/objtool/elf.c
> > +++ b/tools/objtool/elf.c
> > @@ -465,12 +465,14 @@ static int read_relas(struct elf *elf)
> >   	unsigned long nr_rela, max_rela = 0, tot_rela = 0;
> >   	list_for_each_entry(sec, &elf->sections, list) {
> > -		if (sec->sh.sh_type != SHT_RELA)
> > +		if ((sec->sh.sh_type != SHT_RELA) &&
> > +		     (sec->sh.sh_type != SHT_REL))
> >   			continue;
> > -		sec->base = find_section_by_name(elf, sec->name + 5);
> > +		sec->base = find_section_by_name(elf, sec->name +
> > +				((sec->sh.sh_type != SHT_REL) ? 5 : 4));
> >   		if (!sec->base) {
> > -			WARN("can't find base section for rela section %s",
> > +			WARN("can't find base section for relocation section %s",
> >   			     sec->name);
> >   			return -1;
> >   		}
> > @@ -486,13 +488,26 @@ static int read_relas(struct elf *elf)
> >   			}
> >   			memset(rela, 0, sizeof(*rela));
> > -			if (!gelf_getrela(sec->data, i, &rela->rela)) {
> > -				WARN_ELF("gelf_getrela");
> > -				return -1;
> > +			switch(sec->sh.sh_type) {
> > +			case SHT_REL:
> > +				if (!gelf_getrel(sec->data, i, &rela->rel)) {
> > +					WARN_ELF("gelf_getrel");
> > +					return -1;
> > +				}
> > +				rela->addend = 0;
> > +				break;
> > +			case SHT_RELA:
> > +				if (!gelf_getrela(sec->data, i, &rela->rela)) {
> > +					WARN_ELF("gelf_getrela");
> > +					return -1;
> > +				}
> > +				rela->addend = rela->rela.r_addend;
> > +				break;
> > +			default:
> > +				break;
> >   			}
> >   			rela->type = GELF_R_TYPE(rela->rela.r_info);
> > -			rela->addend = rela->rela.r_addend;
> >   			rela->offset = rela->rela.r_offset;
> >   			symndx = GELF_R_SYM(rela->rela.r_info);
> >   			rela->sym = find_symbol_by_index(elf, symndx);
> > @@ -717,17 +732,27 @@ int elf_rebuild_rela_section(struct section *sec)
> >   	struct rela *rela;
> >   	int nr, idx = 0, size;
> >   	GElf_Rela *relas;
> > +	GElf_Rel *rels;
> >   	nr = 0;
> >   	list_for_each_entry(rela, &sec->rela_list, list)
> >   		nr++;
> > +	/*
> > +	 * Allocate a buffer for relocations with addends but also use
> > +	 * it for other relocations too. The section type determines
> > +	 * the size of the section, the buffer used, and the entries.
> > +	 */
> >   	size = nr * sizeof(*relas);
> >   	relas = malloc(size);
> >   	if (!relas) {
> >   		perror("malloc");
> >   		return -1;
> >   	}
> > +	rels = (void *)relas;
> > +	if (sec->sh.sh_type == SHT_REL) {
> > +		size = nr * sizeof(*rels);
> > +	}
> 
> This looks a bit error prone to me.
> 
> What about having:
> 
>     void *rel_buf;
>     [...]
>     size = nr * (sec->sh.sh_type == SHT_REL ? sizeof(GElf_Rel) :
> sizeof(GElf_Rela));

I like reducing to a single size expression but I'm not a fan of hard-coding
the GElf_Rel[a] types here -- I prefer sizeof(*relas) and sizeof(*rels)
since that makes it clear the sizes will match the types of the pointers
that will be used to access them. So I've changed it to:

	size = nr * ((sec->sh.sh_type == SHT_REL) ? sizeof(*rels) : sizeof(*relas));

>     rel_buf = malloc(size);
>     [...]
> 
> And then casting rel_buf to the correct pointer type in the fitting switch
> cases?

I'm thinking it's simpler with fewer variables. I don't think
moving the cast into the switch cases makes it any clearer. It's also
odd because we'll keep re-initializing relas or rels to rel_buf each loop
iteration. Finally, this approach has the advantage that, when reviewing
the patch, it's clear that the original code handling RELA relocation entries
isn't changing -- you can see it's just shifting into one of the cases
(below).

Do you still prefer introducing rel_buf?

> 
> >   	sec->data->d_buf = relas;
> >   	sec->data->d_size = size;
> > @@ -736,9 +761,19 @@ int elf_rebuild_rela_section(struct section *sec)
> >   	idx = 0;
> >   	list_for_each_entry(rela, &sec->rela_list, list) {
> > -		relas[idx].r_offset = rela->offset;
> > -		relas[idx].r_addend = rela->addend;
> > -		relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> > +		switch(sec->sh.sh_type) {
> > +		case SHT_REL:
> > +			rels[idx].r_offset = rela->offset;
> > +			rels[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> > +			break;
> > +		case SHT_RELA:
> > +			relas[idx].r_addend = rela->addend;
> > +			relas[idx].r_offset = rela->offset;
> > +			relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> > +			break;
> > +		default:
> > +			break;
> > +		}
> >   		idx++;
> >   	}

Cheers,
     -Matt Helsley

Re: [RFC][PATCH 3/5] objtool: Add support for relocations without addends

[Julien Thierry]

On 5/13/20 5:26 PM, Matt Helsley wrote:
> On Tue, May 12, 2020 at 06:04:50PM +0100, Julien Thierry wrote:
>> Hi Matt,
>>
>> On 5/11/20 6:35 PM, Matt Helsley wrote:
>>> Currently objtool only collects information about relocations with
>>> addends. In recordmcount, which we are about to merge into objtool,
>>> some supported architectures do not use rela relocations. Since
>>> object files use one or the other the list can be reused.
>>>
>>> Signed-off-by: Matt Helsley <mhelsley@vmware.com>
>>> ---
>>>    tools/objtool/elf.c | 55 ++++++++++++++++++++++++++++++++++++---------
>>>    tools/objtool/elf.h |  5 ++++-
>>>    2 files changed, 49 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
>>> index c4857fa3f1d1..cd841e3df87d 100644
>>> --- a/tools/objtool/elf.c
>>> +++ b/tools/objtool/elf.c
>>> @@ -465,12 +465,14 @@ static int read_relas(struct elf *elf)
>>>    	unsigned long nr_rela, max_rela = 0, tot_rela = 0;
>>>    	list_for_each_entry(sec, &elf->sections, list) {
>>> -		if (sec->sh.sh_type != SHT_RELA)
>>> +		if ((sec->sh.sh_type != SHT_RELA) &&
>>> +		     (sec->sh.sh_type != SHT_REL))
>>>    			continue;
>>> -		sec->base = find_section_by_name(elf, sec->name + 5);
>>> +		sec->base = find_section_by_name(elf, sec->name +
>>> +				((sec->sh.sh_type != SHT_REL) ? 5 : 4));
>>>    		if (!sec->base) {
>>> -			WARN("can't find base section for rela section %s",
>>> +			WARN("can't find base section for relocation section %s",
>>>    			     sec->name);
>>>    			return -1;
>>>    		}
>>> @@ -486,13 +488,26 @@ static int read_relas(struct elf *elf)
>>>    			}
>>>    			memset(rela, 0, sizeof(*rela));
>>> -			if (!gelf_getrela(sec->data, i, &rela->rela)) {
>>> -				WARN_ELF("gelf_getrela");
>>> -				return -1;
>>> +			switch(sec->sh.sh_type) {
>>> +			case SHT_REL:
>>> +				if (!gelf_getrel(sec->data, i, &rela->rel)) {
>>> +					WARN_ELF("gelf_getrel");
>>> +					return -1;
>>> +				}
>>> +				rela->addend = 0;
>>> +				break;
>>> +			case SHT_RELA:
>>> +				if (!gelf_getrela(sec->data, i, &rela->rela)) {
>>> +					WARN_ELF("gelf_getrela");
>>> +					return -1;
>>> +				}
>>> +				rela->addend = rela->rela.r_addend;
>>> +				break;
>>> +			default:
>>> +				break;
>>>    			}
>>>    			rela->type = GELF_R_TYPE(rela->rela.r_info);
>>> -			rela->addend = rela->rela.r_addend;
>>>    			rela->offset = rela->rela.r_offset;
>>>    			symndx = GELF_R_SYM(rela->rela.r_info);
>>>    			rela->sym = find_symbol_by_index(elf, symndx);
>>> @@ -717,17 +732,27 @@ int elf_rebuild_rela_section(struct section *sec)
>>>    	struct rela *rela;
>>>    	int nr, idx = 0, size;
>>>    	GElf_Rela *relas;
>>> +	GElf_Rel *rels;
>>>    	nr = 0;
>>>    	list_for_each_entry(rela, &sec->rela_list, list)
>>>    		nr++;
>>> +	/*
>>> +	 * Allocate a buffer for relocations with addends but also use
>>> +	 * it for other relocations too. The section type determines
>>> +	 * the size of the section, the buffer used, and the entries.
>>> +	 */
>>>    	size = nr * sizeof(*relas);
>>>    	relas = malloc(size);
>>>    	if (!relas) {
>>>    		perror("malloc");
>>>    		return -1;
>>>    	}
>>> +	rels = (void *)relas;
>>> +	if (sec->sh.sh_type == SHT_REL) {
>>> +		size = nr * sizeof(*rels);
>>> +	}
>>
>> This looks a bit error prone to me.
>>
>> What about having:
>>
>>      void *rel_buf;
>>      [...]
>>      size = nr * (sec->sh.sh_type == SHT_REL ? sizeof(GElf_Rel) :
>> sizeof(GElf_Rela));
> 
> I like reducing to a single size expression but I'm not a fan of hard-coding
> the GElf_Rel[a] types here -- I prefer sizeof(*relas) and sizeof(*rels)
> since that makes it clear the sizes will match the types of the pointers
> that will be used to access them. So I've changed it to:
> 
> 	size = nr * ((sec->sh.sh_type == SHT_REL) ? sizeof(*rels) : sizeof(*relas));
> 
>>      rel_buf = malloc(size);
>>      [...]
>>
>> And then casting rel_buf to the correct pointer type in the fitting switch
>> cases?
> 
> I'm thinking it's simpler with fewer variables. I don't think
> moving the cast into the switch cases makes it any clearer. It's also
> odd because we'll keep re-initializing relas or rels to rel_buf each loop
> iteration. Finally, this approach has the advantage that, when reviewing
> the patch, it's clear that the original code handling RELA relocation entries
> isn't changing -- you can see it's just shifting into one of the cases
> (below).
> 
> Do you still prefer introducing rel_buf?

On a completely personal taste, yes. I do not like having two local 
variables in the same scope pointing at the same data but with an 
implied "you should only use one or the other under the right 
circumstances".

But my main concern was having an allocation of a certain size and then 
modifying the size (might have been valid if sizeof(GElf_Rel) <= 
sizeof(GElf_Rela), but I must admit I did not bother to check). Since 
you've addressed that issue, the rest is just a matter of taste so 
better left to the maintainers.

Thanks,

-- 
Julien Thierry

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

[Julien Thierry]

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

Re: [RFC][PATCH 3/5] objtool: Add support for relocations without addends

[Matt Helsley]

On Wed, May 13, 2020 at 05:55:21PM +0100, Julien Thierry wrote:
> 
> 
> On 5/13/20 5:26 PM, Matt Helsley wrote:
> > On Tue, May 12, 2020 at 06:04:50PM +0100, Julien Thierry wrote:
> > > Hi Matt,
> > > 
> > > On 5/11/20 6:35 PM, Matt Helsley wrote:
> > > > Currently objtool only collects information about relocations with
> > > > addends. In recordmcount, which we are about to merge into objtool,
> > > > some supported architectures do not use rela relocations. Since
> > > > object files use one or the other the list can be reused.
> > > > 
> > > > Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> > > > ---
> > > >    tools/objtool/elf.c | 55 ++++++++++++++++++++++++++++++++++++---------
> > > >    tools/objtool/elf.h |  5 ++++-

<snip>

> > I'm thinking it's simpler with fewer variables. I don't think
> > moving the cast into the switch cases makes it any clearer. It's also
> > odd because we'll keep re-initializing relas or rels to rel_buf each loop
> > iteration. Finally, this approach has the advantage that, when reviewing
> > the patch, it's clear that the original code handling RELA relocation entries
> > isn't changing -- you can see it's just shifting into one of the cases
> > (below).
> > 
> > Do you still prefer introducing rel_buf?
> 
> On a completely personal taste, yes. I do not like having two local
> variables in the same scope pointing at the same data but with an implied
> "you should only use one or the other under the right circumstances".
> 
> But my main concern was having an allocation of a certain size and then
> modifying the size (might have been valid if sizeof(GElf_Rel) <=
> sizeof(GElf_Rela), but I must admit I did not bother to check). Since you've
> addressed that issue, the rest is just a matter of taste so better left to
> the maintainers.

OK. As a sort of tie-breaker I tried to quickly find an analogous piece of code
in objtool which I could use as a guide on maintainer preference. The
only place I see void pointers being used is as keys for comparison
functions. So my guess and preference is to not use a void pointer here.

Cheers,
	-Matt Helsley

Re: [RFC][PATCH 1/5] objtool: Exit successfully when requesting help

[Josh Poimboeuf]

On Mon, May 11, 2020 at 10:35:09AM -0700, Matt Helsley wrote:
> When the user requests help it's not an error so do not exit with
> a non-zero exit code. This is not especially useful for a user but
> any script that might wish to check that objtool --help is at least
> available can't rely on the exit code to crudely check that, for
> example building an objtool executable succeeds.
> 
> Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> ---
>  tools/objtool/objtool.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
> index 0b3528f05053..593ec85915a9 100644
> --- a/tools/objtool/objtool.c
> +++ b/tools/objtool/objtool.c
> @@ -58,7 +58,10 @@ static void cmd_usage(void)
>  
>  	printf("\n");
>  
> -	exit(129);
> +	if (!help)
> +		exit(129);
> +	else
> +		exit(0);

Looks fine, though the 'else' isn't needed.

-- 
Josh

Re: [RFC][PATCH 3/5] objtool: Add support for relocations without addends

[Josh Poimboeuf]

On Mon, May 11, 2020 at 10:35:11AM -0700, Matt Helsley wrote:
> Currently objtool only collects information about relocations with
> addends. In recordmcount, which we are about to merge into objtool,
> some supported architectures do not use rela relocations. Since
> object files use one or the other the list can be reused.
> 
> Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> ---
>  tools/objtool/elf.c | 55 ++++++++++++++++++++++++++++++++++++---------
>  tools/objtool/elf.h |  5 ++++-
>  2 files changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index c4857fa3f1d1..cd841e3df87d 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -465,12 +465,14 @@ static int read_relas(struct elf *elf)

This should probably be called read_relocs() now.

And 'struct rela' should probably be 'struct reloc'.  And I hate to say
it but all the 'rela' based variable/function names should also probably
be changed...

All the renaming might be disruptive for backports, but still I think it
would be a good idea.  It probably belongs in its own commit.  If it can
be done programmatically with 'sed -i' or so, with the exact command in
the commit log, even better :-)

>  	unsigned long nr_rela, max_rela = 0, tot_rela = 0;
>  
>  	list_for_each_entry(sec, &elf->sections, list) {
> -		if (sec->sh.sh_type != SHT_RELA)
> +		if ((sec->sh.sh_type != SHT_RELA) &&
> +		     (sec->sh.sh_type != SHT_REL))
>  			continue;

The alignment is slightly off, should be:

		if ((sec->sh.sh_type != SHT_RELA) &&
		    (sec->sh.sh_type != SHT_REL))
			continue;

>  
> -		sec->base = find_section_by_name(elf, sec->name + 5);
> +		sec->base = find_section_by_name(elf, sec->name +
> +				((sec->sh.sh_type != SHT_REL) ? 5 : 4));

I think there's actually a cleaner way to do this, which we probably
should have been doing in the first place:

		sec->base = find_section_by_index(elf, sec->sh.sh_info);

(completely not tested, btw)

> @@ -486,13 +488,26 @@ static int read_relas(struct elf *elf)
>  			}
>  			memset(rela, 0, sizeof(*rela));
>  
> -			if (!gelf_getrela(sec->data, i, &rela->rela)) {
> -				WARN_ELF("gelf_getrela");
> -				return -1;
> +			switch(sec->sh.sh_type) {
> +			case SHT_REL:
> +				if (!gelf_getrel(sec->data, i, &rela->rel)) {
> +					WARN_ELF("gelf_getrel");
> +					return -1;
> +				}
> +				rela->addend = 0;
> +				break;
> +			case SHT_RELA:
> +				if (!gelf_getrela(sec->data, i, &rela->rela)) {
> +					WARN_ELF("gelf_getrela");
> +					return -1;
> +				}
> +				rela->addend = rela->rela.r_addend;
> +				break;
> +			default:
> +				break;

The default should never happen, but might as well return -1 for extra
robustness.

> @@ -717,17 +732,27 @@ int elf_rebuild_rela_section(struct section *sec)
>  	struct rela *rela;
>  	int nr, idx = 0, size;
>  	GElf_Rela *relas;
> +	GElf_Rel *rels;
>  
>  	nr = 0;
>  	list_for_each_entry(rela, &sec->rela_list, list)
>  		nr++;
>  
> +	/*
> +	 * Allocate a buffer for relocations with addends but also use
> +	 * it for other relocations too. The section type determines
> +	 * the size of the section, the buffer used, and the entries.
> +	 */
>  	size = nr * sizeof(*relas);
>  	relas = malloc(size);
>  	if (!relas) {
>  		perror("malloc");
>  		return -1;
>  	}
> +	rels = (void *)relas;
> +	if (sec->sh.sh_type == SHT_REL) {
> +		size = nr * sizeof(*rels);
> +	}
>  
>  	sec->data->d_buf = relas;
>  	sec->data->d_size = size;
> @@ -736,9 +761,19 @@ int elf_rebuild_rela_section(struct section *sec)
>  
>  	idx = 0;
>  	list_for_each_entry(rela, &sec->rela_list, list) {
> -		relas[idx].r_offset = rela->offset;
> -		relas[idx].r_addend = rela->addend;
> -		relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> +		switch(sec->sh.sh_type) {
> +		case SHT_REL:
> +			rels[idx].r_offset = rela->offset;
> +			rels[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> +			break;
> +		case SHT_RELA:
> +			relas[idx].r_addend = rela->addend;
> +			relas[idx].r_offset = rela->offset;
> +			relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> +			break;
> +		default:
> +			break;
> +		}
>  		idx++;

There's a lot of trickiness going on here, in a valiant attempt to share
code, but really most of the code ends up not being shared anyway.

I think it would be a lot cleaner to just create a new "rel" version of
this function.

Then there could be a top-level

	elf_rebuild_reloc_section()

which calls the appropriate "rel" or "rela" variant.

-- 
Josh

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

[Josh Poimboeuf]

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

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

[Josh Poimboeuf]

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

Re: [RFC][PATCH 5/5] objtool: Report missing support for subcommands

[Josh Poimboeuf]

On Mon, May 11, 2020 at 10:35:13AM -0700, Matt Helsley wrote:
> The check and orc-related subcommands for objtool are x86-specific.
> To make this clear to anyone using the tool return a non-zero exit
> code and indicate in the help and usage output which commands are
> (and are not) available.
> 
> Signed-off-by: Matt Helsley <mhelsley@vmware.com>

I think I'd rather the simplest approach of just having the weak
functions print an error and return an error code.  At least for now I
don't think we need to go through the trouble of advertising whether
certain commands are available.  Technically they are available, they
just always fail :-)

And, people don't tend to use objtool directly anyway.

BTW, several of the patches didn't apply cleanly to tip/master, so
you'll probably need to rebase for v2.  There've been a lot of objtool
changes lately.  Peter's been busy...

-- 
Josh

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

[Matt Helsley]

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

Re: [RFC][PATCH 5/5] objtool: Report missing support for subcommands

[Matt Helsley]

On Fri, May 15, 2020 at 04:04:48PM -0500, Josh Poimboeuf wrote:
> On Mon, May 11, 2020 at 10:35:13AM -0700, Matt Helsley wrote:
> > The check and orc-related subcommands for objtool are x86-specific.
> > To make this clear to anyone using the tool return a non-zero exit
> > code and indicate in the help and usage output which commands are
> > (and are not) available.
> > 
> > Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> 
> I think I'd rather the simplest approach of just having the weak
> functions print an error and return an error code.  At least for now I
> don't think we need to go through the trouble of advertising whether
> certain commands are available.  Technically they are available, they
> just always fail :-)

OK, that'll drop this patch from  the series.

> 
> And, people don't tend to use objtool directly anyway.
> 
> BTW, several of the patches didn't apply cleanly to tip/master, so
> you'll probably need to rebase for v2.  There've been a lot of objtool
> changes lately.  Peter's been busy...

Will do.

Cheers,
    -Matt Helsley

Re: [RFC][PATCH 1/5] objtool: Exit successfully when requesting help

[Matt Helsley]

On Fri, May 15, 2020 at 02:52:53PM -0500, Josh Poimboeuf wrote:
> On Mon, May 11, 2020 at 10:35:09AM -0700, Matt Helsley wrote:
> > When the user requests help it's not an error so do not exit with
> > a non-zero exit code. This is not especially useful for a user but
> > any script that might wish to check that objtool --help is at least
> > available can't rely on the exit code to crudely check that, for
> > example building an objtool executable succeeds.
> > 
> > Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> > ---
> >  tools/objtool/objtool.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
> > index 0b3528f05053..593ec85915a9 100644
> > --- a/tools/objtool/objtool.c
> > +++ b/tools/objtool/objtool.c
> > @@ -58,7 +58,10 @@ static void cmd_usage(void)
> >  
> >  	printf("\n");
> >  
> > -	exit(129);
> > +	if (!help)
> > +		exit(129);
> > +	else
> > +		exit(0);
> 
> Looks fine, though the 'else' isn't needed.

OK, removed the 'else'.

Cheers,
    -Matt Helsley

Re: [RFC][PATCH 3/5] objtool: Add support for relocations without addends

[Matt Helsley]

On Fri, May 15, 2020 at 03:33:38PM -0500, Josh Poimboeuf wrote:
> On Mon, May 11, 2020 at 10:35:11AM -0700, Matt Helsley wrote:
> > Currently objtool only collects information about relocations with
> > addends. In recordmcount, which we are about to merge into objtool,
> > some supported architectures do not use rela relocations. Since
> > object files use one or the other the list can be reused.
> > 
> > Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> > ---
> >  tools/objtool/elf.c | 55 ++++++++++++++++++++++++++++++++++++---------
> >  tools/objtool/elf.h |  5 ++++-
> >  2 files changed, 49 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> > index c4857fa3f1d1..cd841e3df87d 100644
> > --- a/tools/objtool/elf.c
> > +++ b/tools/objtool/elf.c
> > @@ -465,12 +465,14 @@ static int read_relas(struct elf *elf)
> 
> This should probably be called read_relocs() now.
> 
> And 'struct rela' should probably be 'struct reloc'.  And I hate to say
> it but all the 'rela' based variable/function names should also probably
> be changed...
> 
> All the renaming might be disruptive for backports, but still I think it
> would be a good idea.  It probably belongs in its own commit.  If it can
> be done programmatically with 'sed -i' or so, with the exact command in
> the commit log, even better :-)
> 
> >  	unsigned long nr_rela, max_rela = 0, tot_rela = 0;
> >  
> >  	list_for_each_entry(sec, &elf->sections, list) {
> > -		if (sec->sh.sh_type != SHT_RELA)
> > +		if ((sec->sh.sh_type != SHT_RELA) &&
> > +		     (sec->sh.sh_type != SHT_REL))
> >  			continue;
> 
> The alignment is slightly off, should be:
> 
> 		if ((sec->sh.sh_type != SHT_RELA) &&
> 		    (sec->sh.sh_type != SHT_REL))
> 			continue;
>

Ack'd.

> >  
> > -		sec->base = find_section_by_name(elf, sec->name + 5);
> > +		sec->base = find_section_by_name(elf, sec->name +
> > +				((sec->sh.sh_type != SHT_REL) ? 5 : 4));
> 
> I think there's actually a cleaner way to do this, which we probably
> should have been doing in the first place:
> 
> 		sec->base = find_section_by_index(elf, sec->sh.sh_info);
> 
> (completely not tested, btw)
> 

I can split that out as a separate patch so it's easy to test/drop.

> > @@ -486,13 +488,26 @@ static int read_relas(struct elf *elf)
> >  			}
> >  			memset(rela, 0, sizeof(*rela));
> >  
> > -			if (!gelf_getrela(sec->data, i, &rela->rela)) {
> > -				WARN_ELF("gelf_getrela");
> > -				return -1;
> > +			switch(sec->sh.sh_type) {
> > +			case SHT_REL:
> > +				if (!gelf_getrel(sec->data, i, &rela->rel)) {
> > +					WARN_ELF("gelf_getrel");
> > +					return -1;
> > +				}
> > +				rela->addend = 0;
> > +				break;
> > +			case SHT_RELA:
> > +				if (!gelf_getrela(sec->data, i, &rela->rela)) {
> > +					WARN_ELF("gelf_getrela");
> > +					return -1;
> > +				}
> > +				rela->addend = rela->rela.r_addend;
> > +				break;
> > +			default:
> > +				break;
> 
> The default should never happen, but might as well return -1 for extra
> robustness.
> 

Ack'd. See below...

> > @@ -717,17 +732,27 @@ int elf_rebuild_rela_section(struct section *sec)
> >  	struct rela *rela;
> >  	int nr, idx = 0, size;
> >  	GElf_Rela *relas;
> > +	GElf_Rel *rels;
> >  
> >  	nr = 0;
> >  	list_for_each_entry(rela, &sec->rela_list, list)
> >  		nr++;
> >  
> > +	/*
> > +	 * Allocate a buffer for relocations with addends but also use
> > +	 * it for other relocations too. The section type determines
> > +	 * the size of the section, the buffer used, and the entries.
> > +	 */
> >  	size = nr * sizeof(*relas);
> >  	relas = malloc(size);
> >  	if (!relas) {
> >  		perror("malloc");
> >  		return -1;
> >  	}
> > +	rels = (void *)relas;
> > +	if (sec->sh.sh_type == SHT_REL) {
> > +		size = nr * sizeof(*rels);
> > +	}
> >  
> >  	sec->data->d_buf = relas;
> >  	sec->data->d_size = size;
> > @@ -736,9 +761,19 @@ int elf_rebuild_rela_section(struct section *sec)
> >  
> >  	idx = 0;
> >  	list_for_each_entry(rela, &sec->rela_list, list) {
> > -		relas[idx].r_offset = rela->offset;
> > -		relas[idx].r_addend = rela->addend;
> > -		relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> > +		switch(sec->sh.sh_type) {
> > +		case SHT_REL:
> > +			rels[idx].r_offset = rela->offset;
> > +			rels[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> > +			break;
> > +		case SHT_RELA:
> > +			relas[idx].r_addend = rela->addend;
> > +			relas[idx].r_offset = rela->offset;
> > +			relas[idx].r_info = GELF_R_INFO(rela->sym->idx, rela->type);
> > +			break;
> > +		default:
> > +			break;
> > +		}
> >  		idx++;
> 
> There's a lot of trickiness going on here, in a valiant attempt to share
> code, but really most of the code ends up not being shared anyway.
> 
> I think it would be a lot cleaner to just create a new "rel" version of
> this function.
> 
> Then there could be a top-level
> 
> 	elf_rebuild_reloc_section()
> 
> which calls the appropriate "rel" or "rela" variant.

OK, that all makes sense to me.

This can go after the multi-arch bits -- at the moment only recordmcount
will use this, though it might be useful for other archs if check or other
subcommands are made suitable for new archs before then.

So I'll move all of that work to a follow-on set rather than include it
in this multi-arch set. The next posting of this series will include all of
the other changes discussed and rebase on tip/master (to get Peter's changes).

The follow-on set after multi-arch support can:

1) Split the sec->base change into a separate patch
	(since it's untested)

2) Rename patch (roughly starting with: sed -i 's/\brela\b/reloc/g')

3) Rebase this patch on top of that, making a new "reloc" version of
	this function (rather than introduce something that needs
	to be renamed)

Cheers,
	-Matt Helsley

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

[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

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

[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

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

[Josh Poimboeuf]

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

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

[Matt Helsley]

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