[PATCH] D93164: [AST] Add generator for source location introspection

Tue Dec 15 14:04:26 PST 2020

steveire added a comment.

> Do I understand correctly that the workflow is to use the new dumping tool to generate the needed JSON file that then gets used as input to generate_cxx_src_locs.py which creates NodeLocationIntrospection.cpp/.h that then gets used by clang-query (eventually)?

Yes, that's right. I've added the patch for the latter now at D93325 <https://reviews.llvm.org/D93325>.

> So there are two levels of translation involved to get the final source code?

There reason for the separation of the generation of JSON from the generation of C++ (using the JSON) is that the JSON can also be used to generate bindings for other languages. In https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/ I demonstrated that by generating and using Javascript bindings, but I've also proven the concept with Python3 bindings and added a clang-tidy module allowing clang-tidy checks to be written in Python. That resolves https://bugs.llvm.org//show_bug.cgi?id=32739 at least partially (there may be things which can still only be done in C++).

> If so, do you know what the performance/overhead for this looks like compared to a typical build? I'm trying to get an idea for whether this will have negative impacts on the build bots such that we may want to add an LLVM cmake configure option to control whether this work happens or not.

In this patch, the generation is already disabled for Debug builds because the JSON generation is slow in Debug builds (I think we discussed that in Belfast). The reason it is slow is that compiling `AST/AST.h` with a debug-build seems to be slow. The slowness is not caused by the AST matching, but seems to be spent parsing.

With a release build, generating the JSON takes 3 seconds on my laptop. Plenty of compilations in the llvm build take longer. Generating the c++ files with `generate_cxx_src_locs.py` takes `0.04` seconds.

I'm not opposed to adding a condition in cmake for it, in case someone wants to build it even in Debug mode.

================
Comment at: clang/lib/Tooling/DumpTool/APIData.h:21
+
+  bool IsEmpty() const { return Locs.empty() && Rngs.empty(); }
+
----------------
aaron.ballman wrote:
> per the usual naming rules.
I changed it, but are you referring to this proposal? https://llvm.org/docs/Proposals/VariableNames.html

================
Comment at: clang/lib/Tooling/DumpTool/APIData.h:25
+  std::vector<std::string> Rngs;
+  // TODO: Extend this with locations available via typelocs etc.
+};
----------------
aaron.ballman wrote:
> Are these extensions going to add new members for those? If so, perhaps `Locs` and `Rngs` should have more descriptive names initially?
Just for an idea, in a follow-up I have 
```

TypeSourceInfos
TypeLocs
TemplateArgumentLocs
DeclNames
NestedNames
ConstCharStarMethods
StringRefMethods
StdStringMethods
```
We can review those names in the follow-up. 

================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:43
+
+static cl::opt<std::string> JsonOutputPath("json-output-path",
+                                           cl::desc("json output path"),
----------------
aaron.ballman wrote:
> Hmmm, do we want such a long name for this option? I was guessing that `-o` isn't appropriate because there may be multiple files output and so this isn't naming the output file but the output directory, but `json-output-path` is a mouthful for setting the output location. Or do you think users won't be using that option often enough for the length to matter?
Yes, this tool will only be called from the buildsystem. The intention is also that it doesn't generate any other files either. The JSON file should have all the content needed for clang-query `srcloc`, bindings defined and used in the llvm repo etc and any other use we find for this. If we want to enable third parties to do similar things, we would install the JSON file, not this tool.

================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:105
+  auto Driver = std::make_unique<driver::Driver>(
+      "clang++", llvm::sys::getDefaultTargetTriple(), Diagnostics,
+      "ast-api-dump-tool", &Files.getVirtualFileSystem());
----------------
aaron.ballman wrote:
> Do you think we may need the ability for the user to pass other options to this driver invocation for things like `-fms-compatibility` or whatnot?
I don't think the user needs to have that ability, no. If there's any platform-specific options needed, they would be added directly here.

================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:108
+
+  const auto Compilation(Driver->BuildCompilation(llvm::makeArrayRef(Argv)));
+  if (!Compilation)
----------------
aaron.ballman wrote:
> 
`error: ‘Compilation’ was not declared in this scope; did you mean ‘clang::driver::Compilation’?` Is this really necessary?

================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:112
+
+  const auto &Jobs = Compilation->getJobs();
+  if (Jobs.size() != 1 || !isa<driver::Command>(*Jobs.begin())) {
----------------
aaron.ballman wrote:
> 
` error: ‘JobList’ does not name a type` I don't get a suggestion from the compiler. Is this really necessary?

================
Comment at: clang/lib/Tooling/DumpTool/generate_cxx_src_locs.py:76-86
+    if (LHS.first.getBegin() < RHS.first.getBegin())
+      return true;
+    else if (LHS.first.getBegin() != RHS.first.getBegin())
+      return false;
+
+    if (LHS.first.getEnd() < RHS.first.getEnd())
+      return true;
----------------
aaron.ballman wrote:
> Would this be a more clear equivalent?
```
tools/clang/include/clang/Tooling/NodeLocationIntrospection.h: In member function ‘bool clang::tooling::internal::RangeLessThan::operator()(const std::pair<clang::SourceRange, std::__cxx11::basic_string<char> >&, const std::pair<clang::SourceRange, std::__cxx11::basic_string<char> >&) const’:
tools/clang/include/clang/Tooling/NodeLocationIntrospection.h:31:21: error: cannot bind non-const lvalue reference of type ‘clang::SourceLocation&’ to an rvalue of type ‘clang::SourceLocation’
   31 |     return std::tie(SourceLocation(LHS.first.getBegin()), LHS.first.getEnd(),
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/9/bits/unique_ptr.h:37,
                 from /usr/include/c++/9/memory:80,
                 from llvm-project/llvm/include/llvm/Support/Casting.h:20,
                 from llvm-project/clang/include/clang/Basic/LLVM.h:21,
                 from llvm-project/clang/include/clang/Basic/DiagnosticIDs.h:17,
                 from llvm-project/clang/include/clang/Basic/Diagnostic.h:17,
                 from llvm-project/clang/include/clang/AST/NestedNameSpecifier.h:18,
                 from llvm-project/clang/include/clang/AST/Type.h:21,
                 from llvm-project/clang/include/clang/AST/CanonicalType.h:17,
                 from llvm-project/clang/include/clang/AST/ASTContext.h:19,
                 from llvm-project/clang/include/clang/AST/AST.h:17,
                 from tools/clang/lib/Tooling/generated/NodeLocationIntrospection.cpp:10:
/usr/include/c++/9/tuple:1611:19: note:   initializing argument 1 of ‘constexpr std::tuple<_Elements& ...> std::tie(_Elements& ...) [with _Elements = {clang::SourceLocation, clang::SourceLocation, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >}]’
 1611 |     tie(_Elements&... __args) noexcept
      |         ~~~~~~~~~~^~~~~~~~~~

```
I wasn't able to make it work.

================
Comment at: clang/unittests/Introspection/IntrospectionTest.cpp:14
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
----------------
aaron.ballman wrote:
> Might as well fix this lint warning.
Funny, I thought clang-format would fix these things.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93164