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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
