<div dir="ltr">Hi Dave,<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-size:12.800000190734863px">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.</span></blockquote><div><span style="font-size:12.800000190734863px"><br></span></div><div><span style="font-size:12.800000190734863px">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.</span></div><div><span style="font-size:12.800000190734863px"><br></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-size:12.800000190734863px">Also, do you need the null terminator, if you're passing it around as a StringRef?</span></blockquote><div><span style="font-size:12.800000190734863px"><br></span></div><div><span style="font-size:12.800000190734863px"> Maybe, but it would require some careful analysis to make sure we're not interoperating with APIs that expect a char*.</span><br></div><div><span style="font-size:12.800000190734863px"><br></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-size:12.800000190734863px">Looks like the first one, if you don't need the null terminator, could be replaced with:</span><br style="font-size:12.800000190734863px"><span style="font-size:12.800000190734863px">return str.copy(alloc);</span></blockquote><div><span style="font-size:12.800000190734863px"><br></span></div><div><span style="font-size:12.800000190734863px">That's a nifty API. It might be handy to add an argument to copy the null-terminator too, again for easy API interoperability.</span></div><div><span style="font-size:12.800000190734863px"><br></span></div><div><span style="font-size:12.800000190734863px">- Lang.</span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 1, 2016 at 10:59 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.<br><br>Also, do you need the null terminator, if you're passing it around as a StringRef?<br><br>Looks like the first one, if you don't need the null terminator, could be replaced with:<br><br>return str.copy(alloc);<br><br>& the second one still needs a bit of complexity for the '/' appending.<div><div class="h5"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jul 29, 2016 at 1:11 PM Lang Hames via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: lhames<br>
Date: Fri Jul 29 15:04:18 2016<br>
New Revision: 277208<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=277208&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=277208&view=rev</a><br>
Log:<br>
[lld][MachO] Replace some std::string with char* buffers to eliminate mem leaks.<br>
<br>
The MachO debug support code (committed in r276935) occasionally needs to<br>
allocate string copies, and was doing so by creating std::strings on a<br>
BumpPtrAllocator. The strings were untracked, so the destructors weren't being<br>
run and we were leaking the memory when the allocator was thrown away. Since<br>
it's easier than tracking the strings, this patch switches the copies to char<br>
buffers allocated directly in the bump-ptr allocator.<br>
<br>
<br>
Modified:<br>
lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp<br>
lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp?rev=277208&r1=277207&r2=277208&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp?rev=277208&r1=277207&r2=277208&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp (original)<br>
+++ lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp Fri Jul 29 15:04:18 2016<br>
@@ -870,10 +870,12 @@ llvm::Error Util::synthesizeDebugNotes(N<br>
<br>
// If newDirPath doesn't end with a '/' we need to add one:<br>
if (newDirPath.back() != '/') {<br>
- std::string *p = file.ownedAllocations.Allocate<std::string>();<br>
- new (p) std::string();<br>
- *p = (newDirPath + "/").str();<br>
- newDirPath = *p;<br>
+ char *p =<br>
+ file.ownedAllocations.Allocate<char>(newDirPath.size() + 2);<br>
+ memcpy(p, newDirPath.data(), newDirPath.size());<br>
+ p[newDirPath.size()] = '/';<br>
+ p[newDirPath.size() + 1] = '\0';<br>
+ newDirPath = p;<br>
}<br>
<br>
// New translation unit, emit start SOs:<br>
@@ -881,24 +883,24 @@ llvm::Error Util::synthesizeDebugNotes(N<br>
_stabs.push_back(mach_o::Stab(nullptr, N_SO, 0, 0, 0, newFileName));<br>
<br>
// Synthesize OSO for start of file.<br>
- std::string *fullPath = file.ownedAllocations.Allocate<std::string>();<br>
- new (fullPath) std::string();<br>
+ char *fullPath = nullptr;<br>
{<br>
SmallString<1024> pathBuf(atomFile.path());<br>
if (auto EC = llvm::sys::fs::make_absolute(pathBuf))<br>
return llvm::errorCodeToError(EC);<br>
- *fullPath = pathBuf.str();<br>
+ fullPath = file.ownedAllocations.Allocate<char>(pathBuf.size() + 1);<br>
+ memcpy(fullPath, pathBuf.c_str(), pathBuf.size() + 1);<br>
}<br>
<br>
// Get mod time.<br>
uint32_t modTime = 0;<br>
llvm::sys::fs::file_status stat;<br>
- if (!llvm::sys::fs::status(*fullPath, stat))<br>
+ if (!llvm::sys::fs::status(fullPath, stat))<br>
if (llvm::sys::fs::exists(stat))<br>
modTime = stat.getLastModificationTime().toEpochTime();<br>
<br>
_stabs.push_back(mach_o::Stab(nullptr, N_OSO, _ctx.getCPUSubType(), 1,<br>
- modTime, *fullPath));<br>
+ modTime, fullPath));<br>
// <rdar://problem/6337329> linker should put cpusubtype in n_sect field<br>
// of nlist entry for N_OSO debug note entries.<br>
wroteStartSO = true;<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp?rev=277208&r1=277207&r2=277208&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp?rev=277208&r1=277207&r2=277208&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp (original)<br>
+++ lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp Fri Jul 29 15:04:18 2016<br>
@@ -702,10 +702,10 @@ static const Atom* findDefinedAtomByName<br>
}<br>
<br>
static StringRef copyDebugString(StringRef str, BumpPtrAllocator &alloc) {<br>
- std::string *strCopy = alloc.Allocate<std::string>();<br>
- new (strCopy) std::string();<br>
- *strCopy = str;<br>
- return *strCopy;<br>
+ char *strCopy = alloc.Allocate<char>(str.size() + 1);<br>
+ memcpy(strCopy, str.data(), str.size());<br>
+ strCopy[str.size()] = '\0';<br>
+ return strCopy;<br>
}<br>
<br>
llvm::Error parseStabs(MachOFile &file,<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div></div>
</blockquote></div><br></div>