[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

diff --git a/include/llvm/ADT/Twine.h b/include/llvm/ADT/Twine.h
index f5f00dc..c9cefa9 100644 (file)
--- 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 020b67d..0ce9c2f 100644 (file)
--- 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 eb93833..ef820bd 100644 (file)
--- 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 05fe10a..bf17ad3 100644 (file)
--- 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 bb40dbe..63d441f 100644 (file)
--- 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 b4a564f..9aa6008 100644 (file)
--- 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 7d43a84..d4ab916 100644 (file)
--- 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 bb02c03..fe8ffd8 100644 (file)
--- 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 d17cd4e..bf434f3 100644 (file)
--- 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 b968cb8..c64c040 100644 (file)
--- 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 4ad0d95..6b9ae46 100644 (file)
--- 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 f9acf00..f4c159c 100644 (file)
--- 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 f608843..d266912 100644 (file)
--- 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 7f55a43..9fc2897 100644 (file)
--- 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 de7bf36..d30b43a 100644 (file)
--- 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 950eda2..ebe0691 100644 (file)
--- 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 293933f..36f66f4 100644 (file)
--- 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))