Support for multiple source files by sm00th · Pull Request #1204 · dynup/kpatch

sm00th · 2021-08-03T17:29:16Z

This patchset allows create-diff-object to work on objectfiles built from multiple source files.

jpoimboe · 2021-08-03T23:16:49Z

kpatch-build/lookup.c

-			continue;
-
-		if (sym->type == STT_FILE)
-			break;


I'm not quite convinced this won't cause a regression. It's possible to have multiple STT_FILE symbols with the same name. How can we be sure this is the right file? For example, issue #604.

I guessed that shouldn't be that easy but failed to find this case. Pushed v2 that does locals_match() for every STT_FILE section in the file and stores lookup_table->local_syms in each symbol.

jpoimboe · 2021-08-09T20:56:44Z

kpatch-build/create-diff-object.c

+	if (sym1->lookup_table_sym && !sym2->lookup_table_sym)
+		sym2->lookup_table_sym = sym1->lookup_table_sym;
+	else if (!sym1->lookup_table_sym && sym2->lookup_table_sym)
+		sym1->lookup_table_sym = sym2->lookup_table_sym;


I think the else case isn't really needed since 'sym1' is always from the orig object and 'sym2' is always from the patched object. If that's not clear then maybe they need to be renamed to orig_sym and patched_sym.

Yes, all current usage of kpatch_correlate_symbol() follows this logic, I was safeguarding against possible future code, but maybe making expectations clear through argument names will be enough.

jpoimboe · 2021-08-09T21:10:19Z

kpatch-build/lookup.c

-		for (child = child_locals; child->name; child++) {
-			if (child->type == sym->type &&
-			    !strcmp(child->name, sym->name)) {
+		iter_sym = file_sym;


The variable names were already fairly confusing in this function, but the addition of struct symbol to this file confuses the naming even more. There's no logical difference between sym and iter_sym when reading the code.

I'd suggest making the following naming changes here, as well as in other places with struct object_symbol *sym.

rename sym to table_sym.

rename iter_sym to sym.

That way, sym has its traditional meaning (an instance of struct symbol *) and table_sym is more appropriately named.

Thanks, good suggestion. I was struggling with variable naming in this patchset, at some point I had lookup_sym->lookup_sym in lookup_local_symbol().

jpoimboe · 2021-08-09T21:11:42Z

kpatch-build/lookup.c

+				break;
+			if (iter_sym->bind != STB_LOCAL)
+				continue;
+


To match the previous functionality I think we also want to make sure it's either an STT_FUNC or an STT_OBJECT (and otherwise continue).

This is actually intentional, so is the removal in the next comment. The very next statement is iter_sym->type == sym->type which does make sure the type is one of those possibilities so the check was redundant. Or do you think it is worth keeping it for clarity?

Ah I see now. Never mind...

jpoimboe · 2021-08-09T21:13:28Z

kpatch-build/lookup.c

 			if (sym->bind != STB_LOCAL)
 				continue;
-			if (sym->type != STT_FUNC && sym->type != STT_OBJECT)
-				continue;


If my above suggestion to check for STT_{FUNC,OBJECT} is taken, this hunk probably doesn't need removed.

jpoimboe · 2021-08-09T21:20:43Z

kpatch-build/lookup.c

+	}
+}
+
+static void link_local_symbols(struct lookup_table *table, struct kpatch_elf *kelf)


link_local_symbols() makes it sound like something different than finding, when in fact it's just a loop around find_local_syms(). Maybe call it find_local_syms_multiple() or so? Also I think this function deserves a comment describing why it's doing this. I assume it's for analyzing a bigger .o file like a built-in.o or vmlinux.o.

Similarly, the commit message needs more detail, as "support object-files built from multiple sources" didn't make sense to me until after I studied the code. The commit message should also give more info about what problem is being solved and why.

Agreed, will do.

jpoimboe · 2021-08-09T21:23:33Z

kpatch-build/kpatch-elf.h

 	struct section *sec;
 	GElf_Sym sym;
 	char *name;
+	struct object_symbol *lookup_table_sym;


Technically this should probably be called lookup_table_file_sym, since it points to the lookup table's FILE symbol rather than the corresponding symbol itself.

That said, maybe it would be simpler to just set it to the exact corresponding symbol to begin with. locals_match() already finds the corresponding symbol -- twice, in fact. Then, later, lookup_local_symbol() finds it a third time. Can we just set sym->lookup_table_sym to the corresponding symbol to begin with, in find_local_syms() and/or locals_match()? Then lookup_local_symbol() becomes redundant.

Ah, I see. That makes sense, I'll see if it will work.

After looking into this I currently don't see a straightforward way of dropping lookup_local_symbol(). The only reason is sympos calculation it does. Because of that we'll either have to traverse the whole lookup table for each symbol in find_local_syms() (which will make things worse) or keep a list of seen symbol names and write down sympos in symtab_read() but I am not sure it is worth doing that. Any thoughts? Can we leave it as is for now?

That's fine. In that case my only comment here would be to rename 'lookup_table_sym' to 'lookup_table_file_sym'.

jpoimboe · 2021-08-09T21:26:37Z

kpatch-build/lookup.c

@@ -92,9 +91,9 @@ static int maybe_discarded_sym(const char *name)
 }

 static int locals_match(struct lookup_table *table, int idx,


While you're at it, can you change locals_match() and maybe_discarded_sym() to return bool? Their int return codes are opposite than normal kernel error code expectations, making them extra confusing. Might as well clean those up while we're touching the code.

jpoimboe

Would it be possible to add a unit test? vmlinux.o is probably too big, but either a built-in.o file or some manually linked version of multiple .o's might be useful.

jpoimboe · 2021-08-13T00:06:47Z

kpatch-build/create-diff-object.c

+		struct section *sec_patched)
 {
-	CORRELATE_ELEMENT(sec1, sec2, "section");
+	CORRELATE_ELEMENT(sec_base, sec_patched, "section");


This is a nice cleanup IMO. The new naming helps to codify certain existing assumptions.

While making such a broad change, I'd suggest one more naming tweak (but arguments to the contrary are welcome). I've long regretted the use of the term "base" in this code. I'd argue that "orig" is more descriptive than "base" (and matches the directory names in ~/.kpatch/tmp/.) Since you're already doing a mass renaming which adds more "base", I'd suggest going ahead and doing s/base/orig/g pretty much everywhere -- if there are no objections.

And sorry about pulling this PR into the weeds :-)

sm00th · 2021-08-16T13:50:58Z

v3: base->orig rename. I've also created dynup/kpatch-unit-test-objs#34 that will be added to this PR once merged.

jpoimboe · 2021-08-16T23:19:12Z

Looks good to me.

gcc-static-local-var-4.patch is disabled on this distribution, disable the test as well as it will always fail during 'slow' integration test runs. Signed-off-by: Artem Savkov <asavkov@redhat.com>

At the moment lookup_symbol() takes symbol name as an argument which might not be enough in some cases (e.g. for objectfiles built from multiple source files). Make it accept full symbol structure. Signed-off-by: Artem Savkov <asavkov@redhat.com>

So far create-diff-object worked only with objectfiles built from a single source file. To support object-files built from multiple sources such as linked vmlinux.o, we call locals_match() for each of STT_FILE symbols and store the pointer to the beginning of appropriate symbol block in lookup table in each symbol. Signed-off-by: Artem Savkov <asavkov@redhat.com>

…urn bool Change the return type of locals_match() and maybe_discarded_sym() to better reflect their usage. Signed-off-by: Artem Savkov <asavkov@redhat.com>

Rename "base" to "orig" when referencing object files and their contents to be consistent with temporary directory structure. Signed-off-by: Artem Savkov <asavkov@redhat.com>

While theoretically most of these functions can work both ways right now all calls follow the same pattern: first argument is orig element and second is patched element. Rename the arguments so that these functions are used in the same fashion going forward. This allows us to cut some corners such as removing the elseif statement in kpatch_correlate_symbol(). Signed-off-by: Artem Savkov <asavkov@redhat.com>

Update unit-test submodule pointer to include a multi-file unit-test. Signed-off-by: Artem Savkov <asavkov@redhat.com>

sm00th · 2021-08-17T07:40:39Z

v3: included unit-tests and rebased on top of #1207

joe-lawrence · 2021-08-23T19:57:15Z

This patchset allows create-diff-object to work on objectfiles built from multiple source files.

This will eventually help support kernel-LTO, right?
Are there circumstances today, without LTO, where we would see such files from the kernel build?
And finally, what is inside multifile-pr-1204.*.o and could they be created for ppc64le as well?

The code looks good to me, including all the cleanups. (Btw, if you think this PR is ready for merge, ok, if not ... would it be easier to spin off the integration test and cleanup commits to make future rebases easier?)

sm00th · 2021-08-24T07:05:15Z

This will eventually help support kernel-LTO, right?

Right, with LTO we won't have the luxury to work with individual objectfiles.

Are there circumstances today, without LTO, where we would see such files from the kernel build?

No, not to my knowledge at least. This series doesn't make single source file case any worse, just makes create-diff-object more flexible.

And finally, what is inside multifile-pr-1204.*.o

From the test-objs commit message:

Adding unit tests for the case when the objectfile is built from
multiple source files. Added objectfiles were created by building our
standard test-patches for meminfo and cmdline and then linking
fs/proc/meminfo.o and fs/proc/cmdline.o together with "ld -r".

So this is not exactly a real-life scenario.

and could they be created for ppc64le as well?

Do you expect ppc64le elfs to have something special about them in this case? If not I don't see any reason to add them since they will be running through the same code. I think we should focus mostly on x86_64 since they are ran more frequently (travis/github runs only on x86_64) and only add ppc64le ones when that case is different.

The code looks good to me, including all the cleanups. (Btw, if you think this PR is ready for merge, ok, if not ... would it be easier to spin off the integration test and cleanup commits to make future rebases easier?)

It is ready, just wanted to give you a chance to take a look. Will merge it now and we can add unit tests later if it comes to that.

jpoimboe reviewed Aug 3, 2021

View reviewed changes

sm00th force-pushed the files branch from 5b9df5a to 76cfaa1 Compare August 5, 2021 11:26

jpoimboe reviewed Aug 9, 2021

View reviewed changes

sm00th force-pushed the files branch 2 times, most recently from 77371fd to e1bd041 Compare August 12, 2021 08:08

jpoimboe reviewed Aug 13, 2021

View reviewed changes

sm00th force-pushed the files branch from e1bd041 to f35ea4a Compare August 16, 2021 13:50

sm00th added 7 commits August 17, 2021 09:37

test/integration/rhel-8.0: disable unneeded test

a132000

gcc-static-local-var-4.patch is disabled on this distribution, disable the test as well as it will always fail during 'slow' integration test runs. Signed-off-by: Artem Savkov <asavkov@redhat.com>

create-diff-object: make locals_match() and maybe_discarded_sym() ret…

e2b50d7

…urn bool Change the return type of locals_match() and maybe_discarded_sym() to better reflect their usage. Signed-off-by: Artem Savkov <asavkov@redhat.com>

create-diff-object: base->orig renames

22e1661

Rename "base" to "orig" when referencing object files and their contents to be consistent with temporary directory structure. Signed-off-by: Artem Savkov <asavkov@redhat.com>

Include unit-tests for multifile case

941a967

Update unit-test submodule pointer to include a multi-file unit-test. Signed-off-by: Artem Savkov <asavkov@redhat.com>

sm00th force-pushed the files branch from f35ea4a to 941a967 Compare August 17, 2021 07:39

jpoimboe approved these changes Aug 17, 2021

View reviewed changes

sm00th merged commit 8f7e7c2 into dynup:master Aug 24, 2021

		@@ -92,9 +91,9 @@ static int maybe_discarded_sym(const char *name)
		}

		static int locals_match(struct lookup_table *table, int idx,

Conversation

sm00th commented Aug 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpoimboe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm00th commented Aug 16, 2021

Uh oh!

jpoimboe commented Aug 16, 2021

Uh oh!

sm00th commented Aug 17, 2021

Uh oh!

joe-lawrence commented Aug 23, 2021

Uh oh!

sm00th commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants