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

[James Henderson via Phabricator via llvm-commits]

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.



================
Comment at: clang/test/Driver/fbinutils-version.c:1
+// RUN: %clang -### -c -target x86_64-linux %s -fbinutils-version=none 2>&1 | FileCheck %s --check-prefix=NONE
+
----------------
Maybe also add a case for `-fbinutils-version=2` (i.e. no minor version at all).


================
Comment at: llvm/lib/CodeGen/LLVMTargetMachine.cpp:64
 
+  if (Options.BinutilsVersion.first > 0)
+    TmpAsmInfo->setBinutilsVersion(Options.BinutilsVersion);
----------------
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?


================
Comment at: llvm/lib/Target/TargetMachine.cpp:239
+  if (Version == "none")
+    return {INT_MAX, 0}; // Make binutilsIsAtLeast() return true.
+  std::pair<int, int> Ret;
----------------
Maybe `{INT_MAX, INT_MAX}`? Doesn't really matter in practice, but `{INT_MAX, INT_MAX} > {INT_MAX, 0}` in the versioning scheme.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85474