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
build: fix --lto=on build on GCC Linux #4707
base: master
Are you sure you want to change the base?
Conversation
Note that I do not know why --enable-pic fixes the build failure for me and that my knowledge in C/C++ builds is very limited. This fix was found mostly by trial and error of seemingly relevant google findings, as seen in the linked issue. Listing some possibly related ones:
With my testing I can confirm that either |
Until now I mainly tested building HandbrakeCLI, but not actually using it. I still need to see if everything actually works and whether I can notice any negative performance impact. If you have any recommendations on what exactly I should test, please let me know. FYI: For now I plan to focus on testing with my setup, where I build and execute HandbrakeCLI inside a debian:11.5-slim docker image. You can find the Dockerfile here, if its relevant or interesting. |
Your title does say, you want to fix this for Linux, but your patch does apply it for all architectures. At least it does build also on macOS with pic enabled, don't know for windows. But if you want to fix this for Linux only, you would need to ifdef the flag. |
Thanks @Nomis101, Today I wanted to thoroughly test the changes, but I discovered something else instead.
This PR does fix the issue in the GCC images, but it is not necessary in the other two scenarios. Currently I am not too motivated to find out what exactly is going wrong there, so I am considering to just drop this PR. Whats your opinions? |
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 don't have Linux, so I can't really test. I can say, that it builds on Mac with this patch. If it builds on Linux with this patch, I think this is fine then. Its similar to this one
HandBrake/contrib/libvpx/module.defs
Line 37 in 3d99f18
ifeq (linux,$(HOST.system)) |
So, the logic should also be fine.
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've been a bit concerned that enabling PIC might be a band-aid that just happens to work around a bigger issue. Thankfully, I was able to get some input from an FFmpeg developer. I'll attempt to sum up the findings here.
It seems there's an issue with inline assembly in libavcodec/x86/cabac.h
expanding to invalid instructions when GCC's LTO codegen is in play. Basically, the code generation defies certain assumptions made by the inline assembly, which is guarded by compiler version checks that aren't helpful in this case. Setting the BROKEN_RELOCATIONS
flag solves the problem, resulting in proper instructions, but without a compiler define to know we're in LTO mode, the decision to set BROKEN_RELOCATIONS
cannot be made when and where it needs to be made.
For whatever reason, enabling PIC causes the LTO codegen to do things differently enough that the conflict with the assumptions made by the inline assembly is removed. So, it's a semi-practical workaround, but not one that really gets at the root issue, and perhaps even a bit like driving a small nail with a sledgehammer.
It's a tough call, but I think this is too broad. Given the issue seems to only affect certain builds/configurations of GCC and not others, I'm afraid I'm not too comfortable enabling PIC across the board. We should instead treat this as a known issue and try to assist getting a proper fix into FFmpeg, which we can incorporate in the future. PIC can be enabled by CFLAGS if someone experimenting with LTO is so inclined.
Thanks for this insight. Based on your explanation I also now think this might be a semi-practical workaround. Maybe adding a line to the documentation "Building HandBrake for Linux" about enabling PIC by CFLAGS would then be a good idea? |
fixes #4699 ffmpeg build failure with
--lto=on
on GCC Linux, by using ffmpeg's--enable-pic
configure flag.snippet of build failure logs without this PR
Discovered when I tried to build HandbrakeCLI with Docker in debian:11.5-slim using the packaged GCC 10.2.1-6. I also confirmed the same error on GCC 12.2.0 and GCC 11.3.0
Test on: