[Tooling] Use FixedCompilationDatabase when `compile_flags.txt` is found.

[sammccall]

Summary

This is an alternative to JSONCompilationDatabase for simple projects that
don't use a build system such as CMake.
(You can also drop one in ~, to make your tools use e.g. C++11 by default)

There's no facility for varying flags per-source-file or per-machine.
Possibly this could be accommodated backwards-compatibly using cpp, but even if
not the simplicity seems worthwhile for the cases that are addressed.

Tested with clangd, works great! (requires clangd restart)

[klimek]

include/clang/Tooling/CompilationDatabase.h

188 ↗

(On Diff #122081)

Perhaps loadFromTextFile?

lib/Tooling/CompilationDatabase.cpp

312 ↗

(On Diff #122081)

I assume we can't use ErrorOr as return value?

[sammccall]

include/clang/Tooling/CompilationDatabase.h

188 ↗

(On Diff #122081)

This mirrors JSONCompilationDatabase for consistency.
Happy to change this one, change both, or leave as-is. WDYT?

lib/Tooling/CompilationDatabase.cpp

312 ↗

(On Diff #122081)

Again this mirrors JSONCompilationDatabase. (And also CompilationDatabasePlugin, which is harder to change)
Change this one, change both, or leave as-is?

[ilya-biryukov]

I can see that FixedCompilationDatabase does not set a working directory. Is this something we may want to have for compile_flags.txt or one would need to resort to compile_commands.json to get this?
E.g., I'd find it useful to add includes with paths relative to compile_flags.txt by putting -Iproject-dep/include there.

lib/Tooling/CompilationDatabase.cpp

322 ↗

(On Diff #122081)

Maybe use llvm:make_unique?

[sammccall]

In D39799#919388, @ilya-biryukov wrote:

I can see that FixedCompilationDatabase does not set a working directory. Is this something we may want to have for compile_flags.txt or one would need to resort to compile_commands.json to get this?
E.g., I'd find it useful to add includes with paths relative to compile_flags.txt by putting -Iproject-dep/include there.

Currently the working directory is always the parent of compile_flags.txt as you suggest.
Maybe this isn't great though - sometimes it might be important that the WD is the parent of the source file?

[sammccall]

llvm::make_unique

[ilya-biryukov]

In D39799#919415, @sammccall wrote:

Currently the working directory is always the parent of compile_flags.txt as you suggest.
Maybe this isn't great though - sometimes it might be important that the WD is the parent of the source file?

Parent of the source file seems rather inconvenient most of the time. Even when reading the contents of compile_flags.txt one would have to consider it the contexts of every source file.

I'm looking at CompilationDatabase.cpp:321 and don't see the CompileCommand::Directory being set at all. Maybe I'm missing something.

A few ideas for tests:

1. Test relative include paths (-Irelpath/to/include) work.
2. What happens if both compile_commands.json and compile_flags.txt are present? Should we test and document it? I'd expect the first one to take priority and the second one to come into play only if the input file is not found in the first one.

[sammccall]

In D39799#919468, @ilya-biryukov wrote:

In D39799#919415, @sammccall wrote:

Currently the working directory is always the parent of compile_flags.txt as you suggest.
Maybe this isn't great though - sometimes it might be important that the WD is the parent of the source file?

Parent of the source file seems rather inconvenient most of the time. Even when reading the contents of compile_flags.txt one would have to consider it the contexts of every source file.

Understood, this is the best behavior I think. I'm just wondering whether it's a good enough heuristic to be silently applied.
e.g. IIUC, things like #include "sibling.h" won't look for the h file next to the cc file?

I'm looking at CompilationDatabase.cpp:321 and don't see the CompileCommand::Directory being set at all. Maybe I'm missing something.

Result is initialized with a copy of CompilerCommands, whose element is initialized with the directory passed to the constructor.

A few ideas for tests:

1. Test relative include paths (-Irelpath/to/include) work.

Definitely will do this.

2. What happens if both compile_commands.json and compile_flags.txt are present? Should we test and document it? I'd expect the first one to take priority and the second one to come into play only if the input file is not found in the first one.

Ideally yes... i'm not sure this is so easy with the registry mechanism though :-(
They'd have to be present at the same level, so I'm not sure this is a huge deal.

[ilya-biryukov]

In D39799#919494, @sammccall wrote:

e.g. IIUC, things like #include "sibling.h" won't look for the h file next to the cc file?

Actually, this should work as quoted includes are always searched relative to the current file before looking in the include paths. No matter what the WD is.

Result is initialized with a copy of CompilerCommands, whose element is initialized with the directory passed to the constructor.

Missed that, thanks!

Ideally yes... i'm not sure this is so easy with the registry mechanism though :-(
They'd have to be present at the same level, so I'm not sure this is a huge deal.

Yeah, looks like this would require unifying FixedCompilationDatabase and JSONCompilationDatabase. This doesn't seem too crazy, though. "A class that handles reading compilation arguments from the filesystem." seems like a valid entity to me.

[sammccall]

Add test verifying that working directory is compile_flags's parent.

[sammccall]

In D39799#919508, @ilya-biryukov wrote:

In D39799#919494, @sammccall wrote:

e.g. IIUC, things like #include "sibling.h" won't look for the h file next to the cc file?

Actually, this should work as quoted includes are always searched relative to the current file before looking in the include paths. No matter what the WD is.

Right, thanks :-)

Ideally yes... i'm not sure this is so easy with the registry mechanism though :-(
They'd have to be present at the same level, so I'm not sure this is a huge deal.

Yeah, looks like this would require unifying FixedCompilationDatabase and JSONCompilationDatabase. This doesn't seem too crazy, though. "A class that handles reading compilation arguments from the filesystem." seems like a valid entity to me.

It's doable, but then you're fighting the infrastructure here. The same ambiguity can exist between any pair of CompilationDatabasePlugins (try dumping a compile_commands.json in a google source tree). If we want to make this an error or have well-defined priority, that should be handled by autoDetectFromDirectory I think.

(Happy to do that if you think it's important - personally I'm not convinced it will be an issue in practice)

[ilya-biryukov]

In D39799#920195, @sammccall wrote:

It's doable, but then you're fighting the infrastructure here. The same ambiguity can exist between any pair of CompilationDatabasePlugins (try dumping a compile_commands.json in a google source tree). If we want to make this an error or have well-defined priority, that should be handled by autoDetectFromDirectory I think.

(Happy to do that if you think it's important - personally I'm not convinced it will be an issue in practice)

Yeah, I'm not sure if that's an issue either. Let's see whether anyone will be using both compile_flags.txt and compile_commands.json in the first place. We can always do it later.
Oh, and LGTM from my side!

[klimek]

lg

lib/Tooling/CompilationDatabase.cpp

312 ↗

(On Diff #122081)

I think we want to add a FIXME to change the interface.

[sammccall]

Add fixme to upgrade out-param APIs.

[JVApen]

How does this new file know if it should handle it's flags as it does in clang.exe or clang-cl.exe?

[sammccall]

In D39799#920780, @JVApen wrote:

How does this new file know if it should handle it's flags as it does in clang.exe or clang-cl.exe?

It doesn't set argv[0], so it's treated like clang by default.

I believe the flag --driver-mode={gcc,g++,cpp,cl} can be used to change this.
(I should add this to the brief doc)

cfe/trunk/docs/JSONCompilationDatabase.rst

 
 Build System Integration
 ========================
 
 The convention is to name the file compile\_commands.json and put it at
 the top of the build directory. Clang tools are pointed to the top of
 the build directory to detect the file and use the compilation database
 to parse C++ code in the source tree.
+
+Alternatives
+============
+For simple projects, Clang tools also recognize a compile_flags.txt file.
+This should contain one flag per line. The same flags will be used to compile
+any file.

cfe/trunk/include/clang/Tooling/CompilationDatabase.h

public:
   /// list.
   /// \param Argv Points to the command line arguments.
   /// \param ErrorMsg Contains error text if the function returns null pointer.
   /// \param Directory The base directory used in the FixedCompilationDatabase.
   static std::unique_ptr<FixedCompilationDatabase> loadFromCommandLine(
       int &Argc, const char *const *Argv, std::string &ErrorMsg,
       Twine Directory = ".");
 
+  /// Reads flags from the given file, one-per line.
+  /// Returns nullptr and sets ErrorMessage if we can't read the file.
+  static std::unique_ptr<FixedCompilationDatabase>
+  loadFromFile(StringRef Path, std::string &ErrorMsg);
+
   /// \brief Constructs a compilation data base from a specified directory
   /// and command line.
   FixedCompilationDatabase(Twine Directory, ArrayRef<std::string> CommandLine);
 
   /// \brief Returns the given compile command.
   ///
   /// Will always return a vector with one entry that contains the directory
   /// and command line specified at construction with "clang-tool" as argv[0]

cfe/trunk/lib/Tooling/CompilationDatabase.cpp

 //===--- CompilationDatabase.cpp - ----------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file contains implementations of the CompilationDatabase base class
 //  and the FixedCompilationDatabase.
 //
+//  FIXME: Various functions that take a string &ErrorMessage should be upgraded
+//  to Expected.
+//
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Driver/Action.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Job.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Tooling/CompilationDatabasePluginRegistry.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Support/Host.h"
+#include "llvm/Support/LineIterator.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <sstream>
 #include <system_error>
 using namespace clang;
 using namespace tooling;
 
 LLVM_INSTANTIATE_REGISTRY(CompilationDatabasePluginRegistry)
FixedCompilationDatabase::loadFromCommandLine(int &Argc,
   if (DoubleDash == Argv + Argc)
     return nullptr;
   std::vector<const char *> CommandLine(DoubleDash + 1, Argv + Argc);
   Argc = DoubleDash - Argv;
 
   std::vector<std::string> StrippedArgs;
   if (!stripPositionalArgs(CommandLine, StrippedArgs, ErrorMsg))
     return nullptr;
-  return std::unique_ptr<FixedCompilationDatabase>(
-      new FixedCompilationDatabase(Directory, StrippedArgs));
+  return llvm::make_unique<FixedCompilationDatabase>(Directory, StrippedArgs);
+}
+
+std::unique_ptr<FixedCompilationDatabase>
+FixedCompilationDatabase::loadFromFile(StringRef Path, std::string &ErrorMsg) {
+  ErrorMsg.clear();
+  llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> File =
+      llvm::MemoryBuffer::getFile(Path);
+  if (std::error_code Result = File.getError()) {
+    ErrorMsg = "Error while opening fixed database: " + Result.message();
+    return nullptr;
+  }
+  std::vector<std::string> Args{llvm::line_iterator(**File),
+                                llvm::line_iterator()};
+  return llvm::make_unique<FixedCompilationDatabase>(
+      llvm::sys::path::parent_path(Path), std::move(Args));
 }
 
 FixedCompilationDatabase::
 FixedCompilationDatabase(Twine Directory, ArrayRef<std::string> CommandLine) {
   std::vector<std::string> ToolCommandLine(1, "clang-tool");
   ToolCommandLine.insert(ToolCommandLine.end(),
                          CommandLine.begin(), CommandLine.end());
   CompileCommands.emplace_back(Directory, StringRef(),
FixedCompilationDatabase::getAllFiles() const {
   return std::vector<std::string>();
 }
 
 std::vector<CompileCommand>
 FixedCompilationDatabase::getAllCompileCommands() const {
   return std::vector<CompileCommand>();
 }
 
+namespace {
+
+class FixedCompilationDatabasePlugin : public CompilationDatabasePlugin {
+  std::unique_ptr<CompilationDatabase>
+  loadFromDirectory(StringRef Directory, std::string &ErrorMessage) override {
+    SmallString<1024> DatabasePath(Directory);
+    llvm::sys::path::append(DatabasePath, "compile_flags.txt");
+    return FixedCompilationDatabase::loadFromFile(DatabasePath, ErrorMessage);
+  }
+};
+
+static CompilationDatabasePluginRegistry::Add<FixedCompilationDatabasePlugin>
+X("fixed-compilation-database", "Reads plain-text flags file");
+
+} // namespace
+
 namespace clang {
 namespace tooling {
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the JSONCompilationDatabasePlugin.
 extern volatile int JSONAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED JSONAnchorDest = JSONAnchorSource;
 
 } // end namespace tooling
 } // end namespace clang

cfe/trunk/test/Tooling/Inputs/fixed-header.h

+#define SECRET_SYMBOL 1

cfe/trunk/test/Tooling/fixed-database.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t/Src
+// RUN: cp "%s" "%t/Src/test.cpp"
+// RUN: mkdir -p %t/Include
+// RUN: cp "%S/Inputs/fixed-header.h" "%t/Include/"
+// -I flag is relative to %t (where compile_flags is), not Src/.
+// RUN: echo '-IInclude/' >> %t/compile_flags.txt
+// RUN: echo "-Dklazz=class" >> %t/compile_flags.txt
+// RUN: echo '-std=c++11' >> %t/compile_flags.txt
+// RUN: clang-check "%t/Src/test.cpp" 2>&1
+// RUN: not clang-check "%s" 2>&1 | FileCheck "%s" -check-prefix=NODB
+
+// NODB: unknown type name 'klazz'
+klazz F{};
+
+// NODB: 'fixed-header.h' file not found
+#include "fixed-header.h"
+static_assert(SECRET_SYMBOL == 1, "");