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

[bwendling]

We could be generating patches for architectures that are different from
the current one. Use "--arch" to specify the architecture.

Signed-off-by: Bill Wendling morbo@google.com

[bwendling]

This is meant an RFC. If you think it's good, then I can modify it for proper submission. Basically it allows someone to run this program to generate patches for .o files that aren't the same as the host architecture.

[joe-lawrence]

Hi @gwelymernans -- this is an idea we've kicked around in the past, but never got around to actually trying. I'm guessing that you've tried out the RFC with a gcc or llvm cross compiler? As long as it generates the same exact code (and optimization decisions, etc) as the native compiler it should work.

As far as the RFC code, I'm interested to see what @jpoimboe thinks of unpeeling all those #if <ARCH> macros. Not only would it allow cross kpatch compilation, but it would feed more of the code through the compiler.

Also note that it seems to fail the unit tests: https://github.com/dynup/kpatch/pull/1179/checks?check_run_id=2518719533

[jpoimboe]

The general concept looks good to me. Might it be helpful to allow specifying the cross compiler somehow?

[bwendling]

[Sorry for the late reply; I was OOO.]

I'm unsure about specifying the compiler. This seems to work on the ELF level, so the compiler shouldn't be much of an issue...

[jpoimboe]

I'm unsure about specifying the compiler. This seems to work on the ELF level, so the compiler shouldn't be much of an issue...

That's true for create-diff-object, but the kpatch-build script builds the kernel twice.

[joe-lawrence]

Hi @gwelymernans,

Quick comment on this RFC: we would definitely welcome AARCH64 support, but I think we could review this RFC quicker if it focused only on the --arch option changes. I foresee relocation and other arch-specific details that ARM support may drag on for a while and I don't think this RFC should be blocked by all of that (future) work.

Also, we internally trigger integration tests on PR open/push and they are currently failing like:

make[3]: Leaving directory `/root/.kpatch/src'
kpatch/kpatch-build/create-diff-object: unrecognized option '--arch x86_64'
Try `create-diff-object --help' or `create-diff-object --usage' for more
information.

so I dunno if argp_parse() isn't completely happy with 777 / --arch details, or if this sequence in kpatch-build is passing it as a single, combined option:

ARCH_FLAG="--arch x86_64"
...
"$TOOLSDIR"/create-diff-object "$ARCH_FLAG" ...

in which case something like this will work as intended:

ARCH_FLAG=("--arch" "x86_64")
"$TOOLSDIR"/create-diff-object "${ARCH_FLAG[@]}" ...

[sm00th]

or if this sequence in kpatch-build is passing it as a single, combined option

It is this. Another option would be to just drop the quotes around $ARCH_FLAG in create-objecti-diff invocation. I am guessing the quotes are there because shellcheck complained, it can be persuaded like so: https://github.com/dynup/kpatch/blob/master/kpatch-build/kpatch-build#L923

Also this won't build on ppc64le at all because we are not building insn subroutines on architectures other than x86 atm. This section: https://github.com/dynup/kpatch/blob/master/kpatch-build/Makefile#L13 needs to be unconditional now I think.

[joe-lawrence]

Hi @gwelymernans, I'm going to close this RFC PR since we ended up merging most of the changes with #1248. The main change in that PR was to drop the --arch command line flags and rely on the architecture as set in the ELF files themselves. If there is some other use case that --arch flags would offer, feel free to open a new PR or issue. Thanks for the idea and the code changes!