[cdac] Implement ISOSDacInterface2::GetObjectExceptionData #104343
Conversation
elinor-fung
requested review from
lambdageek,
davidwrighton and
AaronRobinsonMSFT
|
Tagging subscribers to this area: @tommcdon |
|
cc @mikem8361 |
| @@ -152,6 +152,18 @@ CDAC_TYPE_FIELD(PreviousNestedInfo, /*pointer*/, PreviousNestedInfo, offsetof(Ex | |||
| #endif | |||
| CDAC_TYPE_END(ExceptionInfo) | |||
|
|
|||
| CDAC_TYPE_BEGIN(ExceptionObject) | |||
There was a problem hiding this comment.
I think that this is first instance where we have a data descriptor for a managed type. It is fine for now. I am wondering whether we will want to have a different infrastructure at some point in future that allows describing data descriptor of managed types without mirroring them into unmanaged runtime.
There was a problem hiding this comment.
Contracts for managed types is going to be required to support Native AOT. Unless I'm mistaken the Exception object isn't mirrored in the C++ side.
There was a problem hiding this comment.
I added an item to the future section of #99298.
| ``` | ||
|
|
||
| ## Version 1 | ||
|
|
||
| Data descriptors used: | ||
| - `ExceptionInfo` | ||
| - `ExceptionObject` |
There was a problem hiding this comment.
Should we have a consistent naming scheme for data descriptors of managed types? The names of the unmanaged mirrors of managed types are not very consistent.
Here are some examples of managed types that may need data descriptor at some point.
System.Exception
System.Runtime.Loader.AssemblyLoadContext
System.Threading.Thread
One possible naming scheme is to replace . with _. It would produce:
System_Exception
System_Runtime_Loader_AssemblyLoadContext
System_Threading_Thread
There was a problem hiding this comment.
Or omit . in the name:
SystemException
SystemRuntimeLoaderAssemblyLoadContext
SystemThreadingThread
Or abbreviate the name space:
S_Exception
SRL_AssemblyLoadContext
ST_Thread
Or drop the namespace completely (I think I like this the most):
Exception
AssemblyLoadContext
Thread
There was a problem hiding this comment.
I think I like the non-abbreviated . -> _, but I could see those getting a bit long - maybe abbreviated is a reasonable middle. For dropping the namespace completely, we'd have to differentiate between unmanaged and managed types that have the same name (like Thread) and I think I'd prefer the managed ones having the indication of being managed.
There was a problem hiding this comment.
we'd have to differentiate between unmanaged and managed types that have the same name (like Thread)
The name collisions have been a problem a few times as we are rewriting more of the native runtime in C#. If the name collisions are the only problem, I would say rename the internal types to avoid them. The ultimate picture can be that nearly all types have managed views, and subset of types have unmanaged mirrors.
The name collisions between managed and unmanaged code are also a source of confusion. Trivia question - when you see ThreadOBJ in SOS, does it mean managed Thread or unmanaged Thread?
| @@ -152,6 +152,18 @@ CDAC_TYPE_FIELD(PreviousNestedInfo, /*pointer*/, PreviousNestedInfo, offsetof(Ex | |||
| #endif | |||
| CDAC_TYPE_END(ExceptionInfo) | |||
|
|
|||
| CDAC_TYPE_BEGIN(ExceptionObject) | |||
| CDAC_TYPE_INDETERMINATE(ExceptionObject) | |||
| CDAC_TYPE_FIELD(ExceptionObject, /*pointer*/, Message, cdac_offsets<ExceptionObject>::Message) | |||
There was a problem hiding this comment.
I would always use the exact managed type field names in the data contract descriptor so that we do not need to worry about having different names for the same thing in different places.
The managed type field names are often set in stone because of binary serialization or because of existing (unwritten) diagnostic contract.
|
|
||
| This contract is for getting information about exceptions in the process. | ||
|
|
||
| ## APIs of contract | ||
|
|
||
| ```csharp | ||
| record struct ManagedExceptionData( |
There was a problem hiding this comment.
Should we call this just ExceptionData?
I think it would be best to avoid Managed prefix in the contract definitions.
There was a problem hiding this comment.
If there is something explicitly unmanaged (like EXCEPTION_INFORMATION - that is unmanaged equivalent of managed exception), I would use a special prefix for that if necessary.
| ``` | ||
|
|
||
| ## Version 1 | ||
|
|
||
| Data descriptors used: | ||
| - `ExceptionInfo` | ||
| - `ExceptionObject` |
There was a problem hiding this comment.
| - `ExceptionObject` | |
| - `Exception` |
?
…otnet#104343)" This reverts commit e733c2f.
ISOSDacInterface2Exception(managed type) to data descriptorGetExceptionDatatoExceptioncontract which gets all the data that SOS-DAC API usesContributes to #99302