Fix printing policy for AST context loaded from file

[jklaehn]

Summary

In ASTUnit::LoadFromASTFile, the context object is set up using default-constructed
LangOptions (which only later get populated). As the language options are used in the constructor
of PrintingPolicy, this needs to be updated explicitly after the language options are available.

[vsk]

I wonder if it's possible to do away with the calls to 'updated()'... it seems strange that we initialize the same preprocessor repeatedly. Is there any way to finalize an ASTInfoCollector after ReadAST happens (or ASTReaderListeners in general)?

[jklaehn]

In D35271#809159, @vsk wrote:

I wonder if it's possible to do away with the calls to 'updated()'... it seems strange that we initialize the same preprocessor repeatedly. Is there any way to finalize an ASTInfoCollector after ReadAST happens (or ASTReaderListeners in general)?

I can look into this but would prefer to do so in a different patch, as this would require refactoring beyond this simple bug fix. Would it be okay to land this patch as-is?

[vsk]

In D35271#847306, @jklaehn wrote:

In D35271#809159, @vsk wrote:

I wonder if it's possible to do away with the calls to 'updated()'... it seems strange that we initialize the same preprocessor repeatedly. Is there any way to finalize an ASTInfoCollector after ReadAST happens (or ASTReaderListeners in general)?

I can look into this but would prefer to do so in a different patch, as this would require refactoring beyond this simple bug fix. Would it be okay to land this patch as-is?

Not having worked on this code I'm afraid I can't say. Usually it's a good idea to include a regression test.

[jklaehn]

Added regression test.

[vsk]

Thanks for adding the test! This is looking good.

unittests/Frontend/ASTUnitTest.cpp

32 ↗

(On Diff #112139)

If a directory isn't necessary to exercise the desired functionality, it'd be better to use createTemporaryFile() to open a temporary file descriptor, and then to use tool_output_file() to manage the descriptor's lifetime. Tool_output_file has logic to clean up files when a signal is received, etc.

48 ↗

(On Diff #112139)

It doesn't look like all of the directories which could be created here would be deleted? With any luck it'll be possible to use tool_output_file() to side step the issue.

[jklaehn]

Update regression test to use createTemporaryFile() and tool_output_file as suggested.

[vsk]

Thanks, LGTM! This seems like a pretty straightforward bug fix. Since it's not my usual area maybe it'd be worth waiting a day or so for more feedback.

[jklaehn]

In D35271#850472, @vsk wrote:

Thanks, LGTM! This seems like a pretty straightforward bug fix. Since it's not my usual area maybe it'd be worth waiting a day or so for more feedback.

Thanks! I do not have commit access so it would be great if you could commit it on my behalf.

cfe/trunk/lib/Frontend/ASTUnit.cpp

void updated() {
     PP.Initialize(*Target);
 
     if (!Context)
       return;
 
     // Initialize the ASTContext
     Context->InitBuiltinTypes(*Target);
 
+    // Adjust printing policy based on language options.
+    Context->setPrintingPolicy(PrintingPolicy(LangOpt));
+
     // We didn't have access to the comment options when the ASTContext was
     // constructed, so register them now.
     Context->getCommentCommandTraits().registerCommentOptions(
         LangOpt.CommentOpts);
   }
 };
 
   /// \brief Diagnostic consumer that saves each diagnostic it is given.

cfe/trunk/unittests/Frontend/ASTUnitTest.cpp

+//===- unittests/Frontend/ASTUnitTest.cpp - ASTUnit tests -----------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <fstream>
+
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/PCHContainerOperations.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+TEST(ASTUnit, SaveLoadPreservesLangOptionsInPrintingPolicy) {
+  // Check that the printing policy is restored with the correct language
+  // options when loading an ASTUnit from a file.  To this end, an ASTUnit
+  // for a C++ translation unit is set up and written to a temporary file.
+
+  // By default `UseVoidForZeroParams` is true for non-C++ language options,
+  // thus we can check this field after loading the ASTUnit to deduce whether
+  // the correct (C++) language options were used when setting up the printing
+  // policy.
+
+  {
+    PrintingPolicy PolicyWithDefaultLangOpt(LangOptions{});
+    EXPECT_TRUE(PolicyWithDefaultLangOpt.UseVoidForZeroParams);
+  }
+
+  int FD;
+  llvm::SmallString<256> InputFileName;
+  ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("ast-unit", "cpp", FD, InputFileName));
+  tool_output_file input_file(InputFileName, FD);
+  input_file.os() << "";
+
+  const char* Args[] = {"clang", "-xc++", InputFileName.c_str()};
+
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+      CompilerInstance::createDiagnostics(new DiagnosticOptions());
+
+  std::shared_ptr<CompilerInvocation> CInvok =
+      createInvocationFromCommandLine(Args, Diags);
+
+  if (!CInvok)
+    FAIL() << "could not create compiler invocation";
+
+  FileManager *FileMgr =
+      new FileManager(FileSystemOptions(), vfs::getRealFileSystem());
+  auto PCHContainerOps = std::make_shared<PCHContainerOperations>();
+
+  std::unique_ptr<ASTUnit> AST = ASTUnit::LoadFromCompilerInvocation(
+      CInvok, PCHContainerOps, Diags, FileMgr);
+
+  if (!AST)
+    FAIL() << "failed to create ASTUnit";
+
+  EXPECT_FALSE(AST->getASTContext().getPrintingPolicy().UseVoidForZeroParams);
+
+  llvm::SmallString<256> ASTFileName;
+  ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("ast-unit", "ast", FD, ASTFileName));
+  tool_output_file ast_file(ASTFileName, FD);
+  AST->Save(ASTFileName.str());
+
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+
+  std::unique_ptr<ASTUnit> AU = ASTUnit::LoadFromASTFile(
+      ASTFileName.str(), PCHContainerOps->getRawReader(), ASTUnit::LoadEverything, Diags,
+      FileSystemOptions(), /*UseDebugInfo=*/false);
+
+  if (!AU)
+    FAIL() << "failed to load ASTUnit";
+
+  EXPECT_FALSE(AU->getASTContext().getPrintingPolicy().UseVoidForZeroParams);
+}
+
+} // anonymous namespace

cfe/trunk/unittests/Frontend/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(FrontendTests
+  ASTUnitTest.cpp
   FrontendActionTest.cpp
   CodeGenActionTest.cpp
   )
 target_link_libraries(FrontendTests
   clangAST
   clangBasic
   clangFrontend
   clangLex
   clangSema
   clangCodeGen
   )