[lldb] Fix RichManglingContext::FromCxxMethodName() leak

[rupprecht]

Summary

RichManglingContext::FromCxxMethodName allocates a m_cxx_method_parser, but never deletes it.

This fixes a -DLLVM_USE_SANITIZER=Leaks failure.

[shafik]

lldb/source/Core/RichManglingContext.cpp

24

This code is duplicated from ResetProvider(...) we should factor it out and call it from both places, so we if we ever change this code in the future we don't have to remember to fix both places.

[rupprecht]

• Refactor cleanup of m_cxx_method_parser

[rupprecht]

• Move comment about switch to the correct spot

[teemperor]

LGTM, thanks for fixing this!

We don't really have a thread for keep track of what other leak sanitiser failures needs to be fixed, but I believe with this revision (+ the revisions that I have to land) the only leaks left are related to reproducers or Python.

[JDevlieghere]

lldb/source/Core/RichManglingContext.cpp

23

Instead of managing memory by hand, can we use a unique_ptr<char[]> instead?

[rupprecht]

lldb/source/Core/RichManglingContext.cpp

23

The buffer here is created by malloc, and from a cursory reading of processIPDStrResult, can be passed to other methods that call realloc on it. It should be paired with free, not delete. You could put that in a unique_ptr<char, FreeDeleter>, but when you go down that road, I think it's probably simpler to leave as-is. (You'd also have to take care to always manually .release() it when updating it to the realloc'd value, because you don't want to delete the pre-realloc'd buffer).

(Note: this line is pulled from the original ~RichManglingContext() definition in the header. I didn't write it so I can't defend it that well :) )

[shafik]

Thank you for reforactoring into ResetCxxMethodParser.

lldb/source/Core/RichManglingContext.cpp

23

I didn't suggest this b/c it was clear it was a quick fix and it seemed a reach to ask for that in this fix.

[JDevlieghere]

lldb/source/Core/RichManglingContext.cpp

23

Thanks for the explanation! That does sound like a bit of overkill. If it's not already documented, would it be useful to leave that as a comment somewhere?

[dblaikie]

lldb/source/Core/RichManglingContext.cpp

23

(I think for this patch, makes sense not to try to address the ownership/allocation model because it is non-trivial, as you've mentioned - but longer term, it might be worth revisiting it at some point - as it has a non-trivial & not well enforced boundary in terms of the allocation scheme shared between RichManglingContext and ItaniumPartialDemangler. It'd be good if that boundary were more clear rather than requiring malloc'd memory to be passed in, and requiring the caller to free it - some kind of abstraction over the memory ownership would probably be good to have)

[rupprecht]

• Add comments about memory management for m_ipd_buf

[rupprecht]

lldb/source/Core/RichManglingContext.cpp

23

Added a comment to the header. Agree about the longer term fixes on the API boundaries -- D100800 is a leak fix for that API, so it's clearly an API with sharp edges.

lldb/include/lldb/Core/RichManglingContext.h

 /// providers. See Mangled::DemangleWithRichManglingInfo()
 class RichManglingContext {
 public:
   RichManglingContext() : m_provider(None), m_ipd_buf_size(2048) {
     m_ipd_buf = static_cast<char *>(std::malloc(m_ipd_buf_size));
     m_ipd_buf[0] = '\0';
   }
 
-  ~RichManglingContext() { std::free(m_ipd_buf); }
+  ~RichManglingContext();
 
   /// Use the ItaniumPartialDemangler to obtain rich mangling information from
   /// the given mangled name.
   bool FromItaniumName(ConstString mangled);
 
   /// Use the legacy language parser implementation to obtain rich mangling
   /// information from the given demangled name.
   bool FromCxxMethodName(ConstString demangled);
private:
   /// Selects the rich mangling info provider.
   InfoProvider m_provider;
 
   /// Reference to the buffer used for results of ParseXy() operations.
   llvm::StringRef m_buffer;
 
   /// Members for ItaniumPartialDemangler
   llvm::ItaniumPartialDemangler m_ipd;
+  /// Note: m_ipd_buf is a raw pointer due to being resized by realloc via
+  /// ItaniumPartialDemangler. It should be managed with malloc/free, not
+  /// new/delete.
   char *m_ipd_buf;
   size_t m_ipd_buf_size;
 
   /// Members for PluginCxxLanguage
   /// Cannot forward declare inner class CPlusPlusLanguage::MethodName. The
   /// respective header is in Plugins and including it from here causes cyclic
   /// dependency. Instead keep a llvm::Any and cast it on-access in the cpp.
   llvm::Any m_cxx_method_parser;
 
+  /// Clean up memory when using PluginCxxLanguage
+  void ResetCxxMethodParser();
+
   /// Clean up memory and set a new info provider for this instance.
   void ResetProvider(InfoProvider new_provider);
 
   /// Uniform handling of string buffers for ItaniumPartialDemangler.
   void processIPDStrResult(char *ipd_res, size_t res_len);
 
   /// Cast the given parser to the given type. Ideally we would have a type
   /// trait to deduce \a ParserT from a given InfoProvider, but unfortunately we

lldb/source/Core/RichManglingContext.cpp

 #include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
 
 #include "llvm/ADT/StringRef.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 // RichManglingContext
-void RichManglingContext::ResetProvider(InfoProvider new_provider) {
+RichManglingContext::~RichManglingContext() {
+  std::free(m_ipd_buf);
+  ResetCxxMethodParser();
+}
+
+void RichManglingContext::ResetCxxMethodParser() {
   // If we want to support parsers for other languages some day, we need a
   // switch here to delete the correct parser type.
   if (m_cxx_method_parser.hasValue()) {
     assert(m_provider == PluginCxxLanguage);
     delete get<CPlusPlusLanguage::MethodName>(m_cxx_method_parser);
     m_cxx_method_parser.reset();
   }
+}
+
+void RichManglingContext::ResetProvider(InfoProvider new_provider) {
+  ResetCxxMethodParser();
 
   assert(new_provider != None && "Only reset to a valid provider");
   m_provider = new_provider;
 }
 
 bool RichManglingContext::FromItaniumName(ConstString mangled) {
   bool err = m_ipd.partialDemangle(mangled.GetCString());
   if (!err) {