[PATCH] D143763: [Clang] Add clangMinimumVersion to multilib.yaml

[Michael Platings via Phabricator via cfe-commits]

michaelplatings abandoned this revision.
michaelplatings marked 3 inline comments as done.
michaelplatings added a comment.

Squashed into D142932 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143763

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143763: [Clang] Add clangMinimumVersion to multilib.yaml

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

No objections to MaskRay's suggestion to merge with an earlier patch. I've made 
some suggestions to make the error messages be a bit more specific about what 
is wrong.




Comment at: clang/lib/Driver/Multilib.cpp:185
+if (M.ClangMinimumVersion.empty())
+  return "missing required key 'clangMinimumVersion'";
+

Perhaps
"missing required key 'clangMinimumVersion' expect MAJOR.MINOR.PATCHLEVEL" to 
give an idea what the key is expecting?



Comment at: clang/lib/Driver/Multilib.cpp:195
+if (MinVerStrings.size() != 3)
+  return "not a valid version string \"" + M.ClangMinimumVersion + "\"";
+

Can we add why the version string isn't valid? For example "expected format 
MAJOR.MINOR.PATCHLEVEL got " + M.CLangMinimumVersion ...





Comment at: clang/lib/Driver/Multilib.cpp:200
+  if (S.getAsInteger(10, V))
+return "not a valid version string \"" + M.ClangMinimumVersion + "\"";
+  ClangMinimumVersion.push_back(V);

Similarly, can we say what we were expecting. For example all components must 
be decimal integers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143763

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143763: [Clang] Add clangMinimumVersion to multilib.yaml

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

I think this patch can be merged into a previous one that introduces `variants` 
and `flagMap`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143763

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143763: [Clang] Add clangMinimumVersion to multilib.yaml

[Michael Platings via Phabricator via cfe-commits]

michaelplatings created this revision.
michaelplatings added a reviewer: peter.smith.
Herald added a subscriber: dmgreen.
Herald added a project: All.
michaelplatings requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

This avoids a potential source of subtle errors if an older version of
Clang were to be used with a multilib.yaml that requires a newer Clang
to work correctly.

This feature is comparable to CMake's cmake_minimum_required.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143763

Files:
  clang/lib/Driver/Multilib.cpp
  clang/unittests/Driver/MultilibTest.cpp

Index: clang/unittests/Driver/MultilibTest.cpp
===
--- clang/unittests/Driver/MultilibTest.cpp
+++ clang/unittests/Driver/MultilibTest.cpp
@@ -13,6 +13,7 @@
 #include "clang/Driver/Multilib.h"
 #include "../../lib/Driver/ToolChains/CommonArgs.h"
 #include "clang/Basic/LLVM.h"
+#include "clang/Basic/Version.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
@@ -197,20 +198,80 @@
   );
 }
 
+static bool parse(MultilibSet , std::string ,
+  const std::string ) {
+  return MS.parse(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
 static bool parse(MultilibSet , const char *Data) {
   return MS.parse(llvm::MemoryBufferRef(Data, "TEST"));
 }
 
+#define _STRINGIFY(x) #x
+#define STRINGIFY(x) _STRINGIFY(x)
+// Avoid using MULTILIB_CLANG_VERSION in case it has extra non-numeric parts.
+#define MULTILIB_CLANG_VERSION \
+  STRINGIFY(CLANG_VERSION_MAJOR)   \
+  "." STRINGIFY(CLANG_VERSION_MINOR) "." STRINGIFY(CLANG_VERSION_PATCHLEVEL)
+#define YAML_PREAMBLE "clangMinimumVersion: " MULTILIB_CLANG_VERSION "\n"
+
 TEST(MultilibTest, ParseInvalid) {
   std::string Diagnostic;
 
   MultilibSet MS;
 
-  EXPECT_FALSE(parse(MS, Diagnostic, "{}"));
-  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'variants'"))
+  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+variants: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic)
+  .contains("missing required key 'clangMinimumVersion'"))
   << Diagnostic;
 
+  // Require all 3 major.minor.patch version components
   EXPECT_FALSE(parse(MS, Diagnostic, R"(
+clangMinimumVersion: )" STRINGIFY(CLANG_VERSION_MAJOR) R"(.0
+variants: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic)
+  .contains("not a valid version string \"" STRINGIFY(
+  CLANG_VERSION_MAJOR) ".0\""))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+clangMinimumVersion: )" MULTILIB_CLANG_VERSION R"(a
+variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("not a valid version string \"" MULTILIB_CLANG_VERSION
+"a\""))
+  << Diagnostic;
+
+  // Reject configurations that require a later clang version
+  EXPECT_FALSE(parse(MS, Diagnostic,
+ R"(
+clangMinimumVersion: )" + std::to_string(CLANG_VERSION_MAJOR + 1) +
+ R"(.0.0
+variants: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic)
+  .contains("clang version " MULTILIB_CLANG_VERSION
+" is less than clangMinimumVersion: " +
+std::to_string(CLANG_VERSION_MAJOR + 1) + ".0.0"))
+  << Diagnostic;
+
+  // but accept configurations that only need an earlier clang version
+  EXPECT_TRUE(parse(MS, Diagnostic, R"(
+clangMinimumVersion: 16.0.0
+variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
 variants:
 - dir: /abc
   flags: []
@@ -219,7 +280,7 @@
   EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
   << Diagnostic;
 
-  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
 variants:
 - flags: []
   printArgs: []
@@ -227,7 +288,7 @@
   EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'dir'"))
   << Diagnostic;
 
-  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
 variants:
 - dir: .
   printArgs: []
@@ -235,7 +296,7 @@
   EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'flags'"))
   << Diagnostic;
 
-  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
 variants:
 - dir: .
   flags: []
@@ -244,7 +305,7 @@
   StringRef(Diagnostic).contains("missing required key 'printArgs'"))
   << Diagnostic;
 
-  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
 variants: []
 flagMap: