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)

Fuchsia has an immediate use case for mixed SHF_LINK_ORDER and
non-SHF_LINK_ORDER components (D76802; will be supported by LLD in D84001;
GNU ld feature request https://sourceware.org/bugzilla/show_bug.cgi?id=16833 is pending).

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=future means that we 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

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
4677

Another option would be unstable.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
740–742

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
4677

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
4677

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
4677

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
3314–3317

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
3314–3317

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
387

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)Tue, Oct 20, 11:08 AM
MaskRay edited the summary of this revision. (Show Details)Tue, Oct 20, 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.EditedWed, Oct 21, 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.)