[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 f5f00dcfafe5f8b5c9346f6421ee6d076ed97e03..c9cefa982d63a23f2d570976fe73371b02d63ae8 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 020b67d6b71107524fc8cc78f681b624adafc89f..0ce9c2f9b11cc869ebda4b3e76475057e7ebbcd1 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 eb938338715d64793dd3eed8e725975c2097869c..ef820bdba31864fef4861e40cafb2c39d01765b3 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 05fe10a770e0204087bc467ad40e38e259851425..bf17ad3f8a85db0c14f7eecae92ef27339c79df0 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 bb40dbee053f044686c495c0438c265c2c5fb20f..63d441fa58e8ea966ed11844ba33b58ec367ca88 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 b4a564ffc26c637c8c4a0069bdaf28261039aea7..9aa60089a80f03791eea8dcca32a52db02917cee 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 7d43a84f3e0e069e625b5e8c35d53a47a822f568..d4ab9163886174aca7ee4b5a8f28bd5ed8b6cee5 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 bb02c03ff2b6b5b9a83ab38f56cf45d68de77702..fe8ffd817f4e05bc077eab92778a9228aec143e4 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 d17cd4e66439ca326f8c03a12ebd1cef576a03c2..bf434f34e43c35b8946a6f03ab1f02dc8e8b5e8c 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 b968cb8c892b05c94cb571db9c64eab1fe1afb71..c64c04087587ea299d7daa570164bfaab86c4840 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 4ad0d95d67f6625b86b88d4ab2254f1b6d9754c0..6b9ae46397943ec2eaffccbe6d7d4721deb012f8 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 f9acf001ae93be08812c5e360080337c84ced675..f4c159cd1cb77b9b4fbdec7b3801d2a64127f712 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 f6088434805d62588d7bf1386f682cacc573f7b0..d266912db0b8ef5a239926f8cb1a15941454fa54 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 7f55a434b334423007d148b780a65889859f4e97..9fc2897959c5aafd6a2d151494f6af16708c7d7d 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 de7bf367ac87b46293bb51eeed07c4985c481cf5..d30b43a46a4f1beff352639d46cc20b48a109262 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 950eda2b058ad0b54e1f10133adc6e9090c806c4..ebe06917cee69132566cec98ca0fc145e9b150fd 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 293933ffb8d27f5557d4c0970a38eccd2559d8e5..36f66f4fe8bdadc7c3602b44ae6d9e583c8c7351 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))