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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG34b60d8a5684: Add -fbinutils-version= to gate ELF features on the specified binutils version (authored by MaskRay). Repository: rG LLVM Github Mon

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

MaskRay updated this revision to Diff 319378. MaskRay added a comment. Add llc validity check and tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85474/new/ https://reviews.llvm.org/D85474 Files: clang/docs/ReleaseNotes.rst clang/include/c

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

MaskRay added a comment. In D85474#2523439 , @rsmith wrote: > 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.

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

rsmith accepted this revision. rsmith added a comment. Clang side looks good to me. On the LLVM side, is it intended that invalid `-binutils-version` values are silently accepted? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85474/new/ https://r

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

MaskRay updated this revision to Diff 319337. MaskRay marked 6 inline comments as done. MaskRay added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85474/new/ https://reviews.llvm.org/D85474 Files: clang/docs/ReleaseN

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

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. 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. C

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

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4642 +int Num; +if (V == "future") + A->render(Args, CmdArgs); rsmith wrote: > dblaikie wrote: > > ro wrote: > > > MaskRay wrote: > > > > phosek wrote: > > > > > Another

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

MaskRay updated this revision to Diff 319174. MaskRay marked 8 inline comments as done. MaskRay edited the summary of this revision. MaskRay added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85474/new/ https://reviews.

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

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. (Jus

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

MaskRay added a comment. Herald added a reviewer: jansvoboda11. Ping:) 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@li

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

MaskRay added a comment. Ping:) 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

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

MaskRay updated this revision to Diff 306538. MaskRay added a comment. Fix comments: 'future' -> 'none' Add a test for llc -binutils-version=none Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85474/new/ https://reviews.llvm.org/D85474 Files: cla

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

MaskRay updated this revision to Diff 306478. MaskRay edited the summary of this revision. MaskRay added a comment. Switch to -fbinutils-version=none from 'future' Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85474/new/ https://reviews.llvm.org/D8

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

jhenderson added a comment. In D85474#2398858 , @MaskRay wrote: > 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

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

MaskRay added a comment. 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 and don't care about GNU as/ld compatibility/limitation", I am open for suggestions. Curre

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

pcc added a comment. 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

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

MaskRay added a comment. Ping @compnerd 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

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

MaskRay added a comment. In D85474#2344823 , @compnerd wrote: > In D85474#2343393 , @MaskRay wrote: > >> In D85474#2343356 , @dexonsmith >> wrote: >> >>> In D85474#2343326 <

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

compnerd added a comment. In D85474#2343393 , @MaskRay wrote: > In D85474#2343356 , @dexonsmith > wrote: > >> In D85474#2343326 , @MaskRay wrote: >> >>> In D85474#2342365

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

MaskRay added a comment. In D85474#2343356 , @dexonsmith wrote: > In D85474#2343326 , @MaskRay wrote: > >> In D85474#2342365 , @compnerd wrote: >> >>> Is there a reason to no

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

dexonsmith added a comment. In D85474#2343326 , @MaskRay wrote: > In D85474#2342365 , @compnerd wrote: > >> Is there a reason to not use the existing `-mlinker-version=` option and >> expanding that beyond just Dar

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

MaskRay added a comment. In D85474#2342365 , @compnerd wrote: > 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-ve

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

compnerd added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85

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

MaskRay updated this revision to Diff 288382. MaskRay marked an inline comment as done. MaskRay added a comment. Reword a comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85474/new/ https://reviews.llvm.org/D85474 Files: clang/docs/ReleaseN

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

jhenderson added a comment. This looks reasonable to me, but I don't know the surrounding code well enough to be comfortable LGTM-ing it. Sorry! Comment at: llvm/include/llvm/MC/MCAsmInfo.h:387 + // Generated object files can only use features support by GNU ld of this + /

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

MaskRay added a comment. Ping... 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/cg

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

MaskRay updated this revision to Diff 285681. MaskRay edited the summary of this revision. MaskRay added a comment. Herald added a subscriber: kristof.beyls. Add release note Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85474/new/ https://reviews.

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

MaskRay added a comment. In D85474#2217598 , @jhenderson wrote: > 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 op

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

MaskRay marked an inline comment as done. MaskRay added inline comments. Comment at: clang/include/clang/Driver/Options.td:3307-3310 +def fbinutils_version_EQ : Joined<["-"], "fbinutils-version=">, Group, + HelpText<"Produced object files can use ELF features only supported by l

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

jhenderson added a comment. 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. Comment at: clang/include/clan

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

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4642 +int Num; +if (V == "future") + A->render(Args, CmdArgs); ro wrote: > MaskRay wrote: > > phosek wrote: > > > Another option would be `unstable`. > > I picked `futu

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

ro added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4642 +int Num; +if (V == "future") + A->render(Args, CmdArgs); MaskRay wrote: > phosek wrote: > > Another option would be `unstable`. > I picked `future` because there is an

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

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4642 +int Num; +if (V == "future") + A->render(Args, CmdArgs); phosek wrote: > Another option would be `unstable`. I picked `future` because there is an precedent: `-mcp

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

MaskRay updated this revision to Diff 284234. MaskRay added a comment. Add binutilsIsAtleast() Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85474/new/ https://reviews.llvm.org/D85474 Files: clang/include/clang/Basic/CodeGenOptions.h clang/inc

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

phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4642 +int Num; +if (V == "future") + A->render(Args, CmdArgs); Another option would be `unstable`. Comment at: llvm/lib/CodeGen/TargetLoweringObjectFil

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

MaskRay created this revision. MaskRay added reviewers: jyknight, pcc, phosek. Herald added subscribers: llvm-commits, cfe-commits, dang, steven.zhang, atanasyan, hiraditya, arichardson, sdardis. Herald added a reviewer: rengolin. Herald added projects: clang, LLVM. MaskRay requested review of thi