bug33388 - Fix formatv_objet copy & move constructors

[belleyb]

Summary

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 move. The copy constructor isn't useful for format objects and can thus be deleted.

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

[belleyb]

unittests/Support/FormatVariadicTest.cpp

555 ↗

(On Diff #103416)

The following tests are failing without the fix and passing with it...

[zturner]

I'm wondering if we shouldn't just delete the copy constructor. Definitely having a move constructor seems reasonable, but I don't see any reason why we should ever be copying format objects. What do you think?

[belleyb]

I wonder the exact same thing! We have no use for a copy constructor ourselves. Maybe, it's safer to delete it for now. We can always re-introduce it back if a clear need shows up.

[zturner]

Cool, lgtm after you delete the copy constructor.

[belleyb]

Delete the formatv_object copy constructor as suggested @zturner.

[belleyb]

include/llvm/Support/FormatVariadic.h

97 ↗

(On Diff #103623)

Deleted copy constructor!

162 ↗

(On Diff #103623)

Deleted copy constructor!

[belleyb]

@zturner : Would you mind committing on my behalf ? (I don't have commit access...)

[zturner]

Sure, thanks for the fix.

llvm/trunk/include/llvm/Support/FormatVariadic.h

protected:
   splitLiteralAndReplacement(StringRef Fmt);
 
 public:
   formatv_object_base(StringRef Fmt, std::size_t ParamCount)
       : Fmt(Fmt), Replacements(parseFormatString(Fmt)) {
     Adapters.reserve(ParamCount);
   }
 
+  formatv_object_base(formatv_object_base const &rhs) = delete;
+
+  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)
         continue;
       if (R.Type == ReplacementType::Literal) {
         S << R.Spec;
         continue;
       }
template <typename Tuple> class formatv_object : public formatv_object_base {
   Tuple Parameters;
 
 public:
   formatv_object(StringRef Fmt, Tuple &&Params)
       : formatv_object_base(Fmt, std::tuple_size<Tuple>::value),
         Parameters(std::move(Params)) {
     Adapters = apply_tuple(create_adapters(), Parameters);
   }
+
+  formatv_object(formatv_object const &rhs) = delete;
+
+  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.
 //
 // ===General Description===
 //
 // Formats textual output.  `Fmt` is a string consisting of one or more
 // replacement sequences with the following grammar:

llvm/trunk/unittests/Support/FormatVariadicTest.cpp

TEST(FormatVariadicTest, Adapter) {
   EXPECT_EQ("171171171171171", formatv("{0}", fmt_repeat(N, 5)).str());
 
   EXPECT_EQ(" ABABABABAB   ",
             formatv("{0:X-}", fmt_pad(fmt_repeat(N, 5), 1, 3)).str());
   EXPECT_EQ("   AB    AB    AB    AB    AB     ",
             formatv("{0,=34:X-}", fmt_repeat(fmt_pad(N, 1, 3), 5)).str());
 }
 
+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);
 
   SmallString<4> S2 = formatv("{0} {1}", 1, 2);
   EXPECT_EQ("1 2", S2);
 }