Page MenuHomePhabricator

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

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 can be consumed by GNU as>=2.35, but older versions may not work.

-fbinutils-version=none means that we can use all ELF features, regardless of
GNU as/ld support.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
4847

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
4847

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
4847

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
4847

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
3972–3975

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
3972–3975

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
409

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.Nov 3 2020, 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.EditedNov 16 2020, 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.Nov 19 2020, 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.Nov 19 2020, 2:29 PM

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

rsmith added inline comments.Jan 25 2021, 5:00 PM
clang/docs/ReleaseNotes.rst
103–104

(Just minor copy-editing.)

clang/include/clang/Basic/CodeGenOptions.h
131–133

I don't understand the description of "none" here; is there a spurious "not" in this sentence? Does "none" mean "features implemented by any known release can be used" (eg, I promise my binutils is not older than my LLVM), or "features implemented by every known release can be used" (eg, my binutils might be arbitrarily old, try to be compatible with it)?

I think perhaps this would express what you mean: " "none" means that all ELF features can be used, regardless of binutils support."

(Please also update the documentation in Options.td to match any changes here.)

clang/include/clang/Driver/Options.td
3974

The suggestion I take from this is that if -fintegrated-as is used, produced assembly cannot use newer ELF features, but I don't think that's right -- I'd expect that under -fintegrated-as, we get to use new ELF features so long as we think the linker supports them, regardless of what we think gas supports.

Also, "newer ELF features" is not clear to me: newer than what? (Newer than the specified version?)

clang/lib/Driver/ToolChains/Clang.cpp
4846

Perhaps use an unsigned type, so that -fbinutils-version=3.-14 is properly rejected.

4847

How about latest? (As in, "I promise I have at least the latest version of binutils that you've heard of")

llvm/include/llvm/MC/MCAsmInfo.h
410

(Support by only a later version isn't enough.)

llvm/lib/Target/TargetMachine.cpp
239
MaskRay updated this revision to Diff 319174.Jan 25 2021, 5:30 PM
MaskRay marked 8 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Address comments

MaskRay added inline comments.Jan 25 2021, 7:44 PM
clang/lib/Driver/ToolChains/Clang.cpp
4847

(offline discussion Richard is happy with 'none')

jhenderson accepted this revision.Jan 26 2021, 1:01 AM

Basically LGTM - (I'm happy for this to land either with or without my comments being addressed). Would be nice to get a second reviewer to confirm they're happy.

clang/test/Driver/fbinutils-version.c
2

Maybe also add a case for -fbinutils-version=2 (i.e. no minor version at all).

llvm/lib/CodeGen/LLVMTargetMachine.cpp
64

Is it possible somebody might do something weird like -fbinutils-version=0.1 and get behaviour different to what might be expected? Should we explicitly forbid major versions of 0?

llvm/lib/Target/TargetMachine.cpp
239

Maybe {INT_MAX, INT_MAX}? Doesn't really matter in practice, but {INT_MAX, INT_MAX} > {INT_MAX, 0} in the versioning scheme.

This revision is now accepted and ready to land.Jan 26 2021, 1:01 AM
MaskRay updated this revision to Diff 319337.Jan 26 2021, 10:16 AM
MaskRay marked 6 inline comments as done.

Address comments

rsmith accepted this revision.Jan 26 2021, 11:47 AM

Clang side looks good to me.

On the LLVM side, is it intended that invalid -binutils-version values are silently accepted?

MaskRay added a comment.EditedJan 26 2021, 11:56 AM

Clang side looks good to me.

On the LLVM side, is it intended that invalid -binutils-version values are silently accepted?

Yes. In llvm/tools/llc/llc.cpp, there is no validity check. For these non-user-facing utilities (opt/llc/llvm-mc/...), validity check for options tends to be scarce.

I'll add some checks and add some tests to llvm/test/tools/llc (which hardly has any test currently).

MaskRay updated this revision to Diff 319378.Jan 26 2021, 12:18 PM

Add llc validity check and tests

This revision was landed with ongoing or failed builds.Jan 26 2021, 12:28 PM
This revision was automatically updated to reflect the committed changes.