[llvm] r307085 - Fix dangling StringRefs found by clang-tidy misc-dangling-handle check.

[Zachary Turner via llvm-commits]

Also agree, original code is a bug. Is it more complicated than just
changing the return type and handful of isolated call sites?
On Tue, Jul 11, 2017 at 8:22 AM David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Jul 11, 2017 at 3:44 AM Alexander Kornienko <alexfh at google.com>
> wrote:
>
>> On Mon, Jul 10, 2017 at 7:15 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Wed, Jul 5, 2017 at 8:02 AM Alexander Kornienko via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> Author: alexfh
>>>> Date: Tue Jul  4 08:13:02 2017
>>>> New Revision: 307085
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=307085&view=rev
>>>> Log:
>>>> Fix dangling StringRefs found by clang-tidy misc-dangling-handle check.
>>>>
>>>> Modified:
>>>>     llvm/trunk/tools/llvm-lto/llvm-lto.cpp
>>>>     llvm/trunk/tools/llvm-readobj/COFFDumper.cpp
>>>>
>>>> Modified: llvm/trunk/tools/llvm-lto/llvm-lto.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-lto/llvm-lto.cpp?rev=307085&r1=307084&r2=307085&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/tools/llvm-lto/llvm-lto.cpp (original)
>>>> +++ llvm/trunk/tools/llvm-lto/llvm-lto.cpp Tue Jul  4 08:13:02 2017
>>>> @@ -383,7 +383,7 @@ loadAllFilesForIndex(const ModuleSummary
>>>>
>>>>    for (auto &ModPath : Index.modulePaths()) {
>>>>      const auto &Filename = ModPath.first();
>>>> -    auto CurrentActivity = "loading file '" + Filename + "'";
>>>> +    std::string CurrentActivity = ("loading file '" + Filename +
>>>> "'").str();
>>>>      auto InputOrErr = MemoryBuffer::getFile(Filename);
>>>>      error(InputOrErr, "error " + CurrentActivity);
>>>>      InputBuffers.push_back(std::move(*InputOrErr));
>>>> @@ -475,7 +475,7 @@ private:
>>>>      std::vector<std::unique_ptr<MemoryBuffer>> InputBuffers;
>>>>      for (unsigned i = 0; i < InputFilenames.size(); ++i) {
>>>>        auto &Filename = InputFilenames[i];
>>>> -      StringRef CurrentActivity = "loading file '" + Filename + "'";
>>>> +      std::string CurrentActivity = "loading file '" + Filename + "'";
>>>>        auto InputOrErr = MemoryBuffer::getFile(Filename);
>>>>        error(InputOrErr, "error " + CurrentActivity);
>>>>        InputBuffers.push_back(std::move(*InputOrErr));
>>>> @@ -710,7 +710,7 @@ private:
>>>>      std::vector<std::unique_ptr<MemoryBuffer>> InputBuffers;
>>>>      for (unsigned i = 0; i < InputFilenames.size(); ++i) {
>>>>        auto &Filename = InputFilenames[i];
>>>> -      StringRef CurrentActivity = "loading file '" + Filename + "'";
>>>> +      std::string CurrentActivity = "loading file '" + Filename + "'";
>>>>        auto InputOrErr = MemoryBuffer::getFile(Filename);
>>>>        error(InputOrErr, "error " + CurrentActivity);
>>>>        InputBuffers.push_back(std::move(*InputOrErr));
>>>>
>>>> Modified: llvm/trunk/tools/llvm-readobj/COFFDumper.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-readobj/COFFDumper.cpp?rev=307085&r1=307084&r2=307085&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/tools/llvm-readobj/COFFDumper.cpp (original)
>>>> +++ llvm/trunk/tools/llvm-readobj/COFFDumper.cpp Tue Jul  4 08:13:02
>>>> 2017
>>>> @@ -1637,7 +1637,11 @@ static StringRef getBaseRelocTypeName(ui
>>>>    case COFF::IMAGE_REL_BASED_HIGHADJ: return "HIGHADJ";
>>>>    case COFF::IMAGE_REL_BASED_ARM_MOV32T: return "ARM_MOV32(T)";
>>>>    case COFF::IMAGE_REL_BASED_DIR64: return "DIR64";
>>>> -  default: return "unknown (" + llvm::utostr(Type) + ")";
>>>> +  default: {
>>>> +    static std::string Result;
>>>> +    Result = "unknown (" + llvm::utostr(Type) + ")";
>>>> +    return Result;
>>>>
>>>
>>> This looks concerning/wrong. Now two callers tot his API could interfere
>>> with each other (the returned StringRef is only valid until the function is
>>> called again)
>>>
>>> Perhaps this function should return std::string instead.
>>>
>>
>> The motivation for this solution is to keep the cheap path cheap (and the
>> expensive one isn't going to happen under normal circumstances anyway).
>>
>
> This is in readobj, and retrieving the string representation of something,
> presumably to show to a user - so I'm not sure the performance is that
> sensitive.
>
> I think it's probably best to err on the side of unsurprising API until
> there's a need to performance tune this to that degree. Put another way:
> This doesn't seem like the way this code should've been written in the
> first place. I'd only expect to see subtle code like this if it were
> particularly strongly motivated by performance data, etc. (& even then,
> probably would use a local std::string in the caller, passed in by
> reference to be populated on the side when needed - I'd expect default
> construction of a std::string to be cheap enough to not interfere with perf
> requirements)
>
>
>> I've inspected the only caller, and it's not going to cause such issues.
>> However, I should have probably added a FIXME about this or somehow draw
>> the attention of someone who cares about this code to fix it properly.
>>
>>
>>>
>>>
>>>> +  }
>>>>    }
>>>>  }
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170711/bbf52929/attachment.html>