Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing a memory leak #1099

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fixing a memory leak #1099

wants to merge 1 commit into from

Conversation

Jaders77
Copy link

@Jaders77 Jaders77 commented May 23, 2018

This part of the code makes a copy of the original reference without calling the destructor at any time for it (original reference).
Instead of using a default constructor / copy constructor and then one destructor for each, such C++ copy elision optimization, it's better to simply remove copy.

Simple example of the problem:

public global::Lldb.SBSymbolContext GetSymbolContext(uint resolve_scope)
{
  var __ret = new global::Lldb.SBSymbolContext.__Internal();
  __Internal.GetSymbolContext((__Instance + __PointerAdjustment), new IntPtr(&__ret), resolve_scope); // here __ret is initialized like a constructor
  return global::Lldb.SBSymbolContext.__CreateInstance(__ret);
}

internal static global::Lldb.SBSymbolContext __CreateInstance(global::Lldb.SBSymbolContext.__Internal native, bool skipVTables = false)
{
  return new global::Lldb.SBSymbolContext(native, skipVTables);
}

private SBSymbolContext(global::Lldb.SBSymbolContext.__Internal native, bool skipVTables = false)
  : this(__CopyValue(native), skipVTables)
{
  __ownsNativeInstance = true;
  NativeToManagedMap[__Instance] = this;
}

private static void* __CopyValue(global::Lldb.SBSymbolContext.__Internal native)
{
  var ret = Marshal.AllocHGlobal(sizeof(global::Lldb.SBSymbolContext.__Internal));
  global::Lldb.SBSymbolContext.__Internal.cctor(ret, new global::System.IntPtr(&native));
  return ret.ToPointer();
}

After the fix:

private static void* __CopyValue(global::Lldb.SBSymbolContext.__Internal native)
{
  var ret = Marshal.AllocHGlobal(sizeof(global::Lldb.SBSymbolContext.__Internal));
  *(global::Lldb.SBSymbolContext.__Internal*) ret = native;
  return ret.ToPointer();
}

This part of the code makes a copy of the original reference without calling the destructor at any time for it (original reference).
Instead of using a default constructor / copy constructor and then one destructor for each, such C++ copy elision optimization, it's better to simply remove copy.

Simple example of the problem:
public global::Lldb.SBSymbolContext GetSymbolContext(uint resolve_scope)
{
  var __ret = new global::Lldb.SBSymbolContext.__Internal();
  __Internal.GetSymbolContext((__Instance + __PointerAdjustment), new IntPtr(&__ret), resolve_scope); // here __ret is initialized like a constructor
  return global::Lldb.SBSymbolContext.__CreateInstance(__ret);
}

internal static global::Lldb.SBSymbolContext __CreateInstance(global::Lldb.SBSymbolContext.__Internal native, bool skipVTables = false)
{
  return new global::Lldb.SBSymbolContext(native, skipVTables);
}

private SBSymbolContext(global::Lldb.SBSymbolContext.__Internal native, bool skipVTables = false)
  : this(__CopyValue(native), skipVTables)
{
  __ownsNativeInstance = true;
  NativeToManagedMap[__Instance] = this;
}

private static void* __CopyValue(global::Lldb.SBSymbolContext.__Internal native)
{
  var ret = Marshal.AllocHGlobal(sizeof(global::Lldb.SBSymbolContext.__Internal));
  global::Lldb.SBSymbolContext.__Internal.cctor(ret, new global::System.IntPtr(&native));
  return ret.ToPointer();
}

After the fix:
private static void* __CopyValue(global::Lldb.SBSymbolContext.__Internal native)
{
  var ret = Marshal.AllocHGlobal(sizeof(global::Lldb.SBSymbolContext.__Internal));
  *(global::Lldb.SBSymbolContext.__Internal*) ret = native;
  return ret.ToPointer();
}
@dnfclas
Copy link

dnfclas commented May 23, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ Jaders77 sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@tritao tritao requested a review from ddobrev May 24, 2018 14:51
@ddobrev ddobrev force-pushed the master branch 12 times, most recently from 0464938 to 637018f Compare October 15, 2018 05:15
Copy link
Contributor

@ddobrev ddobrev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated code now does not call the non-trivial constructor if any. I think this might lead to incorrect behaviours on the native side.

@ddobrev ddobrev force-pushed the master branch 4 times, most recently from 91e219c to 32da859 Compare January 4, 2019 21:27
@tritao tritao force-pushed the master branch 2 times, most recently from a0169c2 to 3ea7e97 Compare March 11, 2019 00:30
@ddobrev ddobrev force-pushed the master branch 2 times, most recently from a1559de to 304d673 Compare April 10, 2019 23:04
@ddobrev ddobrev force-pushed the master branch 2 times, most recently from a08e24b to 0e1fb0b Compare June 26, 2019 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants