-
Notifications
You must be signed in to change notification settings - Fork 305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove host / target architecture assumption from create-diff-object #1248
Conversation
02d9b8b
to
2e1d914
Compare
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 <joe.lawrence@redhat.com>
2e1d914
to
7f828ff
Compare
(Rebased against master to fix merge conflict, also tested successfully on ppc64le with |
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. |
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. |
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.
Might be the most straightforward course. |
43f0319
to
0d08fa1
Compare
Ok, bombs away. I removed the RFC, so kpatch-build source code style / nits and Makefile gotchas welcome. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice change, I added some style nits, as you requested ;-)
kpatch-build/create-diff-object.c
Outdated
PPC64 = 0x1 << 1, | ||
}; | ||
|
||
enum architecture target_arch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this required some plumbing of struct kpatch_elf *kelf through the API, but not too unreasonable.
return ((PPC64_LOCAL_ENTRY_OFFSET(sym->sym.st_other) != 0) && | ||
sym->sym.st_value == 8); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a switch statement here and for all other 'target_arch' checks would help readability and future extensibility instead of if-then-else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I updated all of those sites and also used this opportunity to alphabetize them. (OCD, maybe.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO a switch statement would still help readability here (and be consistent with the other arch checks)
kpatch-build/create-diff-object.c
Outdated
@@ -163,15 +166,13 @@ static int is_bundleable(struct symbol *sym) | |||
*/ | |||
static int is_gcc6_localentry_bundled_sym(struct symbol *sym) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While changing this function, it can be converted to bool to more clearly communicate the return semantic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
rela->offset); | ||
} else if (rela->type == R_X86_64_64 || | ||
rela->type == R_X86_64_32S) | ||
if (target_arch == PPC64) | ||
add_off = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this 'add_off = 0' here is no longer needed since that's now the default initializer at the top of the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
kpatch-build/create-diff-object.c
Outdated
.group_size = static_call_sites_group_size, | ||
}, | ||
{ | ||
.name = ".retpoline_sites", | ||
.arch = X86_64, | ||
.arch = X86_64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised the compiler didn't complain about this ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye. That was me telling the compiler I really want it to assign that value.
kpatch-build/create-diff-object.c
Outdated
target_arch = PPC64; | ||
ABSOLUTE_RELA_TYPE = R_PPC64_ADDR64; | ||
break; | ||
default: ERROR("Unsupported target architecture"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A line break between 'default:' and 'ERROR' would help with readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
0d08fa1
to
6af4c70
Compare
Updated for @jpoimboe's review comments. Incremental diff: changes.diff.txt |
SOURCES += insn/insn.c insn/inat.c | ||
INSN = insn/insn.o insn/inat.o | ||
insn/%.o: CFLAGS := $(filter-out -Wconversion, $(CFLAGS)) | ||
else ifeq ($(ARCH),ppc64le) | ||
ifeq ($(ARCH),ppc64le) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, makes sense
return ((PPC64_LOCAL_ENTRY_OFFSET(sym->sym.st_other) != 0) && | ||
sym->sym.st_value == 8); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO a switch statement would still help readability here (and be consistent with the other arch checks)
kpatch-build/create-diff-object.c
Outdated
return ((PPC64_LOCAL_ENTRY_OFFSET(sym->sym.st_other) != 0) && | ||
sym->sym.st_value == 8); | ||
} | ||
|
||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return false instead of 0
kpatch-build/create-diff-object.c
Outdated
continue; | ||
#endif | ||
switch(kelf->target_arch) { | ||
case PPC64: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kernel style is to put 'case' on the same indentation level as 'switch', which helps reduce excessive indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ew, hopefully we can avoid the need for that.
I see now that we did need it. Looks reasonable though :-)
kpatch-build/create-diff-object.c
Outdated
{ | ||
if (kelf->target_arch == PPC64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the 'target_' prefix in 'target_arch' is redundant since the 'kelf' object already represents the target
6af4c70
to
3b8e0a1
Compare
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 <morbo@google.com> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com> [small tweaks]
Bump the submodule reference and modify the unit test Makefile to check all supported architectures. Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
3b8e0a1
to
f67a008
Compare
Thanks for the review. One (hopefuly last) update.. it seems like I lost a |
This is a remix of @gwelymernans 's #1179 with a few slight tweaks:
--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.With the preparatory first commit and @gwelymernans 's previous heavy lifting, this removes all of the architecture-specific preprocessor blocks from kpatch-build:
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.