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

[Benoit Belley via llvm-commits]

Hi Hans,

It would be nice if you could. But, I wouldn't delay the release just for
this!

AFAICT, the -fno-elide-constructors compiler flag must be used to
reproduce this issue (which is an atypical usage pattern). It would also
be fine to wait for the 5.0.1 release to include this fix.

It would also be nice to include the following fix in 5.0.1:

    URL: http://llvm.org/viewvc/llvm-project?rev=310522&view=rev
    Log: [Linker] PR33527 - Linker::LinkOnlyNeeded should import
AppendingLinkage globals

    Linker::LinkOnlyNeeded should always import globals with
    AppendingLinkage.

    This resolves PR33527.

    Differential Revision: https://reviews.llvm.org/D34448


Thanks,
Benoit

-------------------
Benoit Belley
Sr Principal Developer
M&E-Product Development Group
 
MAIN +1 514 393 1616
DIRECT +1 438 448 6304
FAX +1 514 393 0110
 
Twitter <http://twitter.com/autodesk>
Facebook <https://www.facebook.com/Autodesk>
 
Autodesk, Inc.
10 Duke Street
Montreal, Quebec, Canada H3C 2L7
www.autodesk.com <http://www.autodesk.com/>
 



On 17-08-10 17:26, "hwennborg at google.com on behalf of Hans Wennborg"
<hwennborg at google.com on behalf of hans at chromium.org> wrote:

>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/Forma
>>tVariadic.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/FormatVa
>>riadicTest.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