[llvm] r310475 - [Support] PR33388 - Fix formatv_object move constructor

[Hans Wennborg via llvm-commits]

Should we merge this to 5.0?

On Wed, Aug 9, 2017 at 6:47 AM, Benoit Belley via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> Author: belleyb
> Date: Wed Aug  9 06:47:01 2017
> New Revision: 310475
>
> URL: http://llvm.org/viewvc/llvm-project?rev=310475&view=rev
> Log:
> [Support] PR33388 - Fix formatv_object move constructor
>
> formatv_object currently uses the implicitly defined move constructor,
> but it is buggy. In typical use-cases, the problem doesn't show-up
> because all calls to the move constructor 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, thus making the
> parameters occupy a new memory location 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.
>
> This resolves PR33388.
>
> Differential Revision: https://reviews.llvm.org/D34463
>
> Modified:
>     llvm/trunk/include/llvm/Support/FormatVariadic.h
>     llvm/trunk/unittests/Support/FormatVariadicTest.cpp
>
> Modified: llvm/trunk/include/llvm/Support/FormatVariadic.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/FormatVariadic.h?rev=310475&r1=310474&r2=310475&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/FormatVariadic.h (original)
> +++ llvm/trunk/include/llvm/Support/FormatVariadic.h Wed Aug  9 06:47:01 2017
> @@ -94,6 +94,15 @@ public:
>      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)
> @@ -149,6 +158,14 @@ public:
>          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.
>
> Modified: llvm/trunk/unittests/Support/FormatVariadicTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/FormatVariadicTest.cpp?rev=310475&r1=310474&r2=310475&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/Support/FormatVariadicTest.cpp (original)
> +++ llvm/trunk/unittests/Support/FormatVariadicTest.cpp Wed Aug  9 06:47:01 2017
> @@ -553,6 +553,12 @@ TEST(FormatVariadicTest, Adapter) {
>              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);
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits