[clang] Fix copy constructor of CompilerInvocation

[jansvoboda11]

Summary

The CompilerInvocationBase class factors out members of CompilerInvocation that need special handling (initialization or copy constructor), so that CompilerInvocation can be implemented as a simple value object.

Currently, the AnalyzerOpts member of CompilerInvocation violates that setup. This patch extracts the member to CompilerInvocationBase and handles it in the copy constructor the same way other it handles other members.

[dexonsmith]

I'm not sure a deep copy is entirely sound, due to odd ownership rules in "remapped buffers" (I forget the subclass that includes those). A few months ago I got most of the way to deleting those (lifting the logic up into the clang tooling that needed it)... maybe we need to push that work over the line before this is safe... or if this is in fact safe, can you explain why?

BTW, I may be responsible for the not-very-descriptive naming of CompilerInvocationBase... I could imagine new options coming along that get added to the wrong place in the future as well. Any thoughts on a good name to use instead?

clang/include/clang/Frontend/CompilerInvocation.h

86

I wonder if it would be better to signify this is a deep copy by naming the function, such as by calling it clone(). WDYT?

[jansvoboda11]

In D99568#2659526, @dexonsmith wrote:

I'm not sure a deep copy is entirely sound, due to odd ownership rules in "remapped buffers" (I forget the subclass that includes those). A few months ago I got most of the way to deleting those (lifting the logic up into the clang tooling that needed it)... maybe we need to push that work over the line before this is safe... or if this is in fact safe, can you explain why?

Ah, I didn't notice those. I think you may be referring to PreprocessorOptions::RemappedFileBuffers. I agree that the raw MemoryBuffer pointers don't play well with the concept of deep copy, so getting that sorted would be good.

How about we split this patch into two?

1. Changes to AnalyzerOptions. They make the current implementation of deep copy constructor more correct, so we can commit them right away.
2. Addition of copy assignment. It increases the visibility/surface area of the deep copy operation, so we can commit that when RemappedFileBuffers get fixed. In the meantime, I can work around its absence in my next patch.

WDYT?

Note: I looked at the members of CompilerInvocation and noticed one other field that's a reference type: PreprocessorOptions::FailedModules, which acts as a shared state, not so much as an actual option. I don't think it creates lifetime issues like RemappedFileBuffers. Although sharing it across deep copies of CompilerInvocation may be a bit unexpected, that seems to be the current invariant. Extracting that out of PreprocessorOptions would be nice though.

BTW, I may be responsible for the not-very-descriptive naming of CompilerInvocationBase... I could imagine new options coming along that get added to the wrong place in the future as well. Any thoughts on a good name to use instead?

We could make the split obvious by renaming CompilerInvocationBase to CompilerInvocationBaseReferenceType, renaming CompilerInvocation to CompilerInvocationBaseValueType and creating new CompilerInvocation class that inherits from both. I'm not sure if we have any precedent for this pattern. I'll look around.

clang/include/clang/Frontend/CompilerInvocation.h

86

We can be more explicit about that. In that case, it would probably make sense to delete the copy constructor to make clone() the only way to get a deep copy.

[jansvoboda11]

Ping.

[dexonsmith]

In D99568#2682815, @jansvoboda11 wrote:

Ping.

Thanks for the ping; I'd completely missed your follow-up. Just fixing AnalyzerOptions for now sounds great.

[jansvoboda11]

Remove copy-assignment changes

[dexonsmith]

LGTM.

clang/include/clang/Frontend/CompilerInvocation.h

public:
   IntrusiveRefCntPtr<DiagnosticOptions> DiagnosticOpts;
 
   /// Options controlling the \#include directive.
   std::shared_ptr<HeaderSearchOptions> HeaderSearchOpts;
 
   /// Options controlling the preprocessor (aside from \#include handling).
   std::shared_ptr<PreprocessorOptions> PreprocessorOpts;
 
+  /// Options controlling the static analyzer.
+  AnalyzerOptionsRef AnalyzerOpts;
+
   CompilerInvocationBase();
   CompilerInvocationBase(const CompilerInvocationBase &X);
   CompilerInvocationBase &operator=(const CompilerInvocationBase &) = delete;
   ~CompilerInvocationBase();
 
   LangOptions *getLangOpts() { return LangOpts.get(); }
   const LangOptions *getLangOpts() const { return LangOpts.get(); }
 
   TargetOptions &getTargetOpts() { return *TargetOpts.get(); }
   const TargetOptions &getTargetOpts() const { return *TargetOpts.get(); }
 
std::shared_ptr<PreprocessorOptions> getPreprocessorOptsPtr() {
     return PreprocessorOpts;
   }
 
   PreprocessorOptions &getPreprocessorOpts() { return *PreprocessorOpts; }
 
   const PreprocessorOptions &getPreprocessorOpts() const {
     return *PreprocessorOpts;
   }
+
+  AnalyzerOptionsRef getAnalyzerOpts() const { return AnalyzerOpts; }
 };
 
 /// Helper class for holding the data necessary to invoke the compiler.
 ///
 /// This class is designed to represent an abstract "invocation" of the
 /// compiler, including data such as the include paths, the code generation
 /// options, the warning flags, and so on.
 class CompilerInvocation : public CompilerInvocationBase {
-  /// Options controlling the static analyzer.
-  AnalyzerOptionsRef AnalyzerOpts;
-
   MigratorOptions MigratorOpts;
 
   /// Options controlling IRgen and the backend.
   CodeGenOptions CodeGenOpts;
 
   /// Options controlling dependency output.
   DependencyOutputOptions DependencyOutputOpts;
 
   /// Options controlling file system operations.
   FileSystemOptions FileSystemOpts;
 
   /// Options controlling the frontend itself.
   FrontendOptions FrontendOpts;
 
   /// Options controlling preprocessed output.
   PreprocessorOutputOptions PreprocessorOutputOpts;
 
 public:
-  CompilerInvocation() : AnalyzerOpts(new AnalyzerOptions()) {}
-
   /// @name Utility Methods
   /// @{
 
   /// Create a compiler invocation from a list of input options.
   /// \returns true on success.
   ///
   /// \returns false if an error was encountered while parsing the arguments
   /// and attempts to recover and continue parsing the rest of the arguments.
public:
   /// The returned pointer is what gets appended to Args.
   void generateCC1CommandLine(llvm::SmallVectorImpl<const char *> &Args,
                               StringAllocator SA) const;
 
   /// @}
   /// @name Option Subgroups
   /// @{
 
-  AnalyzerOptionsRef getAnalyzerOpts() const { return AnalyzerOpts; }
-
   MigratorOptions &getMigratorOpts() { return MigratorOpts; }
   const MigratorOptions &getMigratorOpts() const { return MigratorOpts; }
 
   CodeGenOptions &getCodeGenOpts() { return CodeGenOpts; }
   const CodeGenOptions &getCodeGenOpts() const { return CodeGenOpts; }
 
   DependencyOutputOptions &getDependencyOutputOpts() {
     return DependencyOutputOpts;

clang/lib/Frontend/CompilerInvocation.cpp

 //===----------------------------------------------------------------------===//
 // Initialization.
 //===----------------------------------------------------------------------===//
 
 CompilerInvocationBase::CompilerInvocationBase()
     : LangOpts(new LangOptions()), TargetOpts(new TargetOptions()),
       DiagnosticOpts(new DiagnosticOptions()),
       HeaderSearchOpts(new HeaderSearchOptions()),
-      PreprocessorOpts(new PreprocessorOptions()) {}
+      PreprocessorOpts(new PreprocessorOptions()),
+      AnalyzerOpts(new AnalyzerOptions()) {}
 
 CompilerInvocationBase::CompilerInvocationBase(const CompilerInvocationBase &X)
     : LangOpts(new LangOptions(*X.getLangOpts())),
       TargetOpts(new TargetOptions(X.getTargetOpts())),
       DiagnosticOpts(new DiagnosticOptions(X.getDiagnosticOpts())),
       HeaderSearchOpts(new HeaderSearchOptions(X.getHeaderSearchOpts())),
-      PreprocessorOpts(new PreprocessorOptions(X.getPreprocessorOpts())) {}
+      PreprocessorOpts(new PreprocessorOptions(X.getPreprocessorOpts())),
+      AnalyzerOpts(new AnalyzerOptions(*X.getAnalyzerOpts())) {}
 
 CompilerInvocationBase::~CompilerInvocationBase() = default;
 
 //===----------------------------------------------------------------------===//
 // Normalizers
 //===----------------------------------------------------------------------===//
 
 #define SIMPLE_ENUM_VALUE_TABLE

clang/unittests/Frontend/CompilerInvocationTest.cpp

 TEST(ContainsN, Two) {
   const char *Array[] = {"x", "a", "b", "x"};
 
   ASSERT_THAT(Array, Not(ContainsN(StrEq("x"), 0)));
   ASSERT_THAT(Array, Not(ContainsN(StrEq("x"), 1)));
   ASSERT_THAT(Array, ContainsN(StrEq("x"), 2));
 }
 
+// Copy constructor performs a deep copy of reference-counted pointers.
+
+TEST(CompilerInvocationTest, DeepCopyConstructor) {
+  CompilerInvocation A;
+  A.getAnalyzerOpts()->Config["Key"] = "Old";
+
+  CompilerInvocation B(A);
+  B.getAnalyzerOpts()->Config["Key"] = "New";
+
+  ASSERT_EQ(A.getAnalyzerOpts()->Config["Key"], "Old");
+}
+
 // Boolean option with a keypath that defaults to true.
 // The only flag with a negative spelling can set the keypath to false.
 
 TEST_F(CommandLineTest, BoolOptionDefaultTrueSingleFlagNotPresent) {
   const char *Args[] = {""};
 
   ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
   ASSERT_TRUE(Invocation.getFrontendOpts().UseTemporary);