[lld] r277208 - [lld][MachO] Replace some std::string with char* buffers to eliminate mem leaks.

[David Blaikie via llvm-commits]

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/97c8a0fa/attachment.html>