Defer addition of keywords to identifier table when loading AST

[jklaehn]

Summary

In ASTUnit::LoadFromASTFile, the preprocesor object is set up using
default-constructed LangOptions (which only later get populated).
Then, in the constructor of IdentifierTable, these default-constructed
LangOptions were used in the call to AddKeywords, leading to wrong
initialization of the identifier table.

This change defers adding the keywords to the identifier table until
after the language options have been loaded from the AST file.

Prior to this change the included test would fail due to the class
token being reported as an identifier (since the C++11 enum class
construct is not present when using the default language options).

[jklaehn]

ping :)

[arphaman]

Thanks for the patch!

lib/Frontend/ASTUnit.cpp

541 ↗

(On Diff #105781)

Have you tried adding the keywords in PP.Initialize? We might not need an additional parameter to the constructor then.

[jklaehn]

Thanks for taking a look! I removed the constructor argument as suggested; keywords are now added in PP.Initialize.

[jklaehn]

Ping, can you take another look?

[rsmith]

LGTM, but I'd like the old IdentifierTable constructor to be removed if there are no callers left.

include/clang/Basic/IdentifierTable.h

473–476

Can this constructor be removed?

[jklaehn]

In D35181#948925, @rsmith wrote:

LGTM, but I'd like the old IdentifierTable constructor to be removed if there are no callers left.

It's still being used in e.g. FormatTokenLexer, where the populated IdentifierTable is passed to the constructor of another member:

FormatTokenLexer::FormatTokenLexer(const SourceManager &SourceMgr, FileID ID,
                                   unsigned Column, const FormatStyle &Style,
                                   encoding::Encoding Encoding)
    : ..., IdentTable(getFormattingLangOpts(Style)),
      Keywords(IdentTable), ... {

struct AdditionalKeywords {
  AdditionalKeywords(IdentifierTable &IdentTable) {
    kw_final = &IdentTable.get("final");
    ...

Apart from this case (for which I would opt to keep the old constructor) there are three other uses which could easily be changed to the new signature.
Would you prefer to land this change with the old constructor in place or should I make the required changes to remove it?

[jklaehn]

ping (rebased)

[jklaehn]

ping? (rebased)

[jklaehn]

friendly ping? :)

[aaron.ballman]

LGTM aside from a few small nits.

include/clang/Basic/IdentifierTable.h

471

ExternalLookup to match coding conventions. Also, can you mark this constructor as explicit? Might as well change the other constructor as well since it's being reformatted.

[jklaehn]

Thanks for the review! As I do not have commit access, it would be great if you could commit the updated patch.

[aaron.ballman]

Committed in r330159. Thank you for the patch!

include/clang/Basic/IdentifierTable.h

class IdentifierTable {
   // Shark shows that using MallocAllocator is *much* slower than using this
   // BumpPtrAllocator!
   using HashTableTy = llvm::StringMap<IdentifierInfo *, llvm::BumpPtrAllocator>;
   HashTableTy HashTable;
 
   IdentifierInfoLookup* ExternalLookup;
 
 public:
+  /// \brief Create the identifier table.
+  explicit IdentifierTable(IdentifierInfoLookup *ExternalLookup = nullptr);
+
   /// \brief Create the identifier table, populating it with info about the
   /// language keywords for the language specified by \p LangOpts.
-  IdentifierTable(const LangOptions &LangOpts,
-                  IdentifierInfoLookup* externalLookup = nullptr);
+  explicit IdentifierTable(const LangOptions &LangOpts,
+                           IdentifierInfoLookup *ExternalLookup = nullptr);
 
   /// \brief Set the external identifier lookup mechanism.
   void setExternalIdentifierLookup(IdentifierInfoLookup *IILookup) {
     ExternalLookup = IILookup;
   }
 
   /// \brief Retrieve the external identifier lookup object, if any.
   IdentifierInfoLookup *getExternalIdentifierLookup() const {
public:
   iterator begin() const { return HashTable.begin(); }
   iterator end() const   { return HashTable.end(); }
   unsigned size() const  { return HashTable.size(); }
 
   /// \brief Print some statistics to stderr that indicate how well the
   /// hashing is doing.
   void PrintStats() const;
 
+  /// \brief Populate the identifier table with info about the language keywords
+  /// for the language specified by \p LangOpts.
   void AddKeywords(const LangOptions &LangOpts);
 };
 
 /// \brief A family of Objective-C methods. 
 ///
 /// These families have no inherent meaning in the language, but are
 /// nonetheless central enough in the existing implementations to
 /// merit direct AST support.  While, in theory, arbitrary methods can

lib/Basic/IdentifierTable.cpp

 };
 
 } // namespace
 
 IdentifierIterator *IdentifierInfoLookup::getIdentifiers() {
   return new EmptyLookupIterator();
 }
 
-IdentifierTable::IdentifierTable(const LangOptions &LangOpts,
-                                 IdentifierInfoLookup* externalLookup)
+IdentifierTable::IdentifierTable(IdentifierInfoLookup *ExternalLookup)
     : HashTable(8192), // Start with space for 8K identifiers.
-    ExternalLookup(externalLookup) {
+      ExternalLookup(ExternalLookup) {}
+
+IdentifierTable::IdentifierTable(const LangOptions &LangOpts,
+                                 IdentifierInfoLookup *ExternalLookup)
+    : IdentifierTable(ExternalLookup) {
   // Populate the identifier table with info about keywords for the current
   // language.
   AddKeywords(LangOpts);
-
-  // Add the '_experimental_modules_import' contextual keyword.
-  get("import").setModulesImport(true);
 }
 
 //===----------------------------------------------------------------------===//
 // Language Keyword Implementation
 //===----------------------------------------------------------------------===//
 
 // Constants for TokenKinds.def
 namespace {

 #include "clang/Basic/TokenKinds.def"
 
   if (LangOpts.ParseUnknownAnytype)
     AddKeyword("__unknown_anytype", tok::kw___unknown_anytype, KEYALL,
                LangOpts, *this);
 
   if (LangOpts.DeclSpecKeyword)
     AddKeyword("__declspec", tok::kw___declspec, KEYALL, LangOpts, *this);
+
+  // Add the '_experimental_modules_import' contextual keyword.
+  get("import").setModulesImport(true);
 }
 
 /// \brief Checks if the specified token kind represents a keyword in the
 /// specified language.
 /// \returns Status of the keyword in the language.
 static KeywordStatus getTokenKwStatus(const LangOptions &LangOpts,
                                       tok::TokenKind K) {
   switch (K) {

lib/Lex/Preprocessor.cpp

 
 Preprocessor::Preprocessor(std::shared_ptr<PreprocessorOptions> PPOpts,
                            DiagnosticsEngine &diags, LangOptions &opts,
                            SourceManager &SM, MemoryBufferCache &PCMCache,
                            HeaderSearch &Headers, ModuleLoader &TheModuleLoader,
                            IdentifierInfoLookup *IILookup, bool OwnsHeaders,
                            TranslationUnitKind TUKind)
     : PPOpts(std::move(PPOpts)), Diags(&diags), LangOpts(opts),
-      FileMgr(Headers.getFileMgr()), SourceMgr(SM),
-      PCMCache(PCMCache), ScratchBuf(new ScratchBuffer(SourceMgr)),
-      HeaderInfo(Headers), TheModuleLoader(TheModuleLoader),
-      ExternalSource(nullptr), Identifiers(opts, IILookup),
-      PragmaHandlers(new PragmaNamespace(StringRef())), TUKind(TUKind),
-      SkipMainFilePreamble(0, true),
+      FileMgr(Headers.getFileMgr()), SourceMgr(SM), PCMCache(PCMCache),
+      ScratchBuf(new ScratchBuffer(SourceMgr)), HeaderInfo(Headers),
+      TheModuleLoader(TheModuleLoader), ExternalSource(nullptr),
+      // As the language options may have not been loaded yet (when
+      // deserializing an ASTUnit), adding keywords to the identifier table is
+      // deferred to Preprocessor::Initialize().
+      Identifiers(IILookup), PragmaHandlers(new PragmaNamespace(StringRef())),
+      TUKind(TUKind), SkipMainFilePreamble(0, true),
       CurSubmoduleState(&NullSubmoduleState) {
   OwnsHeaderSearch = OwnsHeaders;
   
   // Default to discarding comments.
   KeepComments = false;
   KeepMacroComments = false;
   SuppressIncludeNotFoundError = false;
   
void Preprocessor::Initialize(const TargetInfo &Target,
 
   assert((!this->AuxTarget || this->AuxTarget == AuxTarget) &&
          "Invalid override of aux target information.");
   this->AuxTarget = AuxTarget;
 
   // Initialize information about built-ins.
   BuiltinInfo.InitializeTarget(Target, AuxTarget);
   HeaderInfo.setTarget(Target);
+
+  // Populate the identifier table with info about keywords for the current language.
+  Identifiers.AddKeywords(LangOpts);
 }
 
 void Preprocessor::InitializeForModelFile() {
   NumEnteredSourceFiles = 0;
 
   // Reset pragmas
   PragmaHandlersBackup = std::move(PragmaHandlers);
   PragmaHandlers = llvm::make_unique<PragmaNamespace>(StringRef());

unittests/libclang/LibclangTest.cpp

for (int i = 0; i != 3; ++i) {
     clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
     EXPECT_EQ(2U, line);
     cxl = clang_getRangeStart(Ranges->ranges[1]);
     clang_getSpellingLocation(cxl, nullptr, &line, nullptr, nullptr);
     EXPECT_EQ(4U, line);
     clang_disposeSourceRangeList(Ranges);
   }
 }
+
+class LibclangSerializationTest : public LibclangParseTest {
+public:
+  bool SaveAndLoadTU(const std::string &Filename) {
+    unsigned options = clang_defaultSaveOptions(ClangTU);
+    if (clang_saveTranslationUnit(ClangTU, Filename.c_str(), options) !=
+        CXSaveError_None) {
+      DEBUG(llvm::dbgs() << "Saving failed\n");
+      return false;
+    }
+
+    clang_disposeTranslationUnit(ClangTU);
+
+    ClangTU = clang_createTranslationUnit(Index, Filename.c_str());
+
+    if (!ClangTU) {
+      DEBUG(llvm::dbgs() << "Loading failed\n");
+      return false;
+    }
+
+    return true;
+  }
+};
+
+TEST_F(LibclangSerializationTest, TokenKindsAreCorrectAfterLoading) {
+  // Ensure that "class" is recognized as a keyword token after serializing
+  // and reloading the AST, as it is not a keyword for the default LangOptions.
+  std::string HeaderName = "test.h";
+  WriteFile(HeaderName, "enum class Something {};");
+
+  const char *Argv[] = {"-xc++-header", "-std=c++11"};
+
+  ClangTU = clang_parseTranslationUnit(Index, HeaderName.c_str(), Argv,
+                                       sizeof(Argv) / sizeof(Argv[0]), nullptr,
+                                       0, TUFlags);
+
+  auto CheckTokenKinds = [=]() {
+    CXSourceRange Range =
+        clang_getCursorExtent(clang_getTranslationUnitCursor(ClangTU));
+
+    CXToken *Tokens;
+    unsigned int NumTokens;
+    clang_tokenize(ClangTU, Range, &Tokens, &NumTokens);
+
+    ASSERT_EQ(6u, NumTokens);
+    EXPECT_EQ(CXToken_Keyword, clang_getTokenKind(Tokens[0]));
+    EXPECT_EQ(CXToken_Keyword, clang_getTokenKind(Tokens[1]));
+    EXPECT_EQ(CXToken_Identifier, clang_getTokenKind(Tokens[2]));
+    EXPECT_EQ(CXToken_Punctuation, clang_getTokenKind(Tokens[3]));
+    EXPECT_EQ(CXToken_Punctuation, clang_getTokenKind(Tokens[4]));
+    EXPECT_EQ(CXToken_Punctuation, clang_getTokenKind(Tokens[5]));
+
+    clang_disposeTokens(ClangTU, Tokens, NumTokens);
+  };
+
+  CheckTokenKinds();
+
+  std::string ASTName = "test.ast";
+  WriteFile(ASTName, "");
+
+  ASSERT_TRUE(SaveAndLoadTU(ASTName));
+
+  CheckTokenKinds();
+}