[Lldb-commits] [PATCH] D100795: [lldb] Fix RichManglingContext::FromCxxMethodName() leak

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added inline comments.


================
Comment at: lldb/source/Core/RichManglingContext.cpp:23
+RichManglingContext::~RichManglingContext() {
+  std::free(m_ipd_buf);
+  ResetCxxMethodParser();
----------------
dblaikie wrote:
> JDevlieghere wrote:
> > shafik wrote:
> > > rupprecht wrote:
> > > > JDevlieghere wrote:
> > > > > Instead of managing memory by hand, can we use a `unique_ptr<char[]>` instead?
> > > > 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 :) )
> > > 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.
> > 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? 
> (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)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100795/new/

https://reviews.llvm.org/D100795