LinuxLists.cc - [PATCH v3 0/4] Remove dependency of check subcmd upon orc

[permalink] [raw]

Subject: [PATCH v3 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..698df1fa57f4 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_setup_file(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..5641d759f7a3 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_setup_file(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 a2313ecce6d1..051f2ee6b4bc 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,
@@ -2751,36 +2750,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 = 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;

@@ -2789,41 +2774,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..d935522c7359 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_setup_file(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 = 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..62f0ab49dc0c 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_setup_file(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

2020-07-30 09:44:10

[permalink] [raw]

Subject: [PATCH v3 4/4] objtool: orc_gen: Move orc_entry out of instruction structure

One orc_entry is associated with each instruction in the object file,
but having the orc_entry contained by the instruction structure forces
architectures not implementing the orc subcommands to provide a dummy
definition of the orc_entry.

Avoid that by having orc_entries in a separate list, part of the
objtool_file.

Reviewed-by: Miroslav Benes <[email protected]>
Signed-off-by: Julien Thierry <[email protected]>
---
tools/objtool/check.h | 1 -
tools/objtool/objtool.c | 1 +
tools/objtool/objtool.h | 1 +
tools/objtool/orc_gen.c | 80 ++++++++++++++++++++++-------------------
4 files changed, 46 insertions(+), 37 deletions(-)

diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 061aa96e15d3..059c43bfeb18 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -42,7 +42,6 @@ struct instruction {
struct symbol *func;
struct list_head stack_ops;
struct cfi_state cfi;
- struct orc_entry orc;
};

struct instruction *find_insn(struct objtool_file *file,
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index d935522c7359..fb66126b00b6 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -61,6 +61,7 @@ struct objtool_file *objtool_setup_file(const char *_objname)

INIT_LIST_HEAD(&file.insn_list);
hash_init(file.insn_hash);
+ INIT_LIST_HEAD(&file.orc_data_list);
file.c_file = find_section_by_name(file.elf, ".comment");
file.ignore_unreachables = no_unreachable;
file.hints = false;
diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
index 44415ed8c7be..e61b486d4c05 100644
--- a/tools/objtool/objtool.h
+++ b/tools/objtool/objtool.h
@@ -16,6 +16,7 @@ struct objtool_file {
struct elf *elf;
struct list_head insn_list;
DECLARE_HASHTABLE(insn_hash, 20);
+ struct list_head orc_data_list;
bool ignore_unreachables, c_file, hints, rodata;
};

diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index 66fd56c33303..00f1efd05653 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -9,18 +9,33 @@
#include "check.h"
#include "warn.h"

+struct orc_data {
+ struct list_head list;
+ struct instruction *insn;
+ struct orc_entry orc;
+};
+
int create_orc(struct objtool_file *file)
{
struct instruction *insn;

for_each_insn(file, insn) {
- struct orc_entry *orc = &insn->orc;
struct cfi_reg *cfa = &insn->cfi.cfa;
struct cfi_reg *bp = &insn->cfi.regs[CFI_BP];
+ struct orc_entry *orc;
+ struct orc_data *od;

if (!insn->sec->text)
continue;

+ od = calloc(1, sizeof(*od));
+ if (!od)
+ return -1;
+ od->insn = insn;
+ list_add_tail(&od->list, &file->orc_data_list);
+
+ orc = &od->orc;
+
orc->end = insn->cfi.end;

if (cfa->base == CFI_UNDEFINED) {
@@ -139,7 +154,7 @@ static int create_orc_entry(struct elf *elf, struct section *u_sec, struct secti

int create_orc_sections(struct objtool_file *file)
{
- struct instruction *insn, *prev_insn;
+ struct orc_data *od, *prev_od;
struct section *sec, *u_sec, *ip_relocsec;
unsigned int idx;

@@ -157,23 +172,21 @@ int create_orc_sections(struct objtool_file *file)

/* count the number of needed orcs */
idx = 0;
- for_each_sec(file, sec) {
- if (!sec->text)
- continue;
-
- prev_insn = NULL;
- sec_for_each_insn(file, sec, insn) {
- if (!prev_insn ||
- memcmp(&insn->orc, &prev_insn->orc,
- sizeof(struct orc_entry))) {
- idx++;
- }
- prev_insn = insn;
+ prev_od = NULL;
+ list_for_each_entry(od, &file->orc_data_list, list) {
+ if (!prev_od ||
+ memcmp(&od->orc, &prev_od->orc, sizeof(struct orc_entry))) {
+ idx++;
}

+ prev_od = od;
+
/* section terminator */
- if (prev_insn)
+ if (list_is_last(&od->insn->list, &file->insn_list) ||
+ list_next_entry(od->insn, list)->sec != od->insn->sec) {
+ prev_od = NULL;
idx++;
+ }
}
if (!idx)
return -1;
@@ -194,33 +207,28 @@ int create_orc_sections(struct objtool_file *file)

/* populate sections */
idx = 0;
- for_each_sec(file, sec) {
- if (!sec->text)
- continue;
-
- prev_insn = NULL;
- sec_for_each_insn(file, sec, insn) {
- if (!prev_insn || memcmp(&insn->orc, &prev_insn->orc,
- sizeof(struct orc_entry))) {
-
- if (create_orc_entry(file->elf, u_sec, ip_relocsec, idx,
- insn->sec, insn->offset,
- &insn->orc))
- return -1;
-
- idx++;
- }
- prev_insn = insn;
+ prev_od = NULL;
+ list_for_each_entry(od, &file->orc_data_list, list) {
+ if (!prev_od ||
+ memcmp(&od->orc, &prev_od->orc, sizeof(struct orc_entry))) {
+ if (create_orc_entry(file->elf, u_sec, ip_relocsec, idx,
+ od->insn->sec, od->insn->offset,
+ &od->orc))
+ return -1;
+ idx++;
}

+ prev_od = od;
+
/* section terminator */
- if (prev_insn) {
+ if (list_is_last(&od->insn->list, &file->insn_list) ||
+ list_next_entry(od->insn, list)->sec != od->insn->sec) {
if (create_orc_entry(file->elf, u_sec, ip_relocsec, idx,
- prev_insn->sec,
- prev_insn->offset + prev_insn->len,
+ prev_od->insn->sec,
+ prev_od->insn->offset + prev_od->insn->len,
&empty))
return -1;
-
+ prev_od = NULL;
idx++;
}
}
--
2.21.3

2020-07-30 09:45:05

[permalink] [raw]

Subject: [PATCH v3 2/4] objtool: Move orc outside of check

Now that the objtool_file can be obtained outside of the check function,
orc generation builtin no longer requires check to explicitly call its
orc related functions.

Signed-off-by: Julien Thierry <[email protected]>
---
tools/objtool/builtin-check.c | 2 +-
tools/objtool/builtin-orc.c | 21 ++++++++++++++++++++-
tools/objtool/check.c | 18 +-----------------
tools/objtool/objtool.h | 2 +-
tools/objtool/weak.c | 2 +-
5 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 698df1fa57f4..525dbcdf8394 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -58,5 +58,5 @@ int cmd_check(int argc, const char **argv)
if (!file)
return 1;

- return check(file, false);
+ return check(file);
}
diff --git a/tools/objtool/builtin-orc.c b/tools/objtool/builtin-orc.c
index 5641d759f7a3..2ba2d49bf63c 100644
--- a/tools/objtool/builtin-orc.c
+++ b/tools/objtool/builtin-orc.c
@@ -32,6 +32,7 @@ int cmd_orc(int argc, const char **argv)

if (!strncmp(argv[0], "gen", 3)) {
struct objtool_file *file;
+ int ret;

argc = parse_options(argc, argv, check_options, orc_usage, 0);
if (argc != 1)
@@ -43,7 +44,25 @@ int cmd_orc(int argc, const char **argv)
if (!file)
return 1;

- return check(file, true);
+ ret = check(file);
+ if (ret)
+ return ret;
+
+ if (list_empty(&file->insn_list))
+ return 0;
+
+ ret = create_orc(file);
+ if (ret < 0)
+ return ret;
+
+ ret = create_orc_sections(file);
+ if (ret < 0)
+ return ret;
+
+ if (file->elf->changed)
+ return elf_write(file->elf);
+ else
+ return 0;
}

if (!strcmp(argv[0], "dump")) {
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 051f2ee6b4bc..bb19e4c79e46 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2750,7 +2750,7 @@ static int validate_reachable_instructions(struct objtool_file *file)
return 0;
}

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

@@ -2797,22 +2797,6 @@ int check(struct objtool_file *file, bool orc)
warnings += ret;
}

- if (orc) {
- ret = create_orc(file);
- if (ret < 0)
- goto out;
-
- ret = create_orc_sections(file);
- if (ret < 0)
- goto out;
- }
-
- if (file->elf->changed) {
- ret = elf_write(file->elf);
- if (ret < 0)
- goto out;
- }
-
out:
if (ret < 0) {
/*
diff --git a/tools/objtool/objtool.h b/tools/objtool/objtool.h
index 62f0ab49dc0c..44415ed8c7be 100644
--- a/tools/objtool/objtool.h
+++ b/tools/objtool/objtool.h
@@ -21,7 +21,7 @@ struct objtool_file {

struct objtool_file *objtool_setup_file(const char *_objname);

-int check(struct objtool_file *file, bool orc);
+int check(struct objtool_file *file);
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 82698319f008..29180d599b08 100644
--- a/tools/objtool/weak.c
+++ b/tools/objtool/weak.c
@@ -17,7 +17,7 @@
return ENOSYS; \
})

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

2020-07-30 09:46:18

[permalink] [raw]

Subject: [PATCH v3 3/4] objtool: orc: Skip setting orc_entry for non-text sections

Orc generation is only done for text sections, but some instructions
can be found in non-text sections (e.g. .discard.text sections).

Skip setting their orc sections since their whole sections will be
skipped for orc generation.

Reviewed-by: Miroslav Benes <[email protected]>
Signed-off-by: Julien Thierry <[email protected]>
---
tools/objtool/orc_gen.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index 968f55e6dd94..66fd56c33303 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -18,6 +18,9 @@ int create_orc(struct objtool_file *file)
struct cfi_reg *cfa = &insn->cfi.cfa;
struct cfi_reg *bp = &insn->cfi.regs[CFI_BP];

+ if (!insn->sec->text)
+ continue;
+
orc->end = insn->cfi.end;

if (cfa->base == CFI_UNDEFINED) {
--
2.21.3

2020-07-30 09:58:51

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] objtool: Move orc outside of check

On Thu, Jul 30, 2020 at 10:41:41AM +0100, Julien Thierry wrote:
> + if (file->elf->changed)
> + return elf_write(file->elf);
> + else
> + return 0;
> }

I think we can do without that else :-)

2020-07-30 10:06:23

[permalink] [raw]

Subject: Re: [PATCH v3 4/4] objtool: orc_gen: Move orc_entry out of instruction structure

On Thu, Jul 30, 2020 at 10:41:43AM +0100, Julien Thierry wrote:
> One orc_entry is associated with each instruction in the object file,
> but having the orc_entry contained by the instruction structure forces
> architectures not implementing the orc subcommands to provide a dummy
> definition of the orc_entry.
>
> Avoid that by having orc_entries in a separate list, part of the
> objtool_file.
>

> diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
> index 66fd56c33303..00f1efd05653 100644
> --- a/tools/objtool/orc_gen.c
> +++ b/tools/objtool/orc_gen.c
> @@ -9,18 +9,33 @@
> #include "check.h"
> #include "warn.h"
>
> +struct orc_data {
> + struct list_head list;
> + struct instruction *insn;
> + struct orc_entry orc;
> +};
> +
> int create_orc(struct objtool_file *file)
> {
> struct instruction *insn;
>
> for_each_insn(file, insn) {
> - struct orc_entry *orc = &insn->orc;
> struct cfi_reg *cfa = &insn->cfi.cfa;
> struct cfi_reg *bp = &insn->cfi.regs[CFI_BP];
> + struct orc_entry *orc;
> + struct orc_data *od;
>
> if (!insn->sec->text)
> continue;
>
> + od = calloc(1, sizeof(*od));
> + if (!od)
> + return -1;
> + od->insn = insn;
> + list_add_tail(&od->list, &file->orc_data_list);
> +
> + orc = &od->orc;
> +
> orc->end = insn->cfi.end;
>
> if (cfa->base == CFI_UNDEFINED) {

This will dramatically increase the amount of allocation calls, what, if
anything, does this do for the performance of objtool?

2020-07-30 12:41:59

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] objtool: Move orc outside of check

On 7/30/20 10:57 AM, [email protected] wrote:
> On Thu, Jul 30, 2020 at 10:41:41AM +0100, Julien Thierry wrote:
>> + if (file->elf->changed)
>> + return elf_write(file->elf);
>> + else
>> + return 0;
>> }
>
> I think we can do without that else :-)
>

I did wonder and was not 100% confident about it, but the orc gen will
always change the file, correct?

--
Julien Thierry

2020-07-30 12:42:34

[permalink] [raw]

Subject: Re: [PATCH v3 4/4] objtool: orc_gen: Move orc_entry out of instruction structure

On 7/30/20 11:03 AM, [email protected] wrote:
> On Thu, Jul 30, 2020 at 10:41:43AM +0100, Julien Thierry wrote:
>> One orc_entry is associated with each instruction in the object file,
>> but having the orc_entry contained by the instruction structure forces
>> architectures not implementing the orc subcommands to provide a dummy
>> definition of the orc_entry.
>>
>> Avoid that by having orc_entries in a separate list, part of the
>> objtool_file.
>>
>
>> diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
>> index 66fd56c33303..00f1efd05653 100644
>> --- a/tools/objtool/orc_gen.c
>> +++ b/tools/objtool/orc_gen.c
>> @@ -9,18 +9,33 @@
>> #include "check.h"
>> #include "warn.h"
>>
>> +struct orc_data {
>> + struct list_head list;
>> + struct instruction *insn;
>> + struct orc_entry orc;
>> +};
>> +
>> int create_orc(struct objtool_file *file)
>> {
>> struct instruction *insn;
>>
>> for_each_insn(file, insn) {
>> - struct orc_entry *orc = &insn->orc;
>> struct cfi_reg *cfa = &insn->cfi.cfa;
>> struct cfi_reg *bp = &insn->cfi.regs[CFI_BP];
>> + struct orc_entry *orc;
>> + struct orc_data *od;
>>
>> if (!insn->sec->text)
>> continue;
>>
>> + od = calloc(1, sizeof(*od));
>> + if (!od)
>> + return -1;
>> + od->insn = insn;
>> + list_add_tail(&od->list, &file->orc_data_list);
>> +
>> + orc = &od->orc;
>> +
>> orc->end = insn->cfi.end;
>>
>> if (cfa->base == CFI_UNDEFINED) {
>
> This will dramatically increase the amount of allocation calls, what, if
> anything, does this do for the performance of objtool?
>

I guess I forgot about the usecase of running objtool on vmlinux...

On a kernel build for x86_64 defconfig, the difference in time seems to
be withing the noise.

But I agree the proposed code is not ideal and on the other we've tried
avoiding #ifdef in the code. Ideally I'd have an empty orc_entry
definition when SUBCMD_ORC is not implemented.

Would you have a suggested approach to do that?

Thanks,

--
Julien Thierry

2020-07-30 13:26:06

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] objtool: Move orc outside of check

On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
>
>
> On 7/30/20 10:57 AM, [email protected] wrote:
> > On Thu, Jul 30, 2020 at 10:41:41AM +0100, Julien Thierry wrote:
> > > + if (file->elf->changed)
> > > + return elf_write(file->elf);
> > > + else
> > > + return 0;
> > > }
> >
> > I think we can do without that else :-)
> >
>
> I did wonder and was not 100% confident about it, but the orc gen will
> always change the file, correct?

Not if it already has orc, iirc.

But what I was trying to say is that:

if (file->elf->changed)
return elf_write(file->elf)

return 0;

is identical code and, IMO, easier to read.

2020-07-30 13:32:04

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] objtool: Move orc outside of check

On 7/30/20 2:22 PM, [email protected] wrote:
> On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
>>
>>
>> On 7/30/20 10:57 AM, [email protected] wrote:
>>> On Thu, Jul 30, 2020 at 10:41:41AM +0100, Julien Thierry wrote:
>>>> + if (file->elf->changed)
>>>> + return elf_write(file->elf);
>>>> + else
>>>> + return 0;
>>>> }
>>>
>>> I think we can do without that else :-)
>>>
>>
>> I did wonder and was not 100% confident about it, but the orc gen will
>> always change the file, correct?
>
> Not if it already has orc, iirc.
>
> But what I was trying to say is that:
>
> if (file->elf->changed)
> return elf_write(file->elf)
>
> return 0;
>
> is identical code and, IMO, easier to read.
>

Much easier yes, I'll change it.

Thanks,

--
Julien Thierry

2020-07-30 13:36:15

[permalink] [raw]

Subject: Re: [PATCH v3 4/4] objtool: orc_gen: Move orc_entry out of instruction structure

On Thu, Jul 30, 2020 at 01:40:48PM +0100, Julien Thierry wrote:
>
>
> On 7/30/20 11:03 AM, [email protected] wrote:
> > On Thu, Jul 30, 2020 at 10:41:43AM +0100, Julien Thierry wrote:
> > > One orc_entry is associated with each instruction in the object file,
> > > but having the orc_entry contained by the instruction structure forces
> > > architectures not implementing the orc subcommands to provide a dummy
> > > definition of the orc_entry.

> I guess I forgot about the usecase of running objtool on vmlinux...

Right, and LTO builds will even do ORC at that level.

> On a kernel build for x86_64 defconfig, the difference in time seems to be
> withing the noise.

Good.

> But I agree the proposed code is not ideal and on the other we've tried
> avoiding #ifdef in the code. Ideally I'd have an empty orc_entry definition
> when SUBCMD_ORC is not implemented.
>
> Would you have a suggested approach to do that?

How ugly is having that:

struct orc_entry { };

?

2020-07-30 13:46:55

[permalink] [raw]

Subject: Re: [PATCH v3 4/4] objtool: orc_gen: Move orc_entry out of instruction structure

On 7/30/20 2:33 PM, [email protected] wrote:
> On Thu, Jul 30, 2020 at 01:40:48PM +0100, Julien Thierry wrote:
>>
>>
>> On 7/30/20 11:03 AM, [email protected] wrote:
>>> On Thu, Jul 30, 2020 at 10:41:43AM +0100, Julien Thierry wrote:
>>>> One orc_entry is associated with each instruction in the object file,
>>>> but having the orc_entry contained by the instruction structure forces
>>>> architectures not implementing the orc subcommands to provide a dummy
>>>> definition of the orc_entry.
>
>> I guess I forgot about the usecase of running objtool on vmlinux...
>
> Right, and LTO builds will even do ORC at that level.
>
>> On a kernel build for x86_64 defconfig, the difference in time seems to be
>> withing the noise.
>
> Good.
>
>> But I agree the proposed code is not ideal and on the other we've tried
>> avoiding #ifdef in the code. Ideally I'd have an empty orc_entry definition
>> when SUBCMD_ORC is not implemented.
>>
>> Would you have a suggested approach to do that?
>
> How ugly is having that:
>
> struct orc_entry { };
>
> ?

Not sure I am understanding the suggestion. Without #ifdef this will
conflict with the definition in <asm/orc_types.h> for x86. Or every arch
needs to provide their own <asm/orc_types.h> and definition of struct
orc_entry, even if they don't implement the orc subcommand.

Which would be preferable? #ifdef? or arch provided definition? (or
something I have not thought of)

--
Julien Thierry

2020-07-30 14:09:38

[permalink] [raw]

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

On Thu, Jul 30, 2020 at 10:41:39AM +0100, 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 v2 [2]:
> - Rebased on recent tip/objtool/core

tip/objtool/core is slightly outdated, I got a conflict with patch 1.

I guess linus/master would be best.

--
Josh

2020-07-30 14:12:13

[permalink] [raw]

Subject: Re: [PATCH v3 1/4] objtool: Move object file loading out of check

On Thu, Jul 30, 2020 at 10:41:40AM +0100, Julien Thierry wrote:
> +struct objtool_file *objtool_setup_file(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 = find_section_by_name(file.elf, ".comment");
> + file.ignore_unreachables = no_unreachable;
> + file.hints = false;
> +
> + return &file;
> +}

How about calling it objtool_open_read()? It's (sort of) a wrapper
around elf_open_read().

--
Josh

2020-07-30 14:16:20

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] objtool: Move orc outside of check

On Thu, Jul 30, 2020 at 02:29:20PM +0100, Julien Thierry wrote:
>
>
> On 7/30/20 2:22 PM, [email protected] wrote:
> > On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
> > >
> > >
> > > On 7/30/20 10:57 AM, [email protected] wrote:
> > > > On Thu, Jul 30, 2020 at 10:41:41AM +0100, Julien Thierry wrote:
> > > > > + if (file->elf->changed)
> > > > > + return elf_write(file->elf);
> > > > > + else
> > > > > + return 0;
> > > > > }
> > > >
> > > > I think we can do without that else :-)
> > > >
> > >
> > > I did wonder and was not 100% confident about it, but the orc gen will
> > > always change the file, correct?
> >
> > Not if it already has orc, iirc.
> >
> > But what I was trying to say is that:
> >
> > if (file->elf->changed)
> > return elf_write(file->elf)
> >
> > return 0;
> >
> > is identical code and, IMO, easier to read.
> >
>
> Much easier yes, I'll change it.

But I think file->elf->changed can be assumed at this point anyway, so
it could just be an unconditional

return elf_write(file->elf);

--
Josh

2020-07-30 14:29:19

[permalink] [raw]

Subject: Re: [PATCH v3 4/4] objtool: orc_gen: Move orc_entry out of instruction structure

On Thu, Jul 30, 2020 at 02:45:46PM +0100, Julien Thierry wrote:
> > > But I agree the proposed code is not ideal and on the other we've tried
> > > avoiding #ifdef in the code. Ideally I'd have an empty orc_entry definition
> > > when SUBCMD_ORC is not implemented.
> > >
> > > Would you have a suggested approach to do that?
> >
> > How ugly is having that:
> >
> > struct orc_entry { };
> >
> > ?
>
> Not sure I am understanding the suggestion. Without #ifdef this will
> conflict with the definition in <asm/orc_types.h> for x86. Or every arch
> needs to provide their own <asm/orc_types.h> and definition of struct
> orc_entry, even if they don't implement the orc subcommand.
>
> Which would be preferable? #ifdef? or arch provided definition? (or
> something I have not thought of)

If we wanted to get fancy we could add a 'struct insn_arch_specific
arch' field, and then require every arch to declare it.

But I think just an #ifdef in the 'instruction' struct declaration would
be easiest for now.

--
Josh

2020-07-30 14:43:42

[permalink] [raw]

Subject: Re: [PATCH v3 1/4] objtool: Move object file loading out of check

On 7/30/20 3:09 PM, Josh Poimboeuf wrote:
> On Thu, Jul 30, 2020 at 10:41:40AM +0100, Julien Thierry wrote:
>> +struct objtool_file *objtool_setup_file(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 = find_section_by_name(file.elf, ".comment");
>> + file.ignore_unreachables = no_unreachable;
>> + file.hints = false;
>> +
>> + return &file;
>> +}
>
> How about calling it objtool_open_read()? It's (sort of) a wrapper
> around elf_open_read().
>

Sure, I'll update that.

Thanks,

--
Julien Thierry

2020-07-30 14:44:54

[permalink] [raw]

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

On 7/30/20 3:06 PM, Josh Poimboeuf wrote:
> On Thu, Jul 30, 2020 at 10:41:39AM +0100, 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 v2 [2]:
>> - Rebased on recent tip/objtool/core
>
> tip/objtool/core is slightly outdated, I got a conflict with patch 1.
>
> I guess linus/master would be best.

It looks like linus/master is missing the rela -> reloc rework that is
present in tip/objtool/core, which will conflict with other patches from
this series.

How shall I proceed?

--
Julien Thierry

2020-07-30 14:47:00

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] objtool: Move orc outside of check

On 7/30/20 3:15 PM, Josh Poimboeuf wrote:
> On Thu, Jul 30, 2020 at 02:29:20PM +0100, Julien Thierry wrote:
>>
>>
>> On 7/30/20 2:22 PM, [email protected] wrote:
>>> On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
>>>>
>>>>
>>>> On 7/30/20 10:57 AM, [email protected] wrote:
>>>>> On Thu, Jul 30, 2020 at 10:41:41AM +0100, Julien Thierry wrote:
>>>>>> + if (file->elf->changed)
>>>>>> + return elf_write(file->elf);
>>>>>> + else
>>>>>> + return 0;
>>>>>> }
>>>>>
>>>>> I think we can do without that else :-)
>>>>>
>>>>
>>>> I did wonder and was not 100% confident about it, but the orc gen will
>>>> always change the file, correct?
>>>
>>> Not if it already has orc, iirc.
>>>
>>> But what I was trying to say is that:
>>>
>>> if (file->elf->changed)
>>> return elf_write(file->elf)
>>>
>>> return 0;
>>>
>>> is identical code and, IMO, easier to read.
>>>
>>
>> Much easier yes, I'll change it.
>
> But I think file->elf->changed can be assumed at this point anyway, so
> it could just be an unconditional
>
> return elf_write(file->elf);
>

I'll triple check whether that's the case and remove the if if possible.

--
Julien Thierry

2020-07-30 15:08:54

[permalink] [raw]

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

On Thu, Jul 30, 2020 at 03:42:09PM +0100, Julien Thierry wrote:
>
>
> On 7/30/20 3:06 PM, Josh Poimboeuf wrote:
> > On Thu, Jul 30, 2020 at 10:41:39AM +0100, 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 v2 [2]:
> > > - Rebased on recent tip/objtool/core
> >
> > tip/objtool/core is slightly outdated, I got a conflict with patch 1.
> >
> > I guess linus/master would be best.
>
> It looks like linus/master is missing the rela -> reloc rework that is
> present in tip/objtool/core, which will conflict with other patches from
> this series.
>
> How shall I proceed?

Hm. I guess we'll need guidance from Peter, he's the branch wrangler.
Maybe tip/objtool/core needs to be rebased onto linus/master.

--
Josh

2020-07-31 07:57:47

by Miroslav Benes

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] objtool: Move orc outside of check

On Thu, 30 Jul 2020, Julien Thierry wrote:

>
>
> On 7/30/20 3:15 PM, Josh Poimboeuf wrote:
> > On Thu, Jul 30, 2020 at 02:29:20PM +0100, Julien Thierry wrote:
> >>
> >>
> >> On 7/30/20 2:22 PM, [email protected] wrote:
> >>> On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
> >>>>
> >>>>
> >>>> On 7/30/20 10:57 AM, [email protected] wrote:
> >>>>> On Thu, Jul 30, 2020 at 10:41:41AM +0100, Julien Thierry wrote:
> >>>>>> + if (file->elf->changed)
> >>>>>> + return elf_write(file->elf);
> >>>>>> + else
> >>>>>> + return 0;
> >>>>>> }
> >>>>>
> >>>>> I think we can do without that else :-)
> >>>>>
> >>>>
> >>>> I did wonder and was not 100% confident about it, but the orc gen will
> >>>> always change the file, correct?
> >>>
> >>> Not if it already has orc, iirc.
> >>>
> >>> But what I was trying to say is that:
> >>>
> >>> if (file->elf->changed)
> >>> return elf_write(file->elf)
> >>>
> >>> return 0;
> >>>
> >>> is identical code and, IMO, easier to read.
> >>>
> >>
> >> Much easier yes, I'll change it.
> >
> > But I think file->elf->changed can be assumed at this point anyway, so
> > it could just be an unconditional
> >
> > return elf_write(file->elf);
> >
>
> I'll triple check whether that's the case and remove the if if possible.

I think it is the case. And even if not, it would only cause a pointless
call to elf_update() in the end and that should not do any harm anyway if
I am not mistaken.

However, I think there is a problem with the rebase on top of the current
code. The patch moves elf_write() call to orc_gen.c which was ok before
Peterz introduced elf_write_insn() et al. We need to keep elf_write() in
check.c for this case too.

Miroslav

2020-07-31 08:22:28