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

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

[Julien Thierry]

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

[1] https://www.spinics.net/lists/kernel/msg3510844.html
[2] https://lkml.org/lkml/2020/6/8/59

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: orc_gen: Move orc_entry out of instruction structure

 tools/objtool/builtin-check.c |  7 ++-
 tools/objtool/builtin-orc.c   | 27 +++++++++++-
 tools/objtool/check.c         | 47 ++++----------------
 tools/objtool/check.h         |  1 -
 tools/objtool/objtool.c       | 30 +++++++++++++
 tools/objtool/objtool.h       |  5 ++-
 tools/objtool/orc_gen.c       | 83 ++++++++++++++++++++---------------
 tools/objtool/weak.c          |  4 +-
 8 files changed, 122 insertions(+), 82 deletions(-)

--
2.21.3

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

[Julien Thierry]

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 <mbenes@suse.cz>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 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

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

[Josh Poimboeuf]

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

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

[Julien Thierry]

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

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

[Julien Thierry]

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 <jthierry@redhat.com>
---
 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

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

[peterz]

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 :-)

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

[Julien Thierry]

On 7/30/20 10:57 AM, peterz@infradead.org 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

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

[peterz]

On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
> 
> 
> On 7/30/20 10:57 AM, peterz@infradead.org 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.

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

[Julien Thierry]

On 7/30/20 2:22 PM, peterz@infradead.org wrote:
> On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
>>
>>
>> On 7/30/20 10:57 AM, peterz@infradead.org 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

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

[Josh Poimboeuf]

On Thu, Jul 30, 2020 at 02:29:20PM +0100, Julien Thierry wrote:
> 
> 
> On 7/30/20 2:22 PM, peterz@infradead.org wrote:
> > On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
> > > 
> > > 
> > > On 7/30/20 10:57 AM, peterz@infradead.org 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

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

[Julien Thierry]

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, peterz@infradead.org wrote:
>>> On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
>>>>
>>>>
>>>> On 7/30/20 10:57 AM, peterz@infradead.org 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

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

[Miroslav Benes]

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, peterz@infradead.org wrote:
> >>> On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
> >>>>
> >>>>
> >>>> On 7/30/20 10:57 AM, peterz@infradead.org 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

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

[Julien Thierry]

On 7/31/20 8:56 AM, Miroslav Benes wrote:
> 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, peterz@infradead.org wrote:
>>>>> On Thu, Jul 30, 2020 at 01:40:42PM +0100, Julien Thierry wrote:
>>>>>>
>>>>>>
>>>>>> On 7/30/20 10:57 AM, peterz@infradead.org 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.
> 

Yes, you're right. Looks like I messed things up with the rebase. That 
means I might have to move the elf_write() to builtin-check.c.

Thanks for pointing it out.

-- 
Julien Thierry

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

[Julien Thierry]

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 <mbenes@suse.cz>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 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

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

[Julien Thierry]

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 <mbenes@suse.cz>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
---
 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

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

[peterz]

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?

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

[Julien Thierry]

On 7/30/20 11:03 AM, peterz@infradead.org 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

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

[peterz]

On Thu, Jul 30, 2020 at 01:40:48PM +0100, Julien Thierry wrote:
> 
> 
> On 7/30/20 11:03 AM, peterz@infradead.org 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 { };

?

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

[Julien Thierry]

On 7/30/20 2:33 PM, peterz@infradead.org wrote:
> On Thu, Jul 30, 2020 at 01:40:48PM +0100, Julien Thierry wrote:
>>
>>
>> On 7/30/20 11:03 AM, peterz@infradead.org 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

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

[Josh Poimboeuf]

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

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

[Josh Poimboeuf]

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

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

[Julien Thierry]

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

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

[Josh Poimboeuf]

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