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

[cdac] Add basic cdacreader project #100623

Merged
merged 2 commits into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/native/managed/cdacreader/cmake/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
add_library(cdacreader_api INTERFACE)
target_include_directories(cdacreader_api INTERFACE ${CLR_SRC_NATIVE_DIR}/managed/cdacreader/inc)
20 changes: 20 additions & 0 deletions src/native/managed/cdacreader/inc/cdac_reader.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#ifndef CDAC_READER_H
#define CDAC_READER_H

#ifdef __cplusplus
extern "C"
{
#endif

int cdac_reader_init(intptr_t descriptor, intptr_t* handle);
int cdac_reader_free(intptr_t handle);
int cdac_reader_get_sos_interface(intptr_t handle, IUnknown** obj);

#ifdef __cplusplus
}
#endif

#endif // CDAC_READER_H
50 changes: 50 additions & 0 deletions src/native/managed/cdacreader/src/Entrypoints.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Runtime.InteropServices;
using System.Runtime.InteropServices.Marshalling;

namespace Microsoft.Diagnostics.DataContractReader;

internal static class Entrypoints
{
private const string CDAC = "cdac_reader_";

[UnmanagedCallersOnly(EntryPoint = $"{CDAC}init")]
private static unsafe int Init(nint descriptor, IntPtr* handle)
{
Target target = new(descriptor);
GCHandle gcHandle = GCHandle.Alloc(target);
*handle = GCHandle.ToIntPtr(gcHandle);
return 0;
}

[UnmanagedCallersOnly(EntryPoint = $"{CDAC}free")]
private static unsafe int Free(IntPtr handle)
{
GCHandle h = GCHandle.FromIntPtr(handle);
h.Free();
return 0;
}

/// <summary>
/// Get the SOS-DAC interface implementation.
/// </summary>
/// <param name="handle">Handle crated via cdac initialization</param>
/// <param name="obj"><c>IUnknown</c> pointer that can be queried for ISOSDacInterface*</param>
/// <returns></returns>
[UnmanagedCallersOnly(EntryPoint = $"{CDAC}get_sos_interface")]
private static unsafe int GetSOSInterface(IntPtr handle, nint* obj)
{
ComWrappers cw = new StrategyBasedComWrappers();
Target? target = GCHandle.FromIntPtr(handle).Target as Target;
if (target == null)
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to adopt an HRESULT pattern for error codes or more of a POSIX pattern?

Copy link
Member

Choose a reason for hiding this comment

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

Is the Target class the data target (like ICLRDataTarget) that will have read memory etc. methods on it?

Copy link
Member

Choose a reason for hiding this comment

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

It's Target from https://github.com/dotnet/runtime/blob/main/docs/design/datacontracts/contract_csharp_api_design.cs#L85

it's the cDAC's internal abstraction for the target process

Copy link
Member

Choose a reason for hiding this comment

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

How does hosting code provide a ICLRDataTarget instance?

Copy link
Member

Choose a reason for hiding this comment

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

That's not wired up yet in this PR, but in the previous prototypes the cDAC exported a function like:

typedef cdac_reader_result_t (*cdac_reader_func_t) (cdac_reader_foreignptr_t addr, uint32_t count, void* user_data, uint8_t *dest);

cdac_reader_result_t cdac_reader_set_reader_func(cdac_reader_h handle, cdac_reader_func_t reader, void* user_data);

that was called from the existing DAC, passing a callback that wrapped ReadDataFromTarget:

https://github.com/lambdageek/runtime/blob/ab6e9603222842034c4f4452816e14371db450d9/src/coreclr/debug/daccess/cdac.cpp#L262-L296

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to adopt an HRESULT pattern for error codes or more of a POSIX pattern?

I don't feel strongly either way, but I went HRESULT of negative being error since that is what exports on coreclr and hosting components do.


SOSDacImpl impl = new(target);
nint ptr = cw.GetOrCreateComInterfaceForObject(impl, CreateComInterfaceFlags.None);
*obj = ptr;
return 0;
}
}
36 changes: 36 additions & 0 deletions src/native/managed/cdacreader/src/SOSDacImpl.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Runtime.InteropServices;
using System.Runtime.InteropServices.Marshalling;

namespace Microsoft.Diagnostics.DataContractReader;

[GeneratedComInterface]
[Guid("4eca42d8-7e7b-4c8a-a116-7bfbf6929267")]
internal partial interface ISOSDacInterface9
{
int GetBreakingChangeVersion();
}

/// <summary>
/// Implementation of ISOSDacInterface* interfaces intended to be passed out to consumers
/// interacting with the DAC via those COM interfaces.
/// </summary>
[GeneratedComClass]
internal sealed partial class SOSDacImpl : ISOSDacInterface9
{
private readonly Target _target;

public SOSDacImpl(Target target)
{
_target = target;
}

public int GetBreakingChangeVersion()
{
// TODO: Return non-hard-coded version
Copy link
Member

@jkotas jkotas Apr 4, 2024

Choose a reason for hiding this comment

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

This API represents versions of (poorly defined) set of algorithmic and data contracts that SOS and CLRMD happens to depend on. It should eventually obsoleted, and SOS and CLRMD switched to use the versions of properly defined contracts.

(It is ok to keep it for now if it helps.)

Copy link
Member

Choose a reason for hiding this comment

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

Are we trying to implement the actual C++ ISOSDacInterface9 interface here (HRESULT GetBreakingChangeVersion(int* pVersion))? Or is this part of the new C# cDAC interface being defined?

Copy link
Member

Choose a reason for hiding this comment

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

Are we trying to implement the actual C++ ISOSDacInterface9 interface here ? Or is this part of the new C# cDAC interface being defined?

We're trying to implement the actual ISOSDacInterface9 here. This is going to be consumed by the existing DAC in cases where it will delegate to the cdacreader.

Or is this part of the new C# cDAC interface being defined?

It is not a goal right now to define a public interface to the cDAC

Copy link
Member

Choose a reason for hiding this comment

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

Then shouldn't the C# method be public int GetBreakingChangeVersion(out int version)? Or is there some magic (code generation/COM interop) that will expose the C++ function HRESULT GetBreakingChangeVersion(int* pVersion)?

Copy link
Member

Choose a reason for hiding this comment

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

that will expose the C++ function HRESULT GetBreakingChangeVersion(int* pVersion)?

COM interop has elided the HRESULT by default since at least .NET Framework 2.0. We can have a matching signature as you've suggested, but the out isn't correct either since it is unmanaged memory, so it should be int*. I would prefer we either match the signature precisely using blittable types or we write it in canonical .NET form. In this case that means using the last "out" param as the return and letting the marshalling system throw an Exception for a failing HRESULT or convert an Exception to an HRESULT in the other direction.

The one caveat for this is when S_FALSE is used. In cases where an API can return a non-zero success HRESULT we need to use the PreserveSig mechanism and check the int manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should eventually obsoleted, and SOS and CLRMD switched to use the versions of properly defined contracts.

Agreed. At least for now, since all the sos commands use it, I chose it as the simple starting point for putting together what @lambdageek mentions about the existing DAC consuming/delegating to the cdacreader.

return 4;
}
}
11 changes: 11 additions & 0 deletions src/native/managed/cdacreader/src/Target.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.Diagnostics.DataContractReader;

internal sealed class Target
{
public Target(nint _)
{
}
}
17 changes: 17 additions & 0 deletions src/native/managed/cdacreader/src/cdacreader.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>$(NetCoreAppToolCurrent)</TargetFramework>
<Nullable>enable</Nullable>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<!-- Do not produce a public package. This ships as part of the runtime -->
<IsShippingPackage>false</IsShippingPackage>
</PropertyGroup>

<ItemGroup>
<InstallRuntimeComponentDestination Include="." />
<!-- TODO: [cdac] Output to sharedFramework and add PlatformManifestFileEntry for Microsoft.NETCore.App once ready to include in shipping package -->
<!-- <InstallRuntimeComponentDestination Include="sharedFramework" Condition="'$(RuntimeFlavor)' == 'coreclr'"/> -->
</ItemGroup>

</Project>
1 change: 1 addition & 0 deletions src/native/managed/compile-native.proj
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
<ItemGroup>
<!-- add new projects here -->
<!-- NativeLibsProjectsToBuild Include="$(MSBuildThisFileDirectory)libhellomanaged/src/libhellomanaged.csproj" -->
<NativeLibsProjectsToBuild Include="$(MSBuildThisFileDirectory)cdacreader/src/cdacreader.csproj" />
</ItemGroup>

<!-- Decide if we're going to do the NativeAOT builds -->
Expand Down
Loading