kpatch-build: add cross-compilation support (#1224 fix) #1233
Conversation
|
Well, the --cross-compile option is replaced with picking a CROSS_COMPILE variable from shell environment, changes in kpatch-cc are moved to MAKEVARS in kpatch-build so kpatch-cc won't be changed, and cross-compile prefix is now added to all of the tools used, not just gcc, but also objcopy and ld. |
|
Hi, looked into it a bit and I have a couple of questions. Where does this come from? You mentioned these patches are included in openembedded build, but do you know how is it used there? Looking at the original commit message it looks like openembedded uses it to build patches for same arch but other different variants. Is that correct? If so this might be a good first step but there is a lot more work to be done for proper cross-compilation support. Regarding the patch itself: what is the point of |
|
Hi, this patch is not directly related to openembedded one, but is a prerequisite to upstream it, too, after deciding how to better implement Regarding |
omatiusha
force-pushed
the
master
branch
2 times, most recently
from
f99c5cd to
94a2f29
Compare
That was the reason I was asking. Is there a working cross-compilation scenario that this patch enables? Right now kpatch-build only supports x86_64 and ppc64le, both of those have code wrapped in |
|
Hi, I got into the specifics of the project a little bit and now I want to resume the discussion. The project builds kernels for the same architecture, but:
What do you think about it? |
|
My kbuild understanding is that I think some of the One more question: from the current changes, objcopy is |
In gcc's case this is true, but when using clang kbuild derives more info from CROSS_COMPILE:
I agree, even if we won't be doing real cross-compilation just yet having this stepping stone definitely won't hurt.
With clang we set (HOST)?CC/(HOST)?LD directly.
I think we mostly ignore
It will probably work but the goal is to replicate target's environment, so I do think we want everything prefixed. |
|
I updated the commit.
Didn't test with clang, but added cross-compile prefix in MAKEVARS for it too
idk if we need prefix here, so I left it untouched
I implemented this variation because it was more intuitive for me
and left everything prefixed, so I think this will work fine. |
|
Hi @omatiusha, I think this last push broke the integration tests ( |
|
Hi @joe-lawrence, it seems to be broken due to fallback to distro-specific sources download on line 825. Should I add a fallback option for "dummy" |
Without testing, I'm not 100%, but I think the problem is that before this change if [[ -n "$USERSRCDIR" ]] && [[ -n "$VMLINUX" ]]; then
DISTRO="dummy"
else
DISTRO="$ID" # ID is not set
fi
...
[[ -f "$RELEASE_FILE" ]] && source "$RELEASE_FILE" # ID is set here
DISTRO="$ID"I think that entire conditional and dummy value could be completely removed if the release file sourcing and if [[ -z "$USERSRCDIR" ]] && [[ -f "$RELEASE_FILE" ]]; then
source "$RELEASE_FILE"
DISTRO="$ID"
fiThat should solve for:
|
So should the prefix apply to the readelf invocations as well? |
Ok, I'll try that.
Yes it is, I forgot to prepend readelf and one gcc invocation. |
|
Latest update seems to make internal integration tests happy. Later this week, I'd like to run a few tests with these changes so that we can also document how to use this. In the meantime, a few nits with the commit message. We're not as strict as the kernel, but to tweak it to look more like the project's conventions:
|
|
Thank you very much @joe-lawrence, I've done what you suggested. I'll be waiting patiently for the test results. |
|
Okay, I ran two tests:
I get the same results, even with this small change to point to the cross-compiler's plugins include files: diff --git a/kpatch-build/Makefile b/kpatch-build/Makefile
index 50899b6..3b586fe 100644
--- a/kpatch-build/Makefile
+++ b/kpatch-build/Makefile
@@ -18,7 +18,7 @@ else ifeq ($(ARCH),ppc64le)
SOURCES += gcc-plugins/ppc64le-plugin.c
PLUGIN = gcc-plugins/ppc64le-plugin.so
TARGETS += $(PLUGIN)
-GCC_PLUGINS_DIR := $(shell gcc -print-file-name=plugin)
+GCC_PLUGINS_DIR := $(shell $(CROSS_COMPILE)gcc -print-file-name=plugin)
PLUGIN_CFLAGS := $(filter-out -Wconversion, $(CFLAGS))
PLUGIN_CFLAGS += -shared -I$(GCC_PLUGINS_DIR)/include \
-Igcc-plugins -fPIC -fno-rtti -O2 -WallI can easily reproduce this with a super small repro: I'm not very experienced with gcc plugins, so I'm unsure how to continue debugging this. |
|
Thank you |
Yes in the ppc64le case, I was building on a powerpc host using the alternate compiler from crosstool mentioned above. I'll give the IBM Advance Toolchain for Linux on Power a try and hope to have better luck with that. As far x86_64 goes, the patchset it probably all set... I'd like to figure out what it takes to get ppc64le working, too, since we may have interest in cross-compiling from x86_64 to ppc64le one day. |
|
@joe-lawrence Does this help? |
Ok, that revealed that the cross compilers that Arnd posts up on https://mirrors.edge.kernel.org/pub/tools/crosstool/ don't include g++. As far setting the cross compiler prefix, I had not seen other examples cross compile the plugin itself. Would that make sense if I was targeting a different arch? I did give the IBM compiler a spin, and it does build my toy test.c example with only the header file path updated. But then the kernel build doesn't seem to like its linker. So I'm willing to believe that the problem was in the incomplete toolchain I was trying to use. Since there would be more cross compilation (and arch) work to do, I'm fine approving this change and completing the rest as needed. |
|
Oh right, I guess it doesn't make sense to cross-compile the gcc plugin. |
| gcc_version_from_file() { | ||
| readelf -p .comment "$1" | grep -m 1 -o 'GCC:.*' | ||
| "$CROSS_COMPILE"readelf -p .comment "$1" | grep -m 1 -o 'GCC:.*' | ||
| } |
There was a problem hiding this comment.
clang_version_from_file() and clang_version_check() are missing CROSS_COMPILE.
There was a problem hiding this comment.
Also, instead of sprinkling CROSS_COMPILE everywhere, it may be cleaner to just have GCC, CLANG, READELF, OBJCOPY, LD, LD_LLD variables defined at the top of the file to include CROSS_COMPILE.
(As a further eventual cleanup we might want to just have CC and LD variables instead of GCC/CLANG/LD/LD_LLD, but that might be a little tricky since it depends on the config file, and maybe can be done later.)
There was a problem hiding this comment.
I do not know if these changes work with clang as well, so I can add them at our own risk
kpatch-build/kpatch-build
Outdated
| DISTRO="$ID" | ||
| fi | ||
|
|
||
|
|
There was a problem hiding this comment.
Just one newline here please, for consistency with the rest of the file.
kpatch-build/kpatch-build
Outdated
| rm -f "$LOGFILE" | ||
|
|
||
|
|
||
|
|
Agreed |
I had previous run-ins with this and we definitely don't want the plugin to be built with a cross-compiler. We need it to be built for the host architecture against cross-compiler's headers and linked against it. I believe our makefile should achieve this already, will make sure tomorrow. |
That is it, I've tried this on fedora35 with gcc-c++-ppc64-linux-gnu gcc-ppc64-linux-gnu installed and your testcase works fine. @omatiusha could you please add the |
|
Sure. Should I add also this? I did not test cross compilation on clang. |
Yes, even if it breaks we can fix clang later. We only support it experimentally anyway. |
|
@omatiusha I think you missed one of my comments:
e.g, something like this at the top: index 193a42e..9155a4d 100755
--- a/kpatch-build/kpatch-build
+++ b/kpatch-build/kpatch-build
@@ -58,6 +58,13 @@ APPLIED_PATCHES=0
OOT_MODULE=
KLP_REPLACE=1
+GCC="{$CROSS_COMPILE:-}gcc"
+CLANG="{$CROSS_COMPILE:-}clang"
+LD="{$CROSS_COMPILE:-}ld"
+LLD="{$CROSS_COMPILE:-}lld"
+READELF="{$CROSS_COMPILE:-}readelf"
+OBJCOPY="{$CROSS_COMPILE:-}objcopy"
+
warn() {
echo "ERROR: $1" >&2
}
|
Introduce the ability to pick a CROSS_COMPILE environment variable for specifying toolchain prefix for all gcc and binutils. To facilitate its use, do not set the DISTRO variable when USERSRCDIR is specified. Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com> Signed-off-by: Oleh Matiusha <oleh.matiusha@globallogic.com>
Ignore distro-specific checks if kernel source directory is supplied as a command line argument to remove the distro-specific tunings during cross-compilation. Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com> Signed-off-by: Oleh Matiusha <oleh.matiusha@globallogic.com>
|
@jpoimboe thanks, all is ready, your diff had typos in |
5 participants
This PR fixes some issues discussed in #811 and #1224.
I decided to submit small changes sequentially and hold off on the --distro option.
This patch introduces ability to pick a CROSS_COMPILE
environment variable which can be used for
specifying cross-complier prefix.
It allows to build live patches not only on
target system, but also on hosts for a target other
than the one on which the compiler is running.