[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4794bdab7aed: [Driver] Multilib YAML parsing (authored by 
michaelplatings).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -187,3 +188,350 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const char *Data) {
+  auto ErrorOrMS = MultilibSet::parseYaml(llvm::MemoryBufferRef(Data, "TEST"),
+  diagnosticCallback, );
+  if (ErrorOrMS.getError())
+return false;
+  MS = std::move(ErrorOrMS.get());
+  return true;
+}
+
+static bool parseYaml(MultilibSet , const char *Data) {
+  auto ErrorOrMS = MultilibSet::parseYaml(llvm::MemoryBufferRef(Data, "TEST"));
+  if (ErrorOrMS.getError())
+return false;
+  MS = std::move(ErrorOrMS.get());
+  return true;
+}
+
+// When updating this version also update MultilibVersionCurrent in Multilib.cpp
+#define YAML_PREAMBLE "MultilibVersion: 1.0\n"
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'MultilibVersion'"))
+  << Diagnostic;
+
+  // Reject files with a different major version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 2.0
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 2.0 is unsupported"))
+  << Diagnostic;
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 0.1
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 0.1 is unsupported"))
+  << Diagnostic;
+
+  // Reject files with a later minor version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 1.9
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 1.9 is unsupported"))
+  << Diagnostic;
+
+  // Accept files with the same major version and the same or earlier minor
+  // version
+  EXPECT_TRUE(parseYaml(MS, Diagnostic, R"(
+MultilibVersion: 1.0
+Variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: /abc
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+Mappings:
+- Match: abc
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("value required for 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+Mappings:
+- Dir: .
+  Match: '('
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("parentheses not balanced"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: abc
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: pqr
+  Flags: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/pqr", MS.begin()->gccSuffix());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft"}),
+MS.begin()->flags());
+
+  EXPECT_TRUE(parseYaml(MS, 

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 529563.
michaelplatings added a comment.

FlagMap->Mappings


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -187,3 +188,350 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const char *Data) {
+  auto ErrorOrMS = MultilibSet::parseYaml(llvm::MemoryBufferRef(Data, "TEST"),
+  diagnosticCallback, );
+  if (ErrorOrMS.getError())
+return false;
+  MS = std::move(ErrorOrMS.get());
+  return true;
+}
+
+static bool parseYaml(MultilibSet , const char *Data) {
+  auto ErrorOrMS = MultilibSet::parseYaml(llvm::MemoryBufferRef(Data, "TEST"));
+  if (ErrorOrMS.getError())
+return false;
+  MS = std::move(ErrorOrMS.get());
+  return true;
+}
+
+// When updating this version also update MultilibVersionCurrent in Multilib.cpp
+#define YAML_PREAMBLE "MultilibVersion: 1.0\n"
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'MultilibVersion'"))
+  << Diagnostic;
+
+  // Reject files with a different major version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 2.0
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 2.0 is unsupported"))
+  << Diagnostic;
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 0.1
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 0.1 is unsupported"))
+  << Diagnostic;
+
+  // Reject files with a later minor version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 1.9
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 1.9 is unsupported"))
+  << Diagnostic;
+
+  // Accept files with the same major version and the same or earlier minor
+  // version
+  EXPECT_TRUE(parseYaml(MS, Diagnostic, R"(
+MultilibVersion: 1.0
+Variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: /abc
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+Mappings:
+- Match: abc
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("value required for 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+Mappings:
+- Dir: .
+  Match: '('
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("parentheses not balanced"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: abc
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: pqr
+  Flags: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/pqr", MS.begin()->gccSuffix());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft"}),
+MS.begin()->flags());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: pqr
+  Flags: [-mfloat-abi=soft, 

[PATCH] D142932: Multilib YAML parsing

[Petr Hosek via Phabricator via cfe-commits]

phosek accepted this revision.
phosek added a comment.

In D142932#4402981 , @michaelplatings 
wrote:

> @phosek thanks for your suggestion, that's now implemented. In practise for 
> LLVM Embedded Toolchain for Arm we haven't yet needed `NoMatchFlags` so I've 
> removed that feature.

Great, that simplifies things even further. The only other suggestion I have is 
to maybe rename `FlagMap` to `Mappings` for a better consistency with 
`Variants`, otherwise LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

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

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 529269.
michaelplatings added a comment.

@phosek thanks for your suggestion, that's now implemented. In practise for 
LLVM Embedded Toolchain for Arm we haven't yet needed `NoMatchFlags` so I've 
removed that feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -187,3 +188,350 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const char *Data) {
+  auto ErrorOrMS = MultilibSet::parseYaml(llvm::MemoryBufferRef(Data, "TEST"),
+  diagnosticCallback, );
+  if (ErrorOrMS.getError())
+return false;
+  MS = std::move(ErrorOrMS.get());
+  return true;
+}
+
+static bool parseYaml(MultilibSet , const char *Data) {
+  auto ErrorOrMS = MultilibSet::parseYaml(llvm::MemoryBufferRef(Data, "TEST"));
+  if (ErrorOrMS.getError())
+return false;
+  MS = std::move(ErrorOrMS.get());
+  return true;
+}
+
+// When updating this version also update MultilibVersionCurrent in Multilib.cpp
+#define YAML_PREAMBLE "MultilibVersion: 1.0\n"
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'MultilibVersion'"))
+  << Diagnostic;
+
+  // Reject files with a different major version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 2.0
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 2.0 is unsupported"))
+  << Diagnostic;
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 0.1
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 0.1 is unsupported"))
+  << Diagnostic;
+
+  // Reject files with a later minor version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 1.9
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 1.9 is unsupported"))
+  << Diagnostic;
+
+  // Accept files with the same major version and the same or earlier minor
+  // version
+  EXPECT_TRUE(parseYaml(MS, Diagnostic, R"(
+MultilibVersion: 1.0
+Variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: /abc
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Match: abc
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("value required for 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Dir: .
+  Match: '('
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("parentheses not balanced"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: abc
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: pqr
+  Flags: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/pqr", MS.begin()->gccSuffix());
+  

[PATCH] D142932: Multilib YAML parsing

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

The only remaining concern I have is about the YAML schema, specifically the 
naming of `Regex`, `MatchFlags` and `NoMatchFlags` fields. Specifically, I 
think it something like this might be cleaner:

  FlagMap:
  - Match: --target=thumbv8(\.[0-9]+)?m\.base-none-unknown-eabi
Flags: [--target=thumbv6m-none-unknown-eabi]

The fact that the value is a regular expression is implied by  the name of the 
field `Match` since the term "matching operation" is commonly used with regular 
expression . For the negative case 
we could use `NoMatch`.

I'm open to other ideas though, this is just a suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

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

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 528955.
michaelplatings marked an inline comment as done.
michaelplatings added a comment.

Update unit tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -187,3 +188,352 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const char *Data) {
+  auto ErrorOrMS = MultilibSet::parseYaml(llvm::MemoryBufferRef(Data, "TEST"),
+  diagnosticCallback, );
+  if (ErrorOrMS.getError())
+return false;
+  MS = std::move(ErrorOrMS.get());
+  return true;
+}
+
+static bool parseYaml(MultilibSet , const char *Data) {
+  auto ErrorOrMS = MultilibSet::parseYaml(llvm::MemoryBufferRef(Data, "TEST"));
+  if (ErrorOrMS.getError())
+return false;
+  MS = std::move(ErrorOrMS.get());
+  return true;
+}
+
+// When updating this version also update MultilibVersionCurrent in Multilib.cpp
+#define YAML_PREAMBLE "MultilibVersion: 1.0\n"
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'MultilibVersion'"))
+  << Diagnostic;
+
+  // Reject files with a different major version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 2.0
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 2.0 is unsupported"))
+  << Diagnostic;
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 0.1
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 0.1 is unsupported"))
+  << Diagnostic;
+
+  // Reject files with a later minor version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 1.9
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("multilib version 1.9 is unsupported"))
+  << Diagnostic;
+
+  // Accept files with the same major version and the same or earlier minor
+  // version
+  EXPECT_TRUE(parseYaml(MS, Diagnostic, R"(
+MultilibVersion: 1.0
+Variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: /abc
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Regex: abc
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("value required for 'MatchFlags' or 'NoMatchFlags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Dir: .
+  Regex: '('
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("parentheses not balanced"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: abc
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: pqr
+  Flags: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/pqr", MS.begin()->gccSuffix());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft"}),
+MS.begin()->flags());
+
+  

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings marked 2 inline comments as done.
michaelplatings added inline comments.



Comment at: clang/include/clang/Driver/Multilib.h:150-152
+  bool parseYaml(llvm::MemoryBufferRef,
+ llvm::SourceMgr::DiagHandlerTy = nullptr,
+ void *DiagHandlerCtxt = nullptr);

phosek wrote:
> Rather than `parseYaml` modifying the state of `MultilibSet`, can we instead 
> have a static factory method or a standalone function that parses, constructs 
> and returns `llvm::Expected`. That's more in line with how we 
> handle YAML deserialization elsewhere in LLVM, and also follows the notion of 
> `MultilibSet` being immutable.
Done, although I used ErrorOr instead of Expected to allow passing the error 
directly from the yaml::Input object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

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

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 528932.
michaelplatings added a comment.

Rebase on D151438 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -187,3 +188,352 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const char *Data) {
+  auto ErrorOrMS = MultilibSet::parseYaml(llvm::MemoryBufferRef(Data, "TEST"),
+  diagnosticCallback, );
+  if (ErrorOrMS.getError())
+return false;
+  MS = std::move(ErrorOrMS.get());
+  return true;
+}
+
+static bool parseYaml(MultilibSet , const char *Data) {
+  auto ErrorOrMS = MultilibSet::parseYaml(llvm::MemoryBufferRef(Data, "TEST"));
+  if (ErrorOrMS.getError())
+return false;
+  MS = std::move(ErrorOrMS.get());
+  return true;
+}
+
+// When updating this version also update MultilibVersionCurrent in Multilib.cpp
+#define YAML_PREAMBLE "MultilibVersion: 1.0\n"
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'MultilibVersion'"))
+  << Diagnostic;
+
+  // Reject files with a different major version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 2.0
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 2.0 is unsupported"))
+  << Diagnostic;
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 0.1
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 0.1 is unsupported"))
+  << Diagnostic;
+
+  // Reject files with a later minor version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 1.9
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 1.9 is unsupported"))
+  << Diagnostic;
+
+  // Accept files with the same major version and the same or earlier minor
+  // version
+  EXPECT_TRUE(parseYaml(MS, Diagnostic, R"(
+MultilibVersion: 1.0
+Variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: /abc
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Regex: abc
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("value required for 'MatchFlags' or 'NoMatchFlags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Dir: .
+  Regex: '('
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("parentheses not balanced"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: abc
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: pqr
+  Flags: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/pqr", MS.begin()->gccSuffix());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft"}),
+MS.begin()->flags());
+
+  

[PATCH] D142932: Multilib YAML parsing

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.



Comment at: clang/include/clang/Driver/Multilib.h:150-152
+  bool parseYaml(llvm::MemoryBufferRef,
+ llvm::SourceMgr::DiagHandlerTy = nullptr,
+ void *DiagHandlerCtxt = nullptr);

Rather than `parseYaml` modifying the state of `MultilibSet`, can we instead 
have a static factory method or a standalone function that parses, constructs 
and returns `llvm::Expected`. That's more in line with how we 
handle YAML deserialization elsewhere in LLVM, and also follows the notion of 
`MultilibSet` being immutable.



Comment at: clang/lib/Driver/Multilib.cpp:188
+if (StringRef(V.Dir).starts_with("/"))
+  return "paths must be relative. \"" + V.Dir + "\" starts with \"/\"\n";
+return std::string{};

This is just a nit, but the error messages in this file are inconsistent. 
Sometimes they are full sentences (capitalized, ending with a dot), sometimes 
they are not (not-capitalized, no dot at the end) and in this particular case 
it's a mixture (not-capitalized, ending with a dot, although it seems like dot 
here is only used as a separator). I don't have a strong preference for one 
style over the other, but I think we should pick one and be consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

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

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 518999.
michaelplatings added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -202,3 +203,353 @@
 Multilib({}, {}, {}, {"-y"}, Multilib::PrintOptionsType::Verbatim)
 .getPrintOptions());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const char *Data) {
+  return MS.parseYaml(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
+static bool parseYaml(MultilibSet , const char *Data) {
+  return MS.parseYaml(llvm::MemoryBufferRef(Data, "TEST"));
+}
+
+// When updating this version also update MultilibVersionCurrent in Multilib.cpp
+#define YAML_PREAMBLE "MultilibVersion: 1.0\n"
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'MultilibVersion'"))
+  << Diagnostic;
+
+  // Reject files with a different major version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 2.0
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 2.0 is unsupported"))
+  << Diagnostic;
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 0.1
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 0.1 is unsupported"))
+  << Diagnostic;
+
+  // Reject files with a later minor version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 1.9
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 1.9 is unsupported"))
+  << Diagnostic;
+
+  // Accept files with the same major version and the same or earlier minor
+  // version
+  EXPECT_TRUE(parseYaml(MS, Diagnostic, R"(
+MultilibVersion: 1.0
+Variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: /abc
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Regex: abc
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("value required for 'MatchFlags' or 'NoMatchFlags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Dir: .
+  Regex: '('
+  Flags: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("parentheses not balanced"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: abc
+  Flags: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: pqr
+  Flags: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/pqr", MS.begin()->gccSuffix());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft"}),
+MS.begin()->flags());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: pqr
+  Flags: 

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 508036.
michaelplatings added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -203,3 +204,392 @@
   Multilib({}, {}, {}, {"+x"}, Multilib::PrintOptionsType::List, {"-y"})
   .getPrintOptions());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const char *Data) {
+  return MS.parseYaml(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
+static bool parseYaml(MultilibSet , const char *Data) {
+  return MS.parseYaml(llvm::MemoryBufferRef(Data, "TEST"));
+}
+
+// When updating this version also update MultilibVersionCurrent in Multilib.cpp
+#define YAML_PREAMBLE "MultilibVersion: 1.0\n"
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'MultilibVersion'"))
+  << Diagnostic;
+
+  // Reject files with a different major version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 2.0
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 2.0 is unsupported"))
+  << Diagnostic;
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 0.1
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 0.1 is unsupported"))
+  << Diagnostic;
+
+  // Reject files with a later minor version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 1.9
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 1.9 is unsupported"))
+  << Diagnostic;
+
+  // Accept files with the same major version and the same or earlier minor
+  // version
+  EXPECT_TRUE(parseYaml(MS, Diagnostic, R"(
+MultilibVersion: 1.0
+Variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: /abc
+  Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'PrintOptions'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Regex: abc
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("value required for 'MatchFlags' or 'NoMatchFlags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Dir: .
+  Regex: '('
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("parentheses not balanced"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: abc
+  Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: pqr
+  Flags: []
+  PrintOptions: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/pqr", MS.begin()->gccSuffix());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft"}),
+ 

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 505283.
michaelplatings marked 2 inline comments as done.
michaelplatings added a comment.

Decouple multilib versioning scheme from the Clang version


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -187,3 +188,392 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const char *Data) {
+  return MS.parseYaml(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
+static bool parseYaml(MultilibSet , const char *Data) {
+  return MS.parseYaml(llvm::MemoryBufferRef(Data, "TEST"));
+}
+
+// When updating this version also update MultilibVersionCurrent in Multilib.cpp
+#define YAML_PREAMBLE "MultilibVersion: 1.0\n"
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'MultilibVersion'"))
+  << Diagnostic;
+
+  // Reject files with a different major version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 2.0
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 2.0 is unsupported"))
+  << Diagnostic;
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 0.1
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 0.1 is unsupported"))
+  << Diagnostic;
+
+  // Reject files with a later minor version
+  EXPECT_FALSE(parseYaml(MS, Diagnostic,
+ R"(
+MultilibVersion: 1.9
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("Multilib version 1.9 is unsupported"))
+  << Diagnostic;
+
+  // Accept files with the same major version and the same or earlier minor
+  // version
+  EXPECT_TRUE(parseYaml(MS, Diagnostic, R"(
+MultilibVersion: 1.0
+Variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: /abc
+  Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'PrintOptions'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Regex: abc
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("value required for 'MatchFlags' or 'NoMatchFlags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Dir: .
+  Regex: '('
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("parentheses not balanced"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: abc
+  Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parseYaml(MS, YAML_PREAMBLE R"(
+Variants:
+- Dir: pqr
+  Flags: []
+  PrintOptions: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 505047.
michaelplatings added a comment.

tags->flags


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -187,3 +188,409 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const char *Data) {
+  return MS.parseYaml(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const std::string ) {
+  return MS.parseYaml(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
+static bool parseYaml(MultilibSet , const char *Data) {
+  return MS.parseYaml(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(parseYaml(MS, Diagnostic, R"(
+Variants: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic)
+  .contains("missing required key 'ClangMinimumVersion'"))
+  << Diagnostic;
+
+  // Require all 3 major.minor.patch version components
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+ClangMinimumVersion: )" STRINGIFY(CLANG_VERSION_MAJOR) R"(.0
+Variants: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic)
+  .contains("not a valid version string. Expected "
+"MAJOR.MINOR.PATCHLEVEL but got \"" STRINGIFY(
+CLANG_VERSION_MAJOR) ".0\""))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+ClangMinimumVersion: )" MULTILIB_CLANG_VERSION R"(a
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("not a valid version string. Expected "
+"MAJOR.MINOR.PATCHLEVEL where all components are decimal "
+"integers but got \"" MULTILIB_CLANG_VERSION "a\""))
+  << Diagnostic;
+
+  // Reject configurations that require a later clang version
+  EXPECT_FALSE(parseYaml(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(parseYaml(MS, Diagnostic, R"(
+ClangMinimumVersion: 16.0.0
+Variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: /abc
+  Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'PrintOptions'"))
+  << Diagnostic;
+
+  

[PATCH] D142932: Multilib YAML parsing

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.



Comment at: clang/lib/Driver/Multilib.cpp:137
+struct MultilibSetSerialization {
+  std::string ClangMinimumVersion;
+  std::vector Multilibs;

Can we use a separate versioning scheme rather than tying the version to Clang? 
That is start at version 1.0 and bump it every time we make backwards 
incompatible change. That's the strategy we've for YAML formats used in other 
tools such as 
[llvm-ifs](https://llvm.org/docs/CommandGuide/llvm-ifs.html#ifs-formats).

The reason I think that would be preferable is that Clang will be at version 
//N.0.0// for ~9 months; we bump the version when we branch of the release, 
then spent 6 months developing and then typically another 3 months stabilizing 
the release branch. So for example, we bump the version to 16.0.0 in July 2022, 
and that version will get released in March 2023. However, during those 9 
months, we may want to make more than one breaking change to the format.

This may not be an issue for projects that only follow stable releases, but 
there's plenty of major projects that live at HEAD, such as Android, Chromium 
or Fuchsia, for which this could cause an issue. Using a dedicated versioning 
scheme allows us to decouple the format evolution from Clang releases.



Comment at: clang/lib/Driver/Multilib.cpp:137
+struct MultilibSetSerialization {
+  std::string ClangMinimumVersion;
+  std::vector Multilibs;

phosek wrote:
> Can we use a separate versioning scheme rather than tying the version to 
> Clang? That is start at version 1.0 and bump it every time we make backwards 
> incompatible change. That's the strategy we've for YAML formats used in other 
> tools such as 
> [llvm-ifs](https://llvm.org/docs/CommandGuide/llvm-ifs.html#ifs-formats).
> 
> The reason I think that would be preferable is that Clang will be at version 
> //N.0.0// for ~9 months; we bump the version when we branch of the release, 
> then spent 6 months developing and then typically another 3 months 
> stabilizing the release branch. So for example, we bump the version to 16.0.0 
> in July 2022, and that version will get released in March 2023. However, 
> during those 9 months, we may want to make more than one breaking change to 
> the format.
> 
> This may not be an issue for projects that only follow stable releases, but 
> there's plenty of major projects that live at HEAD, such as Android, Chromium 
> or Fuchsia, for which this could cause an issue. Using a dedicated versioning 
> scheme allows us to decouple the format evolution from Clang releases.
Use `llvm::VersionTuple` rather than `std::string` which is intended for this 
purpose.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

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

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added inline comments.



Comment at: clang/include/clang/Driver/Multilib.h:66
+  /// options and look similar to them, and others can be defined by a
+  /// particular multilib.yaml. A multilib is considered compatible if its tags
+  /// are a subset of the tags derived from the Clang command line options.

peter.smith wrote:
> With the context of https://discourse.llvm.org/t/rfc-multilib/67494 and 
> potential other formats. I think it will be worth making this comment not 
> specific to multilib.yaml. For example if there is another non-YAML DSL that 
> has tags.
> 
> For example `multilib DSL` rather than `multilib.yaml`  
If I were a newcomer to this code I think I'd find it easier to grep for 
multilib.yaml so on that basis I'd prefer to stick with the current wording. 
Should another file format be adopted then the comment could be updated at that 
point.



Comment at: clang/include/clang/Driver/Multilib.h:140
+
+  bool parse(llvm::MemoryBufferRef, llvm::SourceMgr::DiagHandlerTy = nullptr,
+ void *DiagHandlerCtxt = nullptr);

peter.smith wrote:
> With reference to https://discourse.llvm.org/t/rfc-multilib/67494 perhaps 
> rename this to `parseMultilibYaml` as parse is generic, yet there is no 
> indication it is working on a yaml file here.
Agreed. Renamed to `parseYaml`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

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

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 504771.
michaelplatings marked 2 inline comments as done.
michaelplatings added a comment.

parse -> parseYaml


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -187,3 +188,408 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const char *Data) {
+  return MS.parseYaml(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
+static bool parseYaml(MultilibSet , std::string ,
+  const std::string ) {
+  return MS.parseYaml(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
+static bool parseYaml(MultilibSet , const char *Data) {
+  return MS.parseYaml(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(parseYaml(MS, Diagnostic, R"(
+Variants: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic)
+  .contains("missing required key 'ClangMinimumVersion'"))
+  << Diagnostic;
+
+  // Require all 3 major.minor.patch version components
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+ClangMinimumVersion: )" STRINGIFY(CLANG_VERSION_MAJOR) R"(.0
+Variants: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic)
+  .contains("not a valid version string. Expected "
+"MAJOR.MINOR.PATCHLEVEL but got \"" STRINGIFY(
+CLANG_VERSION_MAJOR) ".0\""))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, R"(
+ClangMinimumVersion: )" MULTILIB_CLANG_VERSION R"(a
+Variants: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("not a valid version string. Expected "
+"MAJOR.MINOR.PATCHLEVEL where all components are decimal "
+"integers but got \"" MULTILIB_CLANG_VERSION "a\""))
+  << Diagnostic;
+
+  // Reject configurations that require a later clang version
+  EXPECT_FALSE(parseYaml(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(parseYaml(MS, Diagnostic, R"(
+ClangMinimumVersion: 16.0.0
+Variants: []
+)")) << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Variants'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: /abc
+  Tags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Tags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Tags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parseYaml(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Tags: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 

[PATCH] D142932: Multilib YAML parsing

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions. I've left some 
comments that can be applied prior to commit if you want to take them up.




Comment at: clang/include/clang/Driver/Multilib.h:66
+  /// options and look similar to them, and others can be defined by a
+  /// particular multilib.yaml. A multilib is considered compatible if its tags
+  /// are a subset of the tags derived from the Clang command line options.

With the context of https://discourse.llvm.org/t/rfc-multilib/67494 and 
potential other formats. I think it will be worth making this comment not 
specific to multilib.yaml. For example if there is another non-YAML DSL that 
has tags.

For example `multilib DSL` rather than `multilib.yaml`  



Comment at: clang/include/clang/Driver/Multilib.h:140
+
+  bool parse(llvm::MemoryBufferRef, llvm::SourceMgr::DiagHandlerTy = nullptr,
+ void *DiagHandlerCtxt = nullptr);

With reference to https://discourse.llvm.org/t/rfc-multilib/67494 perhaps 
rename this to `parseMultilibYaml` as parse is generic, yet there is no 
indication it is working on a yaml file here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

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

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 503900.
michaelplatings added a comment.

flags -> tags


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -187,3 +188,407 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parse(MultilibSet , std::string , const char *Data) {
+  return MS.parse(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
+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, 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. Expected "
+"MAJOR.MINOR.PATCHLEVEL but got \"" 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. Expected "
+"MAJOR.MINOR.PATCHLEVEL where all components are decimal "
+"integers but got \"" 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
+  Tags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Tags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Tags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Tags: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'PrintOptions'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+TagMap:
+- Regex: abc
+)"));
+  

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 499824.
michaelplatings added a comment.

PrintArgs -> PrintOptions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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"
@@ -187,3 +188,408 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parse(MultilibSet , std::string , const char *Data) {
+  return MS.parse(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
+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, 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. Expected "
+"MAJOR.MINOR.PATCHLEVEL but got \"" 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. Expected "
+"MAJOR.MINOR.PATCHLEVEL where all components are decimal "
+"integers but got \"" 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: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Flags: []
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  PrintOptions: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'Flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants:
+- Dir: .
+  Flags: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'PrintOptions'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, YAML_PREAMBLE R"(
+Variants: []
+FlagMap:
+- Regex: abc
+)"));
+  

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 494406.
michaelplatings added a comment.

Move the regular expression flag matching functionality into MutlilibSet. This 
(a) simplifies the code for loading from multilib.yaml; and (b) makes the flag 
matching functionality easier to access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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
@@ -187,3 +187,340 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parse(MultilibSet , std::string , const char *Data) {
+  return MS.parse(llvm::MemoryBufferRef(Data, "TEST"), diagnosticCallback,
+  );
+}
+
+static bool parse(MultilibSet , const char *Data) {
+  return MS.parse(llvm::MemoryBufferRef(Data, "TEST"));
+}
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+variants:
+- dir: /abc
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+variants:
+- flags: []
+  printArgs: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+variants:
+- dir: .
+  printArgs: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+variants:
+- dir: .
+  flags: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'printArgs'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+flagMap:
+- regex: abc
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("value required for 'matchFlags' or 'noMatchFlags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, Diagnostic, R"(
+flagMap:
+- dir: .
+  regex: '('
+  printArgs: []
+
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("parentheses not balanced"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  EXPECT_TRUE(parse(MS, R"(
+variants:
+- dir: .
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parse(MS, R"(
+variants:
+- dir: abc
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parse(MS, R"(
+variants:
+- dir: pqr
+  flags: []
+  printArgs: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/pqr", MS.begin()->gccSuffix());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft"}),
+MS.begin()->getPrintArgs());
+
+  EXPECT_TRUE(parse(MS, R"(
+variants:
+- dir: pqr
+  flags: []
+  printArgs: [-mfloat-abi=soft, -fno-exceptions]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft", "-fno-exceptions"}),
+MS.begin()->getPrintArgs());
+
+  EXPECT_TRUE(parse(MS, R"(
+variants:
+- dir: a
+  flags: []
+  printArgs: []
+- dir: b
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_EQ(2U, MS.size());
+}
+
+TEST(MultilibTest, SelectSoft) {
+  MultilibSet MS;
+  Multilib Selected;
+  ASSERT_TRUE(parse(MS, R"(
+variants:
+- dir: s
+  flags: [softabi]
+  printArgs: []
+flagMap:
+- regex: mfloat-abi=soft
+  matchFlags: [softabi]
+- regex: mfloat-abi=softfp
+  matchFlags: [softabi]
+)"));
+  EXPECT_TRUE(MS.select({"mfloat-abi=soft"}, Selected));
+  EXPECT_TRUE(MS.select({"mfloat-abi=softfp"}, Selected));
+  EXPECT_FALSE(MS.select({"mfloat-abi=hard"}, Selected));
+}
+
+TEST(MultilibTest, SelectSoftFP) {
+  MultilibSet MS;
+  Multilib Selected;
+  ASSERT_TRUE(parse(MS, R"(
+variants:
+- dir: f
+  flags: [mfloat-abi=softfp]
+  printArgs: []
+)"));
+  EXPECT_FALSE(MS.select({"mfloat-abi=soft"}, Selected));
+  EXPECT_TRUE(MS.select({"mfloat-abi=softfp"}, Selected));
+  EXPECT_FALSE(MS.select({"mfloat-abi=hard"}, Selected));
+}
+
+TEST(MultilibTest, SelectHard) {
+  // If hard float is all that's available then select that only if compiling
+  // with hard float.
+  MultilibSet MS;
+  Multilib Selected;
+  ASSERT_TRUE(parse(MS, R"(
+variants:
+- dir: h
+  flags: [mfloat-abi=hard]
+  printArgs: []
+)"));
+  EXPECT_FALSE(MS.select({"mfloat-abi=soft"}, Selected));
+  EXPECT_FALSE(MS.select({"mfloat-abi=softfp"}, Selected));
+  EXPECT_TRUE(MS.select({"mfloat-abi=hard"}, Selected));
+}
+
+TEST(MultilibTest, SelectFloatABI) {
+  MultilibSet MS;
+  Multilib 

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

michaelplatings updated this revision to Diff 493968.
michaelplatings added a comment.

Incorporated changes requested by @miyuki


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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
@@ -187,3 +187,357 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parse(MultilibSet , MultilibFlagMap ,
+  std::string , const char *Data) {
+  return MS.parse(llvm::MemoryBufferRef(Data, "TEST"), MFM, diagnosticCallback,
+  );
+}
+
+static bool parse(MultilibSet , MultilibFlagMap , const char *Data) {
+  return MS.parse(llvm::MemoryBufferRef(Data, "TEST"), MFM);
+}
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+  MultilibFlagMap MFM;
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+variants:
+- dir: /abc
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+variants:
+- flags: []
+  printArgs: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+variants:
+- dir: .
+  printArgs: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+variants:
+- dir: .
+  flags: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'printArgs'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+flagMap:
+- regex: abc
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("value required for 'matchFlags' or 'noMatchFlags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+flagMap:
+- dir: .
+  regex: '('
+  printArgs: []
+
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("parentheses not balanced"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  MultilibFlagMap MFM;
+  EXPECT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: .
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: abc
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: pqr
+  flags: []
+  printArgs: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/pqr", MS.begin()->gccSuffix());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft"}),
+MS.begin()->getPrintArgs());
+
+  EXPECT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: pqr
+  flags: []
+  printArgs: [-mfloat-abi=soft, -fno-exceptions]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft", "-fno-exceptions"}),
+MS.begin()->getPrintArgs());
+
+  EXPECT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: a
+  flags: []
+  printArgs: []
+- dir: b
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_EQ(2U, MS.size());
+}
+
+static bool select(const std::vector ,
+   const MultilibSet , const MultilibFlagMap ,
+   Multilib ) {
+  Multilib::flags_list Flags(MFM.getFlags(InFlags));
+  Flags.insert(InFlags.begin(), InFlags.end());
+  return MS.select(Flags, Selected);
+}
+
+TEST(MultilibTest, SelectSoft) {
+  MultilibSet MS;
+  MultilibFlagMap MFM;
+  Multilib Selected;
+  ASSERT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: s
+  flags: [softabi]
+  printArgs: []
+flagMap:
+- regex: mfloat-abi=soft
+  matchFlags: [softabi]
+- regex: mfloat-abi=softfp
+  matchFlags: [softabi]
+)"));
+  EXPECT_TRUE(select({"mfloat-abi=soft"}, MS, MFM, Selected));
+  EXPECT_TRUE(select({"mfloat-abi=softfp"}, MS, MFM, Selected));
+  EXPECT_FALSE(select({"mfloat-abi=hard"}, MS, MFM, Selected));
+}
+
+TEST(MultilibTest, SelectSoftFP) {
+  MultilibSet MS;
+  MultilibFlagMap MFM;
+  Multilib Selected;
+  ASSERT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: f
+  flags: [mfloat-abi=softfp]
+  printArgs: []
+)"));
+  EXPECT_FALSE(select({"mfloat-abi=soft"}, MS, MFM, Selected));
+  EXPECT_TRUE(select({"mfloat-abi=softfp"}, MS, MFM, Selected));
+  EXPECT_FALSE(select({"mfloat-abi=hard"}, MS, MFM, Selected));
+}
+
+TEST(MultilibTest, SelectHard) {
+  // If hard float is all that's available then select that only if compiling
+  // with hard float.
+  

[PATCH] D142932: Multilib YAML parsing

[Mikhail Maltsev via Phabricator via cfe-commits]

miyuki added inline comments.



Comment at: clang/lib/Driver/Multilib.cpp:83
+const std::regex Regex(M.Regex);
+if (std::find_if(InFlags.begin(), InFlags.end(),
+ [](const std::string ) {

Please use `llvm::find_if(InFlags, ...)`



Comment at: clang/lib/Driver/Multilib.cpp:153
+  static std::string validate(IO , MultilibFlagMap::Matcher ) {
+if (M.MatchFlags.empty() && M.NoMatchFlags.empty())
+  return "value required for 'matchFlags' or 'noMatchFlags'";

I think it would make sense to validate regular expression syntax as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

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

[PATCH] D142932: Multilib YAML parsing

[Mikhail Maltsev via Phabricator via cfe-commits]

miyuki added inline comments.



Comment at: clang/lib/Driver/Multilib.cpp:82
+  for (const Matcher  : Matchers) {
+const std::regex Regex(M.Regex);
+if (std::find_if(InFlags.begin(), InFlags.end(),

Please use the LLVM regular expression engine (llvm/Support/Regex.h) instead of 
std::regex. It's more portable, e.g., I've seen bugs and performance issues 
with MSVC implementation that don't manifest on Linux.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

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

[PATCH] D142932: Multilib YAML parsing

[Michael Platings via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142932

Files:
  clang/include/clang/Driver/Multilib.h
  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
@@ -187,3 +187,347 @@
   EXPECT_EQ("/a", Selection[0].gccSuffix());
   EXPECT_EQ("/b", Selection[1].gccSuffix());
 }
+
+static void diagnosticCallback(const llvm::SMDiagnostic , void *Out) {
+  *reinterpret_cast(Out) = D.getMessage();
+}
+
+static bool parse(MultilibSet , MultilibFlagMap ,
+  std::string , const char *Data) {
+  return MS.parse(llvm::MemoryBufferRef(Data, "TEST"), MFM, diagnosticCallback,
+  );
+}
+
+static bool parse(MultilibSet , MultilibFlagMap , const char *Data) {
+  return MS.parse(llvm::MemoryBufferRef(Data, "TEST"), MFM);
+}
+
+TEST(MultilibTest, ParseInvalid) {
+  std::string Diagnostic;
+
+  MultilibSet MS;
+  MultilibFlagMap MFM;
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+variants:
+- dir: /abc
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("paths must be relative"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+variants:
+- flags: []
+  printArgs: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'dir'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+variants:
+- dir: .
+  printArgs: []
+)"));
+  EXPECT_TRUE(StringRef(Diagnostic).contains("missing required key 'flags'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+variants:
+- dir: .
+  flags: []
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic).contains("missing required key 'printArgs'"))
+  << Diagnostic;
+
+  EXPECT_FALSE(parse(MS, MFM, Diagnostic, R"(
+flagMap:
+- regex: abc
+)"));
+  EXPECT_TRUE(
+  StringRef(Diagnostic)
+  .contains("value required for 'matchFlags' or 'noMatchFlags'"))
+  << Diagnostic;
+}
+
+TEST(MultilibTest, Parse) {
+  MultilibSet MS;
+  MultilibFlagMap MFM;
+  EXPECT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: .
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: abc
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/abc", MS.begin()->gccSuffix());
+
+  EXPECT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: pqr
+  flags: []
+  printArgs: [-mfloat-abi=soft]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ("/pqr", MS.begin()->gccSuffix());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft"}),
+MS.begin()->getPrintArgs());
+
+  EXPECT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: pqr
+  flags: []
+  printArgs: [-mfloat-abi=soft, -fno-exceptions]
+)"));
+  EXPECT_EQ(1U, MS.size());
+  EXPECT_EQ(std::vector({"-mfloat-abi=soft", "-fno-exceptions"}),
+MS.begin()->getPrintArgs());
+
+  EXPECT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: a
+  flags: []
+  printArgs: []
+- dir: b
+  flags: []
+  printArgs: []
+)"));
+  EXPECT_EQ(2U, MS.size());
+}
+
+static bool select(const std::vector ,
+   const MultilibSet , const MultilibFlagMap ,
+   Multilib ) {
+  Multilib::flags_list Flags(MFM.getFlags(InFlags));
+  Flags.insert(InFlags.begin(), InFlags.end());
+  return MS.select(Flags, Selected);
+}
+
+TEST(MultilibTest, SelectSoft) {
+  MultilibSet MS;
+  MultilibFlagMap MFM;
+  Multilib Selected;
+  ASSERT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: s
+  flags: [softabi]
+  printArgs: []
+flagMap:
+- regex: mfloat-abi=soft
+  matchFlags: [softabi]
+- regex: mfloat-abi=softfp
+  matchFlags: [softabi]
+)"));
+  EXPECT_TRUE(select({"mfloat-abi=soft"}, MS, MFM, Selected));
+  EXPECT_TRUE(select({"mfloat-abi=softfp"}, MS, MFM, Selected));
+  EXPECT_FALSE(select({"mfloat-abi=hard"}, MS, MFM, Selected));
+}
+
+TEST(MultilibTest, SelectSoftFP) {
+  MultilibSet MS;
+  MultilibFlagMap MFM;
+  Multilib Selected;
+  ASSERT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: f
+  flags: [mfloat-abi=softfp]
+  printArgs: []
+)"));
+  EXPECT_FALSE(select({"mfloat-abi=soft"}, MS, MFM, Selected));
+  EXPECT_TRUE(select({"mfloat-abi=softfp"}, MS, MFM, Selected));
+  EXPECT_FALSE(select({"mfloat-abi=hard"}, MS, MFM, Selected));
+}
+
+TEST(MultilibTest, SelectHard) {
+  // If hard float is all that's available then select that only if compiling
+  // with hard float.
+  MultilibSet MS;
+  MultilibFlagMap MFM;
+  Multilib Selected;
+  ASSERT_TRUE(parse(MS, MFM, R"(
+variants:
+- dir: h
+  flags: [mfloat-abi=hard]
+  printArgs: []
+)"));
+