-
Notifications
You must be signed in to change notification settings - Fork 11.4k
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
[CMake] Check objcopy support for LLVM_SPLIT_DWARF #80500
Conversation
LLVM_SPLIT_DWARF require new binutils support for objcopy etc. This is to check the option and error out if binutils is too old. Still allow use to override the check if they don't need to use objcopy.
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 check is not necessary. Building LLVM requires GCC 7.4/8.1 (2018), which roughly corresponds to the binutils 2.30 era. The default R_X86_64_REX_GOTPCRELX
clang emits requires something like binutils 2.25+.
roughly? I don't think we can assume that users are using binutils and GCC from same era. eg: Users can still use an old OS with old binutils, but just install newer GCC.
We actually distinguished GCC and binutils, and only require 2.17 .
If this is the case, should we at least update the document above to mention we require 2.25+ ? |
@MaskRay : on top of what @jsji wrote, don't you think that this check is "low-cost" while providing a safeguard to our users for a behavior that would be hard to figure out for them? (and for whoever has to provide support for when such user will ask for help on Discourse...)? I mean we require a minimum version of the toolchain and we check for it, so why not do the same for other tools? |
This change will cause trouble if a system does not provide GCC 8 is usually shipped with binutils 2.30+. See https://centos.pkgs.org/7/centos-sclo-rh-x86_64/devtoolset-8-binutils-2.30-47.el7.x86_64.rpm.html , far above than the |
"usually"? When is it not?
But that applies only to clang and not gcc? |
There is a package devtoolset-7-binutils-2.28-7.el7.x86_64.rpm. I assume that GCC 7 is often used with 2.28, nearly 5 years newer than 2.23. Of course you can claim that there are systems out there that use GCC 7 but much older binutils. I think proponents of the change should show evidence.
The objcopy requirement is only for GCC. Clang doesn't use objcopy for -gsplit-dwarf. BFD is part of binutils. I understand the desire to add some check to prevent any inconvenience, but I strongly believe the objcopy check is not only busy work here but also harmful (for systems not using binutils, etc). |
That's fine, we can adjust the check to only being performed if GCC is used...
You seem to understand all the moving pieces here: can you suggest a sequence of checks that would make sense? |
This patch should be abandoned. The binutils 2.23 version requirement has long been irrelevant. |
So for all practical purpose, for Linux systems we support, you're saying that all the tools involved in the build will correctly support split dwarf out of the box, regardless of the combination compiler/linker that is enabled? LGTM then. |
abandoned this first per @MaskRay 's requirement. We can reiterate this later if we get some error report. |
Yeah, I'm OK waiting to see if this is a problem in practice. but to answer some of the questions @joker-eph had:
I don't know precisely which version of binutils this was introduced in, or what different GCC versions likely ship alongside that binutils version, etc... |
LLVM_SPLIT_DWARF require new binutils support for objcopy etc.
This is to check the option and error out if binutils is too old.
Still allow use to override the check if they don't need to use objcopy.