  
Welcome to Ventspace! Most posts here are delayed copies of posts from the real Ventspace.
| Thursday, October 1, 2009 |
  Subtleties of .NET and a SlimDX Crash Bug | 
Posted - 10/1/2009 1:49:25 PM | Let's look at a real, in the trenches SlimDX bug that results from a problematic subtlety in .NET. This one isn't even fixed in trunk yet. Here's our main function of interest:
generic<typename T> where T : value class
Result BaseEffect::SetValue( EffectHandle^ parameter, T value )
{
HRESULT hr;
D3DXHANDLE handle = parameter != nullptr ? parameter->InternalHandle : NULL;
if( T::typeid == bool::typeid )
{
BOOL newValue = Convert::ToInt32( value, CultureInfo::InvariantCulture );
hr = InternalPointer->SetBool( handle, newValue );
}
else if( T::typeid == float::typeid )
{
hr = InternalPointer->SetFloat( handle, static_cast<FLOAT>( value ) );
}
else if( T::typeid == int::typeid )
{
hr = InternalPointer->SetInt( handle, static_cast<INT>( value ) );
}
else if( T::typeid == Matrix::typeid )
{
hr = InternalPointer->SetMatrix( handle, reinterpret_cast<D3DXMATRIX*>( &value ) );
}
else if( T::typeid == Vector4::typeid )
{
hr = InternalPointer->SetVector( handle, reinterpret_cast<D3DXVECTOR4*>( &value ) );
}
else
{
hr = InternalPointer->SetValue( handle, &value, static_cast<DWORD>( sizeof(T) ) );
}
return RECORD_D3D9( hr );
}
This function will crash randomly on good input. Specifically, D3D will return a D3DERR_INVALIDCALL, which then gets translated to an exception. Again, that's on known-good input. The call to the function looks like this:
effect.SetValue<float>("alpha", 0.5f);
When this call is made, an EffectHandle is implicitly constructed that copies the string into native memory. Here's the relevant bits of EffectHandle:
EffectHandle::EffectHandle( String^ name )
{
m_StringData = Marshal::StringToHGlobalAnsi( name );
m_HasString = true;
m_StringDataSize = name->Length;
GC::AddMemoryPressure( m_StringDataSize );
m_Handle = reinterpret_cast<D3DXHANDLE>( m_StringData.ToPointer() );
}
void EffectHandle::Destruct()
{
if( m_HasString )
{
Marshal::FreeHGlobal( m_StringData );
GC::RemoveMemoryPressure( m_StringDataSize );
}
}
So. Do you see where we screwed this up?
In case it's not clear:
* EffectHandle::InternalHandle returns EffectHandle::m_Handle
* D3DXHANDLE is a typedef for char*
* Although there's plenty of interop, the interop is NOT the root cause.
HINT: EffectHandle is finalizable. Answer is in comments.
| |
Comments
 | bubu LV
Member since: 9/12/2005 From: Riga, Riga | 
Posted - 10/1/2009 2:17:21 PM | hm... m_HasString is not being set to false in Destruct() method?
| |
 | Promit Moderator - Graphics Programming and Theory
Member since: 7/29/2001 From: Baltimore, MD | |
Posted - 10/1/2009 2:32:03 PM | While you could make an argument for that being a good idea, it's not why it's crashing. (Though it might have made the crash easier to find.)
| |
| bubu LV
Member since: 9/12/2005 From: Riga, Riga | |
Posted - 10/1/2009 3:22:44 PM | Ah, I think I got it.
Is it because after this line:
D3DXHANDLE handle = parameter != nullptr ? parameter->InternalHandle : NULL;
parameter object is no more referenced in function? So GC is free to dispose it. And when it is disposing it, then parameter->Destruct() is called and m_StringData is freed. But later in InternalPointer->SetFloat call it's memory is used (with native handle pointer).
| |
| Promit Moderator - Graphics Programming and Theory
Member since: 7/29/2001 From: Baltimore, MD | |
Posted - 10/1/2009 3:45:03 PM | That is exactly it. You win a Rate++. Do you know how to solve it?
| |
| bubu LV
Member since: 9/12/2005 From: Riga, Riga | |
Posted - 10/1/2009 4:04:04 PM | :)
You could add GC::KeepAlive(parameter) before return RECORD_D3D9( hr ); line.
But first I thought you could use "parameter != nullptr ? parameter->InternalHandle : NULL" in each place where currently "handle" is written. But that is still incorrect - that would still not prevent GC from runing while current thread is inside DirectX call.
| |
| Promit Moderator - Graphics Programming and Theory
Member since: 7/29/2001 From: Baltimore, MD | |
Posted - 10/1/2009 4:22:57 PM | Yep, correct again. Although the fix actually involves adding it in like thirty functions 
| |
| kirkus
Member since: 10/2/2009 From: Golden Valley, MN | |
Posted - 10/2/2009 9:50:59 AM | Would this be a good situation for a SafeHandle?
| |
| Promit Moderator - Graphics Programming and Theory
Member since: 7/29/2001 From: Baltimore, MD | |
Posted - 10/2/2009 11:33:43 AM | No, SafeHandle is for Win32 HANDLE, which is an entirely different beast.
| |
| kirkus
Member since: 10/2/2009 From: Golden Valley, MN | |
Posted - 10/2/2009 11:43:44 AM | Interesting. The MSDN documentation uses this as an example of a SafeHandle for unmanaged pointers:
// Demand unmanaged code permission to use this class.
[SecurityPermission(SecurityAction.Demand, UnmanagedCode = true)]
sealed class SafeUnmanagedMemoryHandle : SafeHandleZeroOrMinusOneIsInvalid
{
// Set ownsHandle to true for the default constructor.
internal SafeUnmanagedMemoryHandle() : base(true) { }
// Set the handle and set ownsHandle to true.
internal SafeUnmanagedMemoryHandle(IntPtr preexistingHandle, bool ownsHandle)
: base(ownsHandle)
{
SetHandle(preexistingHandle);
}
// Perform any specific actions to release the
// handle in the ReleaseHandle method.
// Often, you need to use Pinvoke to make
// a call into the Win32 API to release the
// handle. In this case, however, we can use
// the Marshal class to release the unmanaged
// memory.
override protected bool ReleaseHandle()
{
// "handle" is the internal
// value for the IntPtr handle.
// If the handle was set,
// free it. Return success.
if (handle != IntPtr.Zero)
{
// Free the handle.
Marshal.FreeHGlobal(handle);
// Set the handle to zero.
handle = IntPtr.Zero;
// Return success.
return true;
}
// Return false.
return false;
}
}
| |
| jvsstudios
Member since: 7/27/2005 | |
Posted - 10/3/2009 10:42:37 AM | What about passing parameter in by reference? To me that would seem to keep the GC from disposing it since it would be referenced outside of the function.
| |
| jvsstudios
Member since: 7/27/2005 | |
Posted - 10/3/2009 10:54:14 AM | Oops, nevermind. It looks like that is already being done. But if I were to guess, I would say pass the EffectHandle in as a const. If I am wrong don't hurt me too much :) I do program in c++, but I have not worked with managed c++.
| |
| bubu LV
Member since: 9/12/2005 From: Riga, Riga | |
Posted - 10/5/2009 11:02:49 AM | Afaik managed types don't have const types like native C++.
| |
|
| S | M | T | W | T | F | S | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20 | 21 | 22 | 23 | 24 | 25 | 26 | 27 | 28 | 29 | 30 | | | | | |
OPTIONS
Track this Journal
ARCHIVES
October, 2009
September, 2009
August, 2009
July, 2009
June, 2009
October, 2008
June, 2008
May, 2008
April, 2008
March, 2008
February, 2008
January, 2008
December, 2007
November, 2007
October, 2007
September, 2007
August, 2007
July, 2007
June, 2007
May, 2007
February, 2007
|
