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] Physical contract descriptor spec #100365

Merged
merged 11 commits into from
Apr 4, 2024

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Mar 27, 2024

Building on #100253 , describe an in-memory representation of the toplevel contract descriptor, comprisied of:

  • some target architecture properties
  • a data descriptor
  • a collection of compatible contracts

Contributes to #99298
Fixes #99299

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 27, 2024
@lambdageek lambdageek added documentation Documentation bug or enhancement, does not impact product or test code area-Meta and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 27, 2024
Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

I think this is a good direction, but I have a few comments.

docs/design/datacontracts/contract-descriptor.md Outdated Show resolved Hide resolved
docs/design/datacontracts/contract-descriptor.md Outdated Show resolved Hide resolved
docs/design/datacontracts/contract-descriptor.md Outdated Show resolved Hide resolved
@lambdageek lambdageek force-pushed the cdac-contract-descriptor branch 3 times, most recently from f37c8a8 to 4bd9a58 Compare March 28, 2024 17:22
@lambdageek lambdageek force-pushed the cdac-contract-descriptor branch from 4bd9a58 to 77afeac Compare March 29, 2024 13:30
@lambdageek lambdageek marked this pull request as ready for review March 29, 2024 13:36
lambdageek and others added 4 commits March 29, 2024 09:37
the issue is that we can't dereference pointers until we know the
target endianness and pointer size.  which we can only discover by
reading the actual descriptor magic and flags
if there are multiple hosted runtimes, diagnostic tooling should look
in each loaded module to discover the contract descriptor
docs/design/datacontracts/datacontracts_design.md Outdated Show resolved Hide resolved
docs/design/datacontracts/contract-descriptor.md Outdated Show resolved Hide resolved
Comment on lines 25 to 26
uint32_t aux_data_count;
uint32_t descriptor_size;
Copy link
Member

Choose a reason for hiding this comment

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

Can we put the count/size right before or after the descriptor/aux_data instead?

docs/design/datacontracts/contract-descriptor.md Outdated Show resolved Hide resolved
docs/design/datacontracts/contract-descriptor.md Outdated Show resolved Hide resolved
docs/design/datacontracts/contract-descriptor.md Outdated Show resolved Hide resolved

To aid in the discovery the contract descriptor should be exported by the module hosting the .NET
runtime with the name `DotNetRuntimeContractDescriptor`. (Using the C symbol conventions of the
target platform. That is, on platforms where such symbols typically have an `_` prepended, this
Copy link
Member

Choose a reason for hiding this comment

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

What are those platforms?

FWIW, the existing DotNetRuntimeDebugHeader seems to be exported without _ everywhere:

<IlcArg Condition="'$(_targetOS)' == 'win' and '$(DebuggerSupport)' != 'false'" Include="--export-dynamic-symbol:DotNetRuntimeDebugHeader,DATA" />
<IlcArg Condition="'$(_targetOS)' != 'win' and '$(DebuggerSupport)' != 'false'" Include="--export-dynamic-symbol:DotNetRuntimeDebugHeader" />

If `ptrSize` is 0, the architecture is 64-bit. If it is 1, the architecture is 32-bit. The
reserved bits should be written as zero. Diagnostic tooling may ignore non-zero reserved bits.

The `descriptor` is a pointer to a JSON string described in [data descriptor physical layout](./data_descriptor.md#Physical_JSON_descriptor). The total length (including nul terminator character) is given by `descriptor_size`.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I would omit the terminating null in the descriptor_size.

The data contract reader cannot assume that the descriptor is null terminated. It needs to be robust against malicious input. Including the terminating null does not seem to be beneficial, it just adds extra logic to the reader to verify that there is a null at the end and strip it.

lambdageek and others added 2 commits April 4, 2024 10:29
Co-authored-by: Elinor Fung <elfung@microsoft.com>
- put the aux data and descriptor sizes closer to the pointers

- Don't include trailing nul `descriptor_size`.  Clarify it is
counting bytes and that `descriptor` is in UTF-8

- Simplify `DotNetRuntimeContractDescriptor` naming discussion
@lambdageek lambdageek merged commit e629779 into dotnet:main Apr 4, 2024
12 checks passed
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
Building on dotnet#100253 , describe an in-memory representation of the toplevel contract descriptor, comprisied of:
* some target architecture properties
* a data descriptor
* a collection of compatible contracts

Contributes to dotnet#99298 
Fixes dotnet#99299

---


* [cdac] Physical contract descriptor spec

* Add "contracts" to the data descriptor

*  one runtime per module

   if there are multiple hosted runtimes, diagnostic tooling should look in each loaded module to discover the contract descriptor

* Apply suggestions from code review

* Review feedback

   - put the aux data and descriptor sizes closer to the pointers

   - Don't include trailing nul `descriptor_size`.  Clarify it is counting bytes and that `descriptor` is in UTF-8

   - Simplify `DotNetRuntimeContractDescriptor` naming discussion

---------

Co-authored-by: Elinor Fung <elfung@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cdac] Publish data stream spec
5 participants