[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:103-104
+  For example, ``-fbinutils-version=2.35`` means compatibility with GNU as/ld
+  before 2.35 is not needed: new features can be used and don't work around old
+  GNU as/ld bugs.
 
----------------
(Just minor copy-editing.)


================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:129-131
+  // If set, override the default value of MCAsmInfo::BinutilsVersion. "none"
+  // means that features not implemented by any known release can be used. If
+  // DisableIntegratedAS is specified, this also affects assembly output.
----------------
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.)


================
Comment at: clang/include/clang/Driver/Options.td:3449
+  HelpText<"Produced object files can use ELF features only supported by ld newer than the specified version. If"
+  "-fno-integrated-as is specified, produced assembly can use newer ELF features. "
+  "'none' means that features not implemented by any known release can be used">;
----------------
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?)


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4772
+    StringRef V = A->getValue();
+    int Num;
+    if (V == "none")
----------------
Perhaps use an unsigned type, so that `-fbinutils-version=3.-14` is properly rejected.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4642
+    int Num;
+    if (V == "future")
+      A->render(Args, CmdArgs);
----------------
dblaikie wrote:
> ro wrote:
> > MaskRay wrote:
> > > phosek wrote:
> > > > Another option would be `unstable`.
> > > I picked `future` because there is an precedent: `-mcpu=future` is used by some ppc folks.
> > 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.
> 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))
How about `latest`? (As in, "I promise I have at least the latest version of binutils that you've heard of")


================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:407
+  // Generated object files can only use features supported by GNU ld of this
+  // binutils version or later.
+  std::pair<int, int> BinutilsVersion = {2, 26};
----------------
(Support by only a later version isn't enough.)


================
Comment at: llvm/lib/Target/TargetMachine.cpp:287
+  if (Version == "none")
+    return {INT_MAX, 0}; // Make binutilsIsAtLeast() fail.
+  std::pair<int, int> Ret;
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85474/new/

https://reviews.llvm.org/D85474