[ADT] Make Twine's copy constructor private.

There's a lot of misuse of Twine scattered around LLVM.  This
ranges in severity from benign (returning a Twine from a function
by value that is just a string literal) to pretty sketchy (storing
a Twine by value in a class).  While there are some uses for
copying Twines, most of the very compelling ones are confined
to the Twine class implementation itself, and other uses are
either dubious or easily worked around.

This patch makes Twine's copy constructor private, and fixes up
all callsites.

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

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@315530 91177308-0d34-0410-b5e6-96231b3b80d8

17 files changed, 72 insertions, 47 deletions

diff --git a/include/llvm/ADT/Twine.h b/include/llvm/ADT/Twine.h
index f5f00dcfafe5..c9cefa982d63 100644
--- a/include/llvm/ADT/Twine.h
+++ b/include/llvm/ADT/Twine.h
@@ -79,6 +79,10 @@ namespace llvm {
   /// overloads) to guarantee that particularly important cases (cstring plus
   /// StringRef) codegen as desired.
   class Twine {
+    friend Twine operator+(const char *LHS, const StringRef &RHS);
+    friend Twine operator+(const StringRef &LHS, const char *RHS);
+    friend Twine operator+(const StringRef &LHS, const StringRef &RHS);
+
     /// NodeKind - Represent the type of an argument.
     enum NodeKind : unsigned char {
       /// An empty string; the result of concatenating anything with it is also
@@ -169,6 +173,12 @@ namespace llvm {
       assert(isNullary() && "Invalid kind!");
     }
 
+    // While there are some valid use cases for copying Twines, most of them
+    // are confined to the implementation of Twine itself, and Twine itself is
+    // not intended to be publicly copyable since it can very easily lead to
+    // dangling pointers / references.
+    Twine(const Twine &) = default;
+
     /// Construct a binary twine.
     explicit Twine(const Twine &LHS, const Twine &RHS)
         : LHSKind(TwineKind), RHSKind(TwineKind) {
@@ -256,8 +266,6 @@ namespace llvm {
       assert(isValid() && "Invalid twine!");
     }
 
-    Twine(const Twine &) = default;
-
     /// Construct from a C string.
     ///
     /// We take care here to optimize "" into the empty twine -- this will be
@@ -274,6 +282,8 @@ namespace llvm {
       assert(isValid() && "Invalid twine!");
     }
 
+    Twine(Twine &&Other) = default;
+
     /// Construct from an std::string.
     /*implicit*/ Twine(const std::string &Str)
       : LHSKind(StdStringKind), RHSKind(EmptyKind) {
@@ -377,6 +387,14 @@ namespace llvm {
       assert(isValid() && "Invalid twine!");
     }
 
+    /// Construct as the concatenation of two StringRefs.
+    /*implicit*/ Twine(const StringRef &LHS, const StringRef &RHS)
+        : LHSKind(StringRefKind), RHSKind(StringRefKind) {
+      this->LHS.stringRef = &LHS;
+      this->RHS.stringRef = &RHS;
+      assert(isValid() && "Invalid twine!");
+    }
+
     /// Since the intended use of twines is as temporary objects, assignments
     /// when concatenating might cause undefined behavior or stack corruptions
     Twine &operator=(const Twine &) = delete;
@@ -487,6 +505,10 @@ namespace llvm {
     /// Dump the representation of this twine to stderr.
     void dumpRepr() const;
 
+    friend inline Twine operator+(const Twine &LHS, const Twine &RHS) {
+      return LHS.concat(RHS);
+    }
+
     /// @}
   };
 
@@ -522,10 +544,6 @@ namespace llvm {
     return Twine(NewLHS, NewLHSKind, NewRHS, NewRHSKind);
   }
 
-  inline Twine operator+(const Twine &LHS, const Twine &RHS) {
-    return LHS.concat(RHS);
-  }
-
   /// Additional overload to guarantee simplified codegen; this is equivalent to
   /// concat().
 
@@ -533,13 +551,14 @@ namespace llvm {
     return Twine(LHS, RHS);
   }
 
-  /// Additional overload to guarantee simplified codegen; this is equivalent to
-  /// concat().
-
   inline Twine operator+(const StringRef &LHS, const char *RHS) {
     return Twine(LHS, RHS);
   }
 
+  inline Twine operator+(const StringRef &LHS, const StringRef &RHS) {
+    return Twine(LHS, RHS);
+  }
+
   inline raw_ostream &operator<<(raw_ostream &OS, const Twine &RHS) {
     RHS.print(OS);
     return OS;
diff --git a/include/llvm/IR/DiagnosticInfo.h b/include/llvm/IR/DiagnosticInfo.h
index 020b67d6b711..0ce9c2f9b11c 100644
--- a/include/llvm/IR/DiagnosticInfo.h
+++ b/include/llvm/IR/DiagnosticInfo.h
@@ -962,7 +962,7 @@ public:
 /// Diagnostic information for unsupported feature in backend.
 class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
 private:
-  Twine Msg;
+  std::string Msg;
 
 public:
   /// \p Fn is the function where the diagnostic is being emitted. \p Loc is
@@ -976,13 +976,13 @@ public:
       const DiagnosticLocation &Loc = DiagnosticLocation(),
       DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfoWithLocationBase(DK_Unsupported, Severity, Fn, Loc),
-        Msg(Msg) {}
+        Msg(Msg.str()) {}
 
   static bool classof(const DiagnosticInfo *DI) {
     return DI->getKind() == DK_Unsupported;
   }
 
-  const Twine &getMessage() const { return Msg; }
+  StringRef getMessage() const { return Msg; }
 
   void print(DiagnosticPrinter &DP) const override;
 };
diff --git a/include/llvm/Object/Error.h b/include/llvm/Object/Error.h
index eb938338715d..ef820bdba318 100644
--- a/include/llvm/Object/Error.h
+++ b/include/llvm/Object/Error.h
@@ -65,8 +65,8 @@ public:
 class GenericBinaryError : public ErrorInfo<GenericBinaryError, BinaryError> {
 public:
   static char ID;
-  GenericBinaryError(Twine Msg);
-  GenericBinaryError(Twine Msg, object_error ECOverride);
+  GenericBinaryError(const Twine &Msg);
+  GenericBinaryError(const Twine &Msg, object_error ECOverride);
   const std::string &getMessage() const { return Msg; }
   void log(raw_ostream &OS) const override;
 private:
diff --git a/include/llvm/Object/WindowsResource.h b/include/llvm/Object/WindowsResource.h
index 05fe10a770e0..bf17ad3f8a85 100644
--- a/include/llvm/Object/WindowsResource.h
+++ b/include/llvm/Object/WindowsResource.h
@@ -87,7 +87,7 @@ struct WinResHeaderSuffix {
 
 class EmptyResError : public GenericBinaryError {
 public:
-  EmptyResError(Twine Msg, object_error ECOverride)
+  EmptyResError(const Twine &Msg, object_error ECOverride)
       : GenericBinaryError(Msg, ECOverride) {}
 };
 
diff --git a/include/llvm/Support/Error.h b/include/llvm/Support/Error.h
index bb40dbee053f..63d441fa58e8 100644
--- a/include/llvm/Support/Error.h
+++ b/include/llvm/Support/Error.h
@@ -931,7 +931,7 @@ Expected<T> handleExpected(Expected<T> ValOrErr, RecoveryFtor &&RecoveryPath,
 /// This is useful in the base level of your program to allow clean termination
 /// (allowing clean deallocation of resources, etc.), while reporting error
 /// information to the user.
-void logAllUnhandledErrors(Error E, raw_ostream &OS, Twine ErrorBanner);
+void logAllUnhandledErrors(Error E, raw_ostream &OS, const Twine &ErrorBanner);
 
 /// Write all error messages (if any) in E to a string. The newline character
 /// is used to separate error messages.
diff --git a/include/llvm/Support/FormatVariadicDetails.h b/include/llvm/Support/FormatVariadicDetails.h
index b4a564ffc26c..9aa60089a80f 100644
--- a/include/llvm/Support/FormatVariadicDetails.h
+++ b/include/llvm/Support/FormatVariadicDetails.h
@@ -28,7 +28,7 @@ public:
 };
 
 template <typename T> class provider_format_adapter : public format_adapter {
-  T Item;
+  T &Item;
 
 public:
   explicit provider_format_adapter(T &&Item) : Item(Item) {}
diff --git a/lib/Object/Error.cpp b/lib/Object/Error.cpp
index 7d43a84f3e0e..d4ab91638861 100644
--- a/lib/Object/Error.cpp
+++ b/lib/Object/Error.cpp
@@ -60,9 +60,10 @@ std::string _object_error_category::message(int EV) const {
 char BinaryError::ID = 0;
 char GenericBinaryError::ID = 0;
 
-GenericBinaryError::GenericBinaryError(Twine Msg) : Msg(Msg.str()) {}
+GenericBinaryError::GenericBinaryError(const Twine &Msg) : Msg(Msg.str()) {}
 
-GenericBinaryError::GenericBinaryError(Twine Msg, object_error ECOverride)
+GenericBinaryError::GenericBinaryError(const Twine &Msg,
+                                       object_error ECOverride)
     : Msg(Msg.str()) {
   setErrorCode(make_error_code(ECOverride));
 }
diff --git a/lib/Support/Error.cpp b/lib/Support/Error.cpp
index bb02c03ff2b6..fe8ffd817f4e 100644
--- a/lib/Support/Error.cpp
+++ b/lib/Support/Error.cpp
@@ -54,7 +54,7 @@ char ErrorList::ID = 0;
 char ECError::ID = 0;
 char StringError::ID = 0;
 
-void logAllUnhandledErrors(Error E, raw_ostream &OS, Twine ErrorBanner) {
+void logAllUnhandledErrors(Error E, raw_ostream &OS, const Twine &ErrorBanner) {
   if (!E)
     return;
   OS << ErrorBanner;
diff --git a/lib/Support/Twine.cpp b/lib/Support/Twine.cpp
index d17cd4e66439..bf434f34e43c 100644
--- a/lib/Support/Twine.cpp
+++ b/lib/Support/Twine.cpp
@@ -120,12 +120,10 @@ void Twine::printOneChildRepr(raw_ostream &OS, Child Ptr,
        << Ptr.cString << "\"";
     break;
   case Twine::StdStringKind:
-    OS << "std::string:\""
-       << Ptr.stdString << "\"";
+    OS << "std::string:\"" << *Ptr.stdString << "\"";
     break;
   case Twine::StringRefKind:
-    OS << "stringref:\""
-       << Ptr.stringRef << "\"";
+    OS << "stringref:\"" << *Ptr.stringRef << "\"";
     break;
   case Twine::SmallStringKind:
     OS << "smallstring:\"" << *Ptr.smallString << "\"";
diff --git a/lib/Transforms/Scalar/SROA.cpp b/lib/Transforms/Scalar/SROA.cpp
index b968cb8c892b..c64c04087587 100644
--- a/lib/Transforms/Scalar/SROA.cpp
+++ b/lib/Transforms/Scalar/SROA.cpp
@@ -130,18 +130,15 @@ namespace {
 class IRBuilderPrefixedInserter : public IRBuilderDefaultInserter {
   std::string Prefix;
 
-  const Twine getNameWithPrefix(const Twine &Name) const {
-    return Name.isTriviallyEmpty() ? Name : Prefix + Name;
-  }
-
 public:
   void SetNamePrefix(const Twine &P) { Prefix = P.str(); }
 
 protected:
   void InsertHelper(Instruction *I, const Twine &Name, BasicBlock *BB,
                     BasicBlock::iterator InsertPt) const {
-    IRBuilderDefaultInserter::InsertHelper(I, getNameWithPrefix(Name), BB,
-                                           InsertPt);
+    const Twine &Prefixed = Prefix + Name;
+    IRBuilderDefaultInserter::InsertHelper(
+        I, Name.isTriviallyEmpty() ? Name : Prefixed, BB, InsertPt);
   }
 };
 
@@ -1355,7 +1352,8 @@ static void speculateSelectInstLoads(SelectInst &SI) {
 /// This will return the BasePtr if that is valid, or build a new GEP
 /// instruction using the IRBuilder if GEP-ing is needed.
 static Value *buildGEP(IRBuilderTy &IRB, Value *BasePtr,
-                       SmallVectorImpl<Value *> &Indices, Twine NamePrefix) {
+                       SmallVectorImpl<Value *> &Indices,
+                       const Twine &NamePrefix) {
   if (Indices.empty())
     return BasePtr;
 
@@ -1380,7 +1378,7 @@ static Value *buildGEP(IRBuilderTy &IRB, Value *BasePtr,
 static Value *getNaturalGEPWithType(IRBuilderTy &IRB, const DataLayout &DL,
                                     Value *BasePtr, Type *Ty, Type *TargetTy,
                                     SmallVectorImpl<Value *> &Indices,
-                                    Twine NamePrefix) {
+                                    const Twine &NamePrefix) {
   if (Ty == TargetTy)
     return buildGEP(IRB, BasePtr, Indices, NamePrefix);
 
@@ -1425,7 +1423,7 @@ static Value *getNaturalGEPRecursively(IRBuilderTy &IRB, const DataLayout &DL,
                                        Value *Ptr, Type *Ty, APInt &Offset,
                                        Type *TargetTy,
                                        SmallVectorImpl<Value *> &Indices,
-                                       Twine NamePrefix) {
+                                       const Twine &NamePrefix) {
   if (Offset == 0)
     return getNaturalGEPWithType(IRB, DL, Ptr, Ty, TargetTy, Indices,
                                  NamePrefix);
@@ -1498,7 +1496,7 @@ static Value *getNaturalGEPRecursively(IRBuilderTy &IRB, const DataLayout &DL,
 static Value *getNaturalGEPWithOffset(IRBuilderTy &IRB, const DataLayout &DL,
                                       Value *Ptr, APInt Offset, Type *TargetTy,
                                       SmallVectorImpl<Value *> &Indices,
-                                      Twine NamePrefix) {
+                                      const Twine &NamePrefix) {
   PointerType *Ty = cast<PointerType>(Ptr->getType());
 
   // Don't consider any GEPs through an i8* as natural unless the TargetTy is
@@ -1536,7 +1534,8 @@ static Value *getNaturalGEPWithOffset(IRBuilderTy &IRB, const DataLayout &DL,
 /// a single GEP as possible, thus making each GEP more independent of the
 /// surrounding code.
 static Value *getAdjustedPtr(IRBuilderTy &IRB, const DataLayout &DL, Value *Ptr,
-                             APInt Offset, Type *PointerTy, Twine NamePrefix) {
+                             APInt Offset, Type *PointerTy,
+                             const Twine &NamePrefix) {
   // Even though we don't look through PHI nodes, we could be called on an
   // instruction in an unreachable block, which may be on a cycle.
   SmallPtrSet<Value *, 4> Visited;
diff --git a/tools/llvm-nm/llvm-nm.cpp b/tools/llvm-nm/llvm-nm.cpp
index 4ad0d95d67f6..6b9ae4639794 100644
--- a/tools/llvm-nm/llvm-nm.cpp
+++ b/tools/llvm-nm/llvm-nm.cpp
@@ -195,12 +195,12 @@ bool HadError = false;
 std::string ToolName;
 } // anonymous namespace
 
-static void error(Twine Message, Twine Path = Twine()) {
+static void error(const Twine &Message, const Twine &Path = Twine()) {
   HadError = true;
   errs() << ToolName << ": " << Path << ": " << Message << ".\n";
 }
 
-static bool error(std::error_code EC, Twine Path = Twine()) {
+static bool error(std::error_code EC, const Twine &Path = Twine()) {
   if (EC) {
     error(EC.message(), Path);
     return true;
diff --git a/tools/llvm-objcopy/Object.cpp b/tools/llvm-objcopy/Object.cpp
index f9acf001ae93..f4c159cd1cb7 100644
--- a/tools/llvm-objcopy/Object.cpp
+++ b/tools/llvm-objcopy/Object.cpp
@@ -428,15 +428,15 @@ void initRelocations(RelocationSection<ELFT> *Relocs,
   }
 }
 
-SectionBase *SectionTableRef::getSection(uint16_t Index, Twine ErrMsg) {
+SectionBase *SectionTableRef::getSection(uint16_t Index, const Twine &ErrMsg) {
   if (Index == SHN_UNDEF || Index > Sections.size())
     error(ErrMsg);
   return Sections[Index - 1].get();
 }
 
 template <class T>
-T *SectionTableRef::getSectionOfType(uint16_t Index, Twine IndexErrMsg,
-                                     Twine TypeErrMsg) {
+T *SectionTableRef::getSectionOfType(uint16_t Index, const Twine &IndexErrMsg,
+                                     const Twine &TypeErrMsg) {
   if (T *Sec = llvm::dyn_cast<T>(getSection(Index, IndexErrMsg)))
     return Sec;
   error(TypeErrMsg);
diff --git a/tools/llvm-objcopy/Object.h b/tools/llvm-objcopy/Object.h
index f6088434805d..d266912db0b8 100644
--- a/tools/llvm-objcopy/Object.h
+++ b/tools/llvm-objcopy/Object.h
@@ -29,11 +29,11 @@ public:
       : Sections(Secs) {}
   SectionTableRef(const SectionTableRef &) = default;
 
-  SectionBase *getSection(uint16_t Index, llvm::Twine ErrMsg);
+  SectionBase *getSection(uint16_t Index, const llvm::Twine &ErrMsg);
 
   template <class T>
-  T *getSectionOfType(uint16_t Index, llvm::Twine IndexErrMsg,
-                      llvm::Twine TypeErrMsg);
+  T *getSectionOfType(uint16_t Index, const llvm::Twine &IndexErrMsg,
+                      const llvm::Twine &TypeErrMsg);
 };
 
 class SectionBase {
diff --git a/tools/llvm-objcopy/llvm-objcopy.cpp b/tools/llvm-objcopy/llvm-objcopy.cpp
index 7f55a434b334..9fc2897959c5 100644
--- a/tools/llvm-objcopy/llvm-objcopy.cpp
+++ b/tools/llvm-objcopy/llvm-objcopy.cpp
@@ -27,7 +27,7 @@ static StringRef ToolName;
 
 namespace llvm {
 
-LLVM_ATTRIBUTE_NORETURN void error(Twine Message) {
+LLVM_ATTRIBUTE_NORETURN void error(const Twine &Message) {
   errs() << ToolName << ": " << Message << ".\n";
   errs().flush();
   exit(1);
diff --git a/tools/llvm-objcopy/llvm-objcopy.h b/tools/llvm-objcopy/llvm-objcopy.h
index de7bf367ac87..d30b43a46a4f 100644
--- a/tools/llvm-objcopy/llvm-objcopy.h
+++ b/tools/llvm-objcopy/llvm-objcopy.h
@@ -14,7 +14,7 @@
 
 namespace llvm {
 
-LLVM_ATTRIBUTE_NORETURN extern void error(Twine Message);
+LLVM_ATTRIBUTE_NORETURN extern void error(const Twine &Message);
 
 // This is taken from llvm-readobj.
 // [see here](llvm/tools/llvm-readobj/llvm-readobj.h:38)
diff --git a/unittests/ADT/TwineTest.cpp b/unittests/ADT/TwineTest.cpp
index 950eda2b058a..ebe06917cee6 100644
--- a/unittests/ADT/TwineTest.cpp
+++ b/unittests/ADT/TwineTest.cpp
@@ -88,6 +88,14 @@ TEST(TwineTest, Concat) {
             repr(Twine("a").concat(Twine(SmallString<3>("b")).concat(Twine("c")))));
 }
 
+TEST(TwineTest, Operators) {
+  EXPECT_EQ(R"((Twine cstring:"a" stringref:"b"))", repr("a" + StringRef("b")));
+
+  EXPECT_EQ(R"((Twine stringref:"a" cstring:"b"))", repr(StringRef("a") + "b"));
+  EXPECT_EQ(R"((Twine stringref:"a" stringref:"b"))",
+            repr(StringRef("a") + StringRef("b")));
+}
+
 TEST(TwineTest, toNullTerminatedStringRef) {
   SmallString<8> storage;
   EXPECT_EQ(0, *Twine("hello").toNullTerminatedStringRef(storage).end());
diff --git a/utils/TableGen/RegisterBankEmitter.cpp b/utils/TableGen/RegisterBankEmitter.cpp
index 293933ffb8d2..36f66f4fe8bd 100644
--- a/utils/TableGen/RegisterBankEmitter.cpp
+++ b/utils/TableGen/RegisterBankEmitter.cpp
@@ -169,7 +169,7 @@ void RegisterBankEmitter::emitBaseClassDefinition(
 ///                to the class.
 static void visitRegisterBankClasses(
     CodeGenRegBank &RegisterClassHierarchy, const CodeGenRegisterClass *RC,
-    const Twine Kind,
+    const Twine &Kind,
     std::function<void(const CodeGenRegisterClass *, StringRef)> VisitFn,
     SmallPtrSetImpl<const CodeGenRegisterClass *> &VisitedRCs) {
 
@@ -183,7 +183,7 @@ static void visitRegisterBankClasses(
 
   for (const auto &PossibleSubclass : RegisterClassHierarchy.getRegClasses()) {
     std::string TmpKind =
-        (Twine(Kind) + " (" + PossibleSubclass.getName() + ")").str();
+        (Kind + " (" + PossibleSubclass.getName() + ")").str();
 
     // Visit each subclass of an explicitly named class.
     if (RC != &PossibleSubclass && RC->hasSubClass(&PossibleSubclass))