-
Notifications
You must be signed in to change notification settings - Fork 88
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
elfloader: Add ElfloaderPrecompile option #175
Conversation
bed25ca
to
6188078
Compare
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 to me.
Though I wonder, would it make sense to avoid the config option, and always define separate targets for the library and the executable versions of the elfloader? (My knowledge of the CMake build system is limited, so it's fine if the answer is no.)
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 from my side (modulo style checker warnings).
Why not even make the binary now use the *.o artifact? So, there would be separate lib and elf targets. Any outer build flow could just pick the target it wants then. Existing build flows would get the binary and see no difference. Assuming the intermediate step of building a lib and then the elf from that does not change the binary. |
@@ -383,6 +403,17 @@ if(ElfloaderImageEFI) | |||
# EFI_SUBSYSTEM=0xa indicates that we're building an EFI application. | |||
" -Wl,-T ${linkerScript} -nostdlib -shared -Wl,-Bsymbolic,--defsym=EFI_SUBSYSTEM=0xa -Wl,--build-id=none" | |||
) | |||
elseif(ElfloaderPrecompile) |
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.
Seems that block does not have to be conditional. The target elfloader_precompile
is not part of ALL
, so it is not considered unless explicitly selected.
This option generates an intermediate build file, elfloader.o, that can be combined in a separate build step with an archive.o containing the cpio archive with the kernel and rootserver images to load. This enables the elfloader to be installed by copying the elfloader.o and generated linker file to an install location and a future build step can link with a supplied archive.o into a bootable elfloader image using something like: aarch64-unknown-linux-gnu-gcc -march=armv8-a elfloader.o archive.o \ -o elfloader -Wl,-T linker.lds_pp -nostdlib -static -lgcc Signed-off-by: Kent McLeod <kent@kry10.com>
6188078
to
15581f5
Compare
The config option means its easy to see that the refactor introduced by the new behavior doesn't change any existing behavior without the config option set. Allowing both to exist at the same time would require rewriting more of the file to handle the need for both targets to have the same compilation target settings, but different linking target settings.
Because I'm not 100% confident that they produce the same binary and didn't want to affect existing behavior. |
This option generates an intermediate build file, elfloader.o, that can be combined in a separate build step with an archive.o containing the cpio archive with the kernel and rootserver images to load. This enables the elfloader to be installed by copying the elfloader.o and generated linker file to an install location and a future build step can link with a supplied archive.o into a bootable elfloader image using something like:
aarch64-unknown-linux-gnu-gcc -march=armv8-a elfloader.o archive.o
-o elfloader -Wl,-T linker.lds_pp -nostdlib -static -lgcc