[lld] r277208 - [lld][MachO] Replace some std::string with char* buffers to eliminate mem leaks.
Lang Hames via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 1 15:38:25 PDT 2016
Hi Dave,
You could avoid the extra implicit strlen from the const char*->StringRef
> conversion in these cases by passing the known length explicitly, if you
> like.
Nice catch. I don't think it's worth optimizing this code though: it's a
workaround for the poor memory-ownership model in the YAML test
infrastructure. We're hoping to fix the underlying issue and back this out
again in the not too distant future.
Also, do you need the null terminator, if you're passing it around as a
> StringRef?
Maybe, but it would require some careful analysis to make sure we're not
interoperating with APIs that expect a char*.
Looks like the first one, if you don't need the null terminator, could be
> replaced with:
> return str.copy(alloc);
That's a nifty API. It might be handy to add an argument to copy the
null-terminator too, again for easy API interoperability.
- Lang.
On Mon, Aug 1, 2016 at 10:59 AM, David Blaikie <dblaikie at gmail.com> wrote:
> You could avoid the extra implicit strlen from the const char*->StringRef
> conversion in these cases by passing the known length explicitly, if you
> like.
>
> Also, do you need the null terminator, if you're passing it around as a
> StringRef?
>
> Looks like the first one, if you don't need the null terminator, could be
> replaced with:
>
> return str.copy(alloc);
>
> & the second one still needs a bit of complexity for the '/' appending.
>
>
> On Fri, Jul 29, 2016 at 1:11 PM Lang Hames via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: lhames
>> Date: Fri Jul 29 15:04:18 2016
>> New Revision: 277208
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=277208&view=rev
>> Log:
>> [lld][MachO] Replace some std::string with char* buffers to eliminate mem
>> leaks.
>>
>> The MachO debug support code (committed in r276935) occasionally needs to
>> allocate string copies, and was doing so by creating std::strings on a
>> BumpPtrAllocator. The strings were untracked, so the destructors weren't
>> being
>> run and we were leaking the memory when the allocator was thrown away.
>> Since
>> it's easier than tracking the strings, this patch switches the copies to
>> char
>> buffers allocated directly in the bump-ptr allocator.
>>
>>
>> Modified:
>> lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp
>> lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp
>>
>> Modified:
>> lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp?rev=277208&r1=277207&r2=277208&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp
>> (original)
>> +++ lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp Fri
>> Jul 29 15:04:18 2016
>> @@ -870,10 +870,12 @@ llvm::Error Util::synthesizeDebugNotes(N
>>
>> // If newDirPath doesn't end with a '/' we need to add one:
>> if (newDirPath.back() != '/') {
>> - std::string *p = file.ownedAllocations.Allocate<std::string>();
>> - new (p) std::string();
>> - *p = (newDirPath + "/").str();
>> - newDirPath = *p;
>> + char *p =
>> + file.ownedAllocations.Allocate<char>(newDirPath.size() + 2);
>> + memcpy(p, newDirPath.data(), newDirPath.size());
>> + p[newDirPath.size()] = '/';
>> + p[newDirPath.size() + 1] = '\0';
>> + newDirPath = p;
>> }
>>
>> // New translation unit, emit start SOs:
>> @@ -881,24 +883,24 @@ llvm::Error Util::synthesizeDebugNotes(N
>> _stabs.push_back(mach_o::Stab(nullptr, N_SO, 0, 0, 0,
>> newFileName));
>>
>> // Synthesize OSO for start of file.
>> - std::string *fullPath =
>> file.ownedAllocations.Allocate<std::string>();
>> - new (fullPath) std::string();
>> + char *fullPath = nullptr;
>> {
>> SmallString<1024> pathBuf(atomFile.path());
>> if (auto EC = llvm::sys::fs::make_absolute(pathBuf))
>> return llvm::errorCodeToError(EC);
>> - *fullPath = pathBuf.str();
>> + fullPath = file.ownedAllocations.Allocate<char>(pathBuf.size() +
>> 1);
>> + memcpy(fullPath, pathBuf.c_str(), pathBuf.size() + 1);
>> }
>>
>> // Get mod time.
>> uint32_t modTime = 0;
>> llvm::sys::fs::file_status stat;
>> - if (!llvm::sys::fs::status(*fullPath, stat))
>> + if (!llvm::sys::fs::status(fullPath, stat))
>> if (llvm::sys::fs::exists(stat))
>> modTime = stat.getLastModificationTime().toEpochTime();
>>
>> _stabs.push_back(mach_o::Stab(nullptr, N_OSO,
>> _ctx.getCPUSubType(), 1,
>> - modTime, *fullPath));
>> + modTime, fullPath));
>> // <rdar://problem/6337329> linker should put cpusubtype in n_sect
>> field
>> // of nlist entry for N_OSO debug note entries.
>> wroteStartSO = true;
>>
>> Modified: lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp?rev=277208&r1=277207&r2=277208&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp
>> (original)
>> +++ lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp Fri
>> Jul 29 15:04:18 2016
>> @@ -702,10 +702,10 @@ static const Atom* findDefinedAtomByName
>> }
>>
>> static StringRef copyDebugString(StringRef str, BumpPtrAllocator &alloc)
>> {
>> - std::string *strCopy = alloc.Allocate<std::string>();
>> - new (strCopy) std::string();
>> - *strCopy = str;
>> - return *strCopy;
>> + char *strCopy = alloc.Allocate<char>(str.size() + 1);
>> + memcpy(strCopy, str.data(), str.size());
>> + strCopy[str.size()] = '\0';
>> + return strCopy;
>> }
>>
>> llvm::Error parseStabs(MachOFile &file,
>>
>>
>> _______________________________________________
>> 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/20160801/56d22a0d/attachment.html>
More information about the llvm-commits
mailing list