[pseudo] Eliminate dependencies from clang-pseudo-gen. NFC

[sammccall]

Summary

ClangBasic dependency eliminated by replacing our usage of
tok::getPunctuatorSpelling etc with direct use of the *.def file.

Implicit dependencies on clang-tablegen-targets removed as we manage to avoid
any transitive tablegen deps.

After these changes, ninja clean; ninja pseudo-gen runs 169 actions only
(basically Support and Demangle).

[hokein]

Nice.

ninja clean; ninja pseudo-gen runs 169 actions only

I think previous version was ~400 actions.

clang-tools-extra/pseudo/lib/grammar/Grammar.cpp

10

Add a trailing comment // for clang::tok::TokenKind?

Maybe even remove this include -- we have an include in the Grammar.h, though it makes this file less IWYU-friendly).

[sammccall]

clang-tools-extra/pseudo/lib/grammar/Grammar.cpp

10

I'm not convinced about commenting particular includes (we include it in other places too, and these comments tend to get stale), and especially not dropping them in favor of transitive ones. Can you explain what this might help with?

(will land this but happy to follow up)

[fmayer]

FWIW this is not really NFC, I found this as the culprit in bisecting the Android LLVM toolchain build, causing the following error:

FAILED: tools/clang/tools/extra/pseudo/include/CXXBNF.inc /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include/CXXBNF.inc 
cd /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include && /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/bin/pseudo-gen --grammar /usr/local/google/home/fmayer/llvm-toolchain/out/llvm-project/clang-tools-extra/pseudo/include/../lib/cxx.bnf --emit-grammar-content -o /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include/CXXBNF.inc
/usr/local/google/home/fmayer/llvm-toolchain/out/stage2/bin/pseudo-gen: error while loading shared libraries: libc++.so.1: cannot open shared object file: No such file or directory
[2437/6345] Generating nonterminal symbol file for cxx grammar...
FAILED: tools/clang/tools/extra/pseudo/include/CXXSymbols.inc /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include/CXXSymbols.inc 
cd /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include && /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/bin/pseudo-gen --grammar /usr/local/google/home/fmayer/llvm-toolchain/out/llvm-project/clang-tools-extra/pseudo/include/../lib/cxx.bnf --emit-symbol-list -o /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include/CXXSymbols.inc
/usr/local/google/home/fmayer/llvm-toolchain/out/stage2/bin/pseudo-gen: error while loading shared libraries: libc++.so.1: cannot open shared object file: No such file or directory

[sammccall]

In D126731#3609511, @fmayer wrote:

FWIW this is not really NFC, I found this as the culprit in bisecting the Android LLVM toolchain build, causing the following error:

FAILED: tools/clang/tools/extra/pseudo/include/CXXBNF.inc /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include/CXXBNF.inc 
cd /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include && /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/bin/pseudo-gen --grammar /usr/local/google/home/fmayer/llvm-toolchain/out/llvm-project/clang-tools-extra/pseudo/include/../lib/cxx.bnf --emit-grammar-content -o /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include/CXXBNF.inc
/usr/local/google/home/fmayer/llvm-toolchain/out/stage2/bin/pseudo-gen: error while loading shared libraries: libc++.so.1: cannot open shared object file: No such file or directory
[2437/6345] Generating nonterminal symbol file for cxx grammar...
FAILED: tools/clang/tools/extra/pseudo/include/CXXSymbols.inc /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include/CXXSymbols.inc 
cd /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include && /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/bin/pseudo-gen --grammar /usr/local/google/home/fmayer/llvm-toolchain/out/llvm-project/clang-tools-extra/pseudo/include/../lib/cxx.bnf --emit-symbol-list -o /usr/local/google/home/fmayer/llvm-toolchain/out/stage2/tools/clang/tools/extra/pseudo/include/CXXSymbols.inc
/usr/local/google/home/fmayer/llvm-toolchain/out/stage2/bin/pseudo-gen: error while loading shared libraries: libc++.so.1: cannot open shared object file: No such file or directory

Sorry about that. I suspect it was working accidentally before (neither depending on clangBasic nor anything tablegen related is the right way for us to resolve a standard library dependency).
In any case it's not a *functional* change.

Do you want help resolving this? Can you provide more details about the setup?
My guess is that the build is specifying libc++ somehow but it's neither installed systemwide (ld.so.cache) nor is LD_LIBRARY_PATH set.

[vsapsai]

Looks like this breaks a modular build. I.e.,

cmake -GNinja ~/Projects/llvm/llvm-project/llvm \
  -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" \
  -DLLVM_ENABLE_ASSERTIONS=ON \
  -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DLLVM_ENABLE_MODULES=ON
ninja clangPseudoGrammar

fails with

While building module 'Clang_Basic' imported from /Users/vsapsai/Projects/llvm/llvm-project/clang-tools-extra/pseudo/include/clang-pseudo/grammar/Grammar.h:55:
In file included from <module-includes>:4:
/Users/vsapsai/Projects/llvm/llvm-project/clang/include/clang/Basic/AttrKinds.h:27:10: fatal error: 'clang/Basic/AttrList.inc' file not found
#include "clang/Basic/AttrList.inc"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~
While building module 'Clang_Basic' imported from /Users/vsapsai/Projects/llvm/llvm-project/clang-tools-extra/pseudo/include/clang-pseudo/grammar/Grammar.h:55:
While building module 'LLVM_Frontend_OpenMP' imported from /Users/vsapsai/Projects/llvm/llvm-project/clang/include/clang/Basic/TargetInfo.h:34:
In file included from <module-includes>:2:
/Users/vsapsai/Projects/llvm/llvm-project/llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:20:10: fatal error: 'llvm/Frontend/OpenMP/OMP.h.inc' file not found
#include "llvm/Frontend/OpenMP/OMP.h.inc"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/vsapsai/Projects/llvm/llvm-project/clang-tools-extra/pseudo/lib/grammar/LRGraph.cpp:9:
In file included from /Users/vsapsai/Projects/llvm/llvm-project/clang-tools-extra/pseudo/include/clang-pseudo/grammar/LRGraph.h:34:
/Users/vsapsai/Projects/llvm/llvm-project/clang-tools-extra/pseudo/include/clang-pseudo/grammar/Grammar.h:55:10: fatal error: could not build module 'Clang_Basic'
#include "clang/Basic/TokenKinds.h"
 ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.

My understanding is that the module 'Clang_Basic' needs .inc headers and this module is pulled in by #include "clang/Basic/TokenKinds.h" at "clang-tools-extra/pseudo/include/clang-pseudo/grammar/Grammar.h:55". When those .inc headers aren't generated, the build fails. Don't know what would be the best way to fix the issue, I was able to fix the build by commenting out list(REMOVE_ITEM LLVM_COMMON_DEPENDS clang-tablegen-targets) in "lib/grammar/CMakeLists.txt".

[sammccall]

In D126731#3670265, @vsapsai wrote:

Looks like this breaks a modular build. I.e.,

My understanding is that the module 'Clang_Basic' needs .inc headers and this module is pulled in by #include "clang/Basic/TokenKinds.h" at "clang-tools-extra/pseudo/include/clang-pseudo/grammar/Grammar.h:55". When those .inc headers aren't generated, the build fails. Don't know what would be the best way to fix the issue, I was able to fix the build by commenting out list(REMOVE_ITEM LLVM_COMMON_DEPENDS clang-tablegen-targets) in "lib/grammar/CMakeLists.txt".

Hmm, I also don't know.
The idea here is that we specifically depend only on the TokenKind enum from TokenKinds.h (which doesn't need any generated headers), not on other headers from clang/include/Basic (which might), and not on building/linking clangBasic itself.
In a modules world, maybe that means we need TokenKinds.h to be its own module, or a non-modular header, or something?

I'm also not sure on the support status of the modules build: I can't find a buildbot covering it or docs. Is this something I need to fix, or is it an experimental build like GN/Bazel?

[vsapsai]

In D126731#3670896, @sammccall wrote:

Hmm, I also don't know.
The idea here is that we specifically depend only on the TokenKind enum from TokenKinds.h (which doesn't need any generated headers), not on other headers from clang/include/Basic (which might), and not on building/linking clangBasic itself.
In a modules world, maybe that means we need TokenKinds.h to be its own module, or a non-modular header, or something?

I went with the approach of putting TokenKinds.h (and TokenKinds.def) into its own module D130377.

In D126731#3670896, @sammccall wrote:

I'm also not sure on the support status of the modules build: I can't find a buildbot covering it or docs. Is this something I need to fix, or is it an experimental build like GN/Bazel?

I know at least about https://green.lab.llvm.org/green/job/lldb-cmake/ building with modules. I agree it's not a common configuration on buildbots but it's not experimental like GN/Bazel. As for the fixing the issue, it is covered by https://llvm.org/docs/DeveloperPolicy.html#quality , i.e., it is the author's responsibility but the reporter is responsible for making sure the author is equipped to do so (or fix themselves if they cannot help the author).

[sammccall]

In D126731#3672666, @vsapsai wrote:

In D126731#3670896, @sammccall wrote:

Hmm, I also don't know.
The idea here is that we specifically depend only on the TokenKind enum from TokenKinds.h (which doesn't need any generated headers), not on other headers from clang/include/Basic (which might), and not on building/linking clangBasic itself.
In a modules world, maybe that means we need TokenKinds.h to be its own module, or a non-modular header, or something?

I went with the approach of putting TokenKinds.h (and TokenKinds.def) into its own module D130377.

In D126731#3670896, @sammccall wrote:

I'm also not sure on the support status of the modules build: I can't find a buildbot covering it or docs. Is this something I need to fix, or is it an experimental build like GN/Bazel?

I know at least about https://green.lab.llvm.org/green/job/lldb-cmake/ building with modules. I agree it's not a common configuration on buildbots but it's not experimental like GN/Bazel. As for the fixing the issue, it is covered by https://llvm.org/docs/DeveloperPolicy.html#quality , i.e., it is the author's responsibility but the reporter is responsible for making sure the author is equipped to do so (or fix themselves if they cannot help the author).

Thank you! I had forgotten about the greendragon bots.
And thanks for the module fix, if it's not that simple I'm happy to re-add the dep while i try to work something out.

clang-tools-extra/pseudo/gen/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
+list(REMOVE_ITEM LLVM_COMMON_DEPENDS clang-tablegen-targets)
 
 add_clang_executable(pseudo-gen
   Main.cpp
   )
 
 target_link_libraries(pseudo-gen
   PRIVATE
   clangPseudoGrammar
   )

clang-tools-extra/pseudo/lib/grammar/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
 
-# This library intents to keep as minimal dependencies as possible, it is a base
-# library of the cxx generator, to avoid creating long dep paths in the build
-# graph.
+# This library is used by the clang-pseudo-gen tool which runs at build time.
+# Dependencies should be minimal to avoid long dep paths in the build graph.
+# It does use clangBasic headers (tok::TokenKind), but linking is not needed.
+# We have no transitive dependencies on tablegen files.
+list(REMOVE_ITEM LLVM_COMMON_DEPENDS clang-tablegen-targets)
 add_clang_library(clangPseudoGrammar
   Grammar.cpp
   GrammarBNF.cpp
   LRGraph.cpp
   LRTable.cpp
   LRTableBuild.cpp
-
-  # FIXME: can we get rid of the clangBasic dependency? We need it for the
-  # clang::tok::getTokenName and clang::tok::getPunctuatorSpelling functions, we
-  # could consider remimplement these functions.
-  LINK_LIBS
-  clangBasic
   )
+

clang-tools-extra/pseudo/lib/grammar/Grammar.cpp

 //===--- Grammar.cpp - Grammar for clang pseudoparser  -----------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang-pseudo/Grammar.h"
 #include "clang/Basic/TokenKinds.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace pseudo {
for (const auto &R : G.table().Rules) {
       if (isNonterminal(Z))
         Changed |= ExpandFollowSet(Z, FollowSets[R.Target]);
     }
   }
   return FollowSets;
 }
 
 static llvm::ArrayRef<std::string> getTerminalNames() {
-  static const std::vector<std::string> *TerminalNames = []() {
-    static std::vector<std::string> TerminalNames;
-    TerminalNames.reserve(NumTerminals);
-    for (unsigned I = 0; I < NumTerminals; ++I) {
-      tok::TokenKind K = static_cast<tok::TokenKind>(I);
-      if (const auto *Punc = tok::getPunctuatorSpelling(K))
-        TerminalNames.push_back(Punc);
-      else
-        TerminalNames.push_back(llvm::StringRef(tok::getTokenName(K)).upper());
-    }
-    return &TerminalNames;
+  static const auto &TerminalNames = []() {
+    auto &TerminalNames = *new std::array<std::string, NumTerminals>;
+#define PUNCTUATOR(Tok, Spelling) TerminalNames[tok::Tok] = Spelling;
+#define KEYWORD(Keyword, Condition)                                            \
+  TerminalNames[tok::kw_##Keyword] = llvm::StringRef(#Keyword).upper();
+#define TOK(Tok) TerminalNames[tok::Tok] = llvm::StringRef(#Tok).upper();
+#include "clang/Basic/TokenKinds.def"
+    return TerminalNames;
   }();
-  return *TerminalNames;
+  return TerminalNames;
 }
 GrammarTable::GrammarTable() : Terminals(getTerminalNames()) {}
 
 } // namespace pseudo
 } // namespace clang