Page MenuHomePhabricator

Add -fbinutils-version= to gate ELF features on the specified binutils version
Needs ReviewPublic

Authored by MaskRay on Aug 6 2020, 1:46 PM.

Details

Summary

There are two use cases.

Assembler
We have accrued some code gated on MCAsmInfo::useIntegratedAssembler(). Some
features are supported by latest GNU as, but we have to use
MCAsmInfo::useIntegratedAs() because the newer versions have not been widely
adopted (e.g. SHF_LINK_ORDER 'o' and 'unique' linkage in 2.35, --compress-debug-sections= in 2.26).

Linker
We want to use features supported only by LLD or very new GNU ld, or don't want
to work around older GNU ld. We currently can't represent that "we don't care
about old GNU ld". You can find such workarounds in a few other places, e.g.
Mips/MipsAsmprinter.cpp PowerPC/PPCTOCRegDeps.cpp X86/X86MCInstrLower.cpp
AArch64 TLS workaround for R_AARCH64_TLSLD_MOVW_DTPREL_* (PR ld/18276),
R_AARCH64_TLSLE_LDST8_TPREL_LO12 (https://bugs.llvm.org/show_bug.cgi?id=36727 https://sourceware.org/bugzilla/show_bug.cgi?id=22969)

Mixed SHF_LINK_ORDER and non-SHF_LINK_ORDER components (supported by LLD in D84001;
GNU ld feature request https://sourceware.org/bugzilla/show_bug.cgi?id=16833 may take a while before available).
This feature allows to garbage collect some unused sections (e.g. fragmented .gcc_except_table).

This patch adds -fbinutils-version= to clang and -binutils-version to llc.
It changes one codegen place in SHF_MERGE to demonstrate its usage.
-fbinutils-version=2.35 means the produced object file does not care
about GNU ld<2.35 compatibility. When -fno-integrated-as is specified,
the produced assembly does not care about GNU as<2.35 compatibility.

-fbinutils-version=none means that we don't care about binutils
compatibility and can freely use unimplemented GNU as/ld features.

Both clang and llc need parseBinutilsVersion. Such command line parsing is usually implemented in
llvm/lib/CodeGen/CommandFlags.cpp (LLVMCodeGen), however, ClangCodeGen does not
depend on LLVMCodeGen. So I add parseBinutilsVersion to
llvm/lib/Target/TargetMachine.cpp (LLVMTarget).

Diff Detail

Unit TestsFailed

TimeTest
380 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

MaskRay created this revision.Aug 6 2020, 1:46 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
MaskRay requested review of this revision.Aug 6 2020, 1:46 PM
MaskRay edited the summary of this revision. (Show Details)Aug 6 2020, 1:47 PM
phosek added inline comments.Aug 6 2020, 2:25 PM
clang/lib/Driver/ToolChains/Clang.cpp
4773

Another option would be unstable.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
746–748

I'd prefer to spell it out in full, i.e. getContext().getAsmInfo()->useIntegratedAssembler() || !getContext().getAsmInfo()->binutilsVersion(2, 35), it's more verbose but it's cleaner to me what it does just when looking at this line alone.

MaskRay updated this revision to Diff 284234.Aug 9 2020, 2:38 PM

Add binutilsIsAtleast()

MaskRay added inline comments.Aug 13 2020, 10:33 AM
clang/lib/Driver/ToolChains/Clang.cpp
4773

I picked future because there is an precedent: -mcpu=future is used by some ppc folks.

ro added a subscriber: ro.Aug 13 2020, 10:41 AM
ro added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
4773

I fear that's a terrible precedent: this name had to be chosen because for some unknown, but certainly silly, reason, IBM didn't wan't to call it power10 before release.

dblaikie added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
4773

A little more respect for other developers would be great here - hard to know it's silly if you don't know the reason & even if you think it is silly, folks may not have the same goals/requirements you do.

(that's not to dismiss the concern that the precedent for naming -mcpu may not apply as well here (not suggesting it doesn't either - I have no opinion either way))

Looks like there's only an llc test, but the option is in clang too? Should we have a test there?

Is there any documentation for clang this option needs adding to? Also, this likely deserves a release note, I feel.

clang/include/clang/Driver/Options.td
3447–3450

It might be helpful to indicate what the default is.

MaskRay marked an inline comment as done.Aug 14 2020, 9:55 AM
MaskRay added inline comments.
clang/include/clang/Driver/Options.td
3447–3450

The default can be overridden by targets. Don't know how to document it.

(If this ever gets wider use, people may want to add a configure time variable.)

Looks like there's only an llc test, but the option is in clang too? Should we have a test there?

There are generally some dotted edges in the testing of such MC level options. For options affecting IR generating, clang/test/CodeGen can do the tests. For MC level options, IR is not affected at all. Such tests belong to llvm/test/CodeGen/. However, clang cannot be used due to layering concerns. The closest (which is used by other options, e.g. -fuse-integrated-as, -Wa,-mrelax-relocations=) is to have some llc options similar to the clang driver options.

MaskRay updated this revision to Diff 285681.Aug 14 2020, 10:06 AM
MaskRay edited the summary of this revision. (Show Details)

Add release note

This looks reasonable to me, but I don't know the surrounding code well enough to be comfortable LGTM-ing it. Sorry!

llvm/include/llvm/MC/MCAsmInfo.h
406

This sentence implies this specific version, but I think it's meant to be a minimum. If so, I'd add "or later" to the end of the sentence.

MaskRay updated this revision to Diff 288382.Aug 27 2020, 10:16 AM
MaskRay marked an inline comment as done.

Reword a comment

MaskRay edited the summary of this revision. (Show Details)Oct 20 2020, 11:08 AM
MaskRay edited the summary of this revision. (Show Details)Oct 20 2020, 11:09 AM

Is there a reason to not use the existing -mlinker-version= option and expanding that beyond just Darwin targets? I feel like having the same option would be much nicer.

Is there a reason to not use the existing -mlinker-version= option and expanding that beyond just Darwin targets? I feel like having the same option would be much nicer.

-mlinker-version is currently a macOS option. We could reuse it but that would be confusing: when -fuse-ld=lld is specified, does it specify the lld version? (No)
There are also GNU as related issues.

Is there a reason to not use the existing -mlinker-version= option and expanding that beyond just Darwin targets? I feel like having the same option would be much nicer.

I agree with @compnerd that it seems better to reuse this if possible. I wasn't around back then, but I have to assume it was named generically in order to enable that.

-mlinker-version is currently a macOS option. We could reuse it but that would be confusing: when -fuse-ld=lld is specified, does it specify the lld version? (No)

Maybe it should?

There are also GNU as related issues.

I don't understand this bit.

Is there a reason to not use the existing -mlinker-version= option and expanding that beyond just Darwin targets? I feel like having the same option would be much nicer.

I agree with @compnerd that it seems better to reuse this if possible. I wasn't around back then, but I have to assume it was named generically in order to enable that.

-mlinker-version is currently a macOS option. We could reuse it but that would be confusing: when -fuse-ld=lld is specified, does it specify the lld version? (No)

Maybe it should?

Then there is an expanded use case, -fuse-ld= which is just a driver option for linking, will now affect code generation.

There are also GNU as related issues.

I don't understand this bit.

This is the main point. Many feature are also related to the version of GNU assembler when -fno-integrated-as is specified.

Is there a reason to not use the existing -mlinker-version= option and expanding that beyond just Darwin targets? I feel like having the same option would be much nicer.

I agree with @compnerd that it seems better to reuse this if possible. I wasn't around back then, but I have to assume it was named generically in order to enable that.

-mlinker-version is currently a macOS option. We could reuse it but that would be confusing: when -fuse-ld=lld is specified, does it specify the lld version? (No)

Maybe it should?

Actually, I think it has to - there is some work underway to add MachO support to lld - so we will need to have this be the lld version anyway in that case.

Then there is an expanded use case, -fuse-ld= which is just a driver option for linking, will now affect code generation.

I don't see this as an expanded use case - this is precisely the use case it is intended for. It is specifying the linker - the linker may have a different set of features and code generation (and the driver behaviour) need to adjust for that. If the linker does not support .drectve processing on PE/COFF, it will need to change the code generation. This seems very much in scope for the option.

There are also GNU as related issues.

I don't understand this bit.

This is the main point. Many feature are also related to the version of GNU assembler when -fno-integrated-as is specified.

I could also be using a different assembler than binutils, so controlling this via a binutils specific option seems less appealing, and seems better to have the assembler be different from the linker seems better.

MaskRay added a comment.EditedOct 21 2020, 4:22 PM

Is there a reason to not use the existing -mlinker-version= option and expanding that beyond just Darwin targets? I feel like having the same option would be much nicer.

I agree with @compnerd that it seems better to reuse this if possible. I wasn't around back then, but I have to assume it was named generically in order to enable that.

-mlinker-version is currently a macOS option. We could reuse it but that would be confusing: when -fuse-ld=lld is specified, does it specify the lld version? (No)

Maybe it should?

Actually, I think it has to - there is some work underway to add MachO support to lld - so we will need to have this be the lld version anyway in that case.

Then there is an expanded use case, -fuse-ld= which is just a driver option for linking, will now affect code generation.

I don't see this as an expanded use case - this is precisely the use case it is intended for. It is specifying the linker - the linker may have a different set of features and code generation (and the driver behaviour) need to adjust for that. If the linker does not support .drectve processing on PE/COFF, it will need to change the code generation. This seems very much in scope for the option.

This seems to assume the an object file is emitted for one specific linker, a standard system linker or LLD (ELF or Mach-O). How do you communicate the fact that the user wants to emit object files suitable for both the standard system linker and LLD? -fuse-ld=bfd,lld? It would not work because -fuse-ld= is also used as a driver option for the linking stage. There needs to be exact one value to name the linker.

This is a much wider design space I somewhat want to avoid. I hope I can see more convincing arguments that -mlinker-version= plus a few other toggles are really better than -fbinutils-version=. I have actually thought about -mlinker-version= when @jyknight or someone else mentioned it. But with more thoughts I don't find it actually better than a very specific -fbinutils-version= which users should immediately know the intention.

There are also GNU as related issues.

I don't understand this bit.

This is the main point. Many feature are also related to the version of GNU assembler when -fno-integrated-as is specified.

I could also be using a different assembler than binutils, so controlling this via a binutils specific option seems less appealing, and seems better to have the assembler be different from the linker seems better.

So do you suggest an option specifically for assembler? -massembler-version? Then how do you communicate the fact that you want to be compatible with GNU as from binutils?
(There are other assemblers on Linux but none has as ubiquitous as GNU as from binutils. A very large of directives in MC were designed for GNU as compatibility.)

pcc added a comment.Tue, Nov 3, 1:18 PM

I agree with @MaskRay that this should be a binutils-specific option. The flag -mlinker-version seems to have been designed around macOS-specific assumptions i.e. there is a single linker (ld64) and that the linker and assembler are not version coupled. Having this option be binutils-specific seems like the best way to reflect the binutils-specific requirements.

MaskRay added a comment.EditedMon, Nov 16, 11:56 PM

Sent https://lists.llvm.org/pipermail/llvm-dev/2020-November/146676.html "Add -fbinutils-version=" (cross posted to cfe-dev) so that more folks can notice it.

About "generate code that doesn't care about GNU as/ld compatibility/limitation", I am open for suggestions. Currently I like -fbinutils-version=none the most (after considering future and latest and 99 (assuming GNU binutils 99 implements everything we need...))

Sent https://lists.llvm.org/pipermail/llvm-dev/2020-November/146676.html "Add -fbinutils-version=" (cross posted to cfe-dev) so that more folks can notice it.

About "generate code that doesn't care about GNU as/ld compatibility/limitation", I am open for suggestions. Currently I like -fbinutils-version=none the most (after considering future and latest and 99 (assuming GNU binutils 99 implements everything we need...))

I like none. latest has some connotations that we attempt compatibility with some specific bleeding edge set of tools, which might not be true, especially if they've released at a different time point to us. none implies there is no specific guarantee for compatibility to me. 99 (or other number) seems risky/short-sighted given the way versioning sometimes shoots up versus the longetivity of some of these code bases.

MaskRay updated this revision to Diff 306478.Thu, Nov 19, 10:48 AM
MaskRay edited the summary of this revision. (Show Details)

Switch to -fbinutils-version=none from 'future'

MaskRay updated this revision to Diff 306538.Thu, Nov 19, 2:29 PM

Fix comments: 'future' -> 'none'
Add a test for llc -binutils-version=none