[cdac] Add basic cdacreader project #100623

elinor-fung · 2024-04-04T03:33:07Z

This change adds a basic cdacreader project under src/native/managed.

Built as part of the clr subset (via runtime-prereqs referencing compile-native.proj)
Not yet integrated into anything in the product (that is, the dac doesn't try to use it yet)
Can return a class that can implement the ISOSDacInterface* interfaces - currently does ISOSDacInterface9

Contributes to #99298
cc @AaronRobinsonMSFT @davidwrighton @jkotas @mikem8361 @noahfalk

Manually tested with:

intptr_t handle;
cdac_reader_init_fn(0, &handle);

IUnknown* unk;
cdac_reader_get_sos_interface_fn(handle, &unk);
ISOSDacInterface9* sos_dac;
HRESULT hr = unk->QueryInterface<ISOSDacInterface9>(&sos_dac);
unk->Release();
if (FAILED(hr))
    return hr;

int version;
int ret = sos_dac->GetBreakingChangeVersion(&version);
sos_dac->Release();
if (ret == 0)
    std::cout << "Version: " << version << std::endl;

cdac_reader_free_fn(handle);

dotnet-policy-service · 2024-04-04T03:33:38Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

src/native/managed/cdacreader/src/SOSDacImpl.cs

AaronRobinsonMSFT · 2024-04-04T15:36:40Z

src/native/managed/cdacreader/src/Entrypoints.cs

+        ComWrappers cw = new StrategyBasedComWrappers();
+        Target? target = GCHandle.FromIntPtr(handle).Target as Target;
+        if (target == null)
+            return -1;


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

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

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

How does hosting code provide a ICLRDataTarget instance?

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

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.

jkotas · 2024-04-04T17:02:14Z

src/native/managed/cdacreader/src/SOSDacImpl.cs

+
+    public void GetBreakingChangeVersion(out int version)
+    {
+        // TODO: Return non-hard-coded version


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.)

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?

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

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)?

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.

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.

Co-authored-by: Aaron Robinson <[email protected]>

After #100623, the official build is broken. Our infrastructure for building native runtime component libraries using NativeAOT is failing for linux-bionic. Disable building on linux-bionic for now to unblock the build.

This change adds a basic `cdacreader` project under src/native/managed. - Built as part of the clr subset (via runtime-prereqs referencing compile-native.proj) - Not yet integrated into anything in the product (that is, the dac doesn't try to use it yet) - Can return a class that can implement the ISOSDacInterface* interfaces - currently does ISOSDacInterface9

After dotnet#100623, the official build is broken. Our infrastructure for building native runtime component libraries using NativeAOT is failing for linux-bionic. Disable building on linux-bionic for now to unblock the build.

Add basic cdacreader project

84ee4ea

ghost added the area-Diagnostics-coreclr label Apr 4, 2024

dotnet-policy-service bot assigned elinor-fung Apr 4, 2024

elinor-fung requested a review from lambdageek April 4, 2024 05:07

lambdageek mentioned this pull request Apr 4, 2024

[cdac] Implement basic data stream reader and writer in C #99300

Closed

lambdageek approved these changes Apr 4, 2024

View reviewed changes

AaronRobinsonMSFT reviewed Apr 4, 2024

View reviewed changes

src/native/managed/cdacreader/src/SOSDacImpl.cs Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Apr 4, 2024

View reviewed changes

src/native/managed/cdacreader/src/SOSDacImpl.cs Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Apr 4, 2024

View reviewed changes

jkotas reviewed Apr 4, 2024

View reviewed changes

Apply suggestions from code review

efeec48

Co-authored-by: Aaron Robinson <[email protected]>

elinor-fung merged commit b973027 into dotnet:main Apr 5, 2024

elinor-fung deleted the cdacReaderProject branch April 5, 2024 15:38

elinor-fung mentioned this pull request Apr 5, 2024

Fix official build: disable compile-native runtime component libraries for linux-bionic #100712

Merged

akoeplinger mentioned this pull request Apr 7, 2024

[main] Update dependencies from dotnet/sdk dotnet/installer#19333

Merged

lambdageek mentioned this pull request Apr 12, 2024

Portable Data Contract-based DAC #99298

Open

37 tasks

github-actions bot locked and limited conversation to collaborators May 6, 2024

[cdac] Add basic cdacreader project #100623

[cdac] Add basic cdacreader project #100623

Uh oh!

Conversation

elinor-fung commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Apr 4, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elinor-fung commented Apr 4, 2024 •

edited

Loading

jkotas Apr 4, 2024 •

edited

Loading