Remove host / target architecture assumption from create-diff-object by joe-lawrence · Pull Request #1248 · dynup/kpatch

joe-lawrence · 2022-01-21T20:01:49Z

This is a remix of @gwelymernans 's #1179 with a few slight tweaks:

Do not add an --arch command line argument to various tools and kpatch-build. Detect the ELF architecture of the input files and pivot based on that. This would allow for a (theoretical at this point) cross-arch-build to build a ppc64le reference/patched kernel and when processing its object files, rely on the ELF header to indicate the target architecture.
Modifications limited to create-diff-object for now. Without testing full cross-arch-build support, this is still useful to the project as we can test the ppc64le unit tests (with a very small patch: unit-objs-cross.diff.txt):

$ arch
x86_64

$ make -s unit OBJDIR=objs/ppc64le
BUILD function-ptr-new
BUILD ASSERT_RTNL-detect
BUILD meminfo-init-FAIL
BUILD bug-table-section
BUILD smp-locks-section
BUILD fixup-section
BUILD meminfo-string
BUILD gcc-static-local-var-5
BUILD meminfo-init2-FAIL
BUILD gcc-static-local-var-3
BUILD mcount_loc-issue-1102
BUILD gcc-constprop-2-issue-935
BUILD ppc64le-bundled-localentry-issue-1007
BUILD static-local-size-mismatch-issue-1053
BUILD data-read-mostly
BUILD sec-addralign
BUILD tracepoints-section
BUILD data-rel
BUILD jump-label-issue-946
BUILD data-new
BUILD rela-common-symbols
BUILD gcc-constprop
BUILD special-static
BUILD warn-detect-FAIL
BUILD ppc64le-sibling-call-issue-1003-FAIL
BUILD new-function
TEST gcc-constprop
TEST gcc-constprop-2-issue-935
TEST gcc-static-local-var-5
TEST function-ptr-new
TEST bug-table-section
TEST meminfo-string
TEST gcc-static-local-var-3
TEST ppc64le-bundled-localentry-issue-1007
TEST ASSERT_RTNL-detect
TEST special-static
TEST static-local-size-mismatch-issue-1053
TEST data-read-mostly
TEST fixup-section
TEST mcount_loc-issue-1102
TEST sec-addralign
TEST tracepoints-section
TEST smp-locks-section
TEST data-rel
TEST data-new
TEST rela-common-symbols
TEST new-function

With the preparatory first commit and @gwelymernans 's previous heavy lifting, this removes all of the architecture-specific preprocessor blocks from kpatch-build:

$ grep -c -e '__powerpc64__' -e '__x86_64__' $(git ls-files kpatch-build/) 
kpatch-build/Makefile:0
kpatch-build/create-diff-object.c:0
kpatch-build/create-klp-module.c:0
kpatch-build/create-kpatch-module.c:0
kpatch-build/gcc-plugins/gcc-common.h:0
kpatch-build/gcc-plugins/gcc-generate-rtl-pass.h:0
kpatch-build/gcc-plugins/ppc64le-plugin.c:0
kpatch-build/insn/asm/inat.h:0
kpatch-build/insn/asm/inat_types.h:0
kpatch-build/insn/asm/insn.h:0
kpatch-build/insn/inat-tables.c:0
kpatch-build/insn/inat.c:0
kpatch-build/insn/insn.c:0
kpatch-build/kpatch-build:0
kpatch-build/kpatch-cc:0
kpatch-build/kpatch-elf.c:0
kpatch-build/kpatch-elf.h:0
kpatch-build/kpatch-intermediate.h:0
kpatch-build/kpatch.h:0
kpatch-build/list.h:0
kpatch-build/log.h:0
kpatch-build/lookup.c:0
kpatch-build/lookup.h:0

Leaving as draft for now as there would be a unit-test-objs update required and I also haven't tried it on a ppc64le host yet.

The last part of kpatch_elf_open() calls kpatch_find_func_profiling_calls() to find and set sym->has_func_profiling. However, only create-diff-object.c requires sym->has_func_profiling, so remove the call from kpatch_elf_open() and let the lone user, create-diff-object, provide and call it as needed. Signed-off-by: Joe Lawrence <[email protected]>

joe-lawrence · 2022-01-26T14:36:39Z

(Rebased against master to fix merge conflict, also tested successfully on ppc64le with make -s unit OBJDIR=objs/x86_64)

joe-lawrence · 2022-01-28T14:25:04Z

bumped the unit-test-objs submodule reference and tweaked the travis file... @sm00th : not sure if you prefer making a change like this, or modify one of the Makefiles (top-level or unit test) to run for both arches.

sm00th · 2022-01-28T15:08:18Z

@sm00th : not sure if you prefer making a change like this, or modify one of the Makefiles (top-level or unit test) to run for both arches.

I am starting to think that we might want to remove the arch-specific logic from unit-tests completely now and just run everything every time. But this can come later.

Travis change is ok as a first step (although you also want to update .github/workflows/unit.yml), the problem with this approach is that people running unit tests locally won't run into possible ppc64le issues until they push an MR.

joe-lawrence · 2022-01-28T15:55:05Z

@sm00th : not sure if you prefer making a change like this, or modify one of the Makefiles (top-level or unit test) to run for both arches.

I am starting to think that we might want to remove the arch-specific logic from unit-tests completely now and just run everything every time. But this can come later.

Travis change is ok as a first step (although you also want to update .github/workflows/unit.yml), the problem with this approach is that people running unit tests locally won't run into possible ppc64le issues until they push an MR.

Oh right, I was wondering why I didn't see my update run here in the github PR checks. I can add the change over in unit.yml, but first: if this were to be introduced gradually, does it make sense to implement it for local execution first, then add to gitlab/travis automation? Alternatively, we could take the plunge now and just remove the arch checks from the unit tests.

sm00th · 2022-01-28T16:23:04Z

if this were to be introduced gradually, does it make sense to implement it for local execution first, then add to gitlab/travis automation?

I don't see why won't we want it running all the time, ci checks for this repo are not required to pass for us to merge a change so it shouldn't hurt.

Alternatively, we could take the plunge now and just remove the arch checks from the unit tests.

Might be the most straightforward course.

joe-lawrence · 2022-01-28T19:31:05Z

Alternatively, we could take the plunge now and just remove the arch checks from the unit tests.

Might be the most straightforward course.

Ok, bombs away. I removed the RFC, so kpatch-build source code style / nits and Makefile gotchas welcome. :)

sm00th

Looks good

joe-lawrence · 2022-01-31T20:05:54Z

Fwiw, there is more to do in create-diff-object for making it endian safe. I have some of it already done, but since s390x is not officially supported yet, I'll post as new PRs as I progress/have time.

jpoimboe

Very nice change, I added some style nits, as you requested ;-)

jpoimboe · 2022-02-01T19:54:47Z

kpatch-build/create-diff-object.c

+	PPC64  = 0x1 << 1,
+};
+
+enum architecture target_arch;


Instead of the global variables could we just make add 'arch' a field of 'kpatch_elf'? Then arch could be set by kpatch_elf_open().

Then ABSOLUTE_RELA_TYPE could be converted to absolute_rela_type(kpatch_elf *kelf) which has a switch statement based on 'kelf.arch', or an ERROR otherwise.

Ok, this required some plumbing of struct kpatch_elf *kelf through the API, but not too unreasonable.

jpoimboe · 2022-02-01T19:55:38Z

kpatch-build/create-diff-object.c

+		return ((PPC64_LOCAL_ENTRY_OFFSET(sym->sym.st_other) != 0) &&
+			sym->sym.st_value == 8);
+	}
+


I think a switch statement here and for all other 'target_arch' checks would help readability and future extensibility instead of if-then-else.

Good point. I updated all of those sites and also used this opportunity to alphabetize them. (OCD, maybe.)

IMO a switch statement would still help readability here (and be consistent with the other arch checks)

jpoimboe · 2022-02-01T19:56:28Z

kpatch-build/create-diff-object.c

@@ -163,15 +166,13 @@ static int is_bundleable(struct symbol *sym)
 */
 static int is_gcc6_localentry_bundled_sym(struct symbol *sym)


While changing this function, it can be converted to bool to more clearly communicate the return semantic.

jpoimboe · 2022-02-01T19:57:49Z

kpatch-build/create-diff-object.c

-			} else if (rela->type == R_X86_64_64 ||
-				   rela->type == R_X86_64_32S)
+			if (target_arch == PPC64)
 				add_off = 0;


I think this 'add_off = 0' here is no longer needed since that's now the default initializer at the top of the loop.

I ended up reverting the change to move the initializer to the top. I believe some of the earlier code shuffling invoked a use-but-not-initialized warning from the -O2 build. I no longer hit that warning, and think the add_off = 0 here helps differentiate the two cases (ppc64le vs x86).

jpoimboe · 2022-02-01T19:58:24Z

kpatch-build/create-diff-object.c

 	{
 		.name		= ".retpoline_sites",
+		.arch		= X86_64,
+		.arch		= X86_64,


I'm surprised the compiler didn't complain about this ;-)

Good eye. That was me telling the compiler I really want it to assign that value.

jpoimboe · 2022-02-01T19:58:59Z

kpatch-build/create-diff-object.c

+		target_arch = PPC64;
+		ABSOLUTE_RELA_TYPE = R_PPC64_ADDR64;
+		break;
+	default: ERROR("Unsupported target architecture");


A line break between 'default:' and 'ERROR' would help with readability

joe-lawrence · 2022-02-02T17:56:29Z

Updated for @jpoimboe's review comments. Incremental diff: changes.diff.txt

jpoimboe · 2022-02-02T19:42:29Z

kpatch-build/Makefile

 INSN     = insn/insn.o insn/inat.o
 insn/%.o: CFLAGS := $(filter-out -Wconversion, $(CFLAGS))
-else ifeq ($(ARCH),ppc64le)
+ifeq ($(ARCH),ppc64le)


It's probably not a big deal, but believe it or not, there are people out there who cross-compile for x86 on a ppc64le host. Does the plugin not compile on x86?

I haven't looked at the plugin just yet. The scope of this PR was to only incrementally help create-diff-object so that it may read/write cross-arch ELF for the pre-built unit test objects. AFAIK, the plugin only comes into play for the kernel builds, right?

Ok, makes sense

jpoimboe · 2022-02-02T19:43:25Z

kpatch-build/create-diff-object.c

+		return ((PPC64_LOCAL_ENTRY_OFFSET(sym->sym.st_other) != 0) &&
+			sym->sym.st_value == 8);
+	}
+


IMO a switch statement would still help readability here (and be consistent with the other arch checks)

jpoimboe · 2022-02-02T19:43:35Z

kpatch-build/create-diff-object.c

+			sym->sym.st_value == 8);
+	}
+
 	return 0;


This should return false instead of 0

jpoimboe · 2022-02-02T19:46:26Z

kpatch-build/create-diff-object.c

-				continue;
-#endif
+			switch(kelf->target_arch) {
+				case PPC64:


The kernel style is to put 'case' on the same indentation level as 'switch', which helps reduce excessive indentation

Is it? I've observed 50/50 across the kernel, and in fact create-diff-object.c as well :P It looks weird when the last case adds it's own bracketed block like:

switch(foo) { case 1: /* code */ case 99: { /* code */ } <- appears unbalanced }

but none of my redundant default cases introduce a local scope, so that never happens here.

Anyway, it's not a big deal, I'll update to help the greater indentation cause.

Is it? I've observed 50/50 across the kernel

Really? git grep -A1 switch seems to show otherwise.

and in fact create-diff-object.c as well :P

I'm not surprised by that, but it's never too late to change :-)

It looks weird when the last case adds it's own bracketed block like:

switch(foo) { case 1: /* code */ case 99: { /* code */ } <- appears unbalanced }

Ew, hopefully we can avoid the need for that.

Anyway, it's not a big deal, I'll update to help the greater indentation cause.

Thanks :-)

Ew, hopefully we can avoid the need for that.

I see now that we did need it. Looks reasonable though :-)

jpoimboe · 2022-02-02T19:46:58Z

kpatch-build/create-diff-object.c

+static bool is_gcc6_localentry_bundled_sym(struct kpatch_elf *kelf,
+					  struct symbol *sym)
 {
+	if (kelf->target_arch == PPC64) {


I think the 'target_' prefix in 'target_arch' is redundant since the 'kelf' object already represents the target

joe-lawrence · 2022-02-02T20:47:00Z

All review comments implemented, Incremental diff: changes.diff.txt

libelf can read and write various architecture ELF files that may differ from the host system. Instead of using preprocessor directives to build architecture-specific code as per the current host, detect the intended target architecture from the input ELF files. Based-on: dynup#1179 Signed-off-by: Bill Wendling <[email protected]> Signed-off-by: Joe Lawrence <[email protected]> [small tweaks]

Bump the submodule reference and modify the unit test Makefile to check all supported architectures. Signed-off-by: Joe Lawrence <[email protected]>

joe-lawrence · 2022-02-02T22:39:59Z

Thanks for the review. One (hopefuly last) update.. it seems like I lost a rela->type assignment along the way in kpatch_create_kpatch_arch_section(), so I added that back in. With that, I'll step back and let our internal integration test bot churn on the PR.

joe-lawrence force-pushed the cross-arch branch from 02d9b8b to 2e1d914 Compare January 21, 2022 20:15

joe-lawrence force-pushed the cross-arch branch from 2e1d914 to 7f828ff Compare January 26, 2022 14:15

joe-lawrence mentioned this pull request Jan 26, 2022

all: replace objdump tests with readelf dynup/kpatch-unit-test-objs#38

Merged

joe-lawrence force-pushed the cross-arch branch from 43f0319 to 0d08fa1 Compare January 28, 2022 19:26

joe-lawrence changed the title ~~RFC: Remove host / target architecture assumption from create-diff-object~~ Remove host / target architecture assumption from create-diff-object Jan 28, 2022

sm00th approved these changes Jan 31, 2022

View reviewed changes

jpoimboe reviewed Feb 1, 2022

View reviewed changes

joe-lawrence force-pushed the cross-arch branch from 0d08fa1 to 6af4c70 Compare February 2, 2022 17:54

jpoimboe reviewed Feb 2, 2022

View reviewed changes

joe-lawrence force-pushed the cross-arch branch from 6af4c70 to 3b8e0a1 Compare February 2, 2022 20:45

jpoimboe approved these changes Feb 2, 2022

View reviewed changes

bwendling and others added 2 commits February 2, 2022 17:36

test/unit: update for cross-arch testing

f67a008

Bump the submodule reference and modify the unit test Makefile to check all supported architectures. Signed-off-by: Joe Lawrence <[email protected]>

joe-lawrence force-pushed the cross-arch branch from 3b8e0a1 to f67a008 Compare February 2, 2022 22:36

joe-lawrence merged commit 1d4b3c2 into dynup:master Feb 4, 2022

joe-lawrence mentioned this pull request Feb 8, 2022

[RFC] create-diff-object: Add --arch option #1179

Closed

joe-lawrence mentioned this pull request Apr 12, 2022

Bump to version v0.9.6 #1260

Merged

		@@ -163,15 +166,13 @@ static int is_bundleable(struct symbol *sym)
		*/
		static int is_gcc6_localentry_bundled_sym(struct symbol *sym)

Conversation

joe-lawrence commented Jan 21, 2022

Uh oh!

joe-lawrence commented Jan 26, 2022

Uh oh!

joe-lawrence commented Jan 28, 2022

Uh oh!

sm00th commented Jan 28, 2022

Uh oh!

joe-lawrence commented Jan 28, 2022

Uh oh!

sm00th commented Jan 28, 2022

Uh oh!

joe-lawrence commented Jan 28, 2022

Uh oh!

sm00th left a comment

Choose a reason for hiding this comment

Uh oh!

joe-lawrence commented Jan 31, 2022

Uh oh!

jpoimboe left a comment

Choose a reason for hiding this comment

Uh oh!

jpoimboe Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

joe-lawrence commented Feb 2, 2022

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!

joe-lawrence commented Feb 2, 2022

Uh oh!

joe-lawrence commented Feb 2, 2022

Uh oh!

Reviewers

Assignees

Labels

jpoimboe Feb 1, 2022 •

edited

Loading