[PATCH] D34463: bug33388 - Fix formatv_objet copy & move constructors

[Benoit Belley via Phabricator via llvm-commits]

belleyb created this revision.

`formatv_object` currently uses the implicitly defined move and copy constructors. It turns out these are buggy. In typical use-cases, the problem doesn't show-up because every single call to the move and copy constructors are elided. Thus, the buggy constructors are never invoked.

The issue especially shows-up when code is compiled using the `-fno-elide-constructors` compiler flag. For instance, this is useful when attempting to collect accurate code coverage statistics.

The exact issue is the following:

The Parameters data member is correctly moved or copied, thus making the parameters occupy new memory locations in the target object. Unfortunately, the default copying of the Adapters blindly copies the vector of pointers, leaving each of these pointers referencing the parameters in the original object instead of the copied one. These pointers quickly become dangling when the original object is deleted. This quickly leads to crashes.

The solution is to update the Adapters pointers when performing a copy or a move.

See: https://bugs.llvm.org/show_bug.cgi?id=33388

Contribution made by Autodesk Inc. under the terms of the University of Illinois/NCSA Open Source License.


https://reviews.llvm.org/D34463

Files:
  include/llvm/Support/FormatVariadic.h
  unittests/Support/FormatVariadicTest.cpp


Index: unittests/Support/FormatVariadicTest.cpp
===================================================================
--- unittests/Support/FormatVariadicTest.cpp
+++ unittests/Support/FormatVariadicTest.cpp
@@ -553,6 +553,19 @@
             formatv("{0,=34:X-}", fmt_repeat(fmt_pad(N, 1, 3), 5)).str());
 }
 
+TEST(FormatVariadicTest, CopyConstructor) {
+  auto fmt = formatv("{0} {1}", 1, 2);
+  auto fmt2 = fmt;
+  std::string S = fmt2;
+  EXPECT_EQ("1 2", S);
+}
+
+TEST(FormatVariadicTest, MoveConstructor) {
+  auto fmt = formatv("{0} {1}", 1, 2);
+  auto fmt2 = std::move(fmt);
+  std::string S = fmt2;
+  EXPECT_EQ("1 2", S);
+}
 TEST(FormatVariadicTest, ImplicitConversions) {
   std::string S = formatv("{0} {1}", 1, 2);
   EXPECT_EQ("1 2", S);
Index: include/llvm/Support/FormatVariadic.h
===================================================================
--- include/llvm/Support/FormatVariadic.h
+++ include/llvm/Support/FormatVariadic.h
@@ -94,6 +94,19 @@
     Adapters.reserve(ParamCount);
   }
 
+  formatv_object_base(formatv_object_base const &rhs)
+      : Fmt(rhs.Fmt), Adapters(), // Adapters are initialized by formatv_object
+        Replacements(rhs.Replacements) {
+    Adapters.reserve(rhs.Adapters.size());
+  };
+
+  formatv_object_base(formatv_object_base &&rhs)
+      : Fmt(std::move(rhs.Fmt)),
+        Adapters(), // Adapters are initialized by formatv_object
+        Replacements(std::move(rhs.Replacements)) {
+    Adapters.reserve(rhs.Adapters.size());
+  };
+
   void format(raw_ostream &S) const {
     for (auto &R : Replacements) {
       if (R.Type == ReplacementType::Empty)
@@ -149,6 +162,17 @@
         Parameters(std::move(Params)) {
     Adapters = apply_tuple(create_adapters(), Parameters);
   }
+
+  formatv_object(formatv_object const &rhs)
+      : formatv_object_base(rhs), Parameters(rhs.Parameters) {
+    Adapters = apply_tuple(create_adapters(), Parameters);
+  }
+
+  formatv_object(formatv_object &&rhs)
+      : formatv_object_base(std::move(rhs)),
+        Parameters(std::move(rhs.Parameters)) {
+    Adapters = apply_tuple(create_adapters(), Parameters);
+  }
 };
 
 // \brief Format text given a format string and replacement parameters.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D34463.103416.patch
Type: text/x-patch
Size: 2201 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170621/8dcb0653/attachment.bin>