From e2f43f500af0a6c35a12751e6f3a9829c008b71c Mon Sep 17 00:00:00 2001
From: Sam McCall <sam.mccall@gmail.com>
Date: Mon, 15 Jan 2018 20:09:09 +0000
Subject: [PATCH] [clangd] Improve const-correctness of Symbol->Detail. NFC

Summary:
This would have caught a bug I wrote in an early version of D42049, where
an index user could overwrite data internal to the index because the Symbol is
not deep-const.

The YAML traits are now a bit more verbose, but separate concerns a bit more
nicely: ArenaPtr can be reused for other similarly-allocated objects, including
scalars etc.

Reviewers: hokein

Subscribers: klimek, ilya-biryukov, cfe-commits, ioeric

Differential Revision: https://reviews.llvm.org/D42059

llvm-svn: 322509
---
 clang-tools-extra/clangd/index/Index.cpp      | 11 +++--
 clang-tools-extra/clangd/index/Index.h        |  2 +-
 clang-tools-extra/clangd/index/SymbolYAML.cpp | 45 ++++++++++++-------
 3 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/clang-tools-extra/clangd/index/Index.cpp b/clang-tools-extra/clangd/index/Index.cpp
index 7f598643423d..f56b1bf56973 100644
--- a/clang-tools-extra/clangd/index/Index.cpp
+++ b/clang-tools-extra/clangd/index/Index.cpp
@@ -64,13 +64,12 @@ static void own(Symbol &S, DenseSet<StringRef> &Strings,
   if (S.Detail) {
     // Copy values of StringRefs into arena.
     auto *Detail = Arena.Allocate<Symbol::Details>();
-    Detail->Documentation = S.Detail->Documentation;
-    Detail->CompletionDetail = S.Detail->CompletionDetail;
-    S.Detail = Detail;
-
+    *Detail = *S.Detail;
     // Intern the actual strings.
-    Intern(S.Detail->Documentation);
-    Intern(S.Detail->CompletionDetail);
+    Intern(Detail->Documentation);
+    Intern(Detail->CompletionDetail);
+    // Replace the detail pointer with our copy.
+    S.Detail = Detail;
   }
 }
 
diff --git a/clang-tools-extra/clangd/index/Index.h b/clang-tools-extra/clangd/index/Index.h
index 01a9af8dd0a0..a70d38b768fd 100644
--- a/clang-tools-extra/clangd/index/Index.h
+++ b/clang-tools-extra/clangd/index/Index.h
@@ -156,7 +156,7 @@ struct Symbol {
   };
 
   // Optional details of the symbol.
-  Details *Detail = nullptr; // FIXME: should be const
+  const Details *Detail = nullptr;
 
   // FIXME: add definition location of the symbol.
   // FIXME: add all occurrences support.
diff --git a/clang-tools-extra/clangd/index/SymbolYAML.cpp b/clang-tools-extra/clangd/index/SymbolYAML.cpp
index 8a58a6dfb8a6..83e8cc64f688 100644
--- a/clang-tools-extra/clangd/index/SymbolYAML.cpp
+++ b/clang-tools-extra/clangd/index/SymbolYAML.cpp
@@ -60,27 +60,39 @@ template <> struct MappingTraits<SymbolInfo> {
   }
 };
 
-template <>
-struct MappingTraits<Symbol::Details *> {
-  static void mapping(IO &io, Symbol::Details *&Detail) {
-    if (!io.outputting()) {
-      assert(io.getContext() && "Expecting an arena (as context) to allocate "
-                                "data for new symbols.");
-      Detail = static_cast<llvm::BumpPtrAllocator *>(io.getContext())
-                   ->Allocate<Symbol::Details>();
-    } else if (!Detail) {
-      // Detail is optional in outputting.
-      return;
-    }
-    assert(Detail);
-    io.mapOptional("Documentation", Detail->Documentation);
-    io.mapOptional("CompletionDetail", Detail->CompletionDetail);
+template <> struct MappingTraits<Symbol::Details> {
+  static void mapping(IO &io, Symbol::Details &Detail) {
+    io.mapOptional("Documentation", Detail.Documentation);
+    io.mapOptional("CompletionDetail", Detail.CompletionDetail);
   }
 };
 
+// A YamlIO normalizer for fields of type "const T*" allocated on an arena.
+// Normalizes to Optional<T>, so traits should be provided for T.
+template <typename T> struct ArenaPtr {
+  ArenaPtr(IO &) {}
+  ArenaPtr(IO &, const T *D) {
+    if (D)
+      Opt = *D;
+  }
+
+  const T *denormalize(IO &IO) {
+    assert(IO.getContext() && "Expecting an arena (as context) to allocate "
+                              "data for read symbols.");
+    if (!Opt)
+      return nullptr;
+    return new (*static_cast<llvm::BumpPtrAllocator *>(IO.getContext()))
+        T(std::move(*Opt)); // Allocate a copy of Opt on the arena.
+  }
+
+  llvm::Optional<T> Opt;
+};
+
 template <> struct MappingTraits<Symbol> {
   static void mapping(IO &IO, Symbol &Sym) {
     MappingNormalization<NormalizedSymbolID, SymbolID> NSymbolID(IO, Sym.ID);
+    MappingNormalization<ArenaPtr<Symbol::Details>, const Symbol::Details *>
+        NDetail(IO, Sym.Detail);
     IO.mapRequired("ID", NSymbolID->HexString);
     IO.mapRequired("Name", Sym.Name);
     IO.mapRequired("Scope", Sym.Scope);
@@ -92,8 +104,7 @@ template <> struct MappingTraits<Symbol> {
 
     IO.mapOptional("CompletionSnippetInsertText",
                    Sym.CompletionSnippetInsertText);
-    if (!IO.outputting() || Sym.Detail)
-      IO.mapOptional("Detail", Sym.Detail);
+    IO.mapOptional("Detail", NDetail->Opt);
   }
 };