2020-08-25 13:11:49

by Julien Thierry

[permalink] [raw]
Subject: [PATCH v4 0/4] Remove dependency of check subcmd upon orc

Hi,

Matt Helsley's change[1] provided a base framework to opt-in/out
objtool subcommands at compile time. This makes it easier for
architectures to port objtool, one subcommand at a time.

Orc generation relies on the check operation implementation. However,
the way this is done causes the check implementation to depend on the
implementation of orc generation functions to call if orc generation is
requested. This means that in order to implement check subcmd, orc
subcmd also need to be implemented.

These patches aim at removing that dependency, having orc subcmd
being built on top of the check subcmd.


Changes since v3 [2]:
- Rebased on v5.9-rc1
- Renamed objtool_setup_file() to objtool_open_read()
- Fixed misplaced elf_write() when file->elf->changed is true
- Avoid additional allocation for orc data and compile out orc
definition when not needed instead

[1] https://www.spinics.net/lists/kernel/msg3510844.html
[2] https://lkml.org/lkml/2020/7/30/415

Cheers,

Julien

-->

Julien Thierry (4):
objtool: Move object file loading out of check
objtool: Move orc outside of check
objtool: orc: Skip setting orc_entry for non-text sections
objtool: check: Use orc definition only when needed

tools/objtool/Makefile | 4 +++
tools/objtool/arch.h | 2 ++
tools/objtool/builtin-check.c | 15 ++++++++++-
tools/objtool/builtin-orc.c | 27 +++++++++++++++++++-
tools/objtool/check.c | 47 ++++++-----------------------------
tools/objtool/check.h | 2 ++
tools/objtool/objtool.c | 29 +++++++++++++++++++++
tools/objtool/objtool.h | 4 ++-
tools/objtool/orc_gen.c | 3 +++
tools/objtool/weak.c | 4 +--
10 files changed, 92 insertions(+), 45 deletions(-)

--
2.21.3


2020-08-25 13:15:46

by Julien Thierry

[permalink] [raw]
Subject: Re: [PATCH v4 0/4] Remove dependency of check subcmd upon orc

Hi,

Sorry for the duplicate send. An issue happened while sending this one.
Please ignore this thread.

Thanks,

Julien

On 8/25/20 1:46 PM, Julien Thierry wrote:
> Hi,
>
> Matt Helsley's change[1] provided a base framework to opt-in/out
> objtool subcommands at compile time. This makes it easier for
> architectures to port objtool, one subcommand at a time.
>
> Orc generation relies on the check operation implementation. However,
> the way this is done causes the check implementation to depend on the
> implementation of orc generation functions to call if orc generation is
> requested. This means that in order to implement check subcmd, orc
> subcmd also need to be implemented.
>
> These patches aim at removing that dependency, having orc subcmd
> being built on top of the check subcmd.
>
>
> Changes since v3 [2]:
> - Rebased on v5.9-rc1
> - Renamed objtool_setup_file() to objtool_open_read()
> - Fixed misplaced elf_write() when file->elf->changed is true
> - Avoid additional allocation for orc data and compile out orc
> definition when not needed instead
>
> [1] https://www.spinics.net/lists/kernel/msg3510844.html
> [2] https://lkml.org/lkml/2020/7/30/415
>
> Cheers,
>
> Julien
>
> -->
>
> Julien Thierry (4):
> objtool: Move object file loading out of check
> objtool: Move orc outside of check
> objtool: orc: Skip setting orc_entry for non-text sections
> objtool: check: Use orc definition only when needed
>
> tools/objtool/Makefile | 4 +++
> tools/objtool/arch.h | 2 ++
> tools/objtool/builtin-check.c | 15 ++++++++++-
> tools/objtool/builtin-orc.c | 27 +++++++++++++++++++-
> tools/objtool/check.c | 47 ++++++-----------------------------
> tools/objtool/check.h | 2 ++
> tools/objtool/objtool.c | 29 +++++++++++++++++++++
> tools/objtool/objtool.h | 4 ++-
> tools/objtool/orc_gen.c | 3 +++
> tools/objtool/weak.c | 4 +--
> 10 files changed, 92 insertions(+), 45 deletions(-)
>
> --
> 2.21.3
>

--
Julien Thierry

2020-08-25 13:38:48

by Julien Thierry

[permalink] [raw]
Subject: [PATCH v4 1/4] objtool: Move object file loading out of check

Structure objtool_file can be used by different subcommands. In fact
it already is, by check and orc.

Provide a function that allows to initialize objtool_file, that builtin
can call, without relying on check to do the correct setup for them and
explicitly hand the objtool_file to them.

Reviewed-by: Miroslav Benes <[email protected]>
Signed-off-by: Julien Thierry <[email protected]>
---
tools/objtool/builtin-check.c | 7 ++++++-
tools/objtool/builtin-orc.c | 8 ++++++-
tools/objtool/check.c | 39 +++++++++++------------------------
tools/objtool/objtool.c | 29 ++++++++++++++++++++++++++
tools/objtool/objtool.h | 4 +++-
tools/objtool/weak.c | 4 +---
6 files changed, 58 insertions(+), 33 deletions(-)

diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 7a44174967b5..0126ec3bb6c9 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -41,6 +41,7 @@ const struct option check_options[] = {
int cmd_check(int argc, const char **argv)
{
const char *objname, *s;
+ struct objtool_file *file;

argc = parse_options(argc, argv, check_options, check_usage, 0);

@@ -53,5 +54,9 @@ int cmd_check(int argc, const char **argv)
if (s && !s[9])
vmlinux = true;

- return check(objname, false);
+ file = objtool_open_read(objname);
+ if (!file)
+ return 1;
+
+ return check(file, false);
}
diff --git a/tools/objtool/builtin-orc.c b/tools/objtool/builtin-orc.c
index b1dfe2007962..3979f275a775 100644
--- a/tools/objtool/builtin-orc.c
+++ b/tools/objtool/builtin-orc.c
@@ -31,13 +31,19 @@ int cmd_orc(int argc, const char **argv)
usage_with_options(orc_usage, check_options);

if (!strncmp(argv[0], "gen", 3)) {
+ struct objtool_file *file;
+
argc = parse_options(argc, argv, check_options, orc_usage, 0);
if (argc != 1)
usage_with_options(orc_usage, check_options);

objname = argv[0];

- return check(objname, true);
+ file = objtool_open_read(objname);
+ if (!file)
+ return 1;
+
+ return check(file, true);
}

if (!strcmp(argv[0], "dump")) {
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e034a8f24f46..ef8e9300e1a3 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -27,7 +27,6 @@ struct alternative {
bool skip_orig;
};

-const char *objname;
struct cfi_init_state initial_func_cfi;

struct instruction *find_insn(struct objtool_file *file,
@@ -2777,36 +2776,22 @@ static int validate_reachable_instructions(struct objtool_file *file)
return 0;
}

-static struct objtool_file file;
-
-int check(const char *_objname, bool orc)
+int check(struct objtool_file *file, bool orc)
{
int ret, warnings = 0;

- objname = _objname;
-
- file.elf = elf_open_read(objname, O_RDWR);
- if (!file.elf)
- return 1;
-
- INIT_LIST_HEAD(&file.insn_list);
- hash_init(file.insn_hash);
- file.c_file = !vmlinux && find_section_by_name(file.elf, ".comment");
- file.ignore_unreachables = no_unreachable;
- file.hints = false;
-
arch_initial_func_cfi_state(&initial_func_cfi);

- ret = decode_sections(&file);
+ ret = decode_sections(file);
if (ret < 0)
goto out;
warnings += ret;

- if (list_empty(&file.insn_list))
+ if (list_empty(&file->insn_list))
goto out;

if (vmlinux && !validate_dup) {
- ret = validate_vmlinux_functions(&file);
+ ret = validate_vmlinux_functions(file);
if (ret < 0)
goto out;

@@ -2815,41 +2800,41 @@ int check(const char *_objname, bool orc)
}

if (retpoline) {
- ret = validate_retpoline(&file);
+ ret = validate_retpoline(file);
if (ret < 0)
return ret;
warnings += ret;
}

- ret = validate_functions(&file);
+ ret = validate_functions(file);
if (ret < 0)
goto out;
warnings += ret;

- ret = validate_unwind_hints(&file, NULL);
+ ret = validate_unwind_hints(file, NULL);
if (ret < 0)
goto out;
warnings += ret;

if (!warnings) {
- ret = validate_reachable_instructions(&file);
+ ret = validate_reachable_instructions(file);
if (ret < 0)
goto out;
warnings += ret;
}

if (orc) {
- ret = create_orc(&file);
+ ret = create_orc(file);
if (ret < 0)
goto out;

- ret = create_orc_sections(&file);
+ ret = create_orc_sections(file);
if (ret < 0)
goto out;
}

- if (file.elf->changed) {
- ret = elf_write(file.elf);
+ if (file->elf->changed) {
+ ret = elf_write(file->elf);
if (ret < 0)
goto out;
}
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 58fdda510653..84e89e79be9b 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -22,6 +22,8 @@
#include <linux/kernel.h>

#include "builtin.h"
+#include "objtool.h"
+#include "warn.h"

struct cmd_struct {
const char *name;
@@ -39,6 +41,33 @@ static struct cmd_struct objtool_cmds[] = {

bool help;

+const char *objname;
+static struct objtool_file file;
+
+struct objtool_file *objtool_open_read(const char *_objname)
+{
+ if (objname) {
+ if (strcmp(objname, _objname)) {
+ WARN("won't handle more than one file at a time");
+ return NULL;
+ }
+ return &file;
+ }
+ objname = _objname;
+
+ file.elf = elf_open_read(objname, O_RDWR);
+ if (!file.elf)
+ return NULL;
+
+ INIT_LIST_HEAD(&file.insn_list);
+ hash_init(file.insn_hash);
+ file.c_file = !vmlinux && find_section_by_name(file.elf, ".comment");
+ file.ignore_unreachables = no_unreachable;
+ file.hints = false;
+
+ return &file;
+}
+
static void cmd_usage(void)
{
unsigned int i, longest = 0;
diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
index 528028a66816..42b27c4ed683 100644
--- a/tools/objtool/objtool.h
+++ b/tools/objtool/objtool.h
@@ -19,7 +19,9 @@ struct objtool_file {
bool ignore_unreachables, c_file, hints, rodata;
};

-int check(const char *objname, bool orc);
+struct objtool_file *objtool_open_read(const char *_objname);
+
+int check(struct objtool_file *file, bool orc);
int orc_dump(const char *objname);
int create_orc(struct objtool_file *file);
int create_orc_sections(struct objtool_file *file);
diff --git a/tools/objtool/weak.c b/tools/objtool/weak.c
index 942ea5e8ac36..82698319f008 100644
--- a/tools/objtool/weak.c
+++ b/tools/objtool/weak.c
@@ -17,9 +17,7 @@
return ENOSYS; \
})

-const char __weak *objname;
-
-int __weak check(const char *_objname, bool orc)
+int __weak check(struct objtool_file *file, bool orc)
{
UNSUPPORTED("check subcommand");
}
--
2.21.3