hamzasood created this revision.
hamzasood added reviewers: rnk, hokein, sammccall.
Herald added a subscriber: cfe-commits.
This patch fixes the handling of clang-cl options in
`InterpolatingCompilationDatabase`.
They were previously ignored completely, which led to a lot of bugs:
- Additional options were being added with the wrong syntax. E.g. a file was
specified as C++ by adding `-x c++`, which causes an error in CL mode.
- The args were parsed and then rendered, which means that the aliasing
information was lost. E.g. `/W3` was rendered to `-Wall`, which in CL mode
means `-Weverything`.
- CL options were ignored when checking things like `-std=`, so a lot of logic
was being bypassed.
Repository:
rC Clang
https://reviews.llvm.org/D51321
Files:
lib/Tooling/InterpolatingCompilationDatabase.cpp
unittests/Tooling/CompilationDatabaseTest.cpp
Index: unittests/Tooling/CompilationDatabaseTest.cpp
===================================================================
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -644,12 +644,15 @@
}
};
-class InterpolateTest : public ::testing::Test {
+class InterpolateTest : public ::testing::TestWithParam<bool> {
protected:
+ bool isTestingCL() const { return GetParam(); }
+ StringRef getClangExe() const { return isTestingCL() ? "clang-cl" : "clang"; }
+
// Adds an entry to the underlying compilation database.
// A flag is injected: -D <File>, so the command used can be identified.
void add(llvm::StringRef File, llvm::StringRef Flags = "") {
- llvm::SmallVector<StringRef, 8> Argv = {"clang", File, "-D", File};
+ llvm::SmallVector<StringRef, 8> Argv = {getClangExe(), File, "-D", File};
llvm::SplitString(Flags, Argv);
llvm::SmallString<32> Dir;
llvm::sys::path::system_temp_directory(false, Dir);
@@ -675,67 +678,92 @@
->getCompileCommands(path(F));
if (Results.empty())
return "none";
- // drop the input file argument, so tests don't have to deal with path().
- EXPECT_EQ(Results[0].CommandLine.back(), path(F))
- << "Last arg should be the file";
- Results[0].CommandLine.pop_back();
- return llvm::join(Results[0].CommandLine, " ");
+
+ ArrayRef<std::string> CmdLine = Results[0].CommandLine;
+
+ // drop the clang path, so tests don't have to deal with getClangExe().
+ EXPECT_EQ(CmdLine.front(), getClangExe()) << "First arg should be clang";
+ CmdLine = CmdLine.drop_front();
+
+ // Drop the input file argument, so tests don't have to deal with path().
+ EXPECT_EQ(CmdLine.back(), path(F)) << "Last arg should be the file";
+ CmdLine = CmdLine.drop_back();
+
+ return llvm::join(CmdLine, " ");
}
MemCDB::EntryMap Entries;
};
-TEST_F(InterpolateTest, Nearby) {
+TEST_P(InterpolateTest, Nearby) {
add("dir/foo.cpp");
add("dir/bar.cpp");
add("an/other/foo.cpp");
// great: dir and name both match (prefix or full, case insensitive)
- EXPECT_EQ(getCommand("dir/f.cpp"), "clang -D dir/foo.cpp");
- EXPECT_EQ(getCommand("dir/FOO.cpp"), "clang -D dir/foo.cpp");
+ EXPECT_EQ(getCommand("dir/f.cpp"), "-D dir/foo.cpp");
+ EXPECT_EQ(getCommand("dir/FOO.cpp"), "-D dir/foo.cpp");
// no name match. prefer matching dir, break ties by alpha
- EXPECT_EQ(getCommand("dir/a.cpp"), "clang -D dir/bar.cpp");
+ EXPECT_EQ(getCommand("dir/a.cpp"), "-D dir/bar.cpp");
// an exact name match beats one segment of directory match
EXPECT_EQ(getCommand("some/other/bar.h"),
- "clang -D dir/bar.cpp -x c++-header");
+ isTestingCL()
+ ? "-D dir/bar.cpp /TP"
+ : "-D dir/bar.cpp -x c++-header");
// two segments of directory match beat a prefix name match
- EXPECT_EQ(getCommand("an/other/b.cpp"), "clang -D an/other/foo.cpp");
+ EXPECT_EQ(getCommand("an/other/b.cpp"), "-D an/other/foo.cpp");
// if nothing matches at all, we still get the closest alpha match
EXPECT_EQ(getCommand("below/some/obscure/path.cpp"),
- "clang -D an/other/foo.cpp");
+ "-D an/other/foo.cpp");
}
-TEST_F(InterpolateTest, Language) {
- add("dir/foo.cpp", "-std=c++17");
- add("dir/baz.cee", "-x c");
+TEST_P(InterpolateTest, Language) {
+ add("dir/foo.cpp", isTestingCL() ? "/std:c++17" : "-std=c++17");
+ add("dir/baz.cee", isTestingCL() ? "/TC" : "-x c");
// .h is ambiguous, so we add explicit language flags
EXPECT_EQ(getCommand("foo.h"),
- "clang -D dir/foo.cpp -x c++-header -std=c++17");
+ isTestingCL()
+ ? "-D dir/foo.cpp /TP /std:c++17"
+ : "-D dir/foo.cpp -x c++-header -std=c++17");
// and don't add -x if the inferred language is correct.
- EXPECT_EQ(getCommand("foo.hpp"), "clang -D dir/foo.cpp -std=c++17");
+ EXPECT_EQ(getCommand("foo.hpp"),
+ isTestingCL()
+ ? "-D dir/foo.cpp /std:c++17"
+ : "-D dir/foo.cpp -std=c++17");
// respect -x if it's already there.
- EXPECT_EQ(getCommand("baz.h"), "clang -D dir/baz.cee -x c-header");
+ EXPECT_EQ(getCommand("baz.h"),
+ isTestingCL()
+ ? "-D dir/baz.cee /TC"
+ : "-D dir/baz.cee -x c-header");
// prefer a worse match with the right language
- EXPECT_EQ(getCommand("foo.c"), "clang -D dir/baz.cee");
+ EXPECT_EQ(getCommand("foo.c"), "-D dir/baz.cee");
Entries.erase(path(StringRef("dir/baz.cee")));
// Now we transfer across languages, so drop -std too.
- EXPECT_EQ(getCommand("foo.c"), "clang -D dir/foo.cpp");
+ EXPECT_EQ(getCommand("foo.c"), "-D dir/foo.cpp");
}
-TEST_F(InterpolateTest, Strip) {
- add("dir/foo.cpp", "-o foo.o -Wall");
- // the -o option and the input file are removed, but -Wall is preserved.
- EXPECT_EQ(getCommand("dir/bar.cpp"), "clang -D dir/foo.cpp -Wall");
+TEST_P(InterpolateTest, Strip) {
+ // The input and output are removed, but the warning level is preserved.
+ if (isTestingCL()) {
+ add("dir/foo.cpp", "/Fofoo.o /W3");
+ EXPECT_EQ(getCommand("dir/bar.cpp"), "-D dir/foo.cpp /W3");
+ } else {
+ add("dir/foo.cpp", "-o foo.o -Wall");
+ EXPECT_EQ(getCommand("dir/bar.cpp"), "-D dir/foo.cpp -Wall");
+ }
}
-TEST_F(InterpolateTest, Case) {
+TEST_P(InterpolateTest, Case) {
add("FOO/BAR/BAZ/SHOUT.cc");
add("foo/bar/baz/quiet.cc");
// Case mismatches are completely ignored, so we choose the name match.
- EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D FOO/BAR/BAZ/SHOUT.cc");
+ EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "-D FOO/BAR/BAZ/SHOUT.cc");
}
+INSTANTIATE_TEST_CASE_P(RegularAndCL, InterpolateTest,
+ ::testing::Bool());
+
TEST(CompileCommandTest, EqualityOperator) {
CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
CompileCommand CCTest = CCRef;
Index: lib/Tooling/InterpolatingCompilationDatabase.cpp
===================================================================
--- lib/Tooling/InterpolatingCompilationDatabase.cpp
+++ lib/Tooling/InterpolatingCompilationDatabase.cpp
@@ -48,6 +48,7 @@
#include "clang/Frontend/LangStandard.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/Optional.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/Option/ArgList.h"
@@ -119,6 +120,11 @@
}
}
+// Determine whether the given file path is the clang-cl executable.
+static bool isCLExe(StringRef File) {
+ return llvm::sys::path::stem(File).endswith_lower("clang-cl");
+}
+
// A CompileCommand that can be applied to another file.
struct TransferableCommand {
// Flags that should not apply to all files are stripped from CommandLine.
@@ -127,47 +133,77 @@
types::ID Type = types::TY_INVALID;
// Standard specified by -std.
LangStandard::Kind Std = LangStandard::lang_unspecified;
+ // Whether the command is for the CL driver.
+ bool IsCLMode = false;
TransferableCommand(CompileCommand C)
- : Cmd(std::move(C)), Type(guessType(Cmd.Filename)) {
- std::vector<std::string> NewArgs = {Cmd.CommandLine.front()};
+ : Cmd(std::move(C)), Type(guessType(Cmd.Filename)),
+ IsCLMode(isCLExe(Cmd.CommandLine.front())) {
+ // Stash the old arguments.
+ std::vector<std::string> OldArgs = std::move(Cmd.CommandLine);
+ Cmd.CommandLine.clear();
+
+ // Transform the command line to an llvm ArgList.
+ // Make sure that OldArgs lives for at least as long as this variable as the
+ // arg list contains pointers to the OldArgs strings.
+ llvm::opt::InputArgList ArgList;
+ {
+ // Unfortunately InputArgList requires an array of c-strings whereas we
+ // have an array of std::string; we'll need an intermediate vector.
+ SmallVector<const char *, 8> TmpArgv;
+ TmpArgv.reserve(OldArgs.size());
+ std::transform(OldArgs.begin(), OldArgs.end(),
+ std::back_inserter(TmpArgv),
+ [](const std::string &S) { return S.c_str(); });
+ ArgList = llvm::opt::InputArgList(TmpArgv.begin(), TmpArgv.end());
+ }
+
// Parse the old args in order to strip out and record unwanted flags.
auto OptTable = clang::driver::createDriverOptTable();
- std::vector<const char *> Argv;
- for (unsigned I = 1; I < Cmd.CommandLine.size(); ++I)
- Argv.push_back(Cmd.CommandLine[I].c_str());
- unsigned MissingI, MissingC;
- auto ArgList = OptTable->ParseArgs(Argv, MissingI, MissingC);
- for (const auto *Arg : ArgList) {
- const auto &option = Arg->getOption();
- // Strip input and output files.
- if (option.matches(clang::driver::options::OPT_INPUT) ||
- option.matches(clang::driver::options::OPT_o)) {
- continue;
+ Cmd.CommandLine.emplace_back(OldArgs.front());
+ for (unsigned Start = 1, End = Start; End < OldArgs.size(); Start = End) {
+ std::unique_ptr<llvm::opt::Arg> Arg;
+ {
+ unsigned Included, Excluded;
+ if (IsCLMode) {
+ Included = driver::options::CoreOption | driver::options::CLOption;
+ Excluded = 0;
+ } else {
+ Included = 0;
+ Excluded = driver::options::CLOption;
+ }
+ Arg = std::unique_ptr<llvm::opt::Arg>(
+ OptTable->ParseOneArg(ArgList, End, Included, Excluded));
}
- // Strip -x, but record the overridden language.
- if (option.matches(clang::driver::options::OPT_x)) {
- for (const char *Value : Arg->getValues())
- Type = types::lookupTypeForTypeSpecifier(Value);
+
+ if (!Arg)
continue;
- }
- // Strip --std, but record the value.
- if (option.matches(clang::driver::options::OPT_std_EQ)) {
- for (const char *Value : Arg->getValues()) {
- Std = llvm::StringSwitch<LangStandard::Kind>(Value)
-#define LANGSTANDARD(id, name, lang, desc, features) \
- .Case(name, LangStandard::lang_##id)
-#define LANGSTANDARD_ALIAS(id, alias) .Case(alias, LangStandard::lang_##id)
-#include "clang/Frontend/LangStandards.def"
- .Default(Std);
+
+ if (Arg->getOption().matches(driver::options::OPT_driver_mode)) {
+ // Process --driver-mode.
+ IsCLMode = std::strcmp(Arg->getValue(), "cl") == 0;
+ } else {
+ // Strip input and output files.
+ if (isUntypedInputOrOutput(*Arg))
+ continue;
+
+ // Strip type specifiers, but record the overridden language.
+ if (const auto GivenType = isTypeSpecArg(*Arg)) {
+ Type = *GivenType;
+ continue;
+ }
+
+ // Strip std, but record the value.
+ if (const auto GivenStd = isStdArg(*Arg)) {
+ if (*GivenStd != LangStandard::lang_unspecified)
+ Std = *GivenStd;
+ continue;
}
- continue;
}
- llvm::opt::ArgStringList ArgStrs;
- Arg->render(ArgList, ArgStrs);
- NewArgs.insert(NewArgs.end(), ArgStrs.begin(), ArgStrs.end());
+
+ Cmd.CommandLine.insert(Cmd.CommandLine.end(),
+ &OldArgs[Start], &OldArgs[End]);
}
- Cmd.CommandLine = std::move(NewArgs);
if (Std != LangStandard::lang_unspecified) // -std take precedence over -x
Type = toType(LangStandard::getLangStandardForKind(Std).getLanguage());
@@ -180,19 +216,26 @@
Result.Filename = Filename;
bool TypeCertain;
auto TargetType = guessType(Filename, &TypeCertain);
- // If the filename doesn't determine the language (.h), transfer with -x.
+ // If the filename doesn't determine the language (.h), use an appropriate
+ // argument to transfer it.
if (!TypeCertain) {
TargetType = types::onlyPrecompileType(TargetType) // header?
? types::lookupHeaderTypeForSourceType(Type)
: Type;
- Result.CommandLine.push_back("-x");
- Result.CommandLine.push_back(types::getTypeName(TargetType));
+ if (IsCLMode) {
+ const StringRef Flag = toCLFlag(TargetType);
+ if (!Flag.empty())
+ Result.CommandLine.push_back(Flag);
+ } else {
+ Result.CommandLine.push_back("-x");
+ Result.CommandLine.push_back(types::getTypeName(TargetType));
+ }
}
// --std flag may only be transferred if the language is the same.
// We may consider "translating" these, e.g. c++11 -> c11.
if (Std != LangStandard::lang_unspecified && foldType(TargetType) == Type) {
Result.CommandLine.push_back(
- "-std=" +
+ (IsCLMode ? "/std:" : "-std=") +
std::string(LangStandard::getLangStandardForKind(Std).getName()));
}
Result.CommandLine.push_back(Filename);
@@ -200,6 +243,54 @@
}
private:
+ bool isUntypedInputOrOutput(const llvm::opt::Arg &Arg) const {
+ const llvm::opt::Option &Opt = Arg.getOption();
+
+ if (Opt.matches(driver::options::OPT_INPUT))
+ return true;
+
+ if (IsCLMode)
+ return Opt.matches(driver::options::OPT__SLASH_Fa) ||
+ Opt.matches(driver::options::OPT__SLASH_Fe) ||
+ Opt.matches(driver::options::OPT__SLASH_Fi) ||
+ Opt.matches(driver::options::OPT__SLASH_Fo);
+ else
+ return Opt.matches(driver::options::OPT_o);
+ }
+
+ Optional<types::ID> isTypeSpecArg(const llvm::opt::Arg &Arg) const {
+ const llvm::opt::Option &Opt = Arg.getOption();
+ if (IsCLMode) {
+ if (Opt.matches(driver::options::OPT__SLASH_TC) ||
+ Opt.matches(driver::options::OPT__SLASH_Tc))
+ return types::TY_C;
+ if (Opt.matches(driver::options::OPT__SLASH_TP) ||
+ Opt.matches(driver::options::OPT__SLASH_Tp))
+ return types::TY_CXX;
+ } else {
+ if (Opt.matches(driver::options::OPT_x))
+ return types::lookupTypeForTypeSpecifier(Arg.getValue());
+ }
+ return None;
+ }
+
+ Optional<LangStandard::Kind> isStdArg(const llvm::opt::Arg &Arg) const {
+ const llvm::opt::Option &Opt = Arg.getOption();
+ if (Opt.matches(IsCLMode ? driver::options::OPT__SLASH_std
+ : driver::options::OPT_std_EQ)) {
+ return llvm::StringSwitch<LangStandard::Kind>(Arg.getValue())
+#define LANGSTANDARD(id, name, lang, desc, features) \
+ .Case(name, LangStandard::lang_##id)
+#define LANGSTANDARD_ALIAS(id, alias) \
+ .Case(alias, LangStandard::lang_##id)
+#include "clang/Frontend/LangStandards.def"
+#undef LANGSTANDARD_ALIAS
+#undef LANGSTANDARD
+ .Default(LangStandard::lang_unspecified);
+ }
+ return None;
+ }
+
// Map the language from the --std flag to that of the -x flag.
static types::ID toType(InputKind::Language Lang) {
switch (Lang) {
@@ -215,6 +306,20 @@
return types::TY_INVALID;
}
}
+
+ static StringRef toCLFlag(types::ID Type) {
+ types::isCXX(Type);
+ switch (Type) {
+ case types::TY_C:
+ case types::TY_CHeader:
+ return "/TC";
+ case types::TY_CXX:
+ case types::TY_CXXHeader:
+ return "/TP";
+ default:
+ return StringRef();
+ }
+ }
};
// CommandIndex does the real work: given a filename, it produces the best
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
